diff --git a/CLAUDE.md b/CLAUDE.md index f91f294..fda2d4a 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -40,7 +40,7 @@ Single entrypoint: `src/entrypoints/run.ts` orchestrates everything — prepare This fork adds Forgejo support via a lean adapter pattern — only 4 genuinely different areas have Forgejo-specific code: -- **`src/platform/detector.ts`** — Three-tier platform detection (explicit input > `FORGEJO` env > `GITHUB_SERVER_URL` heuristic) +- **`src/platform/detector.ts`** — Three-tier platform detection (explicit input > `FORGEJO` env > `GITHUB_SERVER_URL` heuristic) with cross-validation that rejects Forgejo override on github.com - **`src/forgejo/token.ts`** — Direct token handling (skips OIDC exchange) - **`src/forgejo/data/fetcher.ts`** — REST-based data fetching (replaces 3 GraphQL queries with 5-8 parallel REST calls) - **`src/forgejo/comment-logic.ts`** — Forgejo URL patterns for tracking comments @@ -54,9 +54,10 @@ All other `src/github/` modules work on Forgejo via Octokit REST as-is (permissi - `src/entrypoints/run.ts` — Platform branch at top for token setup - `src/modes/tag/index.ts` — Data fetcher swap (`fetchForgejoData` vs `fetchGitHubData`) - `src/github/operations/comments/create-initial.ts` — Platform-aware spinner/URL format -- `src/github/operations/comments/update-with-branch.ts` — Forgejo PR compare URL +- `src/github/operations/comments/update-with-branch.ts` — Forgejo branch link in tracking comment - `src/github/operations/branch.ts` — Uses `repos.getBranch()` on Forgejo (Forgejo has `/git/refs` not `/git/ref`) -- `src/github/validation/actor.ts` — Skipped on Forgejo (no bot accounts) +- `src/github/validation/actor.ts` — Bot-type check skipped on Forgejo; self-trigger guard active +- `src/entrypoints/update-comment-link.ts` — Forgejo PR compare URL (`expand=1` instead of `quick_pull=1`) - `src/mcp/install-mcp-server.ts` — Skips GitHub public MCP server on Forgejo **MCP server names stay the same** (`github_comment`, `github_ci`, etc.) even on Forgejo — changing them would require updating hardcoded tool name lists in `src/modes/tag/index.ts`. diff --git a/docs/forgejo.md b/docs/forgejo.md index f02dc1b..0df768a 100644 --- a/docs/forgejo.md +++ b/docs/forgejo.md @@ -88,6 +88,10 @@ The action detects the platform using a three-tier strategy: **Recommendation**: Always set `platform: "forgejo"` explicitly. Auto-detection works but the explicit input eliminates ambiguity. +> **Security note**: Setting `platform: "forgejo"` on a `github.com` runner is rejected — the action cross-validates the `GITHUB_SERVER_URL` and falls through to standard detection. This prevents bypassing GitHub-specific security checks (e.g., actor validation). + +> **Note for GitHub Enterprise Server (GHES) users**: The tier-3 heuristic classifies any non-`github.com` URL as Forgejo. If you run on GHES (e.g., `https://github.mycompany.com`), you **must** set `platform: "github"` explicitly to avoid misdetection. + ## Differences from GitHub ### Authentication @@ -103,15 +107,15 @@ On Forgejo, there is no OIDC token exchange with Anthropic. The action uses the ### API Differences -| Feature | GitHub | Forgejo | -| --------------------- | ------------------------------------------- | ------------------------------------------------------------------- | -| **GraphQL API** | Used for PR/issue data fetching | Not available — REST API only | -| **Data fetching** | Single GraphQL query per entity | 5-8 parallel REST API calls | -| **Actor validation** | Checks for bot/app accounts | Skipped — no bot accounts on Forgejo | -| **GitHub MCP server** | `ghcr.io/github/github-mcp-server` included | Skipped — GitHub-specific | -| **CI/Actions MCP** | Full GitHub Actions API | Works via Octokit REST + `GITHUB_API_URL` | -| **Permission check** | `getCollaboratorPermissionLevel()` | `repos.get()` fallback (workflow tokens lack collaborator endpoint) | -| **Git ref lookup** | `git.getRef()` (`/git/ref/heads/...`) | `repos.getBranch()` (`/git/refs` is plural on Forgejo) | +| Feature | GitHub | Forgejo | +| --------------------- | ------------------------------------------- | --------------------------------------------------------------------- | +| **GraphQL API** | Used for PR/issue data fetching | Not available — REST API only | +| **Data fetching** | Single GraphQL query per entity | 5-8 parallel REST API calls | +| **Actor validation** | Checks for bot/app accounts | Bot-type check skipped; self-trigger guard active | +| **GitHub MCP server** | `ghcr.io/github/github-mcp-server` included | Skipped — GitHub-specific | +| **CI/Actions MCP** | Full GitHub Actions API | Works via Octokit REST + `GITHUB_API_URL` | +| **Permission check** | `getCollaboratorPermissionLevel()` | `getCollaboratorPermissionLevel()` with `repos.get()` fallback on 403 | +| **Git ref lookup** | `git.getRef()` (`/git/ref/heads/...`) | `repos.getBranch()` (`/git/refs` is plural on Forgejo) | ### Webhook & Event Differences @@ -255,12 +259,10 @@ Runs on push to `main` and pull requests. Three parallel jobs: ### `test-action.yml` — Action Self-Test -Tests the action itself on Forgejo in agent mode. Runs weekly and on PRs that modify `src/`, `action.yml`, or `.forgejo/`. Validates: +Tests the action itself on Forgejo in agent mode. Runs weekly and on PRs that modify `src/`, `action.yml`, `.forgejo/`, or `base-action/`. Validates: -- Platform detection returns "forgejo" -- Token authentication works -- Claude Code executes successfully -- Output file is produced +- Claude Code executes successfully with `platform: "forgejo"` +- Output file is produced with expected content ### `upstream-sync.yml` — Upstream Sync Check @@ -340,13 +342,14 @@ All execution modes and trigger types have been validated end-to-end on a Forgej ## Known Limitations +- **GitHub Enterprise Server (GHES) misdetection**: The tier-3 platform detection heuristic treats any `GITHUB_SERVER_URL` that isn't `github.com` as Forgejo. GHES instances (e.g., `https://github.mycompany.com`) will be misidentified. Set `platform: "github"` explicitly when running on GHES. - **Git ref API**: Forgejo uses `/git/refs` (plural) instead of GitHub's `/git/ref` (singular). The action handles this by using `repos.getBranch()` on Forgejo for branch SHA lookups. - **User display name fetch**: `GET /users/{username}` returns 403 without `read:user` scope. Non-fatal — falls back to login name. - **`pull_request_target` event**: May not exist in Forgejo. The action degrades gracefully (maps to `pull_request`). - **GitHub MCP server**: The `ghcr.io/github/github-mcp-server` Docker image is GitHub-specific and is skipped on Forgejo. - **Commit signing**: SSH signing has not been tested on Forgejo. The `use_commit_signing` option (GitHub API signing) won't work since it uses GitHub's commit API. - **`downloadJobLogForWorkflowRun`**: Forgejo's Actions API may lack this endpoint. The CI MCP server attempts it via Octokit REST but may return limited data. -- **Bot filtering**: `allowed_bots` checks won't match `claude[bot]` since Forgejo has no bot accounts. Actor validation is skipped entirely on Forgejo. +- **Bot filtering**: `allowed_bots` checks won't match `claude[bot]` since Forgejo has no bot accounts. Bot-type actor validation is skipped on Forgejo, but self-trigger prevention is active — the action blocks events where the actor matches the configured `bot_name` to prevent infinite loops. - **`pull_request_review` trigger is dead code**: Forgejo sends `pull_request_comment` for both review comments and review submissions. The `pull_request_review` trigger in the workflow never fires — all review-related events are handled through the `pull_request_comment` codepath. - **Review comment tracking comments are always issue comments**: On Forgejo, review comment events lack the top-level `comment` object needed to create PR review comment replies. The tracking comment is always created as a regular issue comment instead. - **Workflow `if:` conditions are unreliable for Forgejo events**: Forgejo does not populate `github.event.comment.body`, `github.event.review.body`, or `github.event.action` in the workflow expression context for several event types. All trigger validation happens in application code (`trigger.ts`), not in workflow conditions. @@ -379,7 +382,7 @@ Forgejo doesn't populate `github.event.comment.body`, `github.event.review.body` ### Workflow `if:` expression fails with "invalid token" error -Forgejo's expression parser rejects `#` characters as invalid tokens. Move any comments outside the `if:` block and use `>-` scalar folding instead of `|` for multi-line expressions. +Forgejo's expression parser may reject `#` characters as invalid tokens when using `|` (literal block) scalars, since each line is preserved and `#` starts a comment that extends to end-of-line. With `>-` (folded scalar), newlines become spaces, and `#` comments may be tolerated by some Forgejo versions — but for maximum compatibility, move comments outside the `if:` block entirely. ### Label trigger fires twice when creating issue with label diff --git a/prds/7-senior-review-forgejo.md b/prds/done/7-senior-review-forgejo.md similarity index 94% rename from prds/7-senior-review-forgejo.md rename to prds/done/7-senior-review-forgejo.md index 4d7279d..a198701 100644 --- a/prds/7-senior-review-forgejo.md +++ b/prds/done/7-senior-review-forgejo.md @@ -58,7 +58,7 @@ Perform a thorough, documented code review of all Forgejo-related changes. The r ## Milestones -- [ ] **M1: Architecture & design review** +- [x] **M1: Architecture & design review** - Review the lean adapter pattern: is it the right abstraction level? - Assess merge-conflict surface with upstream: are platform branches minimal? @@ -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 @@ -76,7 +76,7 @@ Perform a thorough, documented code review of all Forgejo-related changes. The r - Review fallback behavior (e.g., `repos.get()` fallback for permissions) - **Deliverable**: List of error handling gaps with fixes -- [ ] **M3: Security review** +- [x] **M3: Security review** - Token handling: is the token properly scoped and never logged? - Input sanitization: can webhook payloads inject content? @@ -84,7 +84,7 @@ Perform a thorough, documented code review of all Forgejo-related changes. The r - Secrets in logs: review all `core.info()` / `core.debug()` calls - **Deliverable**: Security findings with severity ratings -- [ ] **M4: Test coverage review & improvements** +- [x] **M4: Test coverage review & improvements** - Assess existing unit test coverage for Forgejo code paths - Identify critical paths without test coverage @@ -92,7 +92,7 @@ Perform a thorough, documented code review of all Forgejo-related changes. The r - Verify mocks accurately represent Forgejo API responses - **Deliverable**: Test coverage improved, gaps documented -- [ ] **M5: Code quality & maintainability review** +- [x] **M5: Code quality & maintainability review** - TypeScript strictness: are types accurate or using `any` / type assertions? - Naming conventions: consistent with the rest of the codebase? @@ -101,7 +101,7 @@ Perform a thorough, documented code review of all Forgejo-related changes. The r - Code comments: are complex decisions documented? - **Deliverable**: Code quality fixes applied -- [ ] **M6: Documentation accuracy review** +- [x] **M6: Documentation accuracy review** - Cross-check `docs/forgejo.md` against actual code behavior - Verify all "Known Limitations" are still accurate - Check that code examples and configuration snippets work 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/entrypoints/update-comment-link.ts b/src/entrypoints/update-comment-link.ts index de08d2c..89158f9 100644 --- a/src/entrypoints/update-comment-link.ts +++ b/src/entrypoints/update-comment-link.ts @@ -8,7 +8,6 @@ import { type CommentUpdateInput, } from "../github/operations/comment-logic"; import { - updateForgejoCommentBody, createForgejoPrCompareUrl, getForgejoRunIndex, } from "../forgejo/comment-logic"; @@ -19,7 +18,7 @@ import { isEntityContext, } from "../github/context"; import type { ParsedGitHubContext } from "../github/context"; -import { GITHUB_SERVER_URL } from "../github/api/config"; +import { getServerUrl } from "../github/api/config"; import { checkAndCommitOrDeleteBranch } from "../github/operations/branch-cleanup"; import { updateClaudeComment } from "../github/operations/comments/update-claude-comment"; @@ -54,7 +53,7 @@ export async function updateCommentLink( const { owner, repo } = context.repository; const platform = detectPlatform(); - const serverUrl = GITHUB_SERVER_URL; + const serverUrl = getServerUrl(); const runIdentifier = platform === "forgejo" ? getForgejoRunIndex() : process.env.GITHUB_RUN_ID; const jobUrl = `${serverUrl}/${owner}/${repo}/actions/runs/${runIdentifier}`; @@ -250,10 +249,7 @@ export async function updateCommentLink( errorDetails, }; - const updatedBody = - platform === "forgejo" - ? updateForgejoCommentBody(commentInput) - : updateCommentBody(commentInput); + const updatedBody = updateCommentBody(commentInput); try { await updateClaudeComment(octokit.rest, { diff --git a/src/forgejo/comment-logic.ts b/src/forgejo/comment-logic.ts index 2fccefb..aa697f6 100644 --- a/src/forgejo/comment-logic.ts +++ b/src/forgejo/comment-logic.ts @@ -1,38 +1,16 @@ /** * Forgejo-specific comment URL formatting and body construction. * - * Mirrors the GitHub versions in: - * - src/github/operations/comments/common.ts (spinner, link builders, initial comment) - * - src/github/operations/comment-logic.ts (updateCommentBody for final comment) - * - * Differences from GitHub: + * Contains Forgejo-specific helpers for: * - Unicode spinner instead of GitHub CDN image (CDN URLs don't resolve on Forgejo) - * - PR compare URL omits `quick_pull=1` (GitHub-specific parameter) - * - Dynamic server URL extraction instead of hardcoded github.com regex - */ - -import { ensureProperlyEncodedUrl } from "../github/operations/comment-logic"; -import type { CommentUpdateInput } from "../github/operations/comment-logic"; - -// Re-export types so consumers don't need to import from two places -export type { - CommentUpdateInput, - ExecutionDetails, -} from "../github/operations/comment-logic"; -export { ensureProperlyEncodedUrl } from "../github/operations/comment-logic"; - -/** - * Read the server URL at call time (not module load time). + * - PR compare URL with `expand=1` instead of GitHub's `quick_pull=1` + * - Forgejo run index (GITHUB_RUN_NUMBER vs GITHUB_RUN_ID) * - * The GitHub module uses a top-level constant from config.ts, which is fine - * when GITHUB_SERVER_URL doesn't change. For Forgejo we read from env at - * call time so that: - * 1. Tests can override the env var and see the effect immediately. - * 2. The same module works regardless of import order. + * The final comment body logic (updateCommentBody) is unified in + * src/github/operations/comment-logic.ts and works for both platforms. */ -function getServerUrl(): string { - return process.env.GITHUB_SERVER_URL || "https://github.com"; -} + +import { getServerUrl } from "../github/api/config"; // --------------------------------------------------------------------------- // Run index helper @@ -66,6 +44,12 @@ export const FORGEJO_SPINNER_HTML = "⏳"; // URL Builders // --------------------------------------------------------------------------- +// Note: owner, repo, runId, and branchName are NOT URI-encoded in path +// segments because: (1) owner/repo are validated by Forgejo (alphanumeric + +// .-_), (2) branchName is validated by validateBranchName() which restricts +// to [a-zA-Z0-9._/-], and (3) encoding "/" in branch names breaks Forgejo +// URLs (e.g., "claude/fix-bug" → "claude%2Ffix-bug" is invalid). + export function createForgejoJobRunLink( owner: string, repo: string, @@ -118,189 +102,3 @@ I'll analyze this and get back to you. ${jobRunLink}${branchLink}`; } - -// --------------------------------------------------------------------------- -// Owner/repo extraction helper -// --------------------------------------------------------------------------- - -/** - * Extract owner/repo from a URL using the configured server URL. - * - * Unlike the GitHub version (which hardcodes `github.com`), this dynamically - * matches against any server URL — works for both GitHub and Forgejo. - */ -export function extractOwnerRepoFromUrl( - url: string, - serverUrl: string = getServerUrl(), -): { owner: string; repo: string } | null { - try { - const parsedServer = new URL(serverUrl); - const parsedUrl = new URL(url); - - // Host must match (handles both github.com and git.wxs.ro etc.) - if (parsedUrl.host !== parsedServer.host) { - return null; - } - - // Extract first two path segments as owner/repo - const segments = parsedUrl.pathname.split("/").filter(Boolean); - const owner = segments[0]; - const repo = segments[1]; - if (owner && repo) { - return { owner, repo }; - } - } catch { - // Invalid URLs — fall through to null - } - return null; -} - -// --------------------------------------------------------------------------- -// Final comment body (shown after Claude finishes) -// --------------------------------------------------------------------------- - -/** - * Forgejo version of updateCommentBody. - * - * Identical logic to src/github/operations/comment-logic.ts:updateCommentBody - * with one critical fix: uses extractOwnerRepoFromUrl() for dynamic server URL - * extraction instead of the hardcoded `github.com` regex. - */ -export function updateForgejoCommentBody(input: CommentUpdateInput): string { - const originalBody = input.currentBody; - const { - executionDetails, - jobUrl, - branchLink, - prLink, - actionFailed, - branchName, - triggerUsername, - errorDetails, - } = input; - - // Extract content from the original comment body - // First, remove the "Claude Code is working…" or "Claude Code is working..." message - const workingPattern = /Claude Code is working[…\.]{1,3}(?:\s*]*>)?/i; - let bodyContent = originalBody.replace(workingPattern, "").trim(); - - // Also remove the Unicode spinner used on Forgejo - bodyContent = bodyContent.replace(/⏳/g, "").trim(); - - // Check if there's a PR link in the content - let prLinkFromContent = ""; - - // Match the entire markdown link structure - const prLinkPattern = /\[Create .* PR\]\((.*)\)$/m; - const prLinkMatch = bodyContent.match(prLinkPattern); - - if (prLinkMatch && prLinkMatch[1]) { - const encodedUrl = ensureProperlyEncodedUrl(prLinkMatch[1]); - if (encodedUrl) { - prLinkFromContent = encodedUrl; - // Remove the PR link from the content - bodyContent = bodyContent.replace(prLinkMatch[0], "").trim(); - } - } - - // Calculate duration string if available - let durationStr = ""; - if (executionDetails?.duration_ms !== undefined) { - const totalSeconds = Math.round(executionDetails.duration_ms / 1000); - const minutes = Math.floor(totalSeconds / 60); - const seconds = totalSeconds % 60; - durationStr = minutes > 0 ? `${minutes}m ${seconds}s` : `${seconds}s`; - } - - // Build the header - let header = ""; - - if (actionFailed) { - header = "**Claude encountered an error"; - if (durationStr) { - header += ` after ${durationStr}`; - } - header += "**"; - } else { - // Get the username from triggerUsername or extract from content - const usernameMatch = bodyContent.match(/@([a-zA-Z0-9-]+)/); - const username = - triggerUsername || (usernameMatch ? usernameMatch[1] : "user"); - - header = `**Claude finished @${username}'s task`; - if (durationStr) { - header += ` in ${durationStr}`; - } - header += "**"; - } - - // Add links section - let links = ` —— [View job](${jobUrl})`; - - // Add branch name with link - if (branchName || branchLink) { - let finalBranchName = branchName; - let branchUrl = ""; - - if (branchLink) { - // Extract the branch URL from the link - const urlMatch = branchLink.match(/\((https?:\/\/.*)\)/); - if (urlMatch && urlMatch[1]) { - branchUrl = urlMatch[1]; - } - - // Extract branch name from link if not provided - if (!finalBranchName) { - const branchNameMatch = branchLink.match(/tree\/([^"'\)]+)/); - if (branchNameMatch) { - finalBranchName = branchNameMatch[1]; - } - } - } - - // If we don't have a URL yet but have a branch name, construct it - // FIX: Uses extractOwnerRepoFromUrl instead of hardcoded github.com regex - if (!branchUrl && finalBranchName) { - const extracted = extractOwnerRepoFromUrl(jobUrl); - if (extracted) { - branchUrl = `${getServerUrl()}/${extracted.owner}/${extracted.repo}/tree/${finalBranchName}`; - } - } - - if (finalBranchName && branchUrl) { - links += ` • [\`${finalBranchName}\`](${branchUrl})`; - } else if (finalBranchName) { - links += ` • \`${finalBranchName}\``; - } - } - - // Add PR link (either from content or provided) - const prUrl = - prLinkFromContent || (prLink ? prLink.match(/\(([^)]+)\)/)?.[1] : ""); - if (prUrl) { - links += ` • [Create PR ➔](${prUrl})`; - } - - // Build the new body with blank line between header and separator - let newBody = `${header}${links}`; - - // Add error details if available - if (actionFailed && errorDetails) { - newBody += `\n\n\`\`\`\n${errorDetails}\n\`\`\``; - } - - newBody += `\n\n---\n`; - - // Clean up the body content - // Remove any existing View job run, branch links from the bottom - bodyContent = bodyContent.replace(/\n?\[View job run\]\([^\)]+\)/g, ""); - bodyContent = bodyContent.replace(/\n?\[View branch\]\([^\)]+\)/g, ""); - - // Remove any existing duration info at the bottom - bodyContent = bodyContent.replace(/\n*---\n*Duration: [0-9]+m? [0-9]+s/g, ""); - - // Add the cleaned body content - newBody += bodyContent; - - return newBody.trim(); -} 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/api/config.ts b/src/github/api/config.ts index 9e533e5..026a859 100644 --- a/src/github/api/config.ts +++ b/src/github/api/config.ts @@ -2,3 +2,13 @@ export const GITHUB_API_URL = process.env.GITHUB_API_URL || "https://api.github.com"; export const GITHUB_SERVER_URL = process.env.GITHUB_SERVER_URL || "https://github.com"; + +/** + * Read the server URL at call time (not module load time). + * + * Use this instead of GITHUB_SERVER_URL in code paths where tests may + * override process.env.GITHUB_SERVER_URL after module import. + */ +export function getServerUrl(): string { + return process.env.GITHUB_SERVER_URL || "https://github.com"; +} 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/github/operations/comment-logic.ts b/src/github/operations/comment-logic.ts index 03b5d86..c490df5 100644 --- a/src/github/operations/comment-logic.ts +++ b/src/github/operations/comment-logic.ts @@ -1,4 +1,4 @@ -import { GITHUB_SERVER_URL } from "../api/config"; +import { getServerUrl } from "../api/config"; export type ExecutionDetails = { total_cost_usd?: number; @@ -66,6 +66,37 @@ export function ensureProperlyEncodedUrl(url: string): string | null { } } +/** + * Extract owner/repo from a URL using the configured server URL. + * + * Dynamically matches against any server URL — works for both GitHub and Forgejo. + */ +export function extractOwnerRepoFromUrl( + url: string, + serverUrl: string = getServerUrl(), +): { owner: string; repo: string } | null { + try { + const parsedServer = new URL(serverUrl); + const parsedUrl = new URL(url); + + // Host must match (handles both github.com and custom domains) + if (parsedUrl.host !== parsedServer.host) { + return null; + } + + // Extract first two path segments as owner/repo + const segments = parsedUrl.pathname.split("/").filter(Boolean); + const owner = segments[0]; + const repo = segments[1]; + if (owner && repo) { + return { owner, repo }; + } + } catch { + // Invalid URLs — fall through to null + } + return null; +} + export function updateCommentBody(input: CommentUpdateInput): string { const originalBody = input.currentBody; const { @@ -84,6 +115,9 @@ export function updateCommentBody(input: CommentUpdateInput): string { const workingPattern = /Claude Code is working[…\.]{1,3}(?:\s*]*>)?/i; let bodyContent = originalBody.replace(workingPattern, "").trim(); + // Also remove the Unicode spinner used on Forgejo (safe no-op on GitHub) + bodyContent = bodyContent.replace(/⏳/g, "").trim(); + // Check if there's a PR link in the content let prLinkFromContent = ""; @@ -141,7 +175,7 @@ export function updateCommentBody(input: CommentUpdateInput): string { if (branchLink) { // Extract the branch URL from the link - const urlMatch = branchLink.match(/\((https:\/\/.*)\)/); + const urlMatch = branchLink.match(/\((https?:\/\/.*)\)/); if (urlMatch && urlMatch[1]) { branchUrl = urlMatch[1]; } @@ -156,11 +190,11 @@ export function updateCommentBody(input: CommentUpdateInput): string { } // If we don't have a URL yet but have a branch name, construct it + // Uses dynamic URL extraction that works for both GitHub and Forgejo if (!branchUrl && finalBranchName) { - // Extract owner/repo from jobUrl - const repoMatch = jobUrl.match(/github\.com\/([^\/]+)\/([^\/]+)\//); - if (repoMatch) { - branchUrl = `${GITHUB_SERVER_URL}/${repoMatch[1]}/${repoMatch[2]}/tree/${finalBranchName}`; + const extracted = extractOwnerRepoFromUrl(jobUrl); + if (extracted) { + branchUrl = `${getServerUrl()}/${extracted.owner}/${extracted.repo}/tree/${finalBranchName}`; } } diff --git a/src/github/operations/comments/create-initial.ts b/src/github/operations/comments/create-initial.ts index e9d044b..d53e5da 100644 --- a/src/github/operations/comments/create-initial.ts +++ b/src/github/operations/comments/create-initial.ts @@ -56,9 +56,14 @@ export async function createInitialComment( const botNameMatch = comment.user?.type === "Bot" && comment.user?.login.toLowerCase().includes("claude"); + // On Forgejo, the bot is a regular user account identified by bot_name input + const forgejoBotMatch = + platform === "forgejo" && + context.inputs.botName && + comment.user?.login === context.inputs.botName; const bodyMatch = comment.body === initialBody; - return idMatch || botNameMatch || bodyMatch; + return idMatch || botNameMatch || forgejoBotMatch || bodyMatch; }); if (existingComment) { response = await octokit.rest.issues.updateComment({ diff --git a/src/github/operations/comments/update-with-branch.ts b/src/github/operations/comments/update-with-branch.ts index 6059f72..62eadcd 100644 --- a/src/github/operations/comments/update-with-branch.ts +++ b/src/github/operations/comments/update-with-branch.ts @@ -14,6 +14,7 @@ import { createForgejoJobRunLink, createForgejoBranchLink, createForgejoCommentBody, + getForgejoRunIndex, } from "../../../forgejo/comment-logic"; import { detectPlatform } from "../../../platform/detector"; import { type Octokits } from "../../api/client"; @@ -34,7 +35,7 @@ export async function updateTrackingComment( const jobRunLink = platform === "forgejo" - ? createForgejoJobRunLink(owner, repo, context.runId) + ? createForgejoJobRunLink(owner, repo, getForgejoRunIndex()) : createJobRunLink(owner, repo, context.runId); // Add branch link for issues (not PRs) diff --git a/src/github/validation/actor.ts b/src/github/validation/actor.ts index ed7f5b4..763d704 100644 --- a/src/github/validation/actor.ts +++ b/src/github/validation/actor.ts @@ -14,10 +14,20 @@ export async function checkHumanActor( githubContext: GitHubContext, ) { // Forgejo doesn't have GitHub-style bot/app accounts, and the user API - // requires read:user scope which may not be available. Skip entirely. + // requires read:user scope which may not be available. Skip the bot-type + // check but still guard against self-triggering (infinite loop prevention). if (detectPlatform() === "forgejo") { + const botName = githubContext.inputs.botName.replace(/\[bot\]$/, ""); + if ( + githubContext.actor === botName || + githubContext.actor === githubContext.inputs.botName + ) { + throw new Error( + `Blocking self-trigger on Forgejo: actor '${githubContext.actor}' matches bot_name '${githubContext.inputs.botName}'. This prevents infinite loops.`, + ); + } console.log( - `Skipping human actor check on Forgejo for: ${githubContext.actor}`, + `Forgejo actor check passed for: ${githubContext.actor} (bot_name: ${githubContext.inputs.botName})`, ); return; } diff --git a/src/github/validation/permissions.ts b/src/github/validation/permissions.ts index 59a6742..b0666b2 100644 --- a/src/github/validation/permissions.ts +++ b/src/github/validation/permissions.ts @@ -83,16 +83,51 @@ export async function checkWritePermissions( } /** - * Forgejo permission check using the repo endpoint. - * GET /repos/{owner}/{repo} returns a `permissions` object for the - * authenticated token with { admin, push, pull } booleans. We check - * that the token has push (write) access. + * Forgejo permission check using the collaborator permission endpoint. + * + * Primary: GET /repos/{owner}/{repo}/collaborators/{username}/permission + * Returns { "permission": "admin"|"write"|"read"|"none" } — checks the + * actor's permission, not the token's. + * + * Fallback: GET /repos/{owner}/{repo} — checks the token's permission + * (less precise but works if the collaborator endpoint returns 403). */ async function checkForgejoPermissions( octokit: Octokit, repository: { owner: string; repo: string }, actor: string, ): Promise { + // Try actor-level permission check first (requires repo admin or token with + // sufficient scope to read collaborators). + try { + const response = await octokit.repos.getCollaboratorPermissionLevel({ + owner: repository.owner, + repo: repository.repo, + username: actor, + }); + + const permissionLevel = response.data.permission; + core.info(`Forgejo actor '${actor}' permission level: ${permissionLevel}`); + + if (permissionLevel === "admin" || permissionLevel === "write") { + return true; + } + + core.warning( + `Forgejo actor '${actor}' has insufficient permissions: ${permissionLevel}`, + ); + return false; + } catch (error) { + // Forgejo may return 403 if the token lacks admin scope. Fall back to + // token-level check and log a warning about the reduced security. + core.warning( + `Forgejo collaborator permission check failed for '${actor}' (${error instanceof Error ? error.message : String(error)}). ` + + `Falling back to token-level permission check. ` + + `For actor-level checks, use a token with read:organization scope.`, + ); + } + + // Fallback: check the token's own permission on the repo const response = await octokit.repos.get({ owner: repository.owner, repo: repository.repo, @@ -111,7 +146,7 @@ async function checkForgejoPermissions( if (permissions.admin || permissions.push) { core.info( - `Forgejo token has write access (admin=${permissions.admin}, push=${permissions.push}) for actor: ${actor}`, + `Forgejo token has write access (admin=${permissions.admin}, push=${permissions.push}) for actor: ${actor} (token-level fallback)`, ); return true; } diff --git a/src/github/validation/trigger.ts b/src/github/validation/trigger.ts index 031feb8..ac4782d 100644 --- a/src/github/validation/trigger.ts +++ b/src/github/validation/trigger.ts @@ -145,18 +145,20 @@ export function checkContainsTrigger(context: ParsedGitHubContext): boolean { return true; } - // On Forgejo, if no body was found in the payload at all, still trigger — - // the actual comment data will be fetched via REST in the data fetcher. - // This handles edge cases where Forgejo strips body from the webhook payload. + // On Forgejo, pull_request_review_comment payloads may arrive with an + // empty body. Rather than triggering unconditionally (which bypasses the + // trigger phrase check), we skip. The user can re-trigger with a comment + // that includes the trigger phrase. This prevents Claude from responding + // to comments not directed at it. if ( isPullRequestReviewCommentEvent(context) && detectPlatform() === "forgejo" && !commentBody ) { console.log( - "Forgejo pull_request_review_comment: no body in payload, triggering (data fetched via REST)", + "Forgejo pull_request_review_comment: no body in payload, skipping (cannot verify trigger phrase)", ); - return true; + return false; } } 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}`); } } diff --git a/src/platform/detector.ts b/src/platform/detector.ts index a40a30f..fd6af93 100644 --- a/src/platform/detector.ts +++ b/src/platform/detector.ts @@ -6,24 +6,57 @@ * 2. FORGEJO env var (set by Forgejo runner v7.0+) * 3. GITHUB_SERVER_URL heuristic (non-github.com = Forgejo) * 4. Default to GitHub + * + * The result is memoized after first detection — platform doesn't change + * at runtime and this avoids redundant env reads across 12+ call sites. */ export type Platform = "github" | "forgejo"; +let _cachedPlatform: Platform | undefined; + export function detectPlatform(): Platform { + if (_cachedPlatform !== undefined) return _cachedPlatform; + + let result: Platform = "github"; + // 1. Explicit platform from action.yml detect step or user input const platformEnv = process.env.PLATFORM; - if (platformEnv === "forgejo") return "forgejo"; - if (platformEnv === "github") return "github"; - + if (platformEnv === "forgejo") { + // Cross-validate: prevent forcing Forgejo mode on GitHub.com, which would + // skip actor validation and use a weaker permission model. + const serverUrl = process.env.GITHUB_SERVER_URL || ""; + if (serverUrl.includes("github.com")) { + console.warn( + `Platform override 'forgejo' ignored: GITHUB_SERVER_URL (${serverUrl}) indicates GitHub.com. ` + + `Falling through to standard detection.`, + ); + } else { + result = "forgejo"; + } + } else if (platformEnv === "github") { + result = "github"; + } // 2. FORGEJO env var (set by Forgejo runner v7.0+) - if (process.env.FORGEJO === "true") return "forgejo"; - + else if (process.env.FORGEJO === "true") { + result = "forgejo"; + } // 3. GITHUB_SERVER_URL heuristic — if not github.com, likely Forgejo/Gitea - const serverUrl = process.env.GITHUB_SERVER_URL || ""; - if (serverUrl && !serverUrl.includes("github.com")) { - return "forgejo"; + else { + const serverUrl = process.env.GITHUB_SERVER_URL || ""; + if (serverUrl && !serverUrl.includes("github.com")) { + result = "forgejo"; + } } - return "github"; + _cachedPlatform = result; + return result; +} + +/** + * Reset the memoized platform detection result. + * Only needed in tests that change platform-related env vars between cases. + */ +export function _resetPlatformCache(): void { + _cachedPlatform = undefined; } diff --git a/test/COVERAGE-NOTES.md b/test/COVERAGE-NOTES.md new file mode 100644 index 0000000..249d880 --- /dev/null +++ b/test/COVERAGE-NOTES.md @@ -0,0 +1,51 @@ +# Test Coverage Notes — Forgejo Platform Support + +## Well Covered (unit tested) + +| Module | Tests | File | +| -------------------------------- | --------------- | ------------------------------------ | +| `src/forgejo/token.ts` | 8 tests | `test/forgejo-token.test.ts` | +| `src/platform/detector.ts` | 17 tests | `test/platform-detector.test.ts` | +| `src/forgejo/comment-logic.ts` | 30+ tests | `test/forgejo-comment-logic.test.ts` | +| `src/forgejo/data/fetcher.ts` | 50+ tests | `test/forgejo-data-fetcher.test.ts` | +| `src/github/validation/actor.ts` | 6 Forgejo tests | `test/actor.test.ts` | +| `src/mcp/install-mcp-server.ts` | 8 Forgejo tests | `test/install-mcp-server.test.ts` | + +## Added in M4 + +| Module | Tests | File | +| ------------------------------------------------------ | -------- | -------------------------------------- | +| `src/github/operations/comments/create-initial.ts` | 15 tests | `test/create-initial-comment.test.ts` | +| `src/github/operations/comments/update-with-branch.ts` | 11 tests | `test/update-tracking-comment.test.ts` | +| `src/github/operations/branch.ts` (API divergence) | 2 tests | `test/setup-branch-forgejo.test.ts` | + +## Accepted Gaps (orchestrators) + +### `src/modes/tag/index.ts` — `prepareTagMode()` + +**Why not unit tested**: Calls 8+ modules (checkHumanActor, createInitialComment, +fetchForgejoData/fetchGitHubData, setupBranch, configureGitAuth, createPrompt, +prepareMcpConfig). Each is individually unit tested. + +**Risk**: Low. The Forgejo branch is a simple `if (platform === "forgejo")` +choosing `fetchForgejoData` vs `fetchGitHubData`. Both are thoroughly tested. + +**Coverage path**: Integration tests in the separate `install-test` repo. + +### `src/entrypoints/run.ts` — main orchestrator + +**Why not unit tested**: Token setup path is a trivial conditional (1 line). Testing +requires mocking the entire execution environment (GitHub Actions context, CLI +installation, process lifecycle). + +**Risk**: Very low. The Forgejo token path calls `setupForgejoToken()` which +has 8 dedicated tests. + +**Coverage path**: Integration tests in the separate `install-test` repo. + +## Known Issue: CLAUDE_APP_BOT_ID Discrepancy + +`src/github/operations/comments/create-initial.ts` (line 23) defines a local +`CLAUDE_APP_BOT_ID = 209825114`, while `src/github/constants.ts` exports +`CLAUDE_APP_BOT_ID = 41898282`. These are different values with the same name. +This should be investigated in M5 (code quality review). diff --git a/test/actor.test.ts b/test/actor.test.ts index 946347a..79e75d6 100644 --- a/test/actor.test.ts +++ b/test/actor.test.ts @@ -2,6 +2,7 @@ import { describe, test, expect, beforeEach, afterEach } from "bun:test"; import { checkHumanActor } from "../src/github/validation/actor"; +import { _resetPlatformCache } from "../src/platform/detector"; import type { Octokit } from "@octokit/rest"; import { createMockContext } from "./mockContext"; @@ -116,9 +117,12 @@ describe("checkHumanActor", () => { let savedServerUrl: string | undefined; beforeEach(() => { + _resetPlatformCache(); savedPlatform = process.env.PLATFORM; savedServerUrl = process.env.GITHUB_SERVER_URL; process.env.PLATFORM = "forgejo"; + // Ensure cross-validation doesn't reject PLATFORM=forgejo + process.env.GITHUB_SERVER_URL = "https://git.example.com"; }); afterEach(() => { @@ -127,9 +131,10 @@ describe("checkHumanActor", () => { if (savedServerUrl !== undefined) process.env.GITHUB_SERVER_URL = savedServerUrl; else delete process.env.GITHUB_SERVER_URL; + _resetPlatformCache(); }); - test("should skip actor check entirely on Forgejo", async () => { + test("should skip GitHub API actor check on Forgejo for regular users", async () => { let apiCalled = false; const mockOctokit = { users: { @@ -148,7 +153,7 @@ describe("checkHumanActor", () => { expect(apiCalled).toBe(false); }); - test("should skip actor check for bot-like names on Forgejo", async () => { + test("should skip actor check for bot-like names that don't match bot_name", async () => { let apiCalled = false; const mockOctokit = { users: { @@ -167,5 +172,38 @@ describe("checkHumanActor", () => { ).resolves.toBeUndefined(); expect(apiCalled).toBe(false); }); + + test("should block self-trigger when actor matches bot_name (with [bot] suffix)", async () => { + const mockOctokit = createMockOctokit("User"); + const context = createMockContext(); + // Default botName is "claude[bot]" from CLAUDE_BOT_LOGIN + context.actor = "claude[bot]"; + + await expect(checkHumanActor(mockOctokit, context)).rejects.toThrow( + "Blocking self-trigger on Forgejo", + ); + }); + + test("should block self-trigger when actor matches bot_name without [bot] suffix", async () => { + const mockOctokit = createMockOctokit("User"); + const context = createMockContext(); + context.inputs.botName = "claude[bot]"; + context.actor = "claude"; + + await expect(checkHumanActor(mockOctokit, context)).rejects.toThrow( + "Blocking self-trigger on Forgejo", + ); + }); + + test("should block self-trigger with custom bot_name", async () => { + const mockOctokit = createMockOctokit("User"); + const context = createMockContext(); + context.inputs.botName = "my-assistant"; + context.actor = "my-assistant"; + + await expect(checkHumanActor(mockOctokit, context)).rejects.toThrow( + "Blocking self-trigger on Forgejo", + ); + }); }); }); diff --git a/test/create-initial-comment.test.ts b/test/create-initial-comment.test.ts new file mode 100644 index 0000000..c1386d2 --- /dev/null +++ b/test/create-initial-comment.test.ts @@ -0,0 +1,527 @@ +#!/usr/bin/env bun + +import { + describe, + test, + expect, + beforeEach, + afterEach, + spyOn, + type Mock, +} from "bun:test"; +import { tmpdir } from "os"; +import { join } from "path"; +import { readFileSync, unlinkSync, writeFileSync } from "fs"; +import { createInitialComment } from "../src/github/operations/comments/create-initial"; +import { _resetPlatformCache } from "../src/platform/detector"; +import { createMockContext } from "./mockContext"; +import type { Octokit } from "@octokit/rest"; + +// Note: create-initial.ts defines its own CLAUDE_APP_BOT_ID = 209825114 (line 23), +// which differs from src/github/constants.ts CLAUDE_APP_BOT_ID = 41898282. +// Tests use 209825114 to match the actual code in create-initial.ts. +const LOCAL_CLAUDE_APP_BOT_ID = 209825114; + +function createMockOctokit(overrides: Record = {}) { + // createInitialComment receives an Octokit instance and accesses octokit.rest.issues.* + return { + rest: { + issues: { + listComments: overrides.listComments ?? (async () => ({ data: [] })), + createComment: + overrides.createComment ?? (async () => ({ data: { id: 100 } })), + updateComment: + overrides.updateComment ?? (async () => ({ data: { id: 200 } })), + }, + pulls: { + createReplyForReviewComment: + overrides.createReplyForReviewComment ?? + (async () => ({ data: { id: 300 } })), + }, + }, + } as unknown as Octokit; +} + +describe("createInitialComment", () => { + let savedPlatform: string | undefined; + let savedServerUrl: string | undefined; + let savedRunNumber: string | undefined; + let savedRunId: string | undefined; + let savedGithubOutput: string | undefined; + let tempOutputFile: string; + let consoleLogSpy: Mock<(...args: any[]) => void>; + let consoleErrorSpy: Mock<(...args: any[]) => void>; + + beforeEach(() => { + _resetPlatformCache(); + savedPlatform = process.env.PLATFORM; + savedServerUrl = process.env.GITHUB_SERVER_URL; + savedRunNumber = process.env.GITHUB_RUN_NUMBER; + savedRunId = process.env.GITHUB_RUN_ID; + savedGithubOutput = process.env.GITHUB_OUTPUT; + + // Create a real temp file for GITHUB_OUTPUT + tempOutputFile = join( + tmpdir(), + `test-gh-output-${Date.now()}-${Math.random().toString(36).slice(2)}`, + ); + writeFileSync(tempOutputFile, ""); + process.env.GITHUB_OUTPUT = tempOutputFile; + + // Default to GitHub platform + delete process.env.PLATFORM; + process.env.GITHUB_SERVER_URL = "https://github.com"; + process.env.GITHUB_RUN_ID = "9876543"; + + consoleLogSpy = spyOn(console, "log").mockImplementation(() => {}); + consoleErrorSpy = spyOn(console, "error").mockImplementation(() => {}); + }); + + afterEach(() => { + if (savedPlatform !== undefined) process.env.PLATFORM = savedPlatform; + else delete process.env.PLATFORM; + if (savedServerUrl !== undefined) + process.env.GITHUB_SERVER_URL = savedServerUrl; + else delete process.env.GITHUB_SERVER_URL; + if (savedRunNumber !== undefined) + process.env.GITHUB_RUN_NUMBER = savedRunNumber; + else delete process.env.GITHUB_RUN_NUMBER; + if (savedRunId !== undefined) process.env.GITHUB_RUN_ID = savedRunId; + else delete process.env.GITHUB_RUN_ID; + if (savedGithubOutput !== undefined) + process.env.GITHUB_OUTPUT = savedGithubOutput; + else delete process.env.GITHUB_OUTPUT; + + try { + unlinkSync(tempOutputFile); + } catch {} + + consoleLogSpy.mockRestore(); + consoleErrorSpy.mockRestore(); + _resetPlatformCache(); + }); + + describe("GitHub platform — basic comment creation", () => { + test("creates issue comment for issue_comment event", async () => { + const createComment = async () => ({ data: { id: 42 } }); + const createCommentSpy = spyOn({ createComment }, "createComment"); + const mockOctokit = createMockOctokit({ + createComment: createCommentSpy, + }); + + const context = createMockContext({ entityNumber: 10 }); + const result = await createInitialComment(mockOctokit, context); + + expect(result.id).toBe(42); + expect(createCommentSpy).toHaveBeenCalled(); + const callArgs = (createCommentSpy.mock.calls as any[][])[0]![0]; + expect(callArgs.owner).toBe("test-owner"); + expect(callArgs.repo).toBe("test-repo"); + expect(callArgs.issue_number).toBe(10); + // GitHub body uses CDN spinner image + expect(callArgs.body).toContain("user-attachments/assets"); + expect(callArgs.body).toContain("Claude Code is working"); + }); + + test("writes comment ID to GITHUB_OUTPUT file", async () => { + const mockOctokit = createMockOctokit({ + createComment: async () => ({ data: { id: 777 } }), + }); + const context = createMockContext(); + + await createInitialComment(mockOctokit, context); + + const output = readFileSync(tempOutputFile, "utf-8"); + expect(output).toContain("claude_comment_id=777"); + }); + + test("uses GitHub CDN spinner and actions/runs URL in body", async () => { + let capturedBody = ""; + const mockOctokit = createMockOctokit({ + createComment: async (args: any) => { + capturedBody = args.body; + return { data: { id: 1 } }; + }, + }); + const context = createMockContext({ runId: "12345" }); + + await createInitialComment(mockOctokit, context); + + // GitHub spinner is a CDN image + expect(capturedBody).toContain("user-attachments/assets"); + expect(capturedBody).not.toContain("⏳"); + // GitHub URL uses GITHUB_SERVER_URL + actions/runs/ + expect(capturedBody).toContain( + "https://github.com/test-owner/test-repo/actions/runs/12345", + ); + }); + }); + + describe("GitHub platform — sticky comment on PR", () => { + test("creates new comment when no existing comment matches", async () => { + const createComment = async () => ({ data: { id: 50 } }); + const updateComment = async () => ({ data: { id: 99 } }); + const createSpy = spyOn({ createComment }, "createComment"); + const updateSpy = spyOn({ updateComment }, "updateComment"); + const mockOctokit = createMockOctokit({ + listComments: async () => ({ data: [] }), + createComment: createSpy, + updateComment: updateSpy, + }); + + const context = createMockContext({ + eventName: "pull_request", + isPR: true, + entityNumber: 5, + inputs: { useStickyComment: true }, + payload: { + action: "opened", + number: 5, + pull_request: { number: 5 }, + } as any, + }); + + const result = await createInitialComment(mockOctokit, context); + expect(result.id).toBe(50); + expect(createSpy).toHaveBeenCalled(); + expect(updateSpy).not.toHaveBeenCalled(); + }); + + test("updates existing comment matched by CLAUDE_APP_BOT_ID", async () => { + const updateComment = async () => ({ data: { id: 200 } }); + const updateSpy = spyOn({ updateComment }, "updateComment"); + const createComment = async () => ({ data: { id: 100 } }); + const createSpy = spyOn({ createComment }, "createComment"); + const mockOctokit = createMockOctokit({ + listComments: async () => ({ + data: [ + { + id: 200, + user: { + id: LOCAL_CLAUDE_APP_BOT_ID, + type: "User", + login: "someone", + }, + body: "old body", + }, + ], + }), + createComment: createSpy, + updateComment: updateSpy, + }); + + const context = createMockContext({ + eventName: "pull_request", + isPR: true, + entityNumber: 5, + inputs: { useStickyComment: true }, + payload: { + action: "opened", + number: 5, + pull_request: { number: 5 }, + } as any, + }); + + const result = await createInitialComment(mockOctokit, context); + expect(result.id).toBe(200); + expect(updateSpy).toHaveBeenCalled(); + expect(createSpy).not.toHaveBeenCalled(); + }); + + test("updates existing comment matched by Bot type with 'claude' in login", async () => { + const updateComment = async () => ({ data: { id: 201 } }); + const updateSpy = spyOn({ updateComment }, "updateComment"); + const mockOctokit = createMockOctokit({ + listComments: async () => ({ + data: [ + { + id: 201, + user: { id: 999, type: "Bot", login: "claude[bot]" }, + body: "old", + }, + ], + }), + updateComment: updateSpy, + }); + + const context = createMockContext({ + eventName: "pull_request", + isPR: true, + inputs: { useStickyComment: true }, + payload: { + action: "opened", + number: 1, + pull_request: { number: 1 }, + } as any, + }); + + const result = await createInitialComment(mockOctokit, context); + expect(result.id).toBe(201); + expect(updateSpy).toHaveBeenCalled(); + }); + + test("updates existing comment matched by exact body content", async () => { + // To test body matching, we need the mock comment's body to exactly equal + // the generated initialBody. Import the GitHub comment builder to construct + // the matching body with the same runId as the mock context. + const { createCommentBody, createJobRunLink } = await import( + "../src/github/operations/comments/common" + ); + const expectedBody = createCommentBody( + createJobRunLink("test-owner", "test-repo", "1234567890"), + ); + + const updateComment = async () => ({ data: { id: 202 } }); + const updateSpy = spyOn({ updateComment }, "updateComment"); + const mockOctokit = createMockOctokit({ + listComments: async () => ({ + data: [ + { + id: 202, + user: { id: 1, type: "User", login: "random-user" }, + body: expectedBody, + }, + ], + }), + updateComment: updateSpy, + }); + + const context = createMockContext({ + eventName: "pull_request", + isPR: true, + inputs: { useStickyComment: true }, + payload: { + action: "opened", + number: 1, + pull_request: { number: 1 }, + } as any, + }); + + const result = await createInitialComment(mockOctokit, context); + expect(result.id).toBe(202); + expect(updateSpy).toHaveBeenCalled(); + }); + }); + + describe("GitHub platform — PR review comment reply", () => { + test("creates reply via createReplyForReviewComment when payload has comment.id", async () => { + const createReply = async () => ({ data: { id: 301 } }); + const replySpy = spyOn({ createReply }, "createReply"); + const mockOctokit = createMockOctokit({ + createReplyForReviewComment: replySpy, + }); + + const context = createMockContext({ + eventName: "pull_request_review_comment", + isPR: true, + entityNumber: 10, + payload: { + action: "created", + comment: { id: 555 }, + pull_request: { number: 10 }, + } as any, + }); + + const result = await createInitialComment(mockOctokit, context); + expect(result.id).toBe(301); + expect(replySpy).toHaveBeenCalled(); + const callArgs = (replySpy.mock.calls as any[][])[0]![0]; + expect(callArgs.comment_id).toBe(555); + expect(callArgs.pull_number).toBe(10); + }); + + test("falls back to createComment when payload lacks comment.id", async () => { + const createComment = async () => ({ data: { id: 102 } }); + const createSpy = spyOn({ createComment }, "createComment"); + const createReply = async () => ({ data: { id: 301 } }); + const replySpy = spyOn({ createReply }, "createReply"); + const mockOctokit = createMockOctokit({ + createComment: createSpy, + createReplyForReviewComment: replySpy, + }); + + const context = createMockContext({ + eventName: "pull_request_review_comment", + isPR: true, + entityNumber: 10, + // No comment.id in payload (Forgejo behavior) + payload: { + action: "created", + pull_request: { number: 10 }, + } as any, + }); + + const result = await createInitialComment(mockOctokit, context); + expect(result.id).toBe(102); + expect(createSpy).toHaveBeenCalled(); + expect(replySpy).not.toHaveBeenCalled(); + }); + }); + + describe("Forgejo platform", () => { + beforeEach(() => { + _resetPlatformCache(); + process.env.PLATFORM = "forgejo"; + process.env.GITHUB_SERVER_URL = "https://git.example.com"; + process.env.GITHUB_RUN_NUMBER = "42"; + }); + + test("uses Unicode spinner and Forgejo URL format in body", async () => { + let capturedBody = ""; + const mockOctokit = createMockOctokit({ + createComment: async (args: any) => { + capturedBody = args.body; + return { data: { id: 1 } }; + }, + }); + const context = createMockContext(); + + await createInitialComment(mockOctokit, context); + + // Forgejo uses Unicode hourglass, not CDN image + expect(capturedBody).toContain("⏳"); + expect(capturedBody).not.toContain("user-attachments/assets"); + // Forgejo URL uses GITHUB_RUN_NUMBER + expect(capturedBody).toContain( + "https://git.example.com/test-owner/test-repo/actions/runs/42", + ); + }); + + test("sticky comment matches by botName input on Forgejo", async () => { + const updateComment = async () => ({ data: { id: 203 } }); + const updateSpy = spyOn({ updateComment }, "updateComment"); + const mockOctokit = createMockOctokit({ + listComments: async () => ({ + data: [ + { + id: 203, + user: { id: 999, type: "User", login: "claude" }, + body: "some old body", + }, + ], + }), + updateComment: updateSpy, + }); + + const context = createMockContext({ + eventName: "pull_request", + isPR: true, + inputs: { useStickyComment: true, botName: "claude" }, + payload: { + action: "opened", + number: 1, + pull_request: { number: 1 }, + } as any, + }); + + const result = await createInitialComment(mockOctokit, context); + expect(result.id).toBe(203); + expect(updateSpy).toHaveBeenCalled(); + }); + + test("review comment event falls back to createComment (no comment.id in Forgejo payload)", async () => { + const createComment = async () => ({ data: { id: 103 } }); + const createSpy = spyOn({ createComment }, "createComment"); + const mockOctokit = createMockOctokit({ + createComment: createSpy, + }); + + const context = createMockContext({ + eventName: "pull_request_review_comment", + isPR: true, + entityNumber: 7, + payload: { + action: "created", + pull_request: { number: 7 }, + // Forgejo payload lacks top-level comment object + } as any, + }); + + const result = await createInitialComment(mockOctokit, context); + expect(result.id).toBe(103); + expect(createSpy).toHaveBeenCalled(); + }); + }); + + describe("error handling", () => { + test("falls back to issues.createComment when primary path throws", async () => { + let fallbackCalled = false; + const mockOctokit = createMockOctokit({ + listComments: async () => { + throw new Error("API error"); + }, + createComment: async () => { + fallbackCalled = true; + return { data: { id: 999 } }; + }, + }); + + const context = createMockContext({ + eventName: "pull_request", + isPR: true, + inputs: { useStickyComment: true }, + payload: { + action: "opened", + number: 1, + pull_request: { number: 1 }, + } as any, + }); + + const result = await createInitialComment(mockOctokit, context); + expect(result.id).toBe(999); + expect(fallbackCalled).toBe(true); + expect(consoleErrorSpy).toHaveBeenCalled(); + }); + + test("throws when both primary and fallback fail", async () => { + const mockOctokit = createMockOctokit({ + listComments: async () => { + throw new Error("Primary error"); + }, + createComment: async () => { + throw new Error("Fallback error"); + }, + }); + + const context = createMockContext({ + eventName: "pull_request", + isPR: true, + inputs: { useStickyComment: true }, + payload: { + action: "opened", + number: 1, + pull_request: { number: 1 }, + } as any, + }); + + await expect(createInitialComment(mockOctokit, context)).rejects.toThrow( + "Fallback error", + ); + }); + + test("writes comment ID from fallback to GITHUB_OUTPUT", async () => { + const mockOctokit = createMockOctokit({ + listComments: async () => { + throw new Error("API error"); + }, + createComment: async () => ({ data: { id: 888 } }), + }); + + const context = createMockContext({ + eventName: "pull_request", + isPR: true, + inputs: { useStickyComment: true }, + payload: { + action: "opened", + number: 1, + pull_request: { number: 1 }, + } as any, + }); + + await createInitialComment(mockOctokit, context); + + const output = readFileSync(tempOutputFile, "utf-8"); + expect(output).toContain("claude_comment_id=888"); + }); + }); +}); diff --git a/test/forgejo-comment-logic.test.ts b/test/forgejo-comment-logic.test.ts index 093bfe8..8466b5f 100644 --- a/test/forgejo-comment-logic.test.ts +++ b/test/forgejo-comment-logic.test.ts @@ -6,10 +6,12 @@ import { createForgejoBranchLink, createForgejoPrCompareUrl, createForgejoCommentBody, - extractOwnerRepoFromUrl, - updateForgejoCommentBody, - type CommentUpdateInput, } from "../src/forgejo/comment-logic"; +import { + extractOwnerRepoFromUrl, + updateCommentBody, + type CommentUpdateInput, +} from "../src/github/operations/comment-logic"; // --------------------------------------------------------------------------- // Test helpers @@ -307,10 +309,10 @@ describe("extractOwnerRepoFromUrl", () => { }); // --------------------------------------------------------------------------- -// updateForgejoCommentBody +// updateCommentBody // --------------------------------------------------------------------------- -describe("updateForgejoCommentBody", () => { +describe("updateCommentBody", () => { const forgejoBaseInput: CommentUpdateInput = { currentBody: "Initial comment body", actionFailed: false, @@ -332,7 +334,7 @@ describe("updateForgejoCommentBody", () => { triggerUsername: "testuser", }; - const result = updateForgejoCommentBody(input); + const result = updateCommentBody(input); expect(result).toContain("**Claude finished @testuser's task**"); expect(result).not.toContain("Claude Code is working"); expect(result).not.toContain(" { triggerUsername: "testuser", }; - const result = updateForgejoCommentBody(input); + const result = updateCommentBody(input); expect(result).toContain("**Claude finished @testuser's task**"); expect(result).not.toContain("Claude Code is working"); expect(result).not.toContain("⏳"); @@ -359,7 +361,7 @@ describe("updateForgejoCommentBody", () => { triggerUsername: "trigger-user", }; - const result = updateForgejoCommentBody(input); + const result = updateCommentBody(input); expect(result).toContain( "**Claude finished @trigger-user's task in 1m 14s**", ); @@ -373,7 +375,7 @@ describe("updateForgejoCommentBody", () => { executionDetails: { duration_ms: 45000 }, }; - const result = updateForgejoCommentBody(input); + const result = updateCommentBody(input); expect(result).toContain("**Claude encountered an error after 45s**"); }); @@ -386,7 +388,7 @@ describe("updateForgejoCommentBody", () => { errorDetails: "Failed to fetch issue data", }; - const result = updateForgejoCommentBody(input); + const result = updateCommentBody(input); expect(result).toContain("**Claude encountered an error after 45s**"); expect(result).toContain("[View job]"); expect(result).toContain("```\nFailed to fetch issue data\n```"); @@ -400,7 +402,7 @@ describe("updateForgejoCommentBody", () => { currentBody: "Some comment", }; - const result = updateForgejoCommentBody(input); + const result = updateCommentBody(input); expect(result).toContain(`—— [View job](${forgejoBaseInput.jobUrl})`); }); }); @@ -412,7 +414,7 @@ describe("updateForgejoCommentBody", () => { branchName: "claude/issue-5-20240101-1200", }; - const result = updateForgejoCommentBody(input); + const result = updateCommentBody(input); // This is the critical fix: GitHub's version uses a hardcoded github.com regex // which fails for Forgejo URLs. Our version uses extractOwnerRepoFromUrl. expect(result).toContain( @@ -427,7 +429,7 @@ describe("updateForgejoCommentBody", () => { branchName: "feature-branch", }; - const result = updateForgejoCommentBody(input); + const result = updateCommentBody(input); expect(result).toContain("• `feature-branch`"); expect(result).not.toContain("[`feature-branch`]"); }); @@ -440,7 +442,7 @@ describe("updateForgejoCommentBody", () => { "\n[View branch](https://git.wxs.ro/wxs/myrepo/tree/feature-branch)", }; - const result = updateForgejoCommentBody(input); + const result = updateCommentBody(input); expect(result).toContain( "• [`feature-branch`](https://git.wxs.ro/wxs/myrepo/tree/feature-branch)", ); @@ -453,7 +455,7 @@ describe("updateForgejoCommentBody", () => { "\n[View branch](https://git.wxs.ro/wxs/myrepo/tree/auto-detected-branch)", }; - const result = updateForgejoCommentBody(input); + const result = updateCommentBody(input); expect(result).toContain( "• [`auto-detected-branch`](https://git.wxs.ro/wxs/myrepo/tree/auto-detected-branch)", ); @@ -468,7 +470,7 @@ describe("updateForgejoCommentBody", () => { "\n[Create a PR](https://git.wxs.ro/wxs/myrepo/compare/main...feature)", }; - const result = updateForgejoCommentBody(input); + const result = updateCommentBody(input); expect(result).toContain( "• [Create PR ➔](https://git.wxs.ro/wxs/myrepo/compare/main...feature)", ); @@ -481,7 +483,7 @@ describe("updateForgejoCommentBody", () => { "Some comment with [Create a PR](https://git.wxs.ro/wxs/myrepo/compare/main...fix)", }; - const result = updateForgejoCommentBody(input); + const result = updateCommentBody(input); expect(result).toContain( "• [Create PR ➔](https://git.wxs.ro/wxs/myrepo/compare/main...fix)", ); @@ -496,7 +498,7 @@ describe("updateForgejoCommentBody", () => { currentBody: `Done!\n\n[Create a PR](${forgejoUrl})`, }; - const result = updateForgejoCommentBody(input); + const result = updateCommentBody(input); expect(result).toContain(`• [Create PR ➔](${forgejoUrl})`); }); }); @@ -509,7 +511,7 @@ describe("updateForgejoCommentBody", () => { triggerUsername: "testuser", }; - const result = updateForgejoCommentBody(input); + const result = updateCommentBody(input); expect(result).toContain( "**Claude finished @testuser's task in 1m 15s**", ); @@ -522,7 +524,7 @@ describe("updateForgejoCommentBody", () => { triggerUsername: "testuser", }; - const result = updateForgejoCommentBody(input); + const result = updateCommentBody(input); expect(result).toContain("**Claude finished @testuser's task in 31s**"); }); @@ -533,7 +535,7 @@ describe("updateForgejoCommentBody", () => { triggerUsername: "testuser", }; - const result = updateForgejoCommentBody(input); + const result = updateCommentBody(input); expect(result).toContain("**Claude finished @testuser's task**"); expect(result).not.toContain(" in "); }); @@ -553,7 +555,7 @@ describe("updateForgejoCommentBody", () => { triggerUsername: "trigger-user", }; - const result = updateForgejoCommentBody(input); + const result = updateCommentBody(input); // Check header expect(result).toContain( @@ -594,7 +596,7 @@ describe("updateForgejoCommentBody", () => { prLink: "", }; - const result = updateForgejoCommentBody(input); + const result = updateCommentBody(input); expect(result).toContain("Claude finished @claude's task in 1m 30s"); expect(result).toContain(`[View job](${forgejoBaseInput.jobUrl})`); @@ -612,7 +614,7 @@ describe("updateForgejoCommentBody", () => { triggerUsername: "user", }; - const result = updateForgejoCommentBody(input); + const result = updateCommentBody(input); expect(result).not.toContain("[View job run]"); expect(result).toContain("[View job]"); }); @@ -626,7 +628,7 @@ describe("updateForgejoCommentBody", () => { triggerUsername: "user", }; - const result = updateForgejoCommentBody(input); + const result = updateCommentBody(input); // Old link in body removed expect(result).not.toContain("[View branch]"); // New branch in header diff --git a/test/install-mcp-server.test.ts b/test/install-mcp-server.test.ts index 86e87ee..9c79fe1 100644 --- a/test/install-mcp-server.test.ts +++ b/test/install-mcp-server.test.ts @@ -3,6 +3,7 @@ import { prepareMcpConfig } from "../src/mcp/install-mcp-server"; import * as core from "@actions/core"; import type { ParsedGitHubContext } from "../src/github/context"; import { CLAUDE_APP_BOT_ID, CLAUDE_BOT_LOGIN } from "../src/github/constants"; +import { _resetPlatformCache } from "../src/platform/detector"; describe("prepareMcpConfig", () => { let consoleInfoSpy: any; @@ -321,11 +322,14 @@ describe("prepareMcpConfig", () => { let savedServerUrl: string | undefined; beforeEach(() => { + _resetPlatformCache(); savedPlatform = process.env.PLATFORM; savedForgejo = process.env.FORGEJO; savedServerUrl = process.env.GITHUB_SERVER_URL; // Set platform to forgejo for all tests in this block process.env.PLATFORM = "forgejo"; + // Ensure cross-validation doesn't reject PLATFORM=forgejo + process.env.GITHUB_SERVER_URL = "https://git.example.com"; }); afterEach(() => { @@ -344,6 +348,7 @@ describe("prepareMcpConfig", () => { } else { delete process.env.GITHUB_SERVER_URL; } + _resetPlatformCache(); }); test("should skip GitHub public MCP server on Forgejo", async () => { diff --git a/test/m7-integration.test.ts b/test/m7-integration.test.ts index 35178ec..eaeef97 100644 --- a/test/m7-integration.test.ts +++ b/test/m7-integration.test.ts @@ -9,7 +9,7 @@ */ import { describe, it, expect, beforeEach, afterEach } from "bun:test"; -import { detectPlatform } from "../src/platform/detector"; +import { detectPlatform, _resetPlatformCache } from "../src/platform/detector"; // --------------------------------------------------------------------------- // Helpers @@ -40,16 +40,19 @@ function restoreEnv() { describe("M7: Platform routing for mode handlers", () => { beforeEach(() => { + _resetPlatformCache(); saveEnv("PLATFORM", "FORGEJO", "GITHUB_SERVER_URL"); }); afterEach(() => { restoreEnv(); + _resetPlatformCache(); }); describe("detectPlatform() — used by tag mode and update-comment-link", () => { it('returns "forgejo" when PLATFORM=forgejo', () => { process.env.PLATFORM = "forgejo"; + process.env.GITHUB_SERVER_URL = "https://git.example.com"; expect(detectPlatform()).toBe("forgejo"); }); @@ -156,13 +159,13 @@ describe("M7: Forgejo PR compare URL format", () => { // update-comment-link: platform-aware final comment body // --------------------------------------------------------------------------- -describe("M7: updateForgejoCommentBody vs updateCommentBody", () => { - it("Forgejo version strips Unicode spinner", async () => { - const { updateForgejoCommentBody } = await import( - "../src/forgejo/comment-logic" +describe("M7: unified updateCommentBody handles both platforms", () => { + it("strips Unicode spinner (Forgejo format)", async () => { + const { updateCommentBody } = await import( + "../src/github/operations/comment-logic" ); - const result = updateForgejoCommentBody({ + const result = updateCommentBody({ currentBody: "Claude Code is working… ⏳\n\nI'll analyze this and get back to you.\n\n[View job run](https://git.wxs.ro/owner/repo/actions/runs/123)", actionFailed: false, diff --git a/test/permissions.test.ts b/test/permissions.test.ts index 778b1c7..ac456da 100644 --- a/test/permissions.test.ts +++ b/test/permissions.test.ts @@ -3,6 +3,7 @@ import * as core from "@actions/core"; import { checkWritePermissions } from "../src/github/validation/permissions"; import type { ParsedGitHubContext } from "../src/github/context"; import { CLAUDE_APP_BOT_ID, CLAUDE_BOT_LOGIN } from "../src/github/constants"; +import { _resetPlatformCache } from "../src/platform/detector"; describe("checkWritePermissions", () => { let coreInfoSpy: any; @@ -310,10 +311,13 @@ describe("checkWritePermissions", () => { let savedServerUrl: string | undefined; beforeEach(() => { + _resetPlatformCache(); savedPlatform = process.env.PLATFORM; savedForgejo = process.env.FORGEJO; savedServerUrl = process.env.GITHUB_SERVER_URL; process.env.PLATFORM = "forgejo"; + // Ensure cross-validation doesn't reject PLATFORM=forgejo + process.env.GITHUB_SERVER_URL = "https://git.example.com"; }); afterEach(() => { @@ -324,27 +328,78 @@ describe("checkWritePermissions", () => { if (savedServerUrl !== undefined) process.env.GITHUB_SERVER_URL = savedServerUrl; else delete process.env.GITHUB_SERVER_URL; + _resetPlatformCache(); }); - const createForgejoOctokit = (permissions: { + // Helper: Forgejo octokit where collaborator API works (actor-level check) + const createForgejoActorOctokit = (permission: string) => { + return { + repos: { + getCollaboratorPermissionLevel: async () => ({ + data: { permission }, + }), + get: async () => { + throw new Error("Should not fall back to repos.get"); + }, + }, + } as any; + }; + + // Helper: Forgejo octokit where collaborator API fails (falls back to token-level) + const createForgejoFallbackOctokit = (permissions: { admin?: boolean; push?: boolean; pull?: boolean; }) => { return { repos: { + getCollaboratorPermissionLevel: async () => { + throw new Error("403 Forbidden"); + }, get: async () => ({ data: { permissions }, }), - getCollaboratorPermissionLevel: async () => { - throw new Error("Should not be called on Forgejo"); - }, }, } as any; }; - test("should return true when Forgejo token has push access", async () => { - const mockOctokit = createForgejoOctokit({ + test("should use actor-level permission check on Forgejo (write)", async () => { + const mockOctokit = createForgejoActorOctokit("write"); + const context = createContext(); + + const result = await checkWritePermissions(mockOctokit, context); + + expect(result).toBe(true); + expect(coreInfoSpy).toHaveBeenCalledWith( + expect.stringContaining( + "Forgejo actor 'test-user' permission level: write", + ), + ); + }); + + test("should use actor-level permission check on Forgejo (admin)", async () => { + const mockOctokit = createForgejoActorOctokit("admin"); + const context = createContext(); + + const result = await checkWritePermissions(mockOctokit, context); + + expect(result).toBe(true); + }); + + test("should reject actor with read-only permission on Forgejo", async () => { + const mockOctokit = createForgejoActorOctokit("read"); + const context = createContext(); + + const result = await checkWritePermissions(mockOctokit, context); + + expect(result).toBe(false); + expect(coreWarningSpy).toHaveBeenCalledWith( + expect.stringContaining("insufficient permissions: read"), + ); + }); + + test("should fall back to token-level check when collaborator API fails", async () => { + const mockOctokit = createForgejoFallbackOctokit({ admin: false, push: true, pull: true, @@ -354,26 +409,16 @@ describe("checkWritePermissions", () => { const result = await checkWritePermissions(mockOctokit, context); expect(result).toBe(true); + expect(coreWarningSpy).toHaveBeenCalledWith( + expect.stringContaining("Falling back to token-level permission check"), + ); expect(coreInfoSpy).toHaveBeenCalledWith( - expect.stringContaining("Forgejo token has write access"), + expect.stringContaining("token-level fallback"), ); }); - test("should return true when Forgejo token has admin access", async () => { - const mockOctokit = createForgejoOctokit({ - admin: true, - push: false, - pull: true, - }); - const context = createContext(); - - const result = await checkWritePermissions(mockOctokit, context); - - expect(result).toBe(true); - }); - - test("should return false when Forgejo token has only pull access", async () => { - const mockOctokit = createForgejoOctokit({ + test("should return false on fallback when token has only pull access", async () => { + const mockOctokit = createForgejoFallbackOctokit({ admin: false, push: false, pull: true, @@ -383,14 +428,14 @@ describe("checkWritePermissions", () => { const result = await checkWritePermissions(mockOctokit, context); expect(result).toBe(false); - expect(coreWarningSpy).toHaveBeenCalledWith( - expect.stringContaining("insufficient permissions"), - ); }); - test("should return false when permissions object is missing", async () => { + test("should return false on fallback when permissions object is missing", async () => { const mockOctokit = { repos: { + getCollaboratorPermissionLevel: async () => { + throw new Error("403 Forbidden"); + }, get: async () => ({ data: {} }), }, } as any; @@ -404,32 +449,8 @@ describe("checkWritePermissions", () => { ); }); - test("should not call getCollaboratorPermissionLevel on Forgejo", async () => { - let collaboratorCalled = false; - const mockOctokit = { - repos: { - get: async () => ({ - data: { permissions: { admin: false, push: true, pull: true } }, - }), - getCollaboratorPermissionLevel: async () => { - collaboratorCalled = true; - return { data: { permission: "write" } }; - }, - }, - } as any; - const context = createContext(); - - await checkWritePermissions(mockOctokit, context); - - expect(collaboratorCalled).toBe(false); - }); - test("should still bypass for allowed_non_write_users on Forgejo", async () => { - const mockOctokit = createForgejoOctokit({ - admin: false, - push: false, - pull: true, - }); + const mockOctokit = createForgejoActorOctokit("read"); const context = createContext(); const result = await checkWritePermissions( diff --git a/test/platform-detector.test.ts b/test/platform-detector.test.ts index 961dc5b..48e9d03 100644 --- a/test/platform-detector.test.ts +++ b/test/platform-detector.test.ts @@ -1,9 +1,10 @@ -import { detectPlatform } from "../src/platform/detector"; +import { detectPlatform, _resetPlatformCache } from "../src/platform/detector"; import { describe, it, expect, beforeEach } from "bun:test"; describe("detectPlatform", () => { beforeEach(() => { - // Reset relevant env vars before each test + // Reset memoized result and env vars before each test + _resetPlatformCache(); delete process.env.PLATFORM; delete process.env.FORGEJO; delete process.env.GITHUB_SERVER_URL; @@ -40,6 +41,19 @@ describe("detectPlatform", () => { // No other env vars set, should default to github expect(detectPlatform()).toBe("github"); }); + + it("should reject PLATFORM=forgejo when GITHUB_SERVER_URL is github.com", () => { + process.env.PLATFORM = "forgejo"; + process.env.GITHUB_SERVER_URL = "https://github.com"; + // Cross-validation guard: prevent forcing Forgejo mode on GitHub.com + expect(detectPlatform()).toBe("github"); + }); + + it("should accept PLATFORM=forgejo when GITHUB_SERVER_URL is not github.com", () => { + process.env.PLATFORM = "forgejo"; + process.env.GITHUB_SERVER_URL = "https://git.example.com"; + expect(detectPlatform()).toBe("forgejo"); + }); }); describe("FORGEJO env var", () => { diff --git a/test/setup-branch-forgejo.test.ts b/test/setup-branch-forgejo.test.ts new file mode 100644 index 0000000..7e4e492 --- /dev/null +++ b/test/setup-branch-forgejo.test.ts @@ -0,0 +1,150 @@ +#!/usr/bin/env bun + +/** + * Tests the Forgejo API divergence in setupBranch(). + * + * Forgejo uses repos.getBranch() to get source branch SHA because Forgejo's + * /git/refs (plural) API differs from GitHub's /git/ref (singular), causing + * git.getRef() to 404 on Forgejo. + * + * Approach: Call setupBranch() with mocked Octokit. The platform-specific + * API call records that it was called, then throws a sentinel error to stop + * execution before git shell commands run (avoiding real repo side effects). + */ + +import { + describe, + test, + expect, + beforeEach, + afterEach, + spyOn, + type Mock, +} from "bun:test"; +import { setupBranch } from "../src/github/operations/branch"; +import { _resetPlatformCache } from "../src/platform/detector"; +import { createMockContext } from "./mockContext"; +import type { Octokits } from "../src/github/api/client"; +import type { FetchDataResult } from "../src/github/data/fetcher"; + +const mockGithubData: FetchDataResult = { + contextData: { + title: "Test issue", + labels: { nodes: [{ name: "bug" }] }, + }, + comments: [], + changedFiles: [], + changedFilesWithSHA: [], + reviewData: null, + imageUrlMap: new Map(), +} as any; + +function createBranchMockOctokits( + overrides: Record = {}, +): Octokits { + return { + rest: { + repos: { + get: + overrides.reposGet ?? + (async () => ({ data: { default_branch: "main" } })), + getBranch: + overrides.getBranch ?? + (async () => ({ data: { commit: { sha: "abc123" } } })), + }, + git: { + getRef: + overrides.getRef ?? + (async () => ({ data: { object: { sha: "abc123" } } })), + }, + }, + graphql: {} as any, + } as unknown as Octokits; +} + +describe("setupBranch — Forgejo API divergence", () => { + let savedPlatform: string | undefined; + let savedServerUrl: string | undefined; + let consoleLogSpy: Mock<(...args: any[]) => void>; + let consoleErrorSpy: Mock<(...args: any[]) => void>; + + beforeEach(() => { + _resetPlatformCache(); + savedPlatform = process.env.PLATFORM; + savedServerUrl = process.env.GITHUB_SERVER_URL; + + consoleLogSpy = spyOn(console, "log").mockImplementation(() => {}); + consoleErrorSpy = spyOn(console, "error").mockImplementation(() => {}); + }); + + afterEach(() => { + if (savedPlatform !== undefined) process.env.PLATFORM = savedPlatform; + else delete process.env.PLATFORM; + if (savedServerUrl !== undefined) + process.env.GITHUB_SERVER_URL = savedServerUrl; + else delete process.env.GITHUB_SERVER_URL; + + consoleLogSpy.mockRestore(); + consoleErrorSpy.mockRestore(); + _resetPlatformCache(); + }); + + test("on Forgejo: calls repos.getBranch() instead of git.getRef()", async () => { + process.env.PLATFORM = "forgejo"; + process.env.GITHUB_SERVER_URL = "https://git.example.com"; + _resetPlatformCache(); + + let getBranchCalled = false; + let getRefCalled = false; + + const mockOctokits = createBranchMockOctokits({ + getBranch: async () => { + getBranchCalled = true; + // Throw sentinel error to stop before git commands execute + throw new Error("STOP_BEFORE_GIT_COMMANDS"); + }, + getRef: async () => { + getRefCalled = true; + throw new Error("STOP_BEFORE_GIT_COMMANDS"); + }, + }); + + const context = createMockContext({ isPR: false, entityNumber: 1 }); + + await expect( + setupBranch(mockOctokits, mockGithubData, context), + ).rejects.toThrow("STOP_BEFORE_GIT_COMMANDS"); + + expect(getBranchCalled).toBe(true); + expect(getRefCalled).toBe(false); + }); + + test("on GitHub: calls git.getRef() instead of repos.getBranch()", async () => { + delete process.env.PLATFORM; + process.env.GITHUB_SERVER_URL = "https://github.com"; + _resetPlatformCache(); + + let getBranchCalled = false; + let getRefCalled = false; + + const mockOctokits = createBranchMockOctokits({ + getBranch: async () => { + getBranchCalled = true; + throw new Error("STOP_BEFORE_GIT_COMMANDS"); + }, + getRef: async () => { + getRefCalled = true; + throw new Error("STOP_BEFORE_GIT_COMMANDS"); + }, + }); + + const context = createMockContext({ isPR: false, entityNumber: 1 }); + + await expect( + setupBranch(mockOctokits, mockGithubData, context), + ).rejects.toThrow("STOP_BEFORE_GIT_COMMANDS"); + + expect(getRefCalled).toBe(true); + expect(getBranchCalled).toBe(false); + }); +}); diff --git a/test/trigger-validation.test.ts b/test/trigger-validation.test.ts index 36c41f2..2fccd04 100644 --- a/test/trigger-validation.test.ts +++ b/test/trigger-validation.test.ts @@ -2,7 +2,8 @@ import { checkContainsTrigger, escapeRegExp, } from "../src/github/validation/trigger"; -import { describe, it, expect } from "bun:test"; +import { describe, it, expect, beforeEach, afterEach } from "bun:test"; +import { _resetPlatformCache } from "../src/platform/detector"; import { createMockContext, mockIssueAssignedContext, @@ -451,6 +452,75 @@ describe("checkContainsTrigger", () => { }); }); +describe("Forgejo empty-body trigger bypass", () => { + let savedPlatform: string | undefined; + let savedServerUrl: string | undefined; + + beforeEach(() => { + _resetPlatformCache(); + savedPlatform = process.env.PLATFORM; + savedServerUrl = process.env.GITHUB_SERVER_URL; + process.env.PLATFORM = "forgejo"; + process.env.GITHUB_SERVER_URL = "https://git.example.com"; + }); + + afterEach(() => { + if (savedPlatform !== undefined) process.env.PLATFORM = savedPlatform; + else delete process.env.PLATFORM; + if (savedServerUrl !== undefined) + process.env.GITHUB_SERVER_URL = savedServerUrl; + else delete process.env.GITHUB_SERVER_URL; + _resetPlatformCache(); + }); + + it("should NOT trigger when Forgejo PR review comment has no body", () => { + // Previously this returned true unconditionally, bypassing trigger check + const context = { + ...mockPullRequestReviewCommentContext, + payload: { + ...mockPullRequestReviewCommentContext.payload, + comment: { + ...(mockPullRequestReviewCommentContext.payload as any).comment, + body: "", + }, + }, + } as ParsedGitHubContext; + expect(checkContainsTrigger(context)).toBe(false); + }); + + it("should NOT trigger when Forgejo PR review comment body is missing", () => { + const context = { + ...mockPullRequestReviewCommentContext, + payload: { + ...mockPullRequestReviewCommentContext.payload, + comment: { + ...(mockPullRequestReviewCommentContext.payload as any).comment, + body: undefined, + }, + }, + } as ParsedGitHubContext; + expect(checkContainsTrigger(context)).toBe(false); + }); + + it("should still trigger when Forgejo PR review comment contains trigger phrase", () => { + const context = { + ...mockPullRequestReviewCommentContext, + inputs: { + ...mockPullRequestReviewCommentContext.inputs, + triggerPhrase: "@claude", + }, + payload: { + ...mockPullRequestReviewCommentContext.payload, + comment: { + ...(mockPullRequestReviewCommentContext.payload as any).comment, + body: "@claude please review this line", + }, + }, + } as ParsedGitHubContext; + expect(checkContainsTrigger(context)).toBe(true); + }); +}); + describe("escapeRegExp", () => { it("should escape special regex characters", () => { expect(escapeRegExp(".*+?^${}()|[]\\")).toBe( diff --git a/test/update-tracking-comment.test.ts b/test/update-tracking-comment.test.ts new file mode 100644 index 0000000..5555bb3 --- /dev/null +++ b/test/update-tracking-comment.test.ts @@ -0,0 +1,277 @@ +#!/usr/bin/env bun + +import { + describe, + test, + expect, + beforeEach, + afterEach, + spyOn, + type Mock, +} from "bun:test"; +import { updateTrackingComment } from "../src/github/operations/comments/update-with-branch"; +import { _resetPlatformCache } from "../src/platform/detector"; +import { createMockContext } from "./mockContext"; +import type { Octokits } from "../src/github/api/client"; + +function createMockOctokits(overrides: Record = {}): Octokits { + // updateTrackingComment calls updateClaudeComment(octokit.rest, ...), + // which accesses octokit.rest.issues.updateComment / octokit.rest.pulls.updateReviewComment + return { + rest: { + rest: { + issues: { + updateComment: + overrides.updateComment ?? + (async () => ({ data: { id: 1, html_url: "", updated_at: "" } })), + }, + pulls: { + updateReviewComment: + overrides.updateReviewComment ?? + (async () => ({ data: { id: 1, html_url: "", updated_at: "" } })), + }, + }, + }, + graphql: {} as any, + } as unknown as Octokits; +} + +describe("updateTrackingComment", () => { + let savedPlatform: string | undefined; + let savedServerUrl: string | undefined; + let savedRunNumber: string | undefined; + let savedRunId: string | undefined; + let consoleLogSpy: Mock<(...args: any[]) => void>; + let consoleErrorSpy: Mock<(...args: any[]) => void>; + + beforeEach(() => { + _resetPlatformCache(); + savedPlatform = process.env.PLATFORM; + savedServerUrl = process.env.GITHUB_SERVER_URL; + savedRunNumber = process.env.GITHUB_RUN_NUMBER; + savedRunId = process.env.GITHUB_RUN_ID; + + // Default to GitHub platform + delete process.env.PLATFORM; + process.env.GITHUB_SERVER_URL = "https://github.com"; + + consoleLogSpy = spyOn(console, "log").mockImplementation(() => {}); + consoleErrorSpy = spyOn(console, "error").mockImplementation(() => {}); + }); + + afterEach(() => { + if (savedPlatform !== undefined) process.env.PLATFORM = savedPlatform; + else delete process.env.PLATFORM; + if (savedServerUrl !== undefined) + process.env.GITHUB_SERVER_URL = savedServerUrl; + else delete process.env.GITHUB_SERVER_URL; + if (savedRunNumber !== undefined) + process.env.GITHUB_RUN_NUMBER = savedRunNumber; + else delete process.env.GITHUB_RUN_NUMBER; + if (savedRunId !== undefined) process.env.GITHUB_RUN_ID = savedRunId; + else delete process.env.GITHUB_RUN_ID; + + consoleLogSpy.mockRestore(); + consoleErrorSpy.mockRestore(); + _resetPlatformCache(); + }); + + describe("GitHub platform — body construction", () => { + test("includes GitHub job run link using context.runId", async () => { + let capturedBody = ""; + const mockOctokits = createMockOctokits({ + updateComment: async (args: any) => { + capturedBody = args.body; + return { data: { id: 1, html_url: "", updated_at: "" } }; + }, + }); + + const context = createMockContext({ runId: "55555" }); + await updateTrackingComment(mockOctokits, context, 1); + + expect(capturedBody).toContain("actions/runs/55555"); + expect(capturedBody).toContain("https://github.com/test-owner/test-repo"); + }); + + test("includes branch link for issue context", async () => { + let capturedBody = ""; + const mockOctokits = createMockOctokits({ + updateComment: async (args: any) => { + capturedBody = args.body; + return { data: { id: 1, html_url: "", updated_at: "" } }; + }, + }); + + const context = createMockContext({ isPR: false }); + await updateTrackingComment(mockOctokits, context, 1, "claude/fix-123"); + + expect(capturedBody).toContain("View branch"); + expect(capturedBody).toContain("tree/claude/fix-123"); + }); + + test("omits branch link for PR context even when branch provided", async () => { + let capturedBody = ""; + const mockOctokits = createMockOctokits({ + updateComment: async (args: any) => { + capturedBody = args.body; + return { data: { id: 1, html_url: "", updated_at: "" } }; + }, + }); + + const context = createMockContext({ isPR: true }); + await updateTrackingComment(mockOctokits, context, 1, "claude/fix-123"); + + expect(capturedBody).not.toContain("View branch"); + expect(capturedBody).not.toContain("tree/"); + }); + + test("omits branch link when branch is undefined", async () => { + let capturedBody = ""; + const mockOctokits = createMockOctokits({ + updateComment: async (args: any) => { + capturedBody = args.body; + return { data: { id: 1, html_url: "", updated_at: "" } }; + }, + }); + + const context = createMockContext({ isPR: false }); + await updateTrackingComment(mockOctokits, context, 1); + + expect(capturedBody).not.toContain("View branch"); + }); + }); + + describe("Forgejo platform — body construction", () => { + beforeEach(() => { + _resetPlatformCache(); + process.env.PLATFORM = "forgejo"; + process.env.GITHUB_SERVER_URL = "https://git.example.com"; + process.env.GITHUB_RUN_NUMBER = "77"; + }); + + test("uses Forgejo job run link with GITHUB_RUN_NUMBER", async () => { + let capturedBody = ""; + const mockOctokits = createMockOctokits({ + updateComment: async (args: any) => { + capturedBody = args.body; + return { data: { id: 1, html_url: "", updated_at: "" } }; + }, + }); + + const context = createMockContext(); + await updateTrackingComment(mockOctokits, context, 1); + + expect(capturedBody).toContain( + "https://git.example.com/test-owner/test-repo/actions/runs/77", + ); + }); + + test("uses Forgejo branch link for issues", async () => { + let capturedBody = ""; + const mockOctokits = createMockOctokits({ + updateComment: async (args: any) => { + capturedBody = args.body; + return { data: { id: 1, html_url: "", updated_at: "" } }; + }, + }); + + const context = createMockContext({ isPR: false }); + await updateTrackingComment(mockOctokits, context, 1, "claude/fix-42"); + + expect(capturedBody).toContain( + "https://git.example.com/test-owner/test-repo/tree/claude/fix-42", + ); + }); + + test("uses Forgejo comment body with Unicode spinner", async () => { + let capturedBody = ""; + const mockOctokits = createMockOctokits({ + updateComment: async (args: any) => { + capturedBody = args.body; + return { data: { id: 1, html_url: "", updated_at: "" } }; + }, + }); + + const context = createMockContext(); + await updateTrackingComment(mockOctokits, context, 1); + + expect(capturedBody).toContain("⏳"); + expect(capturedBody).not.toContain("user-attachments/assets"); + }); + }); + + describe("comment type routing", () => { + test("uses issue comment update for non-review events", async () => { + let issueUpdateCalled = false; + let reviewUpdateCalled = false; + const mockOctokits = createMockOctokits({ + updateComment: async () => { + issueUpdateCalled = true; + return { data: { id: 1, html_url: "", updated_at: "" } }; + }, + updateReviewComment: async () => { + reviewUpdateCalled = true; + return { data: { id: 1, html_url: "", updated_at: "" } }; + }, + }); + + const context = createMockContext({ eventName: "issue_comment" }); + await updateTrackingComment(mockOctokits, context, 1); + + expect(issueUpdateCalled).toBe(true); + expect(reviewUpdateCalled).toBe(false); + }); + + test("uses PR review comment update for review comment events", async () => { + let issueUpdateCalled = false; + let reviewUpdateCalled = false; + const mockOctokits = createMockOctokits({ + updateComment: async () => { + issueUpdateCalled = true; + return { data: { id: 1, html_url: "", updated_at: "" } }; + }, + updateReviewComment: async () => { + reviewUpdateCalled = true; + return { data: { id: 1, html_url: "", updated_at: "" } }; + }, + }); + + const context = createMockContext({ + eventName: "pull_request_review_comment", + isPR: true, + }); + await updateTrackingComment(mockOctokits, context, 1); + + expect(reviewUpdateCalled).toBe(true); + expect(issueUpdateCalled).toBe(false); + }); + }); + + describe("error handling", () => { + test("throws and logs error when update fails", async () => { + const mockOctokits = createMockOctokits({ + updateComment: async () => { + throw new Error("Update failed"); + }, + }); + + const context = createMockContext(); + await expect( + updateTrackingComment(mockOctokits, context, 1), + ).rejects.toThrow("Update failed"); + expect(consoleErrorSpy).toHaveBeenCalled(); + }); + + test("logs correct comment type label on success", async () => { + const mockOctokits = createMockOctokits(); + + const issueContext = createMockContext({ eventName: "issue_comment" }); + await updateTrackingComment(mockOctokits, issueContext, 42); + + expect(consoleLogSpy).toHaveBeenCalledWith( + expect.stringContaining("issue"), + ); + expect(consoleLogSpy).toHaveBeenCalledWith(expect.stringContaining("42")); + }); + }); +});