PR sessions on agent-authored PRs can't push or edit #61

New issue

Closed

opened 2026-10-23 10:02:43 +02:00 by omp-deck · 0 comments

omp-deck commented

2026-10-23 10:02:43 +02:00

Collaborator

Problem

Unattended Forgejo PR sessions can only discuss PRs, even on PRs the agent wrote. They are supposed to drive those PRs: apply requested changes, commit and push.

Observed on lucas/omp-deck#56 (2026-10-22): @lucas picked option 3 for broker config setup, but the session could only post a plan (comment 16604) and not implement it.

Cause: in apps/server/src/forgejo/dispatcher.ts resolveSession, only a submitted review on an agent-authored PR gets a forgejo_implementer session (write tools, "You maintain pull request #N" prompt). An addressed comment on an agent-authored PR gets a forgejo_pull_request session with ownPr: true, whose tool list is CONVERSATION_TOOL_NAMES + forgejo_post_review (read-only git, Forgejo read/comment) and whose prompt says "You cannot change labels, state, descriptions, branches or code. Propose such actions in your reply."

Decisions:

Comments addressed to the agent on agent-authored PRs are routed to the existing implementer session kind, exactly like submitted reviews. A mapped read-only session is not reused for them.
An explicit instruction from a write-access commenter is enough to act; the session asks in a comment only when the request is ambiguous. No separate confirm step.
PRs authored by humans keep the read-and-reply/review mode.
Sessions maintaining an existing PR are confined to its head branch: git_branch may only create/switch to that branch, git_push only to origin.
No deck tools (deck_inbox_*, deck_task_*) are added; Forgejo comments stay the only channel.
An agent-authored PR whose head branch lives in another repository is a should-never-happen error: the trigger fails and the failure comment on the PR says why.

Plan

Approach

Steps 1–2 are independent; step 3 depends on both; step 4 depends on step 3.

Dispatch API: read a PR's head (apps/server/src/forgejo/dispatch-api.ts).
- Add to DispatchApi:
```
/** Head branch and repositories of pull request `number`. */
readPull(owner: string, repo: string, number: number, signal?: AbortSignal): Promise<ApiResult<PullSummary>>;
```
  and export interface PullSummary { headBranch: string; headRepo: string; baseRepo: string } (headRepo/baseRepo are owner/name full names).
