lucas/omp-deck

Watch

Fork 0

Forgejo: submitted reviews continue the implementer session on its own PR #45

New issue

Closed

opened 2026-10-20 13:03:20 +02:00 by omp-deck · 1 comment

omp-deck commented

2026-10-20 13:03:20 +02:00

Collaborator

Implementation: #51

Problem

A review submitted on the implementer session's own pull request should continue that session. That way revisions can be driven through normal Forgejo reviews (implementer split 3/3; splits 1/3 and 2/3 are merged in #47 and #48).

Webhook: classify submitted reviews. Today apps/server/src/forgejo/webhook.ts ignores the review event types, and its header comment wrongly calls them "inline review comments".
One trigger per submitted review, not per inline comment. The turn reads the whole review and its line comments.
No @mention is needed on the agent's own pull request: a review there is addressed to the agent. Guard: the reviewer needs repo write access.
If the agent authored the PR but no implementer session is mapped to it, create one with a catch-up seed. With no linked issue in the PR body, the session is scoped to the PR alone.
Every verdict triggers, including approvals. An approval with no feedback needs no action.
The agent addresses the feedback: changes, commit, push. Then it replies in every line-comment thread with forgejo_create_pr_thread_comment (what changed plus the commit sha, or why nothing changed). It posts one PR comment only when the review body itself had feedback.
The dispatcher seeds the turn with basic info: PR metadata, reviewer, verdict, review body, and the review id and line-comment count when it can resolve them.

Plan reference: KB project/forgejo-integration-plan.md, review part of step 15. Design: project/forgejo-integration.md (Implementer, Reviewer).

Plan

Forgejo facts (from Forgejo source: `services/webhook/notifier.go` `PullRequestReview`, `modules/webhook/type.go`)

A submitted review fires one delivery. X-Forgejo-Event-Type is pull_request_review_approved, pull_request_review_rejected or pull_request_review_comment. X-Forgejo-Event is pull_request_approved, pull_request_rejected or pull_request_comment. Pending reviews do not fire it.
Payload: PullRequestPayload with action: "reviewed", number, pull_request (incl. user, head.ref, head.sha, base.ref, title, body, html_url), sender = reviewer, and review: { type, content } (content = review body). No review id.
repository.permissions in this payload is computed for the PR author, not the reviewer. On the agent's own PR it is the agent's permission, so the existing perms.push === true check in ForgejoDispatcher.gate (dispatcher.ts ~L290) proves nothing for reviews.
PR conversation comments stay Event issue_comment / Event-Type pull_request_comment; that path is unchanged.

How the merged implementer code works (read before editing)

ForgejoImplementerScope (apps/server/src/bridge/session-policy.ts ~L51): { owner, repo, number /* issue */, baseBranch }. PRs the session opened are not in the scope. InProcessBridge.mapImplementerPr (bridge/in-process.ts ~L745) maps them in forgejo_sessions, and implementerPrNumbers(scope, sessionId) (session-policy ~L231) lists them back.
reuseMapped (dispatcher.ts ~L406) skips the number-equality check for forgejo_implementer, so an issue or PR mapped to an implementer session already routes to it.
resolveSession (~L364) creates forgejo_pull_request / forgejo_conversation sessions only. Implementer sessions are created only by startImplementer (~L585).
Resume restores the checkout in in-process.ts ~L336-351 with kind: "issue" for implementer sessions and number = scope.number.

Step 1: implementer scope can be started from a PR (`session-policy.ts`, `in-process.ts`)

