Vlad MocanuandClaude Opus 4.6
d9c22ffe50
E2E test confirmed: Claude correctly responds to @claude mentions in PR review comments on Forgejo (run #12075, PR #15). Three bugs were found and fixed during validation: 1. Missing comment object in Forgejo webhook payload 2. Event name mismatch (pull_request_comment vs pull_request_review_comment) 3. MCP server routing updates to wrong API (critical — silently dropped responses) Progress: 3/8 milestones complete (37.5%) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
116 lines
7.7 KiB
Markdown
116 lines
7.7 KiB
Markdown
# PRD #6: E2E Test All Untested Forgejo Trigger Scenarios
|
||
|
||
**Issue**: [#6](https://git.wxs.ro/actions/claude-code-action/issues/6)
|
||
**Priority**: High
|
||
**Created**: 2026-02-25
|
||
|
||
---
|
||
|
||
## Problem
|
||
|
||
The Forgejo platform support (PRD #1) was validated for the two most common scenarios (agent mode via `workflow_dispatch` and tag mode via `issue_comment`). However, 5 trigger scenarios remain untested in a live environment:
|
||
|
||
1. PR review comment triggers (`pull_request_review_comment`)
|
||
2. PR review triggers (`pull_request_review`)
|
||
3. Label triggers (`issues` with `labeled` action)
|
||
4. Assignee triggers (`issues` with `assigned` action)
|
||
5. Branch creation + code changes from tag mode (the initial test only asked a question, no code was generated)
|
||
|
||
These untested paths could have silent failures — the code may look correct but break on Forgejo's subtly different webhook payloads, API responses, or event structures.
|
||
|
||
## Solution
|
||
|
||
Systematically exercise each untested scenario on the live Forgejo instance (git.wxs.ro), document results, fix any issues discovered, and update the E2E status table in `docs/forgejo.md`.
|
||
|
||
## Milestones
|
||
|
||
- [x] **M1: Ensure workflow covers all trigger types**
|
||
|
||
- Review `.forgejo/workflows/claude.yml` to confirm all 5 trigger types are listed
|
||
- Add any missing triggers to the workflow file
|
||
- Ensure the workflow has the correct permissions for each trigger type
|
||
- **Validation**: Workflow YAML includes all required `on:` triggers
|
||
|
||
- [x] **M1.5: Fix image-downloader test failures blocking CI**
|
||
|
||
- **Problem**: All 18 `downloadCommentImages` tests failed on Forgejo CI (Linux Bun 1.2.21 in `harbor.wxs.ro/wxs/grunt-runner:v1.9.0`) while passing locally on macOS. This blocked all PRs.
|
||
- **Root cause**: `mock.module("../src/github/utils/image-downloader")` in `test/forgejo-data-fetcher.test.ts` replaced the image-downloader module **globally** in Bun's module registry for the entire process. When Bun's non-deterministic test file ordering ran forgejo-data-fetcher.test.ts BEFORE image-downloader.test.ts, the latter imported the stubbed module instead of the real one, breaking all mock tracking. The failure was intermittent because file ordering changed when other files were added/removed.
|
||
- **Fix (2 parts)**:
|
||
1. `c6ca8a7`: Added DI (`FsDeps` interface + optional `fsDeps` param) to `downloadCommentImages()` — removed `mock.module("fs/promises")` from the image-downloader test itself
|
||
2. `49eedd5`: Removed all image-downloader mocking from forgejo-data-fetcher.test.ts entirely — the real function is safe to call because test comments have no GitHub image URLs, so it returns an empty Map naturally. Both `mock.module` and `spyOn` caused cross-file pollution in Bun's single-process runner.
|
||
- **Key learning**: Never use `mock.module()` in Bun test files — it poisons the module registry for all files in the process. `spyOn` on ESM namespace exports also fails to restore correctly across files on Linux Bun. Best approach: avoid mocking entirely when the real function is safe to call.
|
||
- **Validation**: CI passed (all 3 jobs: test, prettier, typecheck) on commit `49eedd5`
|
||
|
||
- [x] **M2: Test PR review comment trigger**
|
||
|
||
- Create a test PR on the repo ✓ (PR #15)
|
||
- Post a review comment with `@claude` mention ✓ (review comments on `test/e2e-prd6-m2.txt`)
|
||
- Verify: platform detection, context fetching (PR diff, review comment body), tracking comment, Claude response ✓
|
||
- Document any issues and fix them ✓ (3 bugs found and fixed, see below)
|
||
- **Validation**: Claude responds correctly to `@claude` in a PR review comment ✓ (run #12075, 32s, full PR summary posted)
|
||
- **Bugs found & fixed**:
|
||
1. `9d8c96f`–`ac8e22e`: Forgejo `pull_request_review_comment` payload lacks the top-level `comment` object — added fallback to issue comment creation and 404 fallback for review comment fetch
|
||
2. `e73c881`: Forgejo sends event name `pull_request_comment` (not `pull_request_review_comment`) — skipped `commentBody` validation for this event
|
||
3. `c3c3eb5`: **Critical** — MCP comment server set `isPullRequestReviewComment=true` on Forgejo, causing every Claude update to try `pulls.updateReviewComment` (404) and silently fail. Fix: detect Forgejo via `GITHUB_API_URL` and route to issue comment API. Also hardened `update-claude-comment.ts` fallback to catch 422 alongside 404, and skipped the wasted `pulls.getReviewComment` call in `update-comment-link.ts` on Forgejo.
|
||
- **Key learning**: On Forgejo, the tracking comment for review comment events is always an issue comment (not a PR review comment reply) because the payload lacks `comment.id`. All update paths must account for this.
|
||
|
||
- [ ] **M3: Test PR review trigger**
|
||
|
||
- Create a test PR on the repo
|
||
- Submit a review with `@claude` mention in the review body
|
||
- Verify: platform detection, context fetching, tracking comment, Claude response
|
||
- Document any issues and fix them
|
||
- **Validation**: Claude responds correctly to `@claude` in a PR review
|
||
|
||
- [ ] **M4: Test label trigger**
|
||
|
||
- Create a label (e.g., `claude`) that triggers the action when applied to an issue
|
||
- Apply the label to a test issue
|
||
- Verify: trigger detection, context fetching (issue body/title), Claude response
|
||
- Document any issues and fix them
|
||
- **Validation**: Claude responds when the configured label is applied to an issue
|
||
|
||
- [ ] **M5: Test assignee trigger**
|
||
|
||
- Assign a user to an issue to trigger the action
|
||
- Verify: trigger detection, context fetching, Claude response
|
||
- Document any issues and fix them
|
||
- **Validation**: Claude responds when assigned to an issue
|
||
|
||
- [ ] **M6: Test tag mode with code changes (branch creation)**
|
||
|
||
- Create an issue requesting a code change (e.g., "add a comment to file X")
|
||
- Trigger Claude via `@claude` mention
|
||
- Verify: Claude creates a branch, pushes commits, links the branch in tracking comment
|
||
- Verify the Forgejo-specific PR compare URL format is correct
|
||
- **Validation**: Claude successfully creates a branch with code changes on Forgejo
|
||
|
||
- [ ] **M7: Update documentation with E2E results**
|
||
- Update `docs/forgejo.md` E2E status table with results for all 5 scenarios
|
||
- Move tested items from "Not Yet E2E Tested" to the validated table
|
||
- Document any Forgejo-specific quirks or limitations discovered
|
||
- Add troubleshooting entries for any issues encountered
|
||
- **Validation**: `docs/forgejo.md` accurately reflects the current E2E validation status
|
||
|
||
## Dependencies
|
||
|
||
- PRD #5 (Dedicated Forgejo User) is recommended but not strictly required — tests can run with the current token setup
|
||
- A test repository on git.wxs.ro with the action workflow configured
|
||
|
||
## Risks
|
||
|
||
| Risk | Mitigation |
|
||
| -------------------------------------------------------- | ----------------------------------------------------------- |
|
||
| Forgejo webhook payloads differ from expected | Log raw payloads, compare with GitHub format, adapt parsing |
|
||
| Trigger type not supported by Forgejo Actions | Document as known limitation, skip gracefully |
|
||
| Action creates unwanted branches/comments during testing | Use a dedicated test repo or clean up after each test |
|
||
| Bun mock.module breaks mock tracking on Linux | RESOLVED: use spyOn on module namespace, never mock.module |
|
||
| MCP comment updates silently fail on Forgejo | RESOLVED: route to issue comment API when platform=forgejo |
|
||
|
||
## Success Criteria
|
||
|
||
- All 5 previously untested scenarios have been exercised on a live Forgejo instance
|
||
- Any bugs discovered are fixed and tested
|
||
- `docs/forgejo.md` E2E status table is fully up-to-date
|
||
- "Not Yet E2E Tested" section is empty (all items moved to validated or documented as unsupported)
|