- Implement in createDispatchApi like readIssue: client.json<RawPull>("GET", repoApiPath(owner, repo, "pulls", String(number)), { signal }), with private interface RawPull { head?: { ref?: string; repo?: { full_name?: string } | null } | null; base?: { repo?: { full_name?: string } | null } | null }. Missing strings map to "".
- Add readPull to the fake API in apps/server/src/forgejo/dispatcher.test.ts (class around line 80): a public field pull: ApiResult<PullSummary> = ok({ headBranch: "agent/feature", headRepo: "lucas/omp-deck", baseRepo: "lucas/omp-deck" }) returned by readPull.
Policy: confine PR-maintaining implementer sessions; drop ownPr (apps/server/src/bridge/session-policy.ts).
- ForgejoImplementerScope: add optional headBranch?: string with doc comment: "Head branch of pull; when set, the session may only create/switch to this branch and push to origin. Absent for issue-started sessions and older persisted scopes, which stay unconfined." Scopes are free JSON in session_triggers.scope_json (apps/server/src/db/session-triggers.ts), so no migration.
- In forgejoImplementerPolicy.decide, before the generic toolNames.includes allow, when scope.headBranch !== undefined:
  - git_branch: read action and name from params. action === "list" → fall through. action === "create" or "switch" with name === scope.headBranch → fall through. Anything else (other name, delete) → { deny: \only the pull request's head branch ${scope.headBranch} may be created or switched to in this session` }`.
  - git_push: remote param absent or "origin" → fall through; else { deny: "pushes go to origin only in this session" }.
  - git_push itself needs no branch check: the only local branch the session can create or switch to is headBranch, and git_push refuses a detached HEAD (apps/server/src/bridge/git-tools/push.ts).
- forgejoImplementerPolicy.systemContext, pull === number branch (PR-only scope) and the linked-issue branch when headBranch is set: replace the line "Work on the pull request's branch, commit with git_commit, push with git_push. Do not open another pull request." with: - The checkout is a detached HEAD at the pull request's head. Create the local branch `${headBranch}` with `git_branch` (action `create`, no base), commit with `git_commit`, push with `git_push`. You cannot use other branches. Do not open another pull request. and add the line: - When a write-access user asks for a change (in a comment or review), make it, run the relevant checks, commit and push, then reply on #${pull} with what you changed. If the request is ambiguous, ask in a reply instead of guessing. For the linked-issue branch (pull !== number), insert both lines only when headBranch is set, after the "You already opened pull request" line, and skip the "Start from … create your own branch" and "open one pull request" lines in that case. When headBranch is undefined, prompts are unchanged.
- Remove ownPr from ForgejoPullRequestScope, the scope.ownPr deny in forgejoPullRequestPolicy.decide, and the ownPr ? conditional in its systemContext (keep only the review-instructions line). Persisted rows carrying ownPr stay parseable (extra JSON field is ignored).
- Update apps/server/src/bridge/session-policy.test.ts: delete cases asserting the ownPr deny/prompt; add cases: with headBranch: "agent/x", git_branch {action:"create",name:"agent/x"} → "allow", {action:"create",name:"other"} → deny, {action:"delete",name:"agent/x"} → deny, {action:"list"} → "allow", git_push {} → "allow", git_push {remote:"fork"} → deny; without headBranch, git_branch {action:"create",name:"other"} → "allow".
Dispatcher: route own-PR comments to a confined implementer session (apps/server/src/forgejo/dispatcher.ts).
- Ready (line ~153): replace ownPr: boolean with prHead: string | null — the head branch when the trigger is on an agent-authored PR (comment or submitted review), else null.
- In the gate (after the workspace match, before the final return at ~353): when row.isPull && ownPr && !reviewRequest, call api.readPull(owner, repo, row.number).
  - !ok → await this.fail(job, \could not read the pull request: ${res.status} ${res.message}`, "forgejo", null); return null;`
  - headRepo.toLowerCase() !== row.repo.toLowerCase() → await this.fail(job, \head branch is in ${headRepo}, not ${row.repo}`, "fork-head", null); return null;`
  - headBranch === "" → this.fail(job, "the pull request has no head branch", "forgejo", null); return null.
  - else prHead = headBranch.
- Add "fork-head" to FailureKind; failureLabel returns "this pull request's head branch is in another repository, so the agent cannot push to it" for it.
- resolveSession: replace const forReview = ready.reason === "review-submitted" with const maintain = ready.prHead !== null. Use maintain for the trigger choice, the session name prefix ("Implement"), and linkedIssue. linkedIssue = extractLinkedIssues(ready.payload.pull_request?.body ?? ready.payload.issue?.body)[0] ?? null (add body?: string | null to IssueCommentPayloadLike.issue in apps/server/src/forgejo/conversation-detect.ts). The implementer scope gains headBranch: ready.prHead. Non-maintain PR triggers build { kind: "forgejo_pull_request", scope: { owner, repo, number: row.number } }.
- reuseMapped: change the check at ~511 to if (ready.prHead !== null && trigger.kind !== "forgejo_implementer") return "it is not an implementer session"; and update the comment above it ("Acting on the agent's own PR needs the implementer's write tools.").
- runReady seed selection is unchanged: own-PR comments still use buildConversationSeed.
- Update tests in dispatcher.test.ts:
  - "an addressed comment on the agent's own PR starts a session marked ownPr" (~409) → expect kind: "forgejo_implementer", scope { owner, repo, number: 9, baseBranch: null, pull: 9, headBranch: "agent/feature" }, session name Implement: lucas/omp-deck#9.
  - Drop ownPr from the expected forgejo_pull_request scopes (~388, ~431) and the insertSessionTrigger fixture (~1074).
  - Review-submitted tests expecting implementer scopes: add headBranch: "agent/feature".
  - New: own-PR comment with a mapped forgejo_pull_request session → new implementer session replaces the mapping (mirror the test at ~1070 using a comment trigger).
  - New: h.api.pull = ok({ headBranch: "x", headRepo: "alice/omp-deck", baseRepo: "lucas/omp-deck" }) → row failed, no session, posted comment contains head branch is in another repository.
  - New: h.api.pull not ok → row failed, posted comment contains a Forgejo API call failed.
  - Human-authored PR comment → forgejo_pull_request, readPull not called.
baseBranch for comment triggers: use null (issue_comment payloads carry no base ref); the PR-maintaining prompt never uses it.
Docs: update any docs/KB text describing own-PR sessions as read-only (grep -rn "ownPr\|own pull request" docs apps/server/src — touch only prose that states the old behavior).

Critical files & anchors

apps/server/src/forgejo/dispatcher.ts — gate (~270–353), resolveSession (~427), reuseMapped (~491), FailureKind/failureLabel (~115, ~653).
apps/server/src/bridge/session-policy.ts — ForgejoImplementerScope, forgejoImplementerPolicy.decide/systemContext, forgejoPullRequestPolicy.
apps/server/src/forgejo/dispatch-api.ts — DispatchApi, createDispatchApi.
apps/server/src/forgejo/checkout.ts — PR checkouts are detached at refs/pull/<n>/head; this is why the session must git_branch create the head branch before pushing.

Verification

cd apps/server && bun test src/bridge/session-policy.test.ts src/forgejo/dispatcher.test.ts — all pass, including the new cases above.
bun run typecheck (repo root) — no references to ownPr remain: grep -rn ownPr apps/server/src returns nothing.
End-to-end (manual, needs Forgejo configured and the deck server running): on an agent-authored PR in lucas/omp-deck, comment @<agent> please rename X to Y. Expected: a session named Implement: lucas/omp-deck#<n> starts; it creates the local head branch, commits, pushes a new commit to the PR's head branch, and replies on the PR. In the deck, the session's tool list includes edit, bash, git_commit, git_push. On a human-authored PR, the same comment yields a reply-only session.

Assumptions & contingencies

If the Forgejo GET /repos/{owner}/{repo}/pulls/{n} response lacks head.repo (e.g. the head repo was deleted), headRepo is "", which does not match row.repo and fails as fork-head. Accepted: such a PR can't be pushed to anyway.
If extractLinkedIssues finds an issue in the PR body for a comment trigger, the issue mapping follows the existing review-path rule (an issue owned by another usable session keeps it).

## Problem Unattended Forgejo PR sessions can only discuss PRs, even on PRs the agent wrote. They are supposed to drive those PRs: apply requested changes, commit and push. Observed on lucas/omp-deck#56 (2026-10-22): @lucas picked option 3 for broker config setup, but the session could only post a plan (comment 16604) and not implement it. Cause: in `apps/server/src/forgejo/dispatcher.ts` `resolveSession`, only a *submitted review* on an agent-authored PR gets a `forgejo_implementer` session (write tools, "You maintain pull request #N" prompt). An addressed *comment* on an agent-authored PR gets a `forgejo_pull_request` session with `ownPr: true`, whose tool list is `CONVERSATION_TOOL_NAMES` + `forgejo_post_review` (read-only git, Forgejo read/comment) and whose prompt says "You cannot change labels, state, descriptions, branches or code. Propose such actions in your reply." Decisions: - Comments addressed to the agent on agent-authored PRs are routed to the existing implementer session kind, exactly like submitted reviews. A mapped read-only session is not reused for them. - An explicit instruction from a write-access commenter is enough to act; the session asks in a comment only when the request is ambiguous. No separate confirm step. - PRs authored by humans keep the read-and-reply/review mode. - Sessions maintaining an existing PR are confined to its head branch: `git_branch` may only create/switch to that branch, `git_push` only to `origin`. - No deck tools (`deck_inbox_*`, `deck_task_*`) are added; Forgejo comments stay the only channel. - An agent-authored PR whose head branch lives in another repository is a should-never-happen error: the trigger fails and the failure comment on the PR says why. ## Plan ### Approach Steps 1–2 are independent; step 3 depends on both; step 4 depends on step 3. 1. **Dispatch API: read a PR's head** (`apps/server/src/forgejo/dispatch-api.ts`). - Add to `DispatchApi`: ```ts /** Head branch and repositories of pull request `number`. */ readPull(owner: string, repo: string, number: number, signal?: AbortSignal): Promise<ApiResult<PullSummary>>; ``` and export `interface PullSummary { headBranch: string; headRepo: string; baseRepo: string }` (`headRepo`/`baseRepo` are `owner/name` full names). - Implement in `createDispatchApi` like `readIssue`: `client.json<RawPull>("GET", repoApiPath(owner, repo, "pulls", String(number)), { signal })`, with private `interface RawPull { head?: { ref?: string; repo?: { full_name?: string } | null } | null; base?: { repo?: { full_name?: string } | null } | null }`. Missing strings map to `""`. - Add `readPull` to the fake API in `apps/server/src/forgejo/dispatcher.test.ts` (class around line 80): a public field `pull: ApiResult<PullSummary> = ok({ headBranch: "agent/feature", headRepo: "lucas/omp-deck", baseRepo: "lucas/omp-deck" })` returned by `readPull`. 2. **Policy: confine PR-maintaining implementer sessions; drop `ownPr`** (`apps/server/src/bridge/session-policy.ts`). - `ForgejoImplementerScope`: add optional `headBranch?: string` with doc comment: "Head branch of `pull`; when set, the session may only create/switch to this branch and push to `origin`. Absent for issue-started sessions and older persisted scopes, which stay unconfined." Scopes are free JSON in `session_triggers.scope_json` (`apps/server/src/db/session-triggers.ts`), so no migration. - In `forgejoImplementerPolicy.decide`, before the generic `toolNames.includes` allow, when `scope.headBranch !== undefined`: - `git_branch`: read `action` and `name` from params. `action === "list"` → fall through. `action === "create"` or `"switch"` with `name === scope.headBranch` → fall through. Anything else (other name, `delete`) → `{ deny: \`only the pull request's head branch ${scope.headBranch} may be created or switched to in this session\` }`. - `git_push`: `remote` param absent or `"origin"` → fall through; else `{ deny: "pushes go to origin only in this session" }`. - `git_push` itself needs no branch check: the only local branch the session can create or switch to is `headBranch`, and `git_push` refuses a detached HEAD (`apps/server/src/bridge/git-tools/push.ts`). - `forgejoImplementerPolicy.systemContext`, `pull === number` branch (PR-only scope) and the linked-issue branch when `headBranch` is set: replace the line "Work on the pull request's branch, commit with `git_commit`, push with `git_push`. Do not open another pull request." with: `` - The checkout is a detached HEAD at the pull request's head. Create the local branch `${headBranch}` with `git_branch` (action `create`, no base), commit with `git_commit`, push with `git_push`. You cannot use other branches. Do not open another pull request. `` and add the line: `- When a write-access user asks for a change (in a comment or review), make it, run the relevant checks, commit and push, then reply on #${pull} with what you changed. If the request is ambiguous, ask in a reply instead of guessing.` For the linked-issue branch (`pull !== number`), insert both lines only when `headBranch` is set, after the "You already opened pull request" line, and skip the "Start from … create your own branch" and "open one pull request" lines in that case. When `headBranch` is undefined, prompts are unchanged. - Remove `ownPr` from `ForgejoPullRequestScope`, the `scope.ownPr` deny in `forgejoPullRequestPolicy.decide`, and the `ownPr ?` conditional in its `systemContext` (keep only the review-instructions line). Persisted rows carrying `ownPr` stay parseable (extra JSON field is ignored). - Update `apps/server/src/bridge/session-policy.test.ts`: delete cases asserting the `ownPr` deny/prompt; add cases: with `headBranch: "agent/x"`, `git_branch {action:"create",name:"agent/x"}` → `"allow"`, `{action:"create",name:"other"}` → deny, `{action:"delete",name:"agent/x"}` → deny, `{action:"list"}` → `"allow"`, `git_push {}` → `"allow"`, `git_push {remote:"fork"}` → deny; without `headBranch`, `git_branch {action:"create",name:"other"}` → `"allow"`. 3. **Dispatcher: route own-PR comments to a confined implementer session** (`apps/server/src/forgejo/dispatcher.ts`). - `Ready` (line ~153): replace `ownPr: boolean` with `prHead: string | null` — the head branch when the trigger is on an agent-authored PR (comment or submitted review), else null. - In the gate (after the workspace match, before the final `return` at ~353): when `row.isPull && ownPr && !reviewRequest`, call `api.readPull(owner, repo, row.number)`. - `!ok` → `await this.fail(job, \`could not read the pull request: ${res.status} ${res.message}\`, "forgejo", null); return null;` - `headRepo.toLowerCase() !== row.repo.toLowerCase()` → `await this.fail(job, \`head branch is in ${headRepo}, not ${row.repo}\`, "fork-head", null); return null;` - `headBranch === ""` → `this.fail(job, "the pull request has no head branch", "forgejo", null)`; return null. - else `prHead = headBranch`. - Add `"fork-head"` to `FailureKind`; `failureLabel` returns `"this pull request's head branch is in another repository, so the agent cannot push to it"` for it. - `resolveSession`: replace `const forReview = ready.reason === "review-submitted"` with `const maintain = ready.prHead !== null`. Use `maintain` for the trigger choice, the session name prefix (`"Implement"`), and `linkedIssue`. `linkedIssue` = `extractLinkedIssues(ready.payload.pull_request?.body ?? ready.payload.issue?.body)[0] ?? null` (add `body?: string | null` to `IssueCommentPayloadLike.issue` in `apps/server/src/forgejo/conversation-detect.ts`). The implementer scope gains `headBranch: ready.prHead`. Non-maintain PR triggers build `{ kind: "forgejo_pull_request", scope: { owner, repo, number: row.number } }`. - `reuseMapped`: change the check at ~511 to `if (ready.prHead !== null && trigger.kind !== "forgejo_implementer") return "it is not an implementer session";` and update the comment above it ("Acting on the agent's own PR needs the implementer's write tools."). - `runReady` seed selection is unchanged: own-PR comments still use `buildConversationSeed`. - Update tests in `dispatcher.test.ts`: - "an addressed comment on the agent's own PR starts a session marked ownPr" (~409) → expect `kind: "forgejo_implementer"`, scope `{ owner, repo, number: 9, baseBranch: null, pull: 9, headBranch: "agent/feature" }`, session name `Implement: lucas/omp-deck#9`. - Drop `ownPr` from the expected `forgejo_pull_request` scopes (~388, ~431) and the `insertSessionTrigger` fixture (~1074). - Review-submitted tests expecting implementer scopes: add `headBranch: "agent/feature"`. - New: own-PR comment with a mapped `forgejo_pull_request` session → new implementer session replaces the mapping (mirror the test at ~1070 using a comment trigger). - New: `h.api.pull = ok({ headBranch: "x", headRepo: "alice/omp-deck", baseRepo: "lucas/omp-deck" })` → row `failed`, no session, posted comment contains `head branch is in another repository`. - New: `h.api.pull` not ok → row `failed`, posted comment contains `a Forgejo API call failed`. - Human-authored PR comment → `forgejo_pull_request`, `readPull` not called. `baseBranch` for comment triggers: use `null` (issue_comment payloads carry no base ref); the PR-maintaining prompt never uses it. 4. **Docs**: update any docs/KB text describing own-PR sessions as read-only (`grep -rn "ownPr\|own pull request" docs apps/server/src` — touch only prose that states the old behavior). ### Critical files & anchors - `apps/server/src/forgejo/dispatcher.ts` — gate (~270–353), `resolveSession` (~427), `reuseMapped` (~491), `FailureKind`/`failureLabel` (~115, ~653). - `apps/server/src/bridge/session-policy.ts` — `ForgejoImplementerScope`, `forgejoImplementerPolicy.decide`/`systemContext`, `forgejoPullRequestPolicy`. - `apps/server/src/forgejo/dispatch-api.ts` — `DispatchApi`, `createDispatchApi`. - `apps/server/src/forgejo/checkout.ts` — PR checkouts are detached at `refs/pull/<n>/head`; this is why the session must `git_branch create` the head branch before pushing. ### Verification - `cd apps/server && bun test src/bridge/session-policy.test.ts src/forgejo/dispatcher.test.ts` — all pass, including the new cases above. - `bun run typecheck` (repo root) — no references to `ownPr` remain: `grep -rn ownPr apps/server/src` returns nothing. - End-to-end (manual, needs Forgejo configured and the deck server running): on an agent-authored PR in `lucas/omp-deck`, comment `@<agent> please rename X to Y`. Expected: a session named `Implement: lucas/omp-deck#<n>` starts; it creates the local head branch, commits, pushes a new commit to the PR's head branch, and replies on the PR. In the deck, the session's tool list includes `edit`, `bash`, `git_commit`, `git_push`. On a human-authored PR, the same comment yields a reply-only session. ### Assumptions & contingencies - If the Forgejo `GET /repos/{owner}/{repo}/pulls/{n}` response lacks `head.repo` (e.g. the head repo was deleted), `headRepo` is `""`, which does not match `row.repo` and fails as `fork-head`. Accepted: such a PR can't be pushed to anyway. - If `extractLinkedIssues` finds an issue in the PR body for a comment trigger, the issue mapping follows the existing review-path rule (an issue owned by another usable session keeps it).

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

2026-10-23 10:06:53 +02:00

Let sessions on agent-authored PRs push to their head branch #62