public/claude-code-action
1
0 Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

feat(prd-1): implement M4b - Forgejo REST comment/review fetching

Browse Source 
- 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 <noreply@anthropic.com>
This commit is contained in:
Vlad MocanuandClaude Opus 4.6 committed 2026-02-25 17:00:13 +02:00
1 parent 6e104a33dc
commit 4b0737c54c
4 files changed
+1001 -100
Download Patch File Download Diff File Expand all files Collapse all files

No files matched your search

prds/1-forgejo-platform-support.md
+70 -57
View File
@@ -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