Add an optional field to ForgejoImplementerScope, documented: /** The pull request the session was started from (its checkout is that PR's clone); absent for sessions started for an issue. Equal to \number` when the PR names no issue. */ pull?: number;. It is optional because persisted scope_json` rows from #47/#48 lack it.
implementerPrNumbers: return the mapped numbers except scope.number, plus scope.pull first when it is set and not already in the list. So forgejo_create_pr is denied and the PR line appears in systemContext for PR-started sessions, including PR-only ones.
systemContext: when scope.pull === scope.number (PR-only), replace the title and the issue-oriented bullets with:
- ## Unattended Forgejo implementer session (PR #${pull})
- You maintain pull request #${pull}, which you authored. Nobody is watching this session live: the only channel to humans is Forgejo.
- - Read the pull request with \forgejo_read_pr` and its conversation with `forgejo_list_pr_comments_and_reviews`; no issue is linked to it.`
- - Work on the pull request's branch, commit with \git_commit`, push with `git_push`. Do not open another pull request.`
- - Post blockers, open questions and major decisions as comments on #${pull}.
- keep the existing raw-git and untrusted-data bullets.
Otherwise keep the current text. For all implementer sessions, append the bullet - Reviews submitted on your pull request arrive as turns: answer line comments in their threads with \forgejo_create_pr_thread_comment`.`
decide needs no change: comments on scope.number are allowed, and the PR is either scope.number or mapped to the session.
in-process.ts resume (~L341-351): for forgejo_implementer with scope.pull !== undefined, restore with kind: "pr", number: scope.pull, baseBranch: null. Otherwise keep today's arguments.
Tests in the existing session-policy test file (find it with glob apps/server/src/bridge/session-policy*.test.ts): PR-only scope → the PR-only header, and forgejo_create_pr denied with this session already opened pull request #<pull>. Scope with pull ≠ number and no mapping → forgejo_create_pr denied. Old scope without pull → unchanged behavior.

Step 2: classify review deliveries (`apps/server/src/forgejo/webhook.ts`)

Export const REVIEW_EVENT_TYPES = ["pull_request_review_approved", "pull_request_review_rejected", "pull_request_review_comment"] as const.
resolveEventType: without an Event-Type header, map Event pull_request_approved → pull_request_review_approved, pull_request_rejected → pull_request_review_rejected, and pull_request_comment → pull_request_review_comment. Everything else passes through. Rewrite its doc comment and the file-header bullet (~L14-16): those are submitted reviews, not inline comments.
classifyForgejoDelivery: route REVIEW_EVENT_TYPES to a new classifyReviewSubmitted(eventType, payload, agentUsername), modeled on classifyReviewRequest:
- non-object → malformed "payload is not an object"; action !== "reviewed" → ignore "unsupported action <x>".
- missing sender.login → malformed "missing sender.login"; sender equal to the agent (case-insensitive) → ignore "self-authored".
- repository.full_name and the number (payload.number ?? pull_request.number) are validated with the same messages as classifyReviewRequest.
- accept: dedupeKey = sha256 of JSON.stringify(["reviewed", payload]). event: eventType, action: "reviewed", repo, number, isPull: true, commentAuthor: sender.
- The PR author is not checked here; the dispatcher does that.
No migration: the column comments in db/migrations/010-forgejo-triggers.sql are documentation only.

Step 3: Forgejo calls (`apps/server/src/forgejo/dispatch-api.ts`)

Add to DispatchApi and createDispatchApi, in the style of listComments/readIssue (never throw, return ApiResult):

listReviews(owner, repo, number, signal?): Promise<ApiResult<ReviewSummary[]>> → GET pulls/{n}/reviews via fetchAllPages. Exported interface ReviewSummary { id: number; author: string; state: string; commentsCount: number; submittedAt: string }, from id, user.login, state, comments_count, submitted_at; missing fields become ""/0.
reviewerCanWrite(owner, repo, login, signal?): Promise<ApiResult<boolean>>:
1. GET collaborators/{login}/permission. ok → permission ∈ {write, admin, owner}.
2. Only on status 403: GET /repos/{o}/{r}. owner.login equal to login (case-insensitive) → true. Otherwise GET /repos/{o}/{r}/teams, and for each team with permission ∈ {write, admin, owner} or units_map["repo.code"] ∈ {write, admin}, GET /teams/{id}/members/{login}: 200 → true, 404 → next team. A 403/404 on the teams list (user-owned repo, or the bot can't see teams) → false. No match → false.
3. Any other non-ok status → return it (the caller fails closed).
The doc comment states why: review payloads carry the PR author's permission, and Forgejo answers the collaborator-permission endpoint only for admins.
Add both methods to FakeApi in dispatcher.test.ts. Defaults: listReviews returns this.reviews (new ReviewSummary[] field, default []) and reviewerCanWrite returns this.canWrite (default ok(true)). Add a reviewChecks: string[] record of logins passed to reviewerCanWrite.

Step 4: gate and seed review triggers (`dispatcher.ts`, `conversation-detect.ts`)

Widen Ready.reason and the local reason in gate to TriggerReason | "review-requested" | "review-submitted". Extend PayloadFields with review?: { type?: unknown; content?: unknown } | null and number?: unknown. Payload also intersects the new ReviewSubmittedPayloadLike.
In gate, with const reviewSubmitted = row.action === "reviewed":
1. Reason "review-submitted"; no detect.
2. The sender == commentAuthor check stays as is.
3. For review rows, run before the payload-permission block: pull_request.user.login must equal the agent (case-insensitive), else drop "review on a pull request not authored by the agent". Then await api.reviewerCanWrite(owner, repo, row.commentAuthor). Non-ok → this.fail(job, \could not check the reviewer's permission: ${status} ${message}`, "forgejo", null)and return null.false→ drop"reviewer lacks write access"`. The payload-permission block is skipped for review rows; extend its comment to say why.
4. ownPr for review rows is the author match from 3. The OWN_PR_REVIEW branch stays tied to reviewRequest. Workspace matching is unchanged.
ResolvedSession gains created: boolean (true from resolveSession's creation path, false from reuseMapped).
runReady: for "review-submitted", resolve the review first: api.listReviews, then the entry with the highest id whose author matches the reviewer (case-insensitive) and whose state matches the event (_approved → APPROVED, _rejected → REQUEST_CHANGES, _comment → COMMENT). A failure or no match logs a warning and uses null. The seed is buildReviewSubmittedSeed({ repo, number, payload, review, catchUp: created, linkedIssue }), where linkedIssue is the issue number used in step 5 (null for PR-only or reused sessions).
conversation-detect.ts: add the exported ReviewSubmittedPayloadLike (pull_request as in ReviewRequestPayloadLike, plus sender, review?: { type?: string; content?: string | null }) and

export function buildReviewSubmittedSeed(t: {
	repo: string;
	number: number;
	payload: ReviewSubmittedPayloadLike;
	review: { id: number; commentsCount: number; submittedAt: string } | null;
	catchUp: boolean;
	linkedIssue: number | null;
}): string

Lines joined by \n, with the body fenced via fenceFor:

Review submitted on your pull request ${repo} #${number}: "<title>"
Branch: <base.ref> ← <head.ref> (<head.sha first 7>) · <html_url>
Reviewer: @<sender> · Verdict: <approved|changes requested|comment>, then either · Review id ${id} · ${commentsCount} line comment(s) · submitted ${submittedAt} or · Review id unknown: it is the newest review by @<sender> in forgejo_list_pr_comments_and_reviews.
Only when catchUp: This session is new and has no history for this pull request. Before acting, read it with forgejo_read_pr, the diff with forgejo_get_pr_diff and the conversation with forgejo_list_pr_comments_and_reviews + (linkedIssue ? , and issue #${linkedIssue} with forgejo_read_issue : "") + . The checkout is detached at the PR head: create local branch <head.ref> at HEAD with git_branch before committing, so git_push updates the PR branch.
Task: read the whole review and its line comments with forgejo_list_pr_comments_and_reviews. Address the feedback: change the code, commit, push. Reply in every line-comment thread of this review with forgejo_create_pr_thread_comment, naming what changed and the commit sha, or why you did not change it. Post one forgejo_create_pr_comment only if the review body itself has feedback, answering it. If the verdict is approved and there is no feedback, do nothing: no comment, no push.
blank line, Review body:, fence, body or (empty), fence.

Step 5: route reviews to an implementer session (`dispatcher.ts`)

reuseMapped: for "review-submitted" rows, a mapped session whose trigger.kind !== "forgejo_implementer" returns "it is not an implementer session". A conversation/PR session lacks write tools, so a new implementer session replaces the mapping. Other reasons are unchanged.
resolveSession: for "review-submitted" rows, the creation path builds an implementer session instead of the PR session:
- linkedIssue = first number from extractLinkedIssues(payload.pull_request?.body) (apps/server/src/forgejo/tools/format.ts), else null.
- Trigger { kind: "forgejo_implementer", scope: { owner, repo, number: linkedIssue ?? row.number, baseBranch: payload.pull_request?.base?.ref ?? null, pull: row.number } }.
- Checkout createCheckout({ workspacePath, owner, repo, kind: "pr", number: row.number }); same error wrapping and removeCheckout on a failed createSession as today.
- Mappings: setForgejoSession(row.repo, row.number, id). When linkedIssue is set and getForgejoSession(row.repo, linkedIssue) is null or !this.isUsableTriggerSession(it), also map the issue. An issue already owned by a usable session keeps its mapping.
- Name Implement: ${row.repo}#${row.number} (startImplementer's style).
- Return { handle, broughtLive: true, kind: "forgejo_implementer", created: true } and expose linkedIssue to runReady (return it on ResolvedSession as linkedIssue: number | null; null on the reuse path).
The turn timeout already follows SESSION_POLICIES[kind].turnTimeoutMs (2 h for implementers).

Tests

apps/server/src/forgejo/webhook.test.ts: each of the three review types with action: "reviewed" → accept (action: "reviewed", isPull: true, commentAuthor = sender). Self-authored → ignore; action: "submitted" → ignore; missing number → malformed. Identical bodies → equal dedupeKey; different review.content → different key. resolveEventType maps the three bare events.
apps/server/src/routes-forgejo-webhook.test.ts: replace the two tests that pin the old ignore behavior ("without Event-Type, a pull_request_comment event is an inline review comment and ignored" and "pull_request_review_comment: 200 ignored") with acceptance tests: a signed review payload → 202 accepted, and bare Event pull_request_comment → accepted with event pull_request_review_comment. Adjust the x-forgejo-event default in the post helper (~L70) for the approved/rejected types.
apps/server/src/forgejo/dispatcher.test.ts, new describe("ForgejoDispatcher review submissions"):
- the PR is mapped to an implementer session (setForgejoSession as in the existing startImplementer routing test ~L896) → the turn goes to it; the seed has Review id and no catch-up paragraph; the row is done with review-submitted.
- no mapping, PR body Closes: #4 → a forgejo_implementer session with scope { number: 4, pull: 7, baseBranch: <base.ref> } and checkout kind pr; the seed has the catch-up paragraph naming #4; #7 and #4 are mapped to it.
- no mapping, no linked issue → scope { number: 7, pull: 7 }.
- mapped forgejo_pull_request session → replaced by an implementer session; mapping overwritten.
- PR by someone else → dropped review on a pull request not authored by the agent; reviewChecks empty.
- payload permissions.push: true but canWrite = ok(false) → dropped reviewer lacks write access.
- canWrite non-ok → failed, failure comment posted.
- listReviews failing → the turn runs and the seed says Review id unknown.
reviewerCanWrite decision paths (200 write/read, 403 → owner, 403 → team member, 403 → no team, 403 → teams list 404): unit-test them if a ForgejoClient stub pattern exists (check apps/server/src/forgejo/client.test.ts). Otherwise check them once with a throwaway script and do not commit it.

Verification

cd apps/server && bun test src/forgejo src/routes-forgejo-webhook.test.ts src/bridge → pass; then the full bun test in apps/server and the repo's typecheck script (root package.json).
Manual end to end (configured Forgejo, one connected workspace, the webhook with "Pull request reviewed" events enabled): start an implementer on an issue and let it open its PR. As a write-access user, submit "Request changes" with one line comment → one accepted delivery (Settings → Forgejo), one turn in that session, a pushed commit, and a thread reply naming the sha. An empty approval → the turn ends with no comment and no push. A review by a read-only user → the trigger is dropped with reviewer lacks write access. Review a PR that the agent account opened outside any implementer session (no forgejo_sessions row for it) → a new implementer session with the catch-up seed in the pr-<n> clone; after the turn ends and the session is disposed, a second review resumes it and restores that clone.

Implementation: #51 ## Problem A review submitted on the implementer session's own pull request should continue that session. That way revisions can be driven through normal Forgejo reviews (implementer split 3/3; splits 1/3 and 2/3 are merged in #47 and #48). - Webhook: classify submitted reviews. Today `apps/server/src/forgejo/webhook.ts` ignores the review event types, and its header comment wrongly calls them "inline review comments". - One trigger per submitted review, not per inline comment. The turn reads the whole review and its line comments. - No @mention is needed on the agent's own pull request: a review there is addressed to the agent. Guard: the reviewer needs repo write access. - If the agent authored the PR but no implementer session is mapped to it, create one with a catch-up seed. With no linked issue in the PR body, the session is scoped to the PR alone. - Every verdict triggers, including approvals. An approval with no feedback needs no action. - The agent addresses the feedback: changes, commit, push. Then it replies in every line-comment thread with `forgejo_create_pr_thread_comment` (what changed plus the commit sha, or why nothing changed). It posts one PR comment only when the review body itself had feedback. - The dispatcher seeds the turn with basic info: PR metadata, reviewer, verdict, review body, and the review id and line-comment count when it can resolve them. Plan reference: KB `project/forgejo-integration-plan.md`, review part of step 15. Design: `project/forgejo-integration.md` (Implementer, Reviewer). ## Plan ### Forgejo facts (from Forgejo source: `services/webhook/notifier.go` `PullRequestReview`, `modules/webhook/type.go`) - A submitted review fires one delivery. `X-Forgejo-Event-Type` is `pull_request_review_approved`, `pull_request_review_rejected` or `pull_request_review_comment`. `X-Forgejo-Event` is `pull_request_approved`, `pull_request_rejected` or `pull_request_comment`. Pending reviews do not fire it. - Payload: `PullRequestPayload` with `action: "reviewed"`, `number`, `pull_request` (incl. `user`, `head.ref`, `head.sha`, `base.ref`, `title`, `body`, `html_url`), `sender` = reviewer, and `review: { type, content }` (content = review body). **No review id.** - `repository.permissions` in this payload is computed for the **PR author**, not the reviewer. On the agent's own PR it is the agent's permission, so the existing `perms.push === true` check in `ForgejoDispatcher.gate` (`dispatcher.ts` ~L290) proves nothing for reviews. - PR conversation comments stay Event `issue_comment` / Event-Type `pull_request_comment`; that path is unchanged. ### How the merged implementer code works (read before editing) - `ForgejoImplementerScope` (`apps/server/src/bridge/session-policy.ts` ~L51): `{ owner, repo, number /* issue */, baseBranch }`. PRs the session opened are not in the scope. `InProcessBridge.mapImplementerPr` (`bridge/in-process.ts` ~L745) maps them in `forgejo_sessions`, and `implementerPrNumbers(scope, sessionId)` (session-policy ~L231) lists them back. - `reuseMapped` (`dispatcher.ts` ~L406) skips the number-equality check for `forgejo_implementer`, so an issue or PR mapped to an implementer session already routes to it. - `resolveSession` (~L364) creates `forgejo_pull_request` / `forgejo_conversation` sessions only. Implementer sessions are created only by `startImplementer` (~L585). - Resume restores the checkout in `in-process.ts` ~L336-351 with `kind: "issue"` for implementer sessions and `number = scope.number`. ### Step 1: implementer scope can be started from a PR (`session-policy.ts`, `in-process.ts`) - Add an optional field to `ForgejoImplementerScope`, documented: `/** The pull request the session was started from (its checkout is that PR's clone); absent for sessions started for an issue. Equal to \`number\` when the PR names no issue. */ pull?: number;`. It is optional because persisted `scope_json` rows from #47/#48 lack it. - `implementerPrNumbers`: return the mapped numbers except `scope.number`, plus `scope.pull` first when it is set and not already in the list. So `forgejo_create_pr` is denied and the PR line appears in `systemContext` for PR-started sessions, including PR-only ones. - `systemContext`: when `scope.pull === scope.number` (PR-only), replace the title and the issue-oriented bullets with: - `## Unattended Forgejo implementer session (PR #${pull})` - `You maintain pull request #${pull}, which you authored. Nobody is watching this session live: the only channel to humans is Forgejo.` - `- Read the pull request with \`forgejo_read_pr\` and its conversation with \`forgejo_list_pr_comments_and_reviews\`; no issue is linked to it.` - `- Work on the pull request's branch, commit with \`git_commit\`, push with \`git_push\`. Do not open another pull request.` - `- Post blockers, open questions and major decisions as comments on #${pull}.` - keep the existing raw-git and untrusted-data bullets. Otherwise keep the current text. For all implementer sessions, append the bullet `- Reviews submitted on your pull request arrive as turns: answer line comments in their threads with \`forgejo_create_pr_thread_comment\`.` - `decide` needs no change: comments on `scope.number` are allowed, and the PR is either `scope.number` or mapped to the session. - `in-process.ts` resume (~L341-351): for `forgejo_implementer` with `scope.pull !== undefined`, restore with `kind: "pr"`, `number: scope.pull`, `baseBranch: null`. Otherwise keep today's arguments. - Tests in the existing session-policy test file (find it with `glob apps/server/src/bridge/session-policy*.test.ts`): PR-only scope → the PR-only header, and `forgejo_create_pr` denied with `this session already opened pull request #<pull>`. Scope with `pull` ≠ `number` and no mapping → `forgejo_create_pr` denied. Old scope without `pull` → unchanged behavior. ### Step 2: classify review deliveries (`apps/server/src/forgejo/webhook.ts`) - Export `const REVIEW_EVENT_TYPES = ["pull_request_review_approved", "pull_request_review_rejected", "pull_request_review_comment"] as const`. - `resolveEventType`: without an Event-Type header, map Event `pull_request_approved` → `pull_request_review_approved`, `pull_request_rejected` → `pull_request_review_rejected`, and `pull_request_comment` → `pull_request_review_comment`. Everything else passes through. Rewrite its doc comment and the file-header bullet (~L14-16): those are submitted reviews, not inline comments. - `classifyForgejoDelivery`: route `REVIEW_EVENT_TYPES` to a new `classifyReviewSubmitted(eventType, payload, agentUsername)`, modeled on `classifyReviewRequest`: - non-object → `malformed "payload is not an object"`; `action !== "reviewed"` → `ignore "unsupported action <x>"`. - missing `sender.login` → `malformed "missing sender.login"`; sender equal to the agent (case-insensitive) → `ignore "self-authored"`. - `repository.full_name` and the number (`payload.number ?? pull_request.number`) are validated with the same messages as `classifyReviewRequest`. - accept: `dedupeKey` = sha256 of `JSON.stringify(["reviewed", payload])`. `event: eventType`, `action: "reviewed"`, `repo`, `number`, `isPull: true`, `commentAuthor: sender`. - The PR author is not checked here; the dispatcher does that. - No migration: the column comments in `db/migrations/010-forgejo-triggers.sql` are documentation only. ### Step 3: Forgejo calls (`apps/server/src/forgejo/dispatch-api.ts`) Add to `DispatchApi` and `createDispatchApi`, in the style of `listComments`/`readIssue` (never throw, return `ApiResult`): - `listReviews(owner, repo, number, signal?): Promise<ApiResult<ReviewSummary[]>>` → `GET pulls/{n}/reviews` via `fetchAllPages`. Exported `interface ReviewSummary { id: number; author: string; state: string; commentsCount: number; submittedAt: string }`, from `id`, `user.login`, `state`, `comments_count`, `submitted_at`; missing fields become `""`/`0`. - `reviewerCanWrite(owner, repo, login, signal?): Promise<ApiResult<boolean>>`: 1. `GET collaborators/{login}/permission`. ok → `permission` ∈ {`write`, `admin`, `owner`}. 2. Only on status 403: `GET /repos/{o}/{r}`. `owner.login` equal to `login` (case-insensitive) → true. Otherwise `GET /repos/{o}/{r}/teams`, and for each team with `permission` ∈ {`write`, `admin`, `owner`} or `units_map["repo.code"]` ∈ {`write`, `admin`}, `GET /teams/{id}/members/{login}`: 200 → true, 404 → next team. A 403/404 on the teams list (user-owned repo, or the bot can't see teams) → false. No match → false. 3. Any other non-ok status → return it (the caller fails closed). The doc comment states why: review payloads carry the PR author's permission, and Forgejo answers the collaborator-permission endpoint only for admins. - Add both methods to `FakeApi` in `dispatcher.test.ts`. Defaults: `listReviews` returns `this.reviews` (new `ReviewSummary[]` field, default `[]`) and `reviewerCanWrite` returns `this.canWrite` (default `ok(true)`). Add a `reviewChecks: string[]` record of logins passed to `reviewerCanWrite`. ### Step 4: gate and seed review triggers (`dispatcher.ts`, `conversation-detect.ts`) - Widen `Ready.reason` and the local `reason` in `gate` to `TriggerReason | "review-requested" | "review-submitted"`. Extend `PayloadFields` with `review?: { type?: unknown; content?: unknown } | null` and `number?: unknown`. `Payload` also intersects the new `ReviewSubmittedPayloadLike`. - In `gate`, with `const reviewSubmitted = row.action === "reviewed"`: 1. Reason `"review-submitted"`; no `detect`. 2. The sender == `commentAuthor` check stays as is. 3. For review rows, run before the payload-permission block: `pull_request.user.login` must equal the agent (case-insensitive), else drop `"review on a pull request not authored by the agent"`. Then `await api.reviewerCanWrite(owner, repo, row.commentAuthor)`. Non-ok → `this.fail(job, \`could not check the reviewer's permission: ${status} ${message}\`, "forgejo", null)` and return null. `false` → drop `"reviewer lacks write access"`. The payload-permission block is skipped for review rows; extend its comment to say why. 4. `ownPr` for review rows is the author match from 3. The `OWN_PR_REVIEW` branch stays tied to `reviewRequest`. Workspace matching is unchanged. - `ResolvedSession` gains `created: boolean` (true from `resolveSession`'s creation path, false from `reuseMapped`). - `runReady`: for `"review-submitted"`, resolve the review first: `api.listReviews`, then the entry with the highest `id` whose author matches the reviewer (case-insensitive) and whose `state` matches the event (`_approved` → `APPROVED`, `_rejected` → `REQUEST_CHANGES`, `_comment` → `COMMENT`). A failure or no match logs a warning and uses `null`. The seed is `buildReviewSubmittedSeed({ repo, number, payload, review, catchUp: created, linkedIssue })`, where `linkedIssue` is the issue number used in step 5 (null for PR-only or reused sessions). - `conversation-detect.ts`: add the exported `ReviewSubmittedPayloadLike` (`pull_request` as in `ReviewRequestPayloadLike`, plus `sender`, `review?: { type?: string; content?: string | null }`) and ```ts export function buildReviewSubmittedSeed(t: { repo: string; number: number; payload: ReviewSubmittedPayloadLike; review: { id: number; commentsCount: number; submittedAt: string } | null; catchUp: boolean; linkedIssue: number | null; }): string ``` Lines joined by `\n`, with the body fenced via `fenceFor`: 1. `Review submitted on your pull request ${repo} #${number}: "<title>"` 2. `Branch: <base.ref> ← <head.ref> (<head.sha first 7>) · <html_url>` 3. `Reviewer: @<sender> · Verdict: <approved|changes requested|comment>`, then either ` · Review id ${id} · ${commentsCount} line comment(s) · submitted ${submittedAt}` or ` · Review id unknown: it is the newest review by @<sender> in forgejo_list_pr_comments_and_reviews`. 4. Only when `catchUp`: `This session is new and has no history for this pull request. Before acting, read it with forgejo_read_pr, the diff with forgejo_get_pr_diff and the conversation with forgejo_list_pr_comments_and_reviews` + (linkedIssue ? `, and issue #${linkedIssue} with forgejo_read_issue` : "") + `. The checkout is detached at the PR head: create local branch <head.ref> at HEAD with git_branch before committing, so git_push updates the PR branch.` 5. `Task: read the whole review and its line comments with forgejo_list_pr_comments_and_reviews. Address the feedback: change the code, commit, push. Reply in every line-comment thread of this review with forgejo_create_pr_thread_comment, naming what changed and the commit sha, or why you did not change it. Post one forgejo_create_pr_comment only if the review body itself has feedback, answering it. If the verdict is approved and there is no feedback, do nothing: no comment, no push.` 6. blank line, `Review body:`, fence, body or `(empty)`, fence. ### Step 5: route reviews to an implementer session (`dispatcher.ts`) - `reuseMapped`: for `"review-submitted"` rows, a mapped session whose `trigger.kind !== "forgejo_implementer"` returns `"it is not an implementer session"`. A conversation/PR session lacks write tools, so a new implementer session replaces the mapping. Other reasons are unchanged. - `resolveSession`: for `"review-submitted"` rows, the creation path builds an implementer session instead of the PR session: - `linkedIssue` = first number from `extractLinkedIssues(payload.pull_request?.body)` (`apps/server/src/forgejo/tools/format.ts`), else null. - Trigger `{ kind: "forgejo_implementer", scope: { owner, repo, number: linkedIssue ?? row.number, baseBranch: payload.pull_request?.base?.ref ?? null, pull: row.number } }`. - Checkout `createCheckout({ workspacePath, owner, repo, kind: "pr", number: row.number })`; same error wrapping and `removeCheckout` on a failed `createSession` as today. - Mappings: `setForgejoSession(row.repo, row.number, id)`. When `linkedIssue` is set and `getForgejoSession(row.repo, linkedIssue)` is null or `!this.isUsableTriggerSession(it)`, also map the issue. An issue already owned by a usable session keeps its mapping. - Name `Implement: ${row.repo}#${row.number}` (startImplementer's style). - Return `{ handle, broughtLive: true, kind: "forgejo_implementer", created: true }` and expose `linkedIssue` to `runReady` (return it on `ResolvedSession` as `linkedIssue: number | null`; null on the reuse path). - The turn timeout already follows `SESSION_POLICIES[kind].turnTimeoutMs` (2 h for implementers). ### Tests - `apps/server/src/forgejo/webhook.test.ts`: each of the three review types with `action: "reviewed"` → accept (`action: "reviewed"`, `isPull: true`, `commentAuthor` = sender). Self-authored → ignore; `action: "submitted"` → ignore; missing number → malformed. Identical bodies → equal dedupeKey; different `review.content` → different key. `resolveEventType` maps the three bare events. - `apps/server/src/routes-forgejo-webhook.test.ts`: replace the two tests that pin the old ignore behavior (`"without Event-Type, a pull_request_comment event is an inline review comment and ignored"` and `"pull_request_review_comment: 200 ignored"`) with acceptance tests: a signed review payload → 202 accepted, and bare Event `pull_request_comment` → accepted with event `pull_request_review_comment`. Adjust the `x-forgejo-event` default in the `post` helper (~L70) for the approved/rejected types. - `apps/server/src/forgejo/dispatcher.test.ts`, new `describe("ForgejoDispatcher review submissions")`: - the PR is mapped to an implementer session (`setForgejoSession` as in the existing startImplementer routing test ~L896) → the turn goes to it; the seed has `Review id` and no catch-up paragraph; the row is done with `review-submitted`. - no mapping, PR body `Closes: #4` → a `forgejo_implementer` session with scope `{ number: 4, pull: 7, baseBranch: <base.ref> }` and checkout kind `pr`; the seed has the catch-up paragraph naming `#4`; #7 and #4 are mapped to it. - no mapping, no linked issue → scope `{ number: 7, pull: 7 }`. - mapped `forgejo_pull_request` session → replaced by an implementer session; mapping overwritten. - PR by someone else → dropped `review on a pull request not authored by the agent`; `reviewChecks` empty. - payload `permissions.push: true` but `canWrite = ok(false)` → dropped `reviewer lacks write access`. - `canWrite` non-ok → failed, failure comment posted. - `listReviews` failing → the turn runs and the seed says `Review id unknown`. - `reviewerCanWrite` decision paths (200 write/read, 403 → owner, 403 → team member, 403 → no team, 403 → teams list 404): unit-test them if a `ForgejoClient` stub pattern exists (check `apps/server/src/forgejo/client.test.ts`). Otherwise check them once with a throwaway script and do not commit it. ### Verification - `cd apps/server && bun test src/forgejo src/routes-forgejo-webhook.test.ts src/bridge` → pass; then the full `bun test` in `apps/server` and the repo's typecheck script (root `package.json`). - Manual end to end (configured Forgejo, one connected workspace, the webhook with "Pull request reviewed" events enabled): start an implementer on an issue and let it open its PR. As a write-access user, submit "Request changes" with one line comment → one accepted delivery (Settings → Forgejo), one turn in that session, a pushed commit, and a thread reply naming the sha. An empty approval → the turn ends with no comment and no push. A review by a read-only user → the trigger is dropped with `reviewer lacks write access`. Review a PR that the agent account opened outside any implementer session (no `forgejo_sessions` row for it) → a new implementer session with the catch-up seed in the `pr-<n>` clone; after the turn ends and the session is disposed, a second review resumes it and restores that clone.

lucas added the

labels

2026-10-20 13:04:20 +02:00

omp-deck added the

Status

Blocked

label

2026-10-20 13:34:42 +02:00

omp-deck commented

2026-10-20 13:37:19 +02:00

Author

Collaborator

Blocked: this depends on implementer split 2/3, which hasn't landed. ForgejoImplementerScope (apps/server/src/bridge/session-policy.ts) has no pull request number, and nothing maps an implementer session to the pull request it opens. So a review can't be routed to the implementer session yet.

Resolution: paused until split 2/3 lands; this will be re-triggered then. No code changes were made.

**Blocked:** this depends on implementer split 2/3, which hasn't landed. `ForgejoImplementerScope` (`apps/server/src/bridge/session-policy.ts`) has no pull request number, and nothing maps an implementer session to the pull request it opens. So a review can't be routed to the implementer session yet. **Resolution:** paused until split 2/3 lands; this will be re-triggered then. No code changes were made.

omp-deck added

and removed

labels

2026-10-20 13:57:43 +02:00

omp-deck referenced this issue from a pull request that will close it,

2026-10-20 14:03:43 +02:00