lucas/omp-deck

Watch

Fork 0

Forgejo: link implementer session to its pull request (implementer split 2/3) #46

New issue

Closed

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

omp-deck commented

2026-10-20 13:08:51 +02:00

Collaborator

Implementation: #48

Problem

Once the implementer session opens its pull request, events on that pull request should reach the same session.

The session's pull request is recorded alongside its issue, so @mention comments on the pull request continue the implementer session (same write-access guard).
The policy allows comments on both the issue and the linked pull request.
An implementer session opens at most one pull request. Once one is linked, further forgejo_create_pr calls are denied.

Builds on split 1/3 (#47), which already maps the PR an implementer session opens in forgejo_sessions (bridge tool_execution_end listener mapImplementerPr), allows comments on mapped numbers, and routes mentions on mapped PRs to the session (dispatcher reuseMapped, covered by a dispatcher test). The PR number stays in forgejo_sessions, not in the trigger scope; mapping failures stay log-only. Remaining gaps: deny a second forgejo_create_pr, and name the linked PR in the implementer's system context on resume.

Plan reference: KB project/forgejo-integration-plan.md, steps 10 and 14. Design: project/forgejo-integration.md (Implementer, Session routing).

Plan

3. Reverse lookup in `apps/server/src/db/forgejo-sessions.ts`

There is no existing lookup from session to numbers, so add one below setForgejoSession:

/** Numbers in `repo` mapped to `sessionId`, ascending. `repo` is `owner/repo` in any case. */
export function listForgejoSessionNumbers(repo: string, sessionId: string): number[]

It runs SELECT number FROM forgejo_sessions WHERE repo_key = ? AND session_id = ? ORDER BY number with repo.toLowerCase() and returns rows.map((r) => r.number). Same prepare<…> style as getForgejoSession.

4. Policy: linked PR helper, second-PR deny, system context

In apps/server/src/bridge/session-policy.ts:

Add a private helper:

/** PRs the implementer session opened: everything mapped to it except its issue. */
function implementerPrNumbers(scope: ForgejoImplementerScope, sessionId: string): number[] {
	return listForgejoSessionNumbers(`${scope.owner}/${scope.repo}`, sessionId).filter((n) => n !== scope.number);
}

Import listForgejoSessionNumbers next to the existing getForgejoSession import.

In forgejoImplementerPolicy.decide, put this before the final if (this.toolNames.includes(toolName)) return "allow";:

if (toolName === "forgejo_create_pr") {
	const [pr] = implementerPrNumbers(scope, sessionId);
	if (pr !== undefined) return { deny: `this session already opened pull request #${pr}; push further commits to its branch instead` };
}

When the list is empty, the call falls through to the existing "allow".

Change the interface to systemContext(scope: S, sessionId: string | null): string, with this doc: "Orientation appended to the system prompt of the session. sessionId is null while the session is being created." The conversation and pull-request policies ignore the new parameter: leave their implementations unchanged, since TypeScript accepts the shorter parameter list.
forgejoImplementerPolicy.systemContext({ number, baseBranch }, sessionId): when sessionId !== null and implementerPrNumbers(...) returns a first entry pr, insert this line right after the You were started to implement issue #… line:
`- You already opened pull request #${pr} for this issue. Continue the work on its branch and post updates on #${pr}; issue #${number} stays open for comments. Do not open another pull request.`
Otherwise the text is unchanged.
Change policySystemContext(trigger: SessionTrigger, sessionId: string | null): string and pass sessionId in all three switch branches.

5. Callers of `policySystemContext`

grep -rn "policySystemContext(" apps/server/src must return exactly these call sites; update each:

apps/server/src/bridge/in-process.ts createSession (~line 267): policySystemContext(opts.trigger, null). The session id doesn't exist yet and no PR can be mapped yet.
apps/server/src/bridge/in-process.ts resumeSession (~line 364): policySystemContext(trigger, existingId ?? null).
apps/server/src/bridge/session-policy.test.ts: existing calls get a null second argument.

If the grep finds other callers, pass null unless the caller has the session id at hand, in which case pass it.

Tests

apps/server/src/db/forgejo-sessions.test.ts, created if absent (unverified — confirm first; copy the bootDb temp-DB setup from session-policy.test.ts lines 108–119). listForgejoSessionNumbers should:
- return [4, 9] for o/r after mapping O/R#9 and o/r#4 to s1 and o/r#5 to s2;
- return [] for an unknown session.
apps/server/src/bridge/session-policy.test.ts, describe("forgejo_implementer"):
- forgejo_create_pr is allow with only the issue mapped (setForgejoSession("o/r", 7, "s1")). After setForgejoSession("o/r", 12, "s1") it is denied with this session already opened pull request #12; push further commits to its branch instead, while session s2 is still allow.
- policySystemContext(IMPL, "s1"), with o/r#12 mapped to s1, contains You already opened pull request #12. policySystemContext(IMPL, null) and an unmapped "s1" don't contain it.

Verification

cd apps/server && bun test src/bridge/session-policy.test.ts src/db/forgejo-sessions.test.ts src/forgejo/dispatcher.test.ts: all pass, including the existing routing test.
bun run typecheck from the repo root, or the typecheck script in the root package.json if it has another name: clean.
Record both commands and their results in the PR's ## Verification section.

Implementation: #48 ## Problem Once the implementer session opens its pull request, events on that pull request should reach the same session. - The session's pull request is recorded alongside its issue, so @mention comments on the pull request continue the implementer session (same write-access guard). - The policy allows comments on both the issue and the linked pull request. - An implementer session opens at most one pull request. Once one is linked, further `forgejo_create_pr` calls are denied. **Builds on** split 1/3 (#47), which already maps the PR an implementer session opens in `forgejo_sessions` (bridge `tool_execution_end` listener `mapImplementerPr`), allows comments on mapped numbers, and routes mentions on mapped PRs to the session (dispatcher `reuseMapped`, covered by a dispatcher test). The PR number stays in `forgejo_sessions`, not in the trigger scope; mapping failures stay log-only. Remaining gaps: deny a second `forgejo_create_pr`, and name the linked PR in the implementer's system context on resume. Plan reference: KB `project/forgejo-integration-plan.md`, steps 10 and 14. Design: `project/forgejo-integration.md` (Implementer, Session routing). ## Plan ### 3. Reverse lookup in `apps/server/src/db/forgejo-sessions.ts` There is no existing lookup from session to numbers, so add one below `setForgejoSession`: ```ts /** Numbers in `repo` mapped to `sessionId`, ascending. `repo` is `owner/repo` in any case. */ export function listForgejoSessionNumbers(repo: string, sessionId: string): number[] ``` It runs `SELECT number FROM forgejo_sessions WHERE repo_key = ? AND session_id = ? ORDER BY number` with `repo.toLowerCase()` and returns `rows.map((r) => r.number)`. Same `prepare<…>` style as `getForgejoSession`. ### 4. Policy: linked PR helper, second-PR deny, system context In `apps/server/src/bridge/session-policy.ts`: - Add a private helper: ```ts /** PRs the implementer session opened: everything mapped to it except its issue. */ function implementerPrNumbers(scope: ForgejoImplementerScope, sessionId: string): number[] { return listForgejoSessionNumbers(`${scope.owner}/${scope.repo}`, sessionId).filter((n) => n !== scope.number); } ``` Import `listForgejoSessionNumbers` next to the existing `getForgejoSession` import. - In `forgejoImplementerPolicy.decide`, put this before the final `if (this.toolNames.includes(toolName)) return "allow";`: ```ts if (toolName === "forgejo_create_pr") { const [pr] = implementerPrNumbers(scope, sessionId); if (pr !== undefined) return { deny: `this session already opened pull request #${pr}; push further commits to its branch instead` }; } ``` When the list is empty, the call falls through to the existing `"allow"`. - Change the interface to `systemContext(scope: S, sessionId: string | null): string`, with this doc: "Orientation appended to the system prompt of the session. `sessionId` is null while the session is being created." The conversation and pull-request policies ignore the new parameter: leave their implementations unchanged, since TypeScript accepts the shorter parameter list. - `forgejoImplementerPolicy.systemContext({ number, baseBranch }, sessionId)`: when `sessionId !== null` and `implementerPrNumbers(...)` returns a first entry `pr`, insert this line right after the `You were started to implement issue #…` line: `` `- You already opened pull request #${pr} for this issue. Continue the work on its branch and post updates on #${pr}; issue #${number} stays open for comments. Do not open another pull request.` `` Otherwise the text is unchanged. - Change `policySystemContext(trigger: SessionTrigger, sessionId: string | null): string` and pass `sessionId` in all three switch branches. ### 5. Callers of `policySystemContext` `grep -rn "policySystemContext(" apps/server/src` must return exactly these call sites; update each: - `apps/server/src/bridge/in-process.ts` `createSession` (~line 267): `policySystemContext(opts.trigger, null)`. The session id doesn't exist yet and no PR can be mapped yet. - `apps/server/src/bridge/in-process.ts` `resumeSession` (~line 364): `policySystemContext(trigger, existingId ?? null)`. - `apps/server/src/bridge/session-policy.test.ts`: existing calls get a `null` second argument. If the grep finds other callers, pass `null` unless the caller has the session id at hand, in which case pass it. ### Tests - `apps/server/src/db/forgejo-sessions.test.ts`, created if absent (`unverified — confirm first`; copy the `bootDb` temp-DB setup from `session-policy.test.ts` lines 108–119). `listForgejoSessionNumbers` should: - return `[4, 9]` for `o/r` after mapping `O/R#9` and `o/r#4` to `s1` and `o/r#5` to `s2`; - return `[]` for an unknown session. - `apps/server/src/bridge/session-policy.test.ts`, `describe("forgejo_implementer")`: - `forgejo_create_pr` is `allow` with only the issue mapped (`setForgejoSession("o/r", 7, "s1")`). After `setForgejoSession("o/r", 12, "s1")` it is denied with `this session already opened pull request #12; push further commits to its branch instead`, while session `s2` is still `allow`. - `policySystemContext(IMPL, "s1")`, with `o/r#12` mapped to `s1`, contains `You already opened pull request #12`. `policySystemContext(IMPL, null)` and an unmapped `"s1"` don't contain it. ### Verification - `cd apps/server && bun test src/bridge/session-policy.test.ts src/db/forgejo-sessions.test.ts src/forgejo/dispatcher.test.ts`: all pass, including the existing routing test. - `bun run typecheck` from the repo root, or the typecheck script in the root `package.json` if it has another name: clean. - Record both commands and their results in the PR's `## Verification` section.

lucas added the

labels

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

omp-deck added

and removed

labels

2026-10-20 13:38:06 +02:00

omp-deck commented

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

Author

Collaborator

Blocker resolved: plan rebased on split 1/3 (#47). The original plan assumed 1/3 would keep a pullNumber in the implementer trigger scope. 1/3 landed with a different design: forgejo_sessions records the PR an implementer session opens (bridge tool_execution_end listener), the comment policy and the dispatcher read that mapping, and routing PR mentions to the session is already tested. This issue now builds on that design and covers only the remaining gaps: (1) deny a second forgejo_create_pr once a PR is mapped to the session; (2) name the linked PR in the implementer's system context on resume. If mapping the PR fails, it is still only logged, as in 1/3. The scope keeps no pullNumber and the PR wrapper from the original plan is dropped. The issue description has been updated to the revised plan.

**Blocker resolved: plan rebased on split 1/3 (#47).** The original plan assumed 1/3 would keep a `pullNumber` in the implementer trigger scope. 1/3 landed with a different design: `forgejo_sessions` records the PR an implementer session opens (bridge `tool_execution_end` listener), the comment policy and the dispatcher read that mapping, and routing PR mentions to the session is already tested. This issue now builds on that design and covers only the remaining gaps: (1) deny a second `forgejo_create_pr` once a PR is mapped to the session; (2) name the linked PR in the implementer's system context on resume. If mapping the PR fails, it is still only logged, as in 1/3. The scope keeps no `pullNumber` and the PR wrapper from the original plan is dropped. The issue description has been updated to the revised plan.

omp-deck referenced this issue from a commit

2026-10-20 13:45:32 +02:00

feat(forgejo): deny a second pull request from an implementer session

omp-deck referenced this issue from a commit

2026-10-20 13:45:32 +02:00

feat(forgejo): name the linked pull request when an implementer session resumes

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

2026-10-20 13:45:38 +02:00

feat(forgejo): link an implementer session to its pull request #48

lucas closed this issue

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