Implementer sessions: limit "one pull request" to open pull requests #206

New issue

Closed

opened 2026-11-03 00:52:39 +01:00 by omp-deck · 0 comments

omp-deck commented

2026-11-03 00:52:39 +01:00

Collaborator

Problem

Forgejo implementer sessions refuse forgejo_create_pr with "this session already opened pull request #N; push further commits to its branch instead" as soon as any PR is mapped to the session (forgejo_sessions). The only exception is the newest PR having a failed post-merge CI record (followUpFor). After a session's PR is merged or closed, a legitimate follow-up PR is therefore blocked. The rule must count only PRs that are still open. The same applies to the companion git_branch restriction (a new PR needs a new branch) and to the system-prompt wording. The deck stores no PR open/closed state, so the check queries Forgejo live at call time.

Plan

Context

In apps/server/src/bridge/session-policy.ts, forgejoImplementerPolicy.decide (~L436–441) and the git_branch check (~L443–449) gate on implementerPrNumbers plus followUpFor. followUpFor is the failed-post-merge-CI exception. Target end state: both gates allow the call once every PR mapped to the session is merged or closed, as reported live by Forgejo. They deny while any mapped PR is open. followUpFor and its prompt bullet go away, because "no open PR" subsumes that case.

Approach

Expose PR state. In apps/server/src/forgejo/dispatch-api.ts:
- Add state?: string to RawPull.
- Add /** "closed" also covers merged PRs. */ state: "open" | "closed"; to PullSummary.
- In readPull map it as state: raw.state === "closed" ? "closed" : "open". This mirrors the IssueSummary mapping at ~L338.
- Update every PullSummary literal in tests: grep -rn "mergeCommitSha:" apps/server/src --include=*.test.ts, in particular the fake in forgejo/dispatcher.test.ts. Add state: "open" to each.
Make the policy verdict async with an API source. In session-policy.ts:
- Change SessionPolicy.decide's return type to PolicyDecision | Promise<PolicyDecision>.
- Add a final parameter api: () => DispatchApi | null to decide and to decidePolicy.
- Make decidePolicy async, returning Promise<PolicyDecision>, and await the per-kind decide. Import type DispatchApi from ../forgejo/dispatch-api.ts.
- Other kinds ignore the new param and stay synchronous.
Replace followUpFor with a live open check.
- Delete followUpFor and the getPostMerge import (it is the only user in this file).
- Add async function openPrDenial(scope: ForgejoImplementerScope, sessionId: string, api: () => DispatchApi | null, signal?: AbortSignal): Promise<{ deny: string } | null>. It returns null when implementerPrNumbers(scope, sessionId) is empty. Otherwise it gets const client = api():
  - null client → { deny: "cannot check whether this session's pull requests are still open: Forgejo is not configured" }.
  - For each number, sequentially: await client.readPull(scope.owner, scope.repo, n, signal).
    - Read fails → { deny: \could not check whether pull request #${n} is still open: ${res.status} ${res.message}` }`.
    - state === "open" → { deny: \pull request #${n} of this session is still open; push further commits to its branch instead` }`.
  - All closed → null.
- Make forgejoImplementerPolicy.decide async and add api (and pass signal, see step 4):
  - forgejo_create_pr: const denial = await openPrDenial(...); if (denial) return denial;. This replaces the old block.
- git_branch (inside scope.headBranch !== undefined): keep action !== "list" && !onHead, but replace followUpFor(...) === null with a call to openPrDenial. If it returns a denial, return { deny: \only the pull request's head branch ${scope.headBranch} may be created or switched to while a pull request of this session is open (${denial.deny})` }`. Otherwise fall through.
- Update the comment above git_push (~L450–452) to say the follow-up branch becomes reachable once the session's PRs are all merged or closed.
Wire the filter. In apps/server/src/bridge/permission-filter.ts:
- Add an optional constructor option forgejoApi?: () => DispatchApi | null, defaulting to createDispatchApi from ../forgejo/dispatch-api.ts.
- In evaluatePermission, call await decidePolicy(this.trigger, toolName, params, this.sessionId, this.cwd, this.forgejoApi, signal). Add signal?: AbortSignal as the last parameter of decidePolicy and decide after api, and pass it to openPrDenial.
- The only constructor call site is in-process.ts ~L943. It needs no change, because the default applies.
System prompt wording (forgejoImplementerPolicy.systemContext):
- Remove followUp and followUpRule and every spread of followUpRule.
- Set onePr unconditionally to " While it is open, do not open another pull request; once it is merged or closed you may open a follow-up pull request from a new branch."
- In maintain's first bullet, replace the conditional " You cannot use other branches." with " Other branches are available only once the pull request is merged or closed."

Critical files & anchors

apps/server/src/bridge/session-policy.ts: followUpFor, forgejoImplementerPolicy.decide / systemContext, decidePolicy.
apps/server/src/bridge/permission-filter.ts: evaluatePermission (~L242), constructor options.
apps/server/src/forgejo/dispatch-api.ts: RawPull, PullSummary, readPull.
apps/server/src/bridge/session-policy.test.ts: every decidePolicy( call becomes await decidePolicy( with an api argument, and the test functions become async.

Verification

Work in apps/server.

Update session-policy.test.ts. Add a fake api helper returning an object whose readPull answers from a Map<number, "open" | "closed"> (other methods unused; cast). Pass () => null where Forgejo is irrelevant.
- Replace the test at ~L174–182 with these cases:
  - Mapped #7 (issue) only → create_pr "allow".
  - Map #12 with state open → deny "pull request #12 of this session is still open; push further commits to its branch instead".
  - Set #12 closed → "allow".
  - Map #13 open → deny naming #13.
  - readPull returning { ok: false, status: 500, message: "boom" } → deny "could not check whether pull request #12 is still open: 500 boom".
  - api: () => null with a mapped PR → deny mentioning "Forgejo is not configured".
- Rewrite the post-merge follow-up test (~L245–259) without insertPostMerge:
  - With headBranch: "agent/x" and #12 open, git_branch create "other" → deny.
  - With #12 closed → "allow", and create_pr → "allow".
- Update prompt assertions (~L226, ~L254–255) to expect "While it is open, do not open another pull request" and no "Open exactly one follow-up pull request".
Run bun test src/bridge/session-policy.test.ts src/bridge/permission-filter.test.ts src/forgejo/dispatcher.test.ts src/db/forgejo-post-merge.test.ts. All must pass.
Run bun x tsc --noEmit -p . (or the repo's typecheck script). It must be clean.

Assumptions & contingencies

Every create_pr and non-head git_branch attempt costs one readPull per mapped PR. A Forgejo outage blocks new PRs (fail closed). This is intended.
If PermissionFilter turns out to have other callers of decidePolicy (check grep -rn "decidePolicy(" apps/server/src), make them await it and pass createDispatchApi.

## Problem Forgejo implementer sessions refuse `forgejo_create_pr` with "this session already opened pull request #N; push further commits to its branch instead" as soon as any PR is mapped to the session (`forgejo_sessions`). The only exception is the newest PR having a `failed` post-merge CI record (`followUpFor`). After a session's PR is merged or closed, a legitimate follow-up PR is therefore blocked. The rule must count only PRs that are still **open**. The same applies to the companion `git_branch` restriction (a new PR needs a new branch) and to the system-prompt wording. The deck stores no PR open/closed state, so the check queries Forgejo live at call time. ## Plan ### Context In `apps/server/src/bridge/session-policy.ts`, `forgejoImplementerPolicy.decide` (~L436–441) and the `git_branch` check (~L443–449) gate on `implementerPrNumbers` plus `followUpFor`. `followUpFor` is the failed-post-merge-CI exception. Target end state: both gates allow the call once every PR mapped to the session is merged or closed, as reported live by Forgejo. They deny while any mapped PR is open. `followUpFor` and its prompt bullet go away, because "no open PR" subsumes that case. ### Approach 1. **Expose PR state.** In `apps/server/src/forgejo/dispatch-api.ts`: - Add `state?: string` to `RawPull`. - Add `/** "closed" also covers merged PRs. */ state: "open" | "closed";` to `PullSummary`. - In `readPull` map it as `state: raw.state === "closed" ? "closed" : "open"`. This mirrors the `IssueSummary` mapping at ~L338. - Update every `PullSummary` literal in tests: `grep -rn "mergeCommitSha:" apps/server/src --include=*.test.ts`, in particular the fake in `forgejo/dispatcher.test.ts`. Add `state: "open"` to each. 2. **Make the policy verdict async with an API source.** In `session-policy.ts`: - Change `SessionPolicy.decide`'s return type to `PolicyDecision | Promise<PolicyDecision>`. - Add a final parameter `api: () => DispatchApi | null` to `decide` and to `decidePolicy`. - Make `decidePolicy` `async`, returning `Promise<PolicyDecision>`, and `await` the per-kind `decide`. Import `type DispatchApi` from `../forgejo/dispatch-api.ts`. - Other kinds ignore the new param and stay synchronous. 3. **Replace `followUpFor` with a live open check.** - Delete `followUpFor` and the `getPostMerge` import (it is the only user in this file). - Add `async function openPrDenial(scope: ForgejoImplementerScope, sessionId: string, api: () => DispatchApi | null, signal?: AbortSignal): Promise<{ deny: string } | null>`. It returns `null` when `implementerPrNumbers(scope, sessionId)` is empty. Otherwise it gets `const client = api()`: - `null` client → `{ deny: "cannot check whether this session's pull requests are still open: Forgejo is not configured" }`. - For each number, sequentially: `await client.readPull(scope.owner, scope.repo, n, signal)`. - Read fails → `{ deny: \`could not check whether pull request #${n} is still open: ${res.status} ${res.message}\` }`. - `state === "open"` → `{ deny: \`pull request #${n} of this session is still open; push further commits to its branch instead\` }`. - All closed → `null`. - Make `forgejoImplementerPolicy.decide` `async` and add `api` (and pass `signal`, see step 4): - `forgejo_create_pr`: `const denial = await openPrDenial(...); if (denial) return denial;`. This replaces the old block. - `git_branch` (inside `scope.headBranch !== undefined`): keep `action !== "list" && !onHead`, but replace `followUpFor(...) === null` with a call to `openPrDenial`. If it returns a denial, return `{ deny: \`only the pull request's head branch ${scope.headBranch} may be created or switched to while a pull request of this session is open (${denial.deny})\` }`. Otherwise fall through. - Update the comment above `git_push` (~L450–452) to say the follow-up branch becomes reachable once the session's PRs are all merged or closed. 4. **Wire the filter.** In `apps/server/src/bridge/permission-filter.ts`: - Add an optional constructor option `forgejoApi?: () => DispatchApi | null`, defaulting to `createDispatchApi` from `../forgejo/dispatch-api.ts`. - In `evaluatePermission`, call `await decidePolicy(this.trigger, toolName, params, this.sessionId, this.cwd, this.forgejoApi, signal)`. Add `signal?: AbortSignal` as the last parameter of `decidePolicy` and `decide` after `api`, and pass it to `openPrDenial`. - The only constructor call site is `in-process.ts` ~L943. It needs no change, because the default applies. 5. **System prompt wording** (`forgejoImplementerPolicy.systemContext`): - Remove `followUp` and `followUpRule` and every spread of `followUpRule`. - Set `onePr` unconditionally to `" While it is open, do not open another pull request; once it is merged or closed you may open a follow-up pull request from a new branch."` - In `maintain`'s first bullet, replace the conditional `" You cannot use other branches."` with `" Other branches are available only once the pull request is merged or closed."` ### Critical files & anchors - `apps/server/src/bridge/session-policy.ts`: `followUpFor`, `forgejoImplementerPolicy.decide` / `systemContext`, `decidePolicy`. - `apps/server/src/bridge/permission-filter.ts`: `evaluatePermission` (~L242), constructor options. - `apps/server/src/forgejo/dispatch-api.ts`: `RawPull`, `PullSummary`, `readPull`. - `apps/server/src/bridge/session-policy.test.ts`: every `decidePolicy(` call becomes `await decidePolicy(` with an `api` argument, and the test functions become `async`. ### Verification Work in `apps/server`. 1. Update `session-policy.test.ts`. Add a fake `api` helper returning an object whose `readPull` answers from a `Map<number, "open" | "closed">` (other methods unused; cast). Pass `() => null` where Forgejo is irrelevant. - Replace the test at ~L174–182 with these cases: - Mapped #7 (issue) only → create_pr `"allow"`. - Map #12 with state open → deny `"pull request #12 of this session is still open; push further commits to its branch instead"`. - Set #12 closed → `"allow"`. - Map #13 open → deny naming #13. - `readPull` returning `{ ok: false, status: 500, message: "boom" }` → deny `"could not check whether pull request #12 is still open: 500 boom"`. - `api: () => null` with a mapped PR → deny mentioning "Forgejo is not configured". - Rewrite the post-merge follow-up test (~L245–259) without `insertPostMerge`: - With `headBranch: "agent/x"` and #12 open, `git_branch` create `"other"` → deny. - With #12 closed → `"allow"`, and create_pr → `"allow"`. - Update prompt assertions (~L226, ~L254–255) to expect `"While it is open, do not open another pull request"` and no `"Open exactly one follow-up pull request"`. 2. Run `bun test src/bridge/session-policy.test.ts src/bridge/permission-filter.test.ts src/forgejo/dispatcher.test.ts src/db/forgejo-post-merge.test.ts`. All must pass. 3. Run `bun x tsc --noEmit -p .` (or the repo's `typecheck` script). It must be clean. ### Assumptions & contingencies - Every create_pr and non-head git_branch attempt costs one `readPull` per mapped PR. A Forgejo outage blocks new PRs (fail closed). This is intended. - If `PermissionFilter` turns out to have other callers of `decidePolicy` (check `grep -rn "decidePolicy(" apps/server/src`), make them `await` it and pass `createDispatchApi`.