From 4b0737c54c899e561a661a1bc7c0cbdb7665b145 Mon Sep 17 00:00:00 2001 From: Vlad Mocanu Date: Wed, 25 Feb 2026 17:00:13 +0200 Subject: [PATCH] feat(prd-1): implement M4b - Forgejo REST comment/review fetching - Add 3 REST calls to Promise.all for PRs (listComments, listReviews, listReviewComments) and 1 for issues (listComments) - Add mapping helpers: mapRestComments, mapRestReviewComments, mapRestReviews (groups inline comments by pull_request_review_id) - Import and apply TOCTOU filtering (filterCommentsToTriggerTime, filterReviewsToTriggerTime) and actor filtering from GitHub fetcher - Add image processing pipeline via downloadCommentImages (no-op on Forgejo due to URL pattern mismatch, architecturally correct) - Deep-clone reviews before filtering to preserve contextData immutability - Update test suite: 51 tests covering mapping helpers, full fetch flow, TOCTOU filtering, actor filtering, and contextData immutability - Updated PRD checkboxes for M4b completion Progress: 44% complete (4/9 milestones) - next: M5 or M3 Co-Authored-By: Claude Opus 4.6 --- prds/1-forgejo-platform-support.md | 127 +++--- src/entrypoints/run.ts | 5 +- src/forgejo/data/fetcher.ts | 318 +++++++++++++- test/forgejo-data-fetcher.test.ts | 651 ++++++++++++++++++++++++++++- 4 files changed, 1001 insertions(+), 100 deletions(-) diff --git a/prds/1-forgejo-platform-support.md b/prds/1-forgejo-platform-support.md index afa5270..6839af2 100644 --- a/prds/1-forgejo-platform-support.md +++ b/prds/1-forgejo-platform-support.md @@ -23,31 +23,31 @@ Octokit REST methods (comments, permissions, branches, actor validation) work wi Only **4 areas** require new Forgejo implementations: -| Area | Why It's Different | New File | -|------|--------------------|----------| -| **Token handling** | No OIDC exchange with Anthropic; use `GITHUB_TOKEN`/`FORGEJO_TOKEN` directly | `src/forgejo/token.ts` | -| **Data fetching** | Forgejo has no GraphQL; must replace 3 GraphQL queries with multiple REST calls | `src/forgejo/data/fetcher.ts` | -| **Comment URL formatting** | Forgejo URLs differ from GitHub URLs (job links, PR links, branch links) | `src/forgejo/comment-logic.ts` | +| Area | Why It's Different | New File | +| --------------------------- | -------------------------------------------------------------------------------- | ----------------------------------- | +| **Token handling** | No OIDC exchange with Anthropic; use `GITHUB_TOKEN`/`FORGEJO_TOKEN` directly | `src/forgejo/token.ts` | +| **Data fetching** | Forgejo has no GraphQL; must replace 3 GraphQL queries with multiple REST calls | `src/forgejo/data/fetcher.ts` | +| **Comment URL formatting** | Forgejo URLs differ from GitHub URLs (job links, PR links, branch links) | `src/forgejo/comment-logic.ts` | | **CI status / Actions API** | Forgejo Actions API is less mature; `downloadJobLogForWorkflowRun` may not exist | `src/mcp/forgejo-actions-server.ts` | ### What Re-Uses Existing Code (No Duplication) These `src/github/` modules work with Forgejo via Octokit REST as-is: -| Module | Why It Works | -|--------|-------------| -| `validation/permissions.ts` | `octokit.repos.getCollaboratorPermissionLevel()` exists in Forgejo API | -| `validation/actor.ts` | `octokit.users.getByUsername()` works on Forgejo | -| `validation/trigger.ts` | Pure logic — reads webhook payload fields, no API calls | -| `operations/comments/create-initial.ts` | `octokit.rest.issues.createComment()` works on Forgejo | -| `operations/comments/update-claude-comment.ts` | `octokit.rest.issues.updateComment()` works on Forgejo | -| `operations/branch.ts` | `octokit.rest.git.getRef()`, `octokit.rest.repos.get()` work on Forgejo | -| `operations/git-config.ts` | Uses `GITHUB_SERVER_URL` for noreply domain — works since Forgejo sets this var | -| `utils/sanitizer.ts` | Pure logic, no API dependency | -| `utils/image-downloader.ts` | Uses Octokit REST for fetching — should work | -| `context.ts` | `@actions/github` reads `GITHUB_EVENT_PATH` — Forgejo sets this | -| `data/formatter.ts` | Pure formatting logic | -| `api/config.ts` | Reads `GITHUB_API_URL` / `GITHUB_SERVER_URL` — Forgejo sets these | +| Module | Why It Works | +| ---------------------------------------------- | ------------------------------------------------------------------------------- | +| `validation/permissions.ts` | `octokit.repos.getCollaboratorPermissionLevel()` exists in Forgejo API | +| `validation/actor.ts` | `octokit.users.getByUsername()` works on Forgejo | +| `validation/trigger.ts` | Pure logic — reads webhook payload fields, no API calls | +| `operations/comments/create-initial.ts` | `octokit.rest.issues.createComment()` works on Forgejo | +| `operations/comments/update-claude-comment.ts` | `octokit.rest.issues.updateComment()` works on Forgejo | +| `operations/branch.ts` | `octokit.rest.git.getRef()`, `octokit.rest.repos.get()` work on Forgejo | +| `operations/git-config.ts` | Uses `GITHUB_SERVER_URL` for noreply domain — works since Forgejo sets this var | +| `utils/sanitizer.ts` | Pure logic, no API dependency | +| `utils/image-downloader.ts` | Uses Octokit REST for fetching — should work | +| `context.ts` | `@actions/github` reads `GITHUB_EVENT_PATH` — Forgejo sets this | +| `data/formatter.ts` | Pure formatting logic | +| `api/config.ts` | Reads `GITHUB_API_URL` / `GITHUB_SERVER_URL` — Forgejo sets these | ### Architecture: Lean Adapter @@ -77,24 +77,24 @@ action.yml # MODIFIED - platform input, conditional steps ### Why This Approach -| Approach | Upstream Merge Effort | Maintenance | Chosen? | -|----------|----------------------|-------------|---------| -| **Lean adapter (reuse + minimal new files)** | Low (3-4 conflict-prone files) | Low | **Yes** | -| Full abstraction layer (20+ new files) | Near-zero conflicts but high maintenance syncing duplicated code | High | No | -| Modify GitHub files in-place | High conflict rate on every upstream pull | Low | No | -| Complete fork with divergence | No merges possible after a while | Very high | No | +| Approach | Upstream Merge Effort | Maintenance | Chosen? | +| -------------------------------------------- | ---------------------------------------------------------------- | ----------- | ------- | +| **Lean adapter (reuse + minimal new files)** | Low (3-4 conflict-prone files) | Low | **Yes** | +| Full abstraction layer (20+ new files) | Near-zero conflicts but high maintenance syncing duplicated code | High | No | +| Modify GitHub files in-place | High conflict rate on every upstream pull | Low | No | +| Complete fork with divergence | No merges possible after a while | Very high | No | ### Honest Merge-Conflict Assessment These files **will be modified** and are the merge-conflict surface when pulling upstream: -| File | What We Change | Conflict Risk | -|------|---------------|---------------| -| `src/entrypoints/run.ts` | Platform branch at top for token/client setup | **Medium** - upstream's most active file | -| `src/modes/tag/index.ts` | Swap `fetchGitHubData` call + comment logic based on platform | **Medium** | -| `src/modes/agent/index.ts` | Swap token setup based on platform | **Low** - simpler file | -| `src/mcp/install-mcp-server.ts` | Platform routing for MCP server selection | **Low-Medium** | -| `action.yml` | Add `platform` input, conditional Forgejo steps | **Low** - additive changes | +| File | What We Change | Conflict Risk | +| ------------------------------- | ------------------------------------------------------------- | ---------------------------------------- | +| `src/entrypoints/run.ts` | Platform branch at top for token/client setup | **Medium** - upstream's most active file | +| `src/modes/tag/index.ts` | Swap `fetchGitHubData` call + comment logic based on platform | **Medium** | +| `src/modes/agent/index.ts` | Swap token setup based on platform | **Low** - simpler file | +| `src/mcp/install-mcp-server.ts` | Platform routing for MCP server selection | **Low-Medium** | +| `action.yml` | Add `platform` input, conditional Forgejo steps | **Low** - additive changes | **Mitigation strategy**: Keep all platform changes at the **top** of functions (early return / branch pattern). Avoid restructuring existing code flow. When upstream modifies these files, conflicts will be localized to our small additions, not interleaved throughout. @@ -127,6 +127,7 @@ export function detectPlatform(): "github" | "forgejo" { ``` New `action.yml` input: + ```yaml platform: description: "Platform to run on: 'auto' (default), 'github', or 'forgejo'" @@ -137,6 +138,7 @@ platform: ### Key Technical Differences 1. **No GraphQL**: Forgejo is REST-only. The 3 GraphQL queries in `src/github/api/queries/github.ts` (PR_QUERY, ISSUE_QUERY, USER_QUERY) must be replaced with **5-8 separate paginated REST calls**: + - `GET /repos/{owner}/{repo}/pulls/{index}` - PR details - `GET /repos/{owner}/{repo}/pulls/{index}/files` - changed files (paginated) - `GET /repos/{owner}/{repo}/pulls/{index}/commits` - commits (paginated) @@ -161,18 +163,19 @@ platform: ### `action.yml` Required Modifications -| Line(s) | Current | Problem | Fix | -|----------|---------|---------|-----| -| 103 | `default: "41898282"` (bot_id) | Meaningless on Forgejo | Make platform-aware; use token owner identity on Forgejo | -| 107 | `default: "claude[bot]"` (bot_name) | Doesn't exist on Forgejo | Same — detect from token or use configurable default | -| 163-166 | `uses: oven-sh/setup-bun@...` | References GitHub release; may fail on isolated Forgejo runners | Add conditional: if bun already available, skip; otherwise use direct curl install | -| 166 | `token: ${{ inputs.github_token \|\| github.token }}` | Used to fetch bun release from GitHub | On Forgejo, `github.token` is the Forgejo token — may not auth to GitHub; provide fallback | -| 285-294 | "Revoke app token" step | Calls `${GITHUB_API_URL}/installation/token` which doesn't exist on Forgejo | Make conditional: `if: platform != 'forgejo'` or detect and skip | -| N/A | Missing | No `platform` input | Add `platform` input (auto/github/forgejo) | +| Line(s) | Current | Problem | Fix | +| ------- | ----------------------------------------------------- | --------------------------------------------------------------------------- | ------------------------------------------------------------------------------------------ | +| 103 | `default: "41898282"` (bot_id) | Meaningless on Forgejo | Make platform-aware; use token owner identity on Forgejo | +| 107 | `default: "claude[bot]"` (bot_name) | Doesn't exist on Forgejo | Same — detect from token or use configurable default | +| 163-166 | `uses: oven-sh/setup-bun@...` | References GitHub release; may fail on isolated Forgejo runners | Add conditional: if bun already available, skip; otherwise use direct curl install | +| 166 | `token: ${{ inputs.github_token \|\| github.token }}` | Used to fetch bun release from GitHub | On Forgejo, `github.token` is the Forgejo token — may not auth to GitHub; provide fallback | +| 285-294 | "Revoke app token" step | Calls `${GITHUB_API_URL}/installation/token` which doesn't exist on Forgejo | Make conditional: `if: platform != 'forgejo'` or detect and skip | +| N/A | Missing | No `platform` input | Add `platform` input (auto/github/forgejo) | ### `base-action/` Directory `run.ts` imports directly from `base-action/src/`: + - `validate-env.ts` - checks env vars for Claude CLI (platform-agnostic) - `setup-claude-code-settings.ts` - writes Claude settings (platform-agnostic) - `install-plugins.ts` - installs Claude plugins (platform-agnostic) @@ -207,11 +210,13 @@ The upstream repo has 13 workflows in `.github/workflows/`. Strategy: ### Custom Forgejo Workflows #### 1. CI Test Workflow (`.forgejo/workflows/ci.yml`) + - Triggers on push to main and PRs - Runs `bun install` and `bun test` - Lints with biome (matching upstream's toolchain) #### 2. Action Self-Test Workflow (`.forgejo/workflows/test-action.yml`) + - **Tests the action itself** on Forgejo - Creates a test issue via API, triggers the action with a simple prompt - Verifies: platform detection returns "forgejo", comment is created on the issue, action completes successfully @@ -219,6 +224,7 @@ The upstream repo has 13 workflows in `.github/workflows/`. Strategy: - Runs on a schedule (weekly) and on PRs that modify `src/`, `action.yml`, or `.forgejo/` #### 3. Upstream Sync Check (`.forgejo/workflows/upstream-sync.yml`) + - Scheduled workflow (weekly) + manual trigger - Fetches `upstream/main`, compares with our `main` using `git rev-list` - Creates an issue if upstream has new commits we haven't merged @@ -229,10 +235,12 @@ The upstream repo has 13 workflows in `.github/workflows/`. Strategy: ### Phase 1 - Foundation (parallel, no dependencies) - [x] **M1: Platform detection + `action.yml` modifications** — Add `platform` input to `action.yml`. Create `src/platform/detector.ts` with explicit input + env-var auto-detection. Add conditional logic to skip "Revoke app token" step on Forgejo. Fix bun installation to handle Forgejo runners (direct curl fallback). + - Files: `action.yml` (MODIFIED), `src/platform/detector.ts` (NEW) - Success: `detectPlatform()` returns "forgejo" in Forgejo runner, "github" in GitHub runner. `action.yml` doesn't error on Forgejo. - [x] **M2: Forgejo token handling** — Create `src/forgejo/token.ts` that returns `GITHUB_TOKEN` / `FORGEJO_TOKEN` directly (no OIDC, no Anthropic exchange). Wire into `run.ts` with an early platform branch before the existing `setupGitHubToken()` call. + - Files: `src/forgejo/token.ts` (NEW), `src/entrypoints/run.ts` (MODIFIED — surgical addition at top of `run()`) - Depends on: M1 - Success: Token is available without OIDC on Forgejo. GitHub path unchanged. @@ -244,11 +252,13 @@ The upstream repo has 13 workflows in `.github/workflows/`. Strategy: ### Phase 2 - Core Data Fetching (the hard part) - [x] **M4a: Forgejo REST data fetching — PR/Issue basics** — Implement REST-based fetching for PR details, issue details, changed files, and commits. Produce output matching the `FetchDataResult` type that existing code expects. + - Files: `src/forgejo/data/fetcher.ts` (NEW) - Depends on: M1, M2 - Success: Can fetch PR/issue title, body, author, state, files, commits from Forgejo REST API. -- [ ] **M4b: Forgejo REST data fetching — Comments, reviews, inline comments** — Extend the fetcher with paginated comment/review fetching, including nested inline review comments (requires per-review API calls). Implement TOCTOU filtering using the same `filterCommentsToTriggerTime` / `filterReviewsToTriggerTime` functions (reuse from `src/github/data/fetcher.ts` — these are pure logic). Use `Promise.all()` for parallel API calls. +- [x] **M4b: Forgejo REST data fetching — Comments, reviews, inline comments** — Extend the fetcher with paginated comment/review fetching, including nested inline review comments (requires per-review API calls). Implement TOCTOU filtering using the same `filterCommentsToTriggerTime` / `filterReviewsToTriggerTime` functions (reuse from `src/github/data/fetcher.ts` — these are pure logic). Use `Promise.all()` for parallel API calls. + - Files: `src/forgejo/data/fetcher.ts` (extend from M4a) - Depends on: M4a - Success: Full parity with GitHub GraphQL data fetch. All comments, reviews, and inline comments included with proper TOCTOU filtering. @@ -261,6 +271,7 @@ The upstream repo has 13 workflows in `.github/workflows/`. Strategy: ### Phase 3 - Integration (wiring it together) - [ ] **M6: MCP server platform routing** — Modify `install-mcp-server.ts` to select GitHub or Forgejo server implementations based on platform. Create `src/mcp/forgejo-actions-server.ts` for CI status (may be a subset of GitHub's if some Actions API endpoints don't exist in Forgejo). Keep MCP server names identical (`github_comment`, `github_ci`, etc.) to avoid touching tool name lists. For comment/file-ops/inline-comment servers, the existing GitHub implementations likely work via Octokit REST — verify and use directly. + - Files: `src/mcp/install-mcp-server.ts` (MODIFIED), `src/mcp/forgejo-actions-server.ts` (NEW, possibly) - Depends on: M4b, M5 - Success: Claude can update comments, query CI status on Forgejo. MCP tools work end-to-end. @@ -273,6 +284,7 @@ The upstream repo has 13 workflows in `.github/workflows/`. Strategy: ### Phase 4 - Validation & Polish - [ ] **M8: End-to-end testing on Forgejo** — Test complete workflows: issue trigger (@claude mention), PR review, agent mode (workflow_dispatch with prompt), tag mode. Verify comment updates, branch creation, file commits, CI status queries. + - Depends on: M7 - Success: All supported trigger types work on Forgejo. Self-test workflow passes. @@ -289,6 +301,7 @@ M3 (independent, anytime) ``` **Parallel opportunities:** + - M3 is fully independent — can run anytime - M5 and M2 can run in parallel (both only depend on M1) - M4a can start as soon as M2 is done @@ -296,25 +309,25 @@ M3 (independent, anytime) ## Execution Plan -| Phase | Milestones | Files Touched | Parallel? | -|-------|-----------|---------------|-----------| -| 1 | M1, M2, M3 | `action.yml`, `src/platform/detector.ts`, `src/forgejo/token.ts`, `src/entrypoints/run.ts`, `.forgejo/workflows/*` | M1 first, then M2+M3 in parallel | -| 2 | M4a, M4b, M5 | `src/forgejo/data/fetcher.ts`, `src/forgejo/comment-logic.ts` | M5 parallel with M4a/M4b | -| 3 | M6, M7 | `src/mcp/install-mcp-server.ts`, `src/mcp/forgejo-actions-server.ts`, `src/modes/tag/index.ts`, `src/modes/agent/index.ts` | Sequential | -| 4 | M8, M9 | Tests, docs, `CLAUDE.md` | Sequential | +| Phase | Milestones | Files Touched | Parallel? | +| ----- | ------------ | -------------------------------------------------------------------------------------------------------------------------- | -------------------------------- | +| 1 | M1, M2, M3 | `action.yml`, `src/platform/detector.ts`, `src/forgejo/token.ts`, `src/entrypoints/run.ts`, `.forgejo/workflows/*` | M1 first, then M2+M3 in parallel | +| 2 | M4a, M4b, M5 | `src/forgejo/data/fetcher.ts`, `src/forgejo/comment-logic.ts` | M5 parallel with M4a/M4b | +| 3 | M6, M7 | `src/mcp/install-mcp-server.ts`, `src/mcp/forgejo-actions-server.ts`, `src/modes/tag/index.ts`, `src/modes/agent/index.ts` | Sequential | +| 4 | M8, M9 | Tests, docs, `CLAUDE.md` | Sequential | ## Risk Assessment -| Risk | Likelihood | Impact | Mitigation | -|------|-----------|--------|------------| -| Upstream refactors `run.ts` or mode handlers | Medium | High | Keep entry point changes to <10 lines; use early-return pattern; platform branch at top of functions | -| Octokit REST methods behave differently on Forgejo | Low | Medium | Test each used method early in M4a; fall back to raw `fetch()` if needed | -| Forgejo webhook payloads differ from `@octokit/webhooks-types` | Medium | Medium | Test with real Forgejo webhooks; key check: `payload.issue.pull_request` field for `is_pull` detection | -| `pull_request_target` event missing in Forgejo | High | Low | Already handled — `context.ts:188` maps it to `pull_request` | -| Forgejo Actions API missing endpoints (downloadJobLog) | Medium | Medium | Degrade gracefully — CI MCP server may offer fewer tools on Forgejo | -| `oven-sh/setup-bun` action fails on isolated Forgejo runners | Medium | Medium | Add direct curl bun install as fallback in `action.yml` | -| Upstream `.github/workflows/` run and fail on Forgejo | Medium | Low | Document runner configuration; provide `.forgejo/workflows/` overrides | -| Upstream adds new features requiring new GraphQL fields | Certain | Low | New features won't have Forgejo support until we add REST equivalents to `forgejo/data/fetcher.ts` | +| Risk | Likelihood | Impact | Mitigation | +| -------------------------------------------------------------- | ---------- | ------ | ------------------------------------------------------------------------------------------------------ | +| Upstream refactors `run.ts` or mode handlers | Medium | High | Keep entry point changes to <10 lines; use early-return pattern; platform branch at top of functions | +| Octokit REST methods behave differently on Forgejo | Low | Medium | Test each used method early in M4a; fall back to raw `fetch()` if needed | +| Forgejo webhook payloads differ from `@octokit/webhooks-types` | Medium | Medium | Test with real Forgejo webhooks; key check: `payload.issue.pull_request` field for `is_pull` detection | +| `pull_request_target` event missing in Forgejo | High | Low | Already handled — `context.ts:188` maps it to `pull_request` | +| Forgejo Actions API missing endpoints (downloadJobLog) | Medium | Medium | Degrade gracefully — CI MCP server may offer fewer tools on Forgejo | +| `oven-sh/setup-bun` action fails on isolated Forgejo runners | Medium | Medium | Add direct curl bun install as fallback in `action.yml` | +| Upstream `.github/workflows/` run and fail on Forgejo | Medium | Low | Document runner configuration; provide `.forgejo/workflows/` overrides | +| Upstream adds new features requiring new GraphQL fields | Certain | Low | New features won't have Forgejo support until we add REST equivalents to `forgejo/data/fetcher.ts` | ## Success Criteria diff --git a/src/entrypoints/run.ts b/src/entrypoints/run.ts index 15a9aa4..b79d7bb 100644 --- a/src/entrypoints/run.ts +++ b/src/entrypoints/run.ts @@ -158,10 +158,7 @@ async function run() { githubToken = await setupGitHubToken(); } catch (error) { if (error instanceof WorkflowValidationSkipError) { - core.setOutput( - "skipped_due_to_workflow_validation_mismatch", - "true", - ); + core.setOutput("skipped_due_to_workflow_validation_mismatch", "true"); console.log("Exiting due to workflow validation skip"); return; } diff --git a/src/forgejo/data/fetcher.ts b/src/forgejo/data/fetcher.ts index ec922be..2fd7bc8 100644 --- a/src/forgejo/data/fetcher.ts +++ b/src/forgejo/data/fetcher.ts @@ -4,8 +4,8 @@ * Replaces GitHub's GraphQL queries (PR_QUERY, ISSUE_QUERY, USER_QUERY) * with Octokit REST calls that work on Forgejo's /api/v1 endpoint. * - * M4a scope: PR/issue details, files, commits, user display name. - * M4b scope: Comments, reviews, inline comments, image processing. + * M4a: PR/issue details, files, commits, user display name. + * M4b: Comments, reviews, inline comments, TOCTOU filtering, actor filtering, image processing. */ import { execFileSync } from "child_process"; @@ -16,13 +16,22 @@ import type { GitHubIssue, GitHubPullRequest, GitHubReview, + GitHubReviewComment, } from "../../github/types"; import type { FetchDataResult, GitHubFileWithSHA, } from "../../github/data/fetcher"; +import { + filterCommentsToTriggerTime, + filterReviewsToTriggerTime, + filterCommentsByActor, + isBodySafeToUse, +} from "../../github/data/fetcher"; +import type { CommentWithImages } from "../../github/utils/image-downloader"; +import { downloadCommentImages } from "../../github/utils/image-downloader"; -type FetchForgejoDataParams = { +export type FetchForgejoDataParams = { octokits: Octokits; repository: string; prNumber: string; @@ -31,7 +40,6 @@ type FetchForgejoDataParams = { triggerTime?: string; originalTitle?: string; originalBody?: string | null; - // Accepted for API compatibility — used by M4b includeCommentsByActor?: string; excludeCommentsByActor?: string; }; @@ -41,7 +49,7 @@ type FetchForgejoDataParams = { * REST returns: "added", "removed", "modified", "renamed", "copied", "changed", "unchanged" * GraphQL uses: "ADDED", "DELETED", "MODIFIED", "RENAMED", "COPIED" */ -function mapFileStatus(status: string): string { +export function mapFileStatus(status: string): string { const statusMap: Record = { added: "ADDED", removed: "DELETED", @@ -54,6 +62,103 @@ function mapFileStatus(status: string): string { return statusMap[status] || status.toUpperCase(); } +/** + * Maps REST issue/PR comment objects to GitHubComment type. + */ +export function mapRestComments( + comments: Array<{ + id: number; + body?: string | null; + user?: { login?: string } | null; + created_at: string; + updated_at: string; + }>, +): GitHubComment[] { + return comments.map((c) => ({ + id: c.id.toString(), + databaseId: c.id.toString(), + body: c.body ?? "", + author: { login: c.user?.login ?? "unknown" }, + createdAt: c.created_at, + updatedAt: c.updated_at, + lastEditedAt: undefined, // Forgejo REST doesn't expose lastEditedAt + isMinimized: false, // Forgejo REST doesn't have minimized comments + })); +} + +/** + * Maps REST inline review comment objects to GitHubReviewComment type, + * preserving the review ID for grouping. + */ +export function mapRestReviewComments( + comments: Array<{ + id: number; + body?: string | null; + user?: { login?: string } | null; + path: string; + line?: number | null; + original_line?: number | null; + created_at: string; + updated_at: string; + pull_request_review_id?: number | null; + }>, +): (GitHubReviewComment & { reviewId: number | null })[] { + return comments.map((c) => ({ + id: c.id.toString(), + databaseId: c.id.toString(), + body: c.body ?? "", + author: { login: c.user?.login ?? "unknown" }, + createdAt: c.created_at, + updatedAt: c.updated_at, + lastEditedAt: undefined, + isMinimized: false, + path: c.path, + line: c.line ?? c.original_line ?? null, + reviewId: c.pull_request_review_id ?? null, + })); +} + +/** + * Maps REST review objects to GitHubReview type, grouping inline comments + * by their pull_request_review_id. + */ +export function mapRestReviews( + reviews: Array<{ + id: number; + body?: string | null; + user?: { login?: string } | null; + state: string; + submitted_at?: string; + }>, + inlineComments: (GitHubReviewComment & { reviewId: number | null })[], +): GitHubReview[] { + // Group inline comments by review ID + const commentsByReview = new Map(); + for (const comment of inlineComments) { + if (comment.reviewId != null) { + const existing = commentsByReview.get(comment.reviewId) ?? []; + // Strip reviewId before storing (it's not part of GitHubReviewComment) + const { reviewId: _, ...reviewComment } = comment; + existing.push(reviewComment); + commentsByReview.set(comment.reviewId, existing); + } + } + + return reviews.map((r) => ({ + id: r.id.toString(), + databaseId: r.id.toString(), + author: { login: r.user?.login ?? "unknown" }, + 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 + lastEditedAt: undefined, + comments: { + nodes: commentsByReview.get(r.id) ?? [], + }, + })); +} + /** * Fetch user display name via REST (replaces GraphQL USER_QUERY). */ @@ -75,11 +180,10 @@ async function fetchUserDisplayNameRest( /** * Fetch PR or issue data from Forgejo using REST API calls. * - * For PRs, fetches details + files + commits in parallel via Promise.all(). - * Produces output matching FetchDataResult so downstream code + * For PRs, fetches details + files + commits + comments + reviews + inline + * comments in parallel via Promise.all(). For issues, fetches details + comments + * in parallel. Produces output matching FetchDataResult so downstream code * (formatter, prompt builder, branch setup) works unchanged. - * - * Comments and reviews are empty in M4a — M4b will populate them. */ export async function fetchForgejoData({ octokits, @@ -87,8 +191,11 @@ export async function fetchForgejoData({ prNumber, isPR, triggerUsername, + triggerTime, originalTitle, originalBody, + includeCommentsByActor, + excludeCommentsByActor, }: FetchForgejoDataParams): Promise { const [owner, repo] = repository.split("/"); if (!owner || !repo) { @@ -99,10 +206,19 @@ export async function fetchForgejoData({ let contextData: GitHubPullRequest | GitHubIssue; let changedFiles: GitHubFile[] = []; let changedFilesWithSHA: GitHubFileWithSHA[] = []; + let rawComments: GitHubComment[] = []; + let rawReviews: GitHubReview[] = []; if (isPR) { - // Fetch PR details, files, and commits in parallel - const [prResponse, filesResponse, commitsResponse] = await Promise.all([ + // Fetch PR details, files, commits, comments, reviews, and inline comments in parallel + const [ + prResponse, + filesResponse, + commitsResponse, + commentsResponse, + reviewsResponse, + reviewCommentsResponse, + ] = await Promise.all([ octokits.rest.pulls.get({ owner, repo, pull_number: number }), octokits.rest.pulls.listFiles({ owner, @@ -116,12 +232,37 @@ export async function fetchForgejoData({ pull_number: number, per_page: 100, }), + octokits.rest.issues.listComments({ + owner, + repo, + issue_number: number, + per_page: 100, + }), + octokits.rest.pulls.listReviews({ + owner, + repo, + pull_number: number, + per_page: 100, + }), + octokits.rest.pulls.listReviewComments({ + owner, + repo, + pull_number: number, + per_page: 100, + }), ]); const pr = prResponse.data; const files = filesResponse.data; const commits = commitsResponse.data; + // Map REST responses to internal types + rawComments = mapRestComments(commentsResponse.data); + const mappedInlineComments = mapRestReviewComments( + reviewCommentsResponse.data, + ); + rawReviews = mapRestReviews(reviewsResponse.data, mappedInlineComments); + // Map REST file objects to GitHubFile type changedFiles = files.map((f) => ({ path: f.filename, @@ -164,18 +305,29 @@ export async function fetchForgejoData({ })), }, files: { nodes: changedFiles }, - comments: { nodes: [] as GitHubComment[] }, // M4b - reviews: { nodes: [] as GitHubReview[] }, // M4b + comments: { nodes: rawComments }, + reviews: { nodes: rawReviews }, }; console.log(`Successfully fetched PR #${prNumber} data`); } else { - // Fetch issue details - const { data: issue } = await octokits.rest.issues.get({ - owner, - repo, - issue_number: number, - }); + // Fetch issue details and comments in parallel + const [issueResponse, commentsResponse] = await Promise.all([ + octokits.rest.issues.get({ + owner, + repo, + issue_number: number, + }), + octokits.rest.issues.listComments({ + owner, + repo, + issue_number: number, + per_page: 100, + }), + ]); + + const issue = issueResponse.data; + rawComments = mapRestComments(commentsResponse.data); // Map REST response to GitHubIssue type contextData = { @@ -191,7 +343,7 @@ export async function fetchForgejoData({ name: typeof l === "string" ? l : (l.name ?? ""), })), }, - comments: { nodes: [] as GitHubComment[] }, // M4b + comments: { nodes: rawComments }, }; console.log(`Successfully fetched issue #${prNumber} data`); @@ -225,6 +377,126 @@ export async function fetchForgejoData({ contextData.title = originalTitle; } + // Apply TOCTOU + actor filtering to issue-level comments + const comments = filterCommentsByActor( + filterCommentsToTriggerTime(rawComments, triggerTime), + includeCommentsByActor, + excludeCommentsByActor, + ); + + // Build reviewData with filtering (PRs only) + let reviewData: { nodes: GitHubReview[] } | null = null; + if (isPR) { + // Deep-clone reviews so filtering doesn't mutate contextData + let filteredReviews = rawReviews.map((r) => ({ + ...r, + comments: { nodes: [...r.comments.nodes] }, + })); + + // Actor-filter reviews by review author + filteredReviews = filterCommentsByActor( + filteredReviews, + includeCommentsByActor, + excludeCommentsByActor, + ); + + // Actor-filter inline comments within each review + for (const review of filteredReviews) { + review.comments.nodes = filterCommentsByActor( + review.comments.nodes, + includeCommentsByActor, + excludeCommentsByActor, + ); + } + + reviewData = { nodes: filteredReviews }; + } + + // --- Image processing --- + // Prepare issue-level comments for image URL extraction + const issueComments: CommentWithImages[] = comments + .filter((c) => c.body && !c.isMinimized) + .map((c) => ({ + type: "issue_comment" as const, + id: c.databaseId, + body: c.body, + })); + + // Filter review bodies to trigger time for image processing + const filteredReviewBodies = reviewData?.nodes + ? filterReviewsToTriggerTime(reviewData.nodes, triggerTime).filter( + (r) => r.body, + ) + : []; + + const reviewBodies: CommentWithImages[] = filteredReviewBodies.map((r) => ({ + type: "review_body" as const, + id: r.databaseId, + pullNumber: prNumber, + body: r.body, + })); + + // Filter inline review comments to trigger time for image processing + const allReviewComments = + reviewData?.nodes?.flatMap((r) => r.comments?.nodes ?? []) ?? []; + const filteredReviewComments = filterCommentsToTriggerTime( + allReviewComments, + triggerTime, + ); + + const reviewCommentImages: CommentWithImages[] = filteredReviewComments + .filter((c) => c.body && !c.isMinimized) + .map((c) => ({ + type: "review_comment" as const, + id: c.databaseId, + body: c.body, + })); + + // Process main body for images (with TOCTOU check) + let mainBody: CommentWithImages[] = []; + if (contextData.body) { + if ( + originalBody !== undefined || + isBodySafeToUse(contextData, triggerTime) + ) { + mainBody = [ + isPR + ? { + type: "pr_body" as const, + pullNumber: prNumber, + body: contextData.body, + } + : { + type: "issue_body" as const, + issueNumber: prNumber, + body: contextData.body, + }, + ]; + } else { + console.warn( + `Security: ${isPR ? "PR" : "Issue"} #${prNumber} body was edited after the trigger event. ` + + `Excluding body content to prevent potential injection attacks.`, + ); + } + } + + const allCommentsForImages = [ + ...mainBody, + ...issueComments, + ...reviewBodies, + ...reviewCommentImages, + ]; + + // Download images — uses Octokit REST. On Forgejo the GitHub-specific URL + // patterns won't match, so this is effectively a no-op for Forgejo-hosted + // images. Safe to call regardless. + const imageUrlMap = await downloadCommentImages( + octokits, + owner, + repo, + allCommentsForImages, + ); + // Fetch trigger user display name via REST (replaces GraphQL USER_QUERY) let triggerDisplayName: string | null | undefined; if (triggerUsername) { @@ -236,11 +508,11 @@ export async function fetchForgejoData({ return { contextData, - comments: [], // M4b will populate + comments, changedFiles, changedFilesWithSHA, - reviewData: isPR ? { nodes: [] } : null, // M4b will populate - imageUrlMap: new Map(), // M4b will populate + reviewData, + imageUrlMap, triggerDisplayName, }; } diff --git a/test/forgejo-data-fetcher.test.ts b/test/forgejo-data-fetcher.test.ts index ca1b69b..f20cb20 100644 --- a/test/forgejo-data-fetcher.test.ts +++ b/test/forgejo-data-fetcher.test.ts @@ -1,18 +1,36 @@ -import { describe, expect, it, jest } from "bun:test"; -import { fetchForgejoData } from "../src/forgejo/data/fetcher"; +import { describe, expect, it, jest, mock } from "bun:test"; +import { + fetchForgejoData, + mapFileStatus, + mapRestComments, + mapRestReviewComments, + mapRestReviews, +} from "../src/forgejo/data/fetcher"; import type { Octokits } from "../src/github/api/client"; +import type { GitHubReviewComment } from "../src/github/types"; + +// Mock image downloader — returns empty map (no-op on Forgejo) +mock.module("../src/github/utils/image-downloader", () => ({ + downloadCommentImages: jest.fn().mockResolvedValue(new Map()), +})); // Helper to build a mock Octokits with REST methods function createMockOctokits({ prData, filesData, commitsData, + commentsData, + reviewsData, + reviewCommentsData, issueData, userData, }: { prData?: Record; filesData?: Record[]; commitsData?: Record[]; + commentsData?: Record[]; + reviewsData?: Record[]; + reviewCommentsData?: Record[]; issueData?: Record; userData?: Record; } = {}) { @@ -22,9 +40,14 @@ function createMockOctokits({ get: jest.fn().mockResolvedValue({ data: prData ?? {} }), listFiles: jest.fn().mockResolvedValue({ data: filesData ?? [] }), listCommits: jest.fn().mockResolvedValue({ data: commitsData ?? [] }), + listReviews: jest.fn().mockResolvedValue({ data: reviewsData ?? [] }), + listReviewComments: jest + .fn() + .mockResolvedValue({ data: reviewCommentsData ?? [] }), }, issues: { get: jest.fn().mockResolvedValue({ data: issueData ?? {} }), + listComments: jest.fn().mockResolvedValue({ data: commentsData ?? [] }), }, users: { getByUsername: jest @@ -91,7 +114,7 @@ const mockIssueRestResponse = { describe("fetchForgejoData", () => { describe("PR data fetching", () => { - it("should fetch PR details, files, and commits in parallel", async () => { + it("should fetch PR details, files, commits, comments, reviews, and inline comments in parallel", async () => { const mockOctokits = createMockOctokits({ prData: mockPrRestResponse, filesData: mockFilesRestResponse, @@ -105,7 +128,7 @@ describe("fetchForgejoData", () => { isPR: true, }); - // Verify all three REST calls were made + // Verify all six REST calls were made expect(mockOctokits.rest.pulls.get).toHaveBeenCalledWith({ owner: "test-owner", repo: "test-repo", @@ -123,6 +146,24 @@ describe("fetchForgejoData", () => { pull_number: 42, per_page: 100, }); + expect(mockOctokits.rest.issues.listComments).toHaveBeenCalledWith({ + owner: "test-owner", + repo: "test-repo", + issue_number: 42, + per_page: 100, + }); + expect(mockOctokits.rest.pulls.listReviews).toHaveBeenCalledWith({ + owner: "test-owner", + repo: "test-repo", + pull_number: 42, + per_page: 100, + }); + expect(mockOctokits.rest.pulls.listReviewComments).toHaveBeenCalledWith({ + owner: "test-owner", + repo: "test-repo", + pull_number: 42, + per_page: 100, + }); }); it("should map PR fields to GitHubPullRequest type", async () => { @@ -210,13 +251,9 @@ describe("fetchForgejoData", () => { const pr = result.contextData as any; expect(pr.commits.totalCount).toBe(2); expect(pr.commits.nodes[0].commit.oid).toBe("commit1sha"); - expect(pr.commits.nodes[0].commit.message).toBe( - "fix: patch memory leak", - ); + expect(pr.commits.nodes[0].commit.message).toBe("fix: patch memory leak"); expect(pr.commits.nodes[0].commit.author.name).toBe("Test Author"); - expect(pr.commits.nodes[0].commit.author.email).toBe( - "test@example.com", - ); + expect(pr.commits.nodes[0].commit.author.email).toBe("test@example.com"); }); }); @@ -346,7 +383,7 @@ describe("fetchForgejoData", () => { }); describe("issue data fetching", () => { - it("should fetch issue details", async () => { + it("should fetch issue details and comments in parallel", async () => { const mockOctokits = createMockOctokits({ issueData: mockIssueRestResponse, }); @@ -363,11 +400,19 @@ describe("fetchForgejoData", () => { repo: "test-repo", issue_number: 10, }); + expect(mockOctokits.rest.issues.listComments).toHaveBeenCalledWith({ + owner: "test-owner", + repo: "test-repo", + issue_number: 10, + per_page: 100, + }); // Should NOT call PR endpoints expect(mockOctokits.rest.pulls.get).not.toHaveBeenCalled(); expect(mockOctokits.rest.pulls.listFiles).not.toHaveBeenCalled(); expect(mockOctokits.rest.pulls.listCommits).not.toHaveBeenCalled(); + expect(mockOctokits.rest.pulls.listReviews).not.toHaveBeenCalled(); + expect(mockOctokits.rest.pulls.listReviewComments).not.toHaveBeenCalled(); }); it("should map issue fields to GitHubIssue type", async () => { @@ -564,12 +609,129 @@ describe("fetchForgejoData", () => { }); }); - describe("M4a placeholders (comments/reviews/images)", () => { - it("should return empty comments array", async () => { + describe("comments and reviews (M4b)", () => { + const mockCommentsData = [ + { + id: 100, + body: "First comment", + user: { login: "commenter1" }, + created_at: "2024-01-15T09:00:00Z", + updated_at: "2024-01-15T09:00:00Z", + }, + { + id: 101, + body: "Second comment", + user: { login: "commenter2" }, + created_at: "2024-01-15T09:30:00Z", + updated_at: "2024-01-15T09:30:00Z", + }, + ]; + + const mockReviewsData = [ + { + id: 200, + body: "LGTM", + user: { login: "reviewer" }, + state: "APPROVED", + submitted_at: "2024-01-15T10:00:00Z", + }, + ]; + + const mockReviewCommentsData = [ + { + id: 300, + body: "Fix this line", + user: { login: "reviewer" }, + path: "src/main.ts", + line: 42, + original_line: 42, + created_at: "2024-01-15T10:00:00Z", + updated_at: "2024-01-15T10:00:00Z", + pull_request_review_id: 200, + }, + ]; + + it("should fetch and return comments for PRs", async () => { const mockOctokits = createMockOctokits({ prData: mockPrRestResponse, filesData: [], commitsData: [], + commentsData: mockCommentsData, + }); + + const result = await fetchForgejoData({ + octokits: mockOctokits, + repository: "test-owner/test-repo", + prNumber: "42", + isPR: true, + }); + + expect(result.comments).toHaveLength(2); + expect(result.comments[0]!.id).toBe("100"); + expect(result.comments[0]!.body).toBe("First comment"); + expect(result.comments[0]!.author.login).toBe("commenter1"); + expect(result.comments[1]!.body).toBe("Second comment"); + }); + + it("should fetch and return reviews with grouped inline comments", async () => { + const mockOctokits = createMockOctokits({ + prData: mockPrRestResponse, + filesData: [], + commitsData: [], + reviewsData: mockReviewsData, + reviewCommentsData: mockReviewCommentsData, + }); + + const result = await fetchForgejoData({ + octokits: mockOctokits, + repository: "test-owner/test-repo", + prNumber: "42", + isPR: true, + }); + + expect(result.reviewData).not.toBeNull(); + expect(result.reviewData!.nodes).toHaveLength(1); + expect(result.reviewData!.nodes[0]!.body).toBe("LGTM"); + expect(result.reviewData!.nodes[0]!.state).toBe("APPROVED"); + expect(result.reviewData!.nodes[0]!.comments.nodes).toHaveLength(1); + expect(result.reviewData!.nodes[0]!.comments.nodes[0]!.body).toBe( + "Fix this line", + ); + expect(result.reviewData!.nodes[0]!.comments.nodes[0]!.path).toBe( + "src/main.ts", + ); + }); + + it("should populate contextData.comments.nodes and contextData.reviews.nodes", async () => { + const mockOctokits = createMockOctokits({ + prData: mockPrRestResponse, + filesData: [], + commitsData: [], + commentsData: mockCommentsData, + reviewsData: mockReviewsData, + reviewCommentsData: mockReviewCommentsData, + }); + + const result = await fetchForgejoData({ + octokits: mockOctokits, + repository: "test-owner/test-repo", + prNumber: "42", + isPR: true, + }); + + const pr = result.contextData as any; + expect(pr.comments.nodes).toHaveLength(2); + expect(pr.reviews.nodes).toHaveLength(1); + }); + + it("should return empty comments/reviewData when no data", async () => { + const mockOctokits = createMockOctokits({ + prData: mockPrRestResponse, + filesData: [], + commitsData: [], + commentsData: [], + reviewsData: [], + reviewCommentsData: [], }); const result = await fetchForgejoData({ @@ -580,13 +742,47 @@ describe("fetchForgejoData", () => { }); expect(result.comments).toEqual([]); + expect(result.reviewData).toEqual({ nodes: [] }); }); - it("should return empty reviewData nodes for PRs", async () => { + it("should fetch and return comments for issues", async () => { + const mockOctokits = createMockOctokits({ + issueData: mockIssueRestResponse, + commentsData: mockCommentsData, + }); + + const result = await fetchForgejoData({ + octokits: mockOctokits, + repository: "test-owner/test-repo", + prNumber: "10", + isPR: false, + }); + + expect(result.comments).toHaveLength(2); + expect(result.comments[0]!.body).toBe("First comment"); + }); + + it("should apply TOCTOU filtering to comments", async () => { const mockOctokits = createMockOctokits({ prData: mockPrRestResponse, filesData: [], commitsData: [], + commentsData: [ + { + id: 1, + body: "Before trigger", + user: { login: "alice" }, + created_at: "2024-01-15T09:00:00Z", + updated_at: "2024-01-15T09:00:00Z", + }, + { + id: 2, + body: "After trigger", + user: { login: "bob" }, + created_at: "2024-01-15T11:00:00Z", + updated_at: "2024-01-15T11:00:00Z", + }, + ], }); const result = await fetchForgejoData({ @@ -594,12 +790,145 @@ describe("fetchForgejoData", () => { repository: "test-owner/test-repo", prNumber: "42", isPR: true, + triggerTime: "2024-01-15T10:00:00Z", }); - expect(result.reviewData).toEqual({ nodes: [] }); + // Only the comment before trigger time should be included + expect(result.comments).toHaveLength(1); + expect(result.comments[0]!.body).toBe("Before trigger"); }); - it("should return empty imageUrlMap", async () => { + it("should apply actor filtering to comments", async () => { + const mockOctokits = createMockOctokits({ + prData: mockPrRestResponse, + filesData: [], + commitsData: [], + commentsData: [ + { + id: 1, + body: "Human comment", + user: { login: "alice" }, + created_at: "2024-01-15T09:00:00Z", + updated_at: "2024-01-15T09:00:00Z", + }, + { + id: 2, + body: "Bot comment", + user: { login: "dependabot[bot]" }, + created_at: "2024-01-15T09:00:00Z", + updated_at: "2024-01-15T09:00:00Z", + }, + ], + }); + + const result = await fetchForgejoData({ + octokits: mockOctokits, + repository: "test-owner/test-repo", + prNumber: "42", + isPR: true, + excludeCommentsByActor: "*[bot]", + }); + + expect(result.comments).toHaveLength(1); + expect(result.comments[0]!.author.login).toBe("alice"); + }); + + it("should actor-filter reviews without mutating contextData", async () => { + const mockOctokits = createMockOctokits({ + prData: mockPrRestResponse, + filesData: [], + commitsData: [], + reviewsData: [ + { + id: 200, + body: "Bot review", + user: { login: "bot[bot]" }, + state: "COMMENTED", + submitted_at: "2024-01-15T10:00:00Z", + }, + { + id: 201, + body: "Human review", + user: { login: "alice" }, + state: "APPROVED", + submitted_at: "2024-01-15T10:30:00Z", + }, + ], + reviewCommentsData: [], + }); + + const result = await fetchForgejoData({ + octokits: mockOctokits, + repository: "test-owner/test-repo", + prNumber: "42", + isPR: true, + excludeCommentsByActor: "*[bot]", + }); + + // contextData should still have both reviews (unfiltered) + const pr = result.contextData as any; + expect(pr.reviews.nodes).toHaveLength(2); + + // reviewData should only have the human review (filtered) + expect(result.reviewData!.nodes).toHaveLength(1); + expect(result.reviewData!.nodes[0]!.author.login).toBe("alice"); + }); + + it("should actor-filter inline comments within reviews", async () => { + const mockOctokits = createMockOctokits({ + prData: mockPrRestResponse, + filesData: [], + commitsData: [], + reviewsData: [ + { + id: 200, + body: "Review", + user: { login: "alice" }, + state: "COMMENTED", + submitted_at: "2024-01-15T10:00:00Z", + }, + ], + reviewCommentsData: [ + { + id: 300, + body: "Human inline", + user: { login: "alice" }, + path: "file.ts", + line: 10, + original_line: 10, + created_at: "2024-01-15T10:00:00Z", + updated_at: "2024-01-15T10:00:00Z", + pull_request_review_id: 200, + }, + { + id: 301, + body: "Bot inline", + user: { login: "bot[bot]" }, + path: "file.ts", + line: 20, + original_line: 20, + created_at: "2024-01-15T10:00:00Z", + updated_at: "2024-01-15T10:00:00Z", + pull_request_review_id: 200, + }, + ], + }); + + const result = await fetchForgejoData({ + octokits: mockOctokits, + repository: "test-owner/test-repo", + prNumber: "42", + isPR: true, + excludeCommentsByActor: "*[bot]", + }); + + expect(result.reviewData!.nodes[0]!.comments.nodes).toHaveLength(1); + expect(result.reviewData!.nodes[0]!.comments.nodes[0]!.author.login).toBe( + "alice", + ); + }); + + it("should return imageUrlMap (empty on Forgejo due to URL pattern mismatch)", async () => { const mockOctokits = createMockOctokits({ issueData: mockIssueRestResponse, }); @@ -615,6 +944,49 @@ describe("fetchForgejoData", () => { }); }); + describe("TOCTOU filtering for issue comments", () => { + it("should exclude comments edited after trigger time", async () => { + const mockOctokits = createMockOctokits({ + issueData: mockIssueRestResponse, + commentsData: [ + { + id: 1, + body: "Early comment", + user: { login: "alice" }, + created_at: "2024-01-15T08:00:00Z", + updated_at: "2024-01-15T08:00:00Z", + }, + { + id: 2, + body: "Comment edited after trigger", + user: { login: "bob" }, + created_at: "2024-01-15T08:30:00Z", + updated_at: "2024-01-15T11:00:00Z", + }, + { + id: 3, + body: "Late comment", + user: { login: "charlie" }, + created_at: "2024-01-15T11:00:00Z", + updated_at: "2024-01-15T11:00:00Z", + }, + ], + }); + + const result = await fetchForgejoData({ + octokits: mockOctokits, + repository: "test-owner/test-repo", + prNumber: "5", + isPR: false, + triggerTime: "2024-01-15T10:00:00Z", + }); + + // Only the early, unedited comment should be included + expect(result.comments).toHaveLength(1); + expect(result.comments[0]!.body).toBe("Early comment"); + }); + }); + describe("error handling", () => { it("should throw on invalid repository format", async () => { const mockOctokits = createMockOctokits(); @@ -724,3 +1096,250 @@ describe("fetchForgejoData", () => { }); }); }); + +describe("mapFileStatus", () => { + it("maps REST file statuses to GraphQL-style enums", () => { + expect(mapFileStatus("added")).toBe("ADDED"); + expect(mapFileStatus("removed")).toBe("DELETED"); + expect(mapFileStatus("modified")).toBe("MODIFIED"); + expect(mapFileStatus("renamed")).toBe("RENAMED"); + expect(mapFileStatus("copied")).toBe("COPIED"); + expect(mapFileStatus("changed")).toBe("MODIFIED"); + expect(mapFileStatus("unchanged")).toBe("MODIFIED"); + }); + + it("uppercases unknown statuses", () => { + expect(mapFileStatus("something_else")).toBe("SOMETHING_ELSE"); + }); +}); + +describe("mapRestComments", () => { + it("maps REST comment objects to GitHubComment type", () => { + const result = mapRestComments([ + { + id: 42, + body: "Hello", + user: { login: "alice" }, + created_at: "2024-01-15T10:00:00Z", + updated_at: "2024-01-15T11:00:00Z", + }, + ]); + + expect(result).toHaveLength(1); + expect(result[0]).toEqual({ + id: "42", + databaseId: "42", + body: "Hello", + author: { login: "alice" }, + createdAt: "2024-01-15T10:00:00Z", + updatedAt: "2024-01-15T11:00:00Z", + lastEditedAt: undefined, + isMinimized: false, + }); + }); + + it("handles null body and user", () => { + const result = mapRestComments([ + { + id: 1, + body: null, + user: null, + created_at: "2024-01-15T10:00:00Z", + updated_at: "2024-01-15T10:00:00Z", + }, + ]); + + expect(result[0]!.body).toBe(""); + expect(result[0]!.author.login).toBe("unknown"); + }); + + it("handles empty array", () => { + expect(mapRestComments([])).toEqual([]); + }); +}); + +describe("mapRestReviewComments", () => { + it("maps inline review comments with reviewId", () => { + const result = mapRestReviewComments([ + { + id: 100, + body: "Fix this", + user: { login: "bob" }, + path: "src/main.ts", + line: 42, + original_line: 40, + created_at: "2024-01-15T10:00:00Z", + updated_at: "2024-01-15T10:00:00Z", + pull_request_review_id: 200, + }, + ]); + + expect(result).toHaveLength(1); + expect(result[0]!.path).toBe("src/main.ts"); + expect(result[0]!.line).toBe(42); + expect(result[0]!.reviewId).toBe(200); + }); + + it("falls back to original_line when line is null", () => { + const result = mapRestReviewComments([ + { + id: 101, + body: "comment", + user: { login: "bob" }, + path: "file.ts", + line: null, + original_line: 10, + created_at: "2024-01-15T10:00:00Z", + updated_at: "2024-01-15T10:00:00Z", + }, + ]); + + expect(result[0]!.line).toBe(10); + }); + + it("returns null line when both line and original_line are null", () => { + const result = mapRestReviewComments([ + { + id: 102, + body: "comment", + user: { login: "bob" }, + path: "file.ts", + line: null, + original_line: null, + created_at: "2024-01-15T10:00:00Z", + updated_at: "2024-01-15T10:00:00Z", + }, + ]); + + expect(result[0]!.line).toBeNull(); + }); +}); + +describe("mapRestReviews", () => { + it("groups inline comments by review ID", () => { + const inlineComments: (GitHubReviewComment & { + reviewId: number | null; + })[] = [ + { + id: "100", + databaseId: "100", + body: "Comment for review 200", + author: { login: "alice" }, + createdAt: "2024-01-15T10:00:00Z", + updatedAt: "2024-01-15T10:00:00Z", + path: "file.ts", + line: 10, + reviewId: 200, + }, + { + id: "101", + databaseId: "101", + body: "Another for review 200", + author: { login: "alice" }, + createdAt: "2024-01-15T10:00:00Z", + updatedAt: "2024-01-15T10:00:00Z", + path: "file.ts", + line: 20, + reviewId: 200, + }, + ]; + + const result = mapRestReviews( + [ + { + id: 200, + body: "Review", + user: { login: "alice" }, + state: "COMMENTED", + submitted_at: "2024-01-15T10:00:00Z", + }, + ], + inlineComments, + ); + + expect(result[0]!.comments.nodes).toHaveLength(2); + }); + + it("uppercases review state", () => { + const result = mapRestReviews( + [ + { + id: 200, + body: "", + user: { login: "alice" }, + state: "changes_requested", + submitted_at: "2024-01-15T10:00:00Z", + }, + ], + [], + ); + + expect(result[0]!.state).toBe("CHANGES_REQUESTED"); + }); + + it("excludes orphan inline comments (null reviewId)", () => { + const inlineComments: (GitHubReviewComment & { + reviewId: number | null; + })[] = [ + { + id: "100", + databaseId: "100", + body: "orphan", + author: { login: "alice" }, + createdAt: "2024-01-15T10:00:00Z", + updatedAt: "2024-01-15T10:00:00Z", + path: "file.ts", + line: 1, + reviewId: null, + }, + ]; + + const result = mapRestReviews( + [ + { + id: 200, + body: "", + user: { login: "alice" }, + state: "COMMENTED", + submitted_at: "2024-01-15T10:00:00Z", + }, + ], + inlineComments, + ); + + expect(result[0]!.comments.nodes).toEqual([]); + }); + + it("strips reviewId from stored comments", () => { + const inlineComments: (GitHubReviewComment & { + reviewId: number | null; + })[] = [ + { + id: "100", + databaseId: "100", + body: "inline", + author: { login: "alice" }, + createdAt: "2024-01-15T10:00:00Z", + updatedAt: "2024-01-15T10:00:00Z", + path: "file.ts", + line: 5, + reviewId: 200, + }, + ]; + + const result = mapRestReviews( + [ + { + id: 200, + body: "", + user: { login: "alice" }, + state: "COMMENTED", + submitted_at: "2024-01-15T10:00:00Z", + }, + ], + inlineComments, + ); + + expect((result[0]!.comments.nodes[0] as any).reviewId).toBeUndefined(); + }); +});