Let the implementer switch its own PR between draft and ready #223

New issue

Closed

opened 2026-11-04 15:49:55 +01:00 by omp-deck · 0 comments

omp-deck commented

2026-11-04 15:49:55 +01:00

Collaborator

Problem

An implementer session can't switch its own pull request between draft and ready for review. In Forgejo, a PR is a draft when its title starts with WIP: or [WIP]. The way to toggle that is forgejo_update_pr with a new title (apps/server/src/forgejo/tools/prs.ts, updatePr). The shared lifecycle prompt (apps/server/src/forgejo/tools/context.ts:43) tells agents to do exactly that. But IMPLEMENTER_TOOL_NAMES in apps/server/src/bridge/session-policy.ts doesn't include forgejo_update_pr, so forgejoImplementerPolicy.decide hits its fallback { deny: "not available in unattended Forgejo implementer sessions" }. Implementer PRs opened with draft: true can never be marked ready, and a ready PR can't go back to draft.

Decided scope: the implementer may call forgejo_update_pr only on its own PRs, and may change only title and body. Changing base or state stays denied. Closing a PR is still reserved for forgejo_abandon_implementation.

Plan

Approach

Expose the tool. In apps/server/src/bridge/session-policy.ts, add "forgejo_update_pr" to the IMPLEMENTER_TOOL_NAMES array, directly after "forgejo_create_pr".
Gate it in forgejoImplementerPolicy.decide. Add a branch right after the existing forgejo_create_pr block (before the scope.headBranch block):
```
if (toolName === "forgejo_update_pr") {
	const p = (params as { base?: unknown; state?: unknown } | null) ?? {};
	if (p.base !== undefined || p.state !== undefined) {
		return { deny: "only the title and body of a pull request can be changed in this session" };
	}
	const n = paramNumber(params);
	if (!implementerPrNumbers(scope, sessionId).includes(n)) {
		return { deny: `only this session's own pull requests can be edited; #${n} is not one of them` };
	}
}
```
- Reuse the existing paramNumber and implementerPrNumbers helpers in the same file. Don't add new helpers.
- Use these deny strings exactly as written; the tests assert them.
- When the branch doesn't deny, the call falls through to the existing if (this.toolNames.includes(toolName)) return "allow";. That keeps the call unattended-safe ("allow", not "default").
- implementerPrNumbers reads the local session mapping only (no Forgejo call), so this path needs no API-failure handling.
- It returns [] before a PR exists, so every edit is denied until the PR is opened. That is intended.
Tell the implementer. In forgejoImplementerPolicy.systemContext, add this bullet to the rules array, after the forgejo_create_issue bullet:
"- Mark your pull request ready for review by removing the \WIP:`/`[WIP]` prefix from its title with `forgejo_update_pr`, and turn it back into a draft by adding `WIP: ` in front. You may change only the title and body of your own pull requests."`
Don't change the "Do not close the pull request or change labels yourself" sentence in the abandon bullet.

Critical files & anchors

apps/server/src/bridge/session-policy.ts: IMPLEMENTER_TOOL_NAMES (~L200–233), forgejoImplementerPolicy.decide (~L461), systemContext rules (~L506).
apps/server/src/bridge/session-policy.test.ts: implementer describe block, around the existing forgejo_abandon_implementation and forgejo_create_pr tests (~L181–241). Copy their IMPL / bootDb() / setForgejoSession / NO_API setup.

Verification

Add tests to apps/server/src/bridge/session-policy.test.ts in the implementer section:

policyToolNames(IMPL) contains "forgejo_update_pr".
After bootDb() and setForgejoSession("o/r", 12, "s1"):
- decidePolicy(IMPL, "forgejo_update_pr", { number: 12, title: "Ready" }, "s1", process.cwd(), NO_API) returns "allow".
- The same call with { number: 12, body: "b" } returns "allow".
{ number: 13, title: "x" } (not mapped to s1) returns { deny: "only this session's own pull requests can be edited; #13 is not one of them" }. The same holds for session "s2" with #12.
{ number: 12, state: "closed" } and { number: 12, base: "dev" } each return { deny: "only the title and body of a pull request can be changed in this session" }.
With scope.pull: 12 and no mapping, { number: 12, title: "x" } returns "allow".
policySystemContext(IMPL, "s1") contains "forgejo_update_pr".

Run from the repo root: bun test apps/server/src/bridge/session-policy.test.ts. The new tests MUST pass. Then run bun test apps/server/src/bridge to confirm permission-filter.test.ts still passes.

## Problem An implementer session can't switch its own pull request between draft and ready for review. In Forgejo, a PR is a draft when its title starts with `WIP:` or `[WIP]`. The way to toggle that is `forgejo_update_pr` with a new `title` (`apps/server/src/forgejo/tools/prs.ts`, `updatePr`). The shared lifecycle prompt (`apps/server/src/forgejo/tools/context.ts:43`) tells agents to do exactly that. But `IMPLEMENTER_TOOL_NAMES` in `apps/server/src/bridge/session-policy.ts` doesn't include `forgejo_update_pr`, so `forgejoImplementerPolicy.decide` hits its fallback `{ deny: "not available in unattended Forgejo implementer sessions" }`. Implementer PRs opened with `draft: true` can never be marked ready, and a ready PR can't go back to draft. Decided scope: the implementer may call `forgejo_update_pr` only on its own PRs, and may change only `title` and `body`. Changing `base` or `state` stays denied. Closing a PR is still reserved for `forgejo_abandon_implementation`. ## Plan ### Approach 1. **Expose the tool.** In `apps/server/src/bridge/session-policy.ts`, add `"forgejo_update_pr"` to the `IMPLEMENTER_TOOL_NAMES` array, directly after `"forgejo_create_pr"`. 2. **Gate it in `forgejoImplementerPolicy.decide`.** Add a branch right after the existing `forgejo_create_pr` block (before the `scope.headBranch` block): ```ts if (toolName === "forgejo_update_pr") { const p = (params as { base?: unknown; state?: unknown } | null) ?? {}; if (p.base !== undefined || p.state !== undefined) { return { deny: "only the title and body of a pull request can be changed in this session" }; } const n = paramNumber(params); if (!implementerPrNumbers(scope, sessionId).includes(n)) { return { deny: `only this session's own pull requests can be edited; #${n} is not one of them` }; } } ``` - Reuse the existing `paramNumber` and `implementerPrNumbers` helpers in the same file. Don't add new helpers. - Use these deny strings exactly as written; the tests assert them. - When the branch doesn't deny, the call falls through to the existing `if (this.toolNames.includes(toolName)) return "allow";`. That keeps the call unattended-safe ("allow", not "default"). - `implementerPrNumbers` reads the local session mapping only (no Forgejo call), so this path needs no API-failure handling. - It returns `[]` before a PR exists, so every edit is denied until the PR is opened. That is intended. 3. **Tell the implementer.** In `forgejoImplementerPolicy.systemContext`, add this bullet to the `rules` array, after the `forgejo_create_issue` bullet: `"- Mark your pull request ready for review by removing the \`WIP:\`/\`[WIP]\` prefix from its title with \`forgejo_update_pr\`, and turn it back into a draft by adding \`WIP: \` in front. You may change only the title and body of your own pull requests."` Don't change the "Do not close the pull request or change labels yourself" sentence in the abandon bullet. ### Critical files & anchors - `apps/server/src/bridge/session-policy.ts`: `IMPLEMENTER_TOOL_NAMES` (~L200–233), `forgejoImplementerPolicy.decide` (~L461), `systemContext` `rules` (~L506). - `apps/server/src/bridge/session-policy.test.ts`: implementer describe block, around the existing `forgejo_abandon_implementation` and `forgejo_create_pr` tests (~L181–241). Copy their `IMPL` / `bootDb()` / `setForgejoSession` / `NO_API` setup. ### Verification Add tests to `apps/server/src/bridge/session-policy.test.ts` in the implementer section: - `policyToolNames(IMPL)` contains `"forgejo_update_pr"`. - After `bootDb()` and `setForgejoSession("o/r", 12, "s1")`: - `decidePolicy(IMPL, "forgejo_update_pr", { number: 12, title: "Ready" }, "s1", process.cwd(), NO_API)` returns `"allow"`. - The same call with `{ number: 12, body: "b" }` returns `"allow"`. - `{ number: 13, title: "x" }` (not mapped to s1) returns `{ deny: "only this session's own pull requests can be edited; #13 is not one of them" }`. The same holds for session `"s2"` with `#12`. - `{ number: 12, state: "closed" }` and `{ number: 12, base: "dev" }` each return `{ deny: "only the title and body of a pull request can be changed in this session" }`. - With `scope.pull: 12` and no mapping, `{ number: 12, title: "x" }` returns `"allow"`. - `policySystemContext(IMPL, "s1")` contains `"forgejo_update_pr"`. Run from the repo root: `bun test apps/server/src/bridge/session-policy.test.ts`. The new tests MUST pass. Then run `bun test apps/server/src/bridge` to confirm `permission-filter.test.ts` still passes.