Forgejo: implementer failure and reset (plan step 22) #147

New issue

Closed

opened 2026-10-29 13:24:26 +01:00 by omp-deck · 0 comments

omp-deck commented

2026-10-29 13:24:26 +01:00

Collaborator

Problem

An implementer session can today only end through success (merge → post-merge CI → archive) or a human closing its PR, which merely archives ("archived: closed without merge", handleLifecycle in apps/server/src/forgejo/dispatcher.ts). Nothing resets the issue's labels and nobody records what was tried. Failure must be a regular outcome: the PR is closed unmerged, the issue goes back to planning with a summary of what was attempted, what failed and why, and a later implementer run on the same issue starts cleanly.

Decisions:

Both sides can declare failure. The agent via a new tool forgejo_abandon_implementation; a human by closing the agent's PR without merging, which gives the agent one final turn to write the summary.
Planning state is a new optional per-repo key implementation.planning_label; null means the progress label is only removed. Status/Blocked is not used (it makes checkImplementable refuse restarts).
The failed PR's head branch is kept; the summary names PR and branch, and a new run must pick a different branch name.
Deck-side turn failures (timeout, restart, agent error in runImplementerTurn) keep their current notice and do not reset.
implementation.max_concurrent needs no change: intakeRunning counts only the first turn and is released in runIntake's finally (releaseIntakeSlot), so a failed session holds no slot.

Plan

Context

Add an explicit failure path for implementer sessions: agent tool + human PR close → close PR unmerged, swap progress label for planning label, post failure summary on the issue (issue stays open), archive the session. Re-running an implementer on the issue afterwards must succeed.

Approach

Config key (apps/server/src/forgejo/repo-config.ts)
- Extend RepoConfig["implementation"] to { intakeLabel; progressLabel; planningLabel: string | null; maxConcurrent }; default planningLabel: null in DEFAULT_REPO_CONFIG.
- In parseImplementation, add "planning_label" to the key loop (same non-empty-string validation) and return it.
- In parseRepoConfig, after the existing distinctness checks, reject planningLabel equal to progressLabel → error `implementation.planning_label` must differ from `implementation.progress_label`; equal to intakeLabel is allowed (re-queueing straight to intake is a legitimate setup).
- Update every literal of the implementation shape the compiler flags (tests in repo-config.test.ts, dispatcher.test.ts).
Close-PR API (apps/server/src/forgejo/dispatch-api.ts)
- Add closePull(owner, repo, number, signal?): Promise<ApiResult<unknown>> to DispatchApi = PATCH /repos/{owner}/{repo}/pulls/{number} with { state: "closed" }. Reuse the client call that forgejo_update_pr (apps/server/src/forgejo/tools/prs.ts, updatePr) uses for its state field; add the method to the fake API in dispatcher.test.ts recording calls in closedPulls: number[].
Agent tool forgejo_abandon_implementation (apps/server/src/forgejo/tools/issues.ts, via defineForgejoTool, mutating: true, added to the exported tool list next to closeIssue)
- Params: number (int, the issue), summary (non-empty string, markdown). Description: "Forgejo: end this implementation as failed. Posts summary on the issue; omp-deck then closes your pull request without merging, moves the issue back to planning and ends this session. Use only when the work cannot be completed: CI you cannot fix, a plan that does not hold, or a rejected approach."
- run: post issue comment on number with body ## Implementation abandoned\n\n${summary} and return textResult("Posted the failure summary on #N. omp-deck will close the pull request and end this session; end your turn now."). Errors via apiError.
- Register in apps/server/src/forgejo/tools/index.ts lists that index.test.ts / permission-filter.test.ts enumerate (mutating list).
Implementer policy (apps/server/src/bridge/session-policy.ts)
- Add "forgejo_abandon_implementation" to IMPLEMENTER_TOOL_NAMES only (not conversation/PR policies).
- In forgejoImplementerPolicy.decide: allow only when paramNumber(params) === scope.number, else { deny: "only #${scope.number} can be abandoned in this session" }.
- In systemContext rules add bullet: "- If the work cannot be completed (CI you cannot fix after real attempts, a plan that falls apart, or reviewers rejecting the approach), call forgejo_abandon_implementation on #N with a summary: what you attempted, what failed and why, the pull request and branch, and what re-planning should consider. Do not close the pull request or change labels yourself."
- In the branch-creation bullet (around line 450, agent/issue-${number}-<slug>), append: "pick a name not already on origin (earlier attempts' branches are kept)".
Record the abandon (apps/server/src/bridge/in-process.ts, tool_execution_end block next to the forgejo_create_pr mapping ~line 986)
- When toolName === "forgejo_abandon_implementation", trigger?.kind === "forgejo_implementer" and the result is not an error: call enqueueAbandon(\${scope.owner}/${scope.repo}`, scope.number, sessionId, true)(below). The row follows thequeuePostMergeFixsynthetic-row pattern withevent: "issues", action: "implementation_abandoned", isPull: false, commentAuthor: "", deliveryId: "deck:abandon", dedupeKey: sha256(JSON.stringify(["implementation_abandoned", repoLower, number, sessionId])), payloadJson: JSON.stringify({ sessionId, summaryPosted }). Then wake the dispatcher the way other out-of-dispatcher inserts do (unverified — confirm first; if no hook exists, pass a wake` callback into the bridge from where the dispatcher is constructed).
- Put the row construction in an exported helper enqueueAbandon(repo, number, sessionId, summaryPosted) in dispatcher.ts (module-level function, no class state) so step 7 reuses it.
Abandon handler (dispatcher.ts): route rows with action === "implementation_abandoned" (add next to where post_merge_ci_failed is routed) to new private async handleAbandon(row):
1. Read sessionId from payload; if the session is already archived (getArchivedMap) → finishForgejoTrigger(row.id, "dropped", "session already archived").
2. await this.waitUntilIdle(sessionId, SESSION_POLICIES.forgejo_implementer.turnTimeoutMs) (lets the agent's turn end).
3. For each number in listForgejoSessionNumbers(repo, sessionId) except the issue: readIssue; if isPull and state === "open" → api.closePull. Failures are collected into a reason list, not thrown.
4. Labels: read config with existing readRepoConfig-style logic (api.readRepoFile + parseRepoConfig; invalid/missing → defaults, log.warn). New swapPlanningLabels(api, issue, labels) mirroring swapInProgressLabels: remove = [progressLabel], add = [planningLabel] (each only if non-null), same planLabelDelta + replaceIssueLabels flow, returns "" or reason.
5. If summaryPosted is false, post omp-deck: the implementation of #N was abandoned after @<closer> closed pull request #P without merging; the agent did not write a summary. Details are in omp-deck session \`.` on the issue.
6. If any reason collected: post omp-deck could not fully reset this issue after abandoning the implementation: <reasons> (fallback notificationService.notify like markInProgress).
7. archiveSession via this.archiveAfterIdle(sessionId, "forgejo_implementer"); finish row "done", "implementation abandoned".
- Do not touch this.running/intakeRunning.
Human scrap via PR close (handleLifecycle, the row.isPull branch)
- Before the existing archive: if !merged, trigger.kind === "forgejo_implementer", PR author (payload.pull_request.user.login) is the agent, and payload.sender.login is not the agent: queue a turn instead of archiving. Insert a synthetic row (queuePostMergeFix pattern) with action: "implementation_scrapped", isPull: true, number = PR, commentAuthor = sender, dedupe ["implementation_scrapped", repoLower, prNumber, sessionId]; finish the lifecycle row "done", "scrapped by @sender: final turn queued"; return.
- If !merged and the sender is the agent (the abandon handler closed it): finish "dropped", "closed by the agent while abandoning" without archiving (step 6 archives).
- Handle implementation_scrapped rows by resuming the session through the same path existing turn rows use (ensureSessionLive region ~line 1496), with prompt from new buildScrappedTurn in conversation-detect.ts: "@ closed your pull request #P without merging. The implementation is over. Call forgejo_abandon_implementation on # with a summary of what you attempted, what failed and why, the pull request and its branch, and what re-planning should consider. Make no other changes." After the turn settles (any outcome), call enqueueAbandon(repo, issue, sessionId, false): if the agent's tool call already inserted the row the dedupe key makes this a no-op (confirm insertForgejoTrigger ignores duplicate dedupe keys — unverified; if it throws instead, catch the unique-constraint error), so the fixed notice of step 6.5 is posted only when the agent did not summarize.
- Issue number for the prompt/enqueue: trigger.scope.number.
Re-run on the same issue: no code change beyond the above. After archive, checkImplementable no longer sees a usable mapped session, createCheckout re-creates issue-N, setForgejoSession overwrites the mapping, and the old PR is closed so implementerPrNumbers of the new session is empty. Verified by test in Verification.

Critical files & anchors

apps/server/src/forgejo/dispatcher.ts — handleLifecycle (~1650), swapInProgressLabels (~2043), queuePostMergeFix (~1273) as the synthetic-row pattern.
apps/server/src/bridge/session-policy.ts — IMPLEMENTER_TOOL_NAMES (152), forgejoImplementerPolicy (350).
apps/server/src/bridge/in-process.ts — tool_execution_end hook (~986).
apps/server/src/forgejo/repo-config.ts — parseImplementation.

Verification

Run from /workspace/omp-deck: bun test apps/server/src/forgejo apps/server/src/bridge and the repo typecheck.
New tests:

repo-config.test.ts: implementation: { planning_label: "Status/Planning" } parses; planning_label equal to progress_label → the exact error string.
session-policy.test.ts: implementer decidePolicy(IMPL, "forgejo_abandon_implementation", { number: 7 }, "s1") → "allow"; { number: 8 } → deny; conversation policy denies it.
dispatcher.test.ts (existing fake API/harness):
- abandon row for a session mapped to issue 7 + open PR 12, config progress_label: "agent/in progress", planning_label: "agent/planning" → closedPulls == [12], label PUT removes progress and adds planning, session archived, no extra comment posted.
- closed-unmerged lifecycle row on PR 12 with sender alice → no archive, an implementation_scrapped row queued; running it with an agent that does not call the tool → fixed "did not write a summary" comment on #7, labels reset, archived.
- same lifecycle row with sender = agent → dropped, not archived.
- after abandon, startImplementer on #7 succeeds (no 409).

Assumptions & contingencies

If Forgejo's PATCH pull endpoint is unavailable, close via the issues PATCH { state: "closed" } (PRs share issue routes).

## Problem An implementer session can today only end through success (merge → post-merge CI → archive) or a human closing its PR, which merely archives ("archived: closed without merge", `handleLifecycle` in `apps/server/src/forgejo/dispatcher.ts`). Nothing resets the issue's labels and nobody records what was tried. Failure must be a regular outcome: the PR is closed unmerged, the issue goes back to planning with a summary of what was attempted, what failed and why, and a later implementer run on the same issue starts cleanly. Decisions: - **Both sides can declare failure.** The agent via a new tool `forgejo_abandon_implementation`; a human by closing the agent's PR without merging, which gives the agent one final turn to write the summary. - **Planning state** is a new optional per-repo key `implementation.planning_label`; null means the progress label is only removed. `Status/Blocked` is not used (it makes `checkImplementable` refuse restarts). - **The failed PR's head branch is kept**; the summary names PR and branch, and a new run must pick a different branch name. - Deck-side turn failures (timeout, restart, agent error in `runImplementerTurn`) keep their current notice and do **not** reset. - `implementation.max_concurrent` needs no change: `intakeRunning` counts only the first turn and is released in `runIntake`'s `finally` (`releaseIntakeSlot`), so a failed session holds no slot. ## Plan ### Context Add an explicit failure path for implementer sessions: agent tool + human PR close → close PR unmerged, swap progress label for planning label, post failure summary on the issue (issue stays open), archive the session. Re-running an implementer on the issue afterwards must succeed. ### Approach 1. **Config key** (`apps/server/src/forgejo/repo-config.ts`) - Extend `RepoConfig["implementation"]` to `{ intakeLabel; progressLabel; planningLabel: string | null; maxConcurrent }`; default `planningLabel: null` in `DEFAULT_REPO_CONFIG`. - In `parseImplementation`, add `"planning_label"` to the key loop (same non-empty-string validation) and return it. - In `parseRepoConfig`, after the existing distinctness checks, reject `planningLabel` equal to `progressLabel` → error `` `implementation.planning_label` must differ from `implementation.progress_label` ``; equal to `intakeLabel` is allowed (re-queueing straight to intake is a legitimate setup). - Update every literal of the implementation shape the compiler flags (tests in `repo-config.test.ts`, `dispatcher.test.ts`). 2. **Close-PR API** (`apps/server/src/forgejo/dispatch-api.ts`) - Add `closePull(owner, repo, number, signal?): Promise<ApiResult<unknown>>` to `DispatchApi` = `PATCH /repos/{owner}/{repo}/pulls/{number}` with `{ state: "closed" }`. Reuse the client call that `forgejo_update_pr` (`apps/server/src/forgejo/tools/prs.ts`, `updatePr`) uses for its `state` field; add the method to the fake API in `dispatcher.test.ts` recording calls in `closedPulls: number[]`. 3. **Agent tool** `forgejo_abandon_implementation` (`apps/server/src/forgejo/tools/issues.ts`, via `defineForgejoTool`, `mutating: true`, added to the exported tool list next to `closeIssue`) - Params: `number` (int, the issue), `summary` (non-empty string, markdown). Description: "Forgejo: end this implementation as failed. Posts `summary` on the issue; omp-deck then closes your pull request without merging, moves the issue back to planning and ends this session. Use only when the work cannot be completed: CI you cannot fix, a plan that does not hold, or a rejected approach." - `run`: post issue comment on `number` with body `## Implementation abandoned\n\n${summary}` and return `textResult("Posted the failure summary on #N. omp-deck will close the pull request and end this session; end your turn now.")`. Errors via `apiError`. - Register in `apps/server/src/forgejo/tools/index.ts` lists that `index.test.ts` / `permission-filter.test.ts` enumerate (mutating list). 4. **Implementer policy** (`apps/server/src/bridge/session-policy.ts`) - Add `"forgejo_abandon_implementation"` to `IMPLEMENTER_TOOL_NAMES` only (not conversation/PR policies). - In `forgejoImplementerPolicy.decide`: allow only when `paramNumber(params) === scope.number`, else `{ deny: "only #${scope.number} can be abandoned in this session" }`. - In `systemContext` rules add bullet: "- If the work cannot be completed (CI you cannot fix after real attempts, a plan that falls apart, or reviewers rejecting the approach), call `forgejo_abandon_implementation` on #N with a summary: what you attempted, what failed and why, the pull request and branch, and what re-planning should consider. Do not close the pull request or change labels yourself." - In the branch-creation bullet (around line 450, `agent/issue-${number}-<slug>`), append: "pick a name not already on origin (earlier attempts' branches are kept)". 5. **Record the abandon** (`apps/server/src/bridge/in-process.ts`, `tool_execution_end` block next to the `forgejo_create_pr` mapping ~line 986) - When `toolName === "forgejo_abandon_implementation"`, `trigger?.kind === "forgejo_implementer"` and the result is not an error: call `enqueueAbandon(\`${scope.owner}/${scope.repo}\`, scope.number, sessionId, true)` (below). The row follows the `queuePostMergeFix` synthetic-row pattern with `event: "issues"`, `action: "implementation_abandoned"`, `isPull: false`, `commentAuthor: ""`, `deliveryId: "deck:abandon"`, `dedupeKey: sha256(JSON.stringify(["implementation_abandoned", repoLower, number, sessionId]))`, `payloadJson: JSON.stringify({ sessionId, summaryPosted })`. Then wake the dispatcher the way other out-of-dispatcher inserts do (unverified — confirm first; if no hook exists, pass a `wake` callback into the bridge from where the dispatcher is constructed). - Put the row construction in an exported helper `enqueueAbandon(repo, number, sessionId, summaryPosted)` in `dispatcher.ts` (module-level function, no class state) so step 7 reuses it. 6. **Abandon handler** (`dispatcher.ts`): route rows with `action === "implementation_abandoned"` (add next to where `post_merge_ci_failed` is routed) to new `private async handleAbandon(row)`: 1. Read `sessionId` from payload; if the session is already archived (`getArchivedMap`) → `finishForgejoTrigger(row.id, "dropped", "session already archived")`. 2. `await this.waitUntilIdle(sessionId, SESSION_POLICIES.forgejo_implementer.turnTimeoutMs)` (lets the agent's turn end). 3. For each number in `listForgejoSessionNumbers(repo, sessionId)` except the issue: `readIssue`; if `isPull` and `state === "open"` → `api.closePull`. Failures are collected into a reason list, not thrown. 4. Labels: read config with existing `readRepoConfig`-style logic (`api.readRepoFile` + `parseRepoConfig`; invalid/missing → defaults, log.warn). New `swapPlanningLabels(api, issue, labels)` mirroring `swapInProgressLabels`: `remove = [progressLabel]`, `add = [planningLabel]` (each only if non-null), same `planLabelDelta` + `replaceIssueLabels` flow, returns "" or reason. 5. If `summaryPosted` is false, post `omp-deck: the implementation of #N was abandoned after @<closer> closed pull request #P without merging; the agent did not write a summary. Details are in omp-deck session \`<id>\`.` on the issue. 6. If any reason collected: post `omp-deck could not fully reset this issue after abandoning the implementation: <reasons>` (fallback `notificationService.notify` like `markInProgress`). 7. `archiveSession` via `this.archiveAfterIdle(sessionId, "forgejo_implementer")`; finish row `"done"`, `"implementation abandoned"`. - Do not touch `this.running`/`intakeRunning`. 7. **Human scrap via PR close** (`handleLifecycle`, the `row.isPull` branch) - Before the existing archive: if `!merged`, `trigger.kind === "forgejo_implementer"`, PR author (`payload.pull_request.user.login`) is the agent, and `payload.sender.login` is **not** the agent: queue a turn instead of archiving. Insert a synthetic row (`queuePostMergeFix` pattern) with `action: "implementation_scrapped"`, `isPull: true`, number = PR, `commentAuthor` = sender, dedupe `["implementation_scrapped", repoLower, prNumber, sessionId]`; finish the lifecycle row `"done"`, `"scrapped by @sender: final turn queued"`; return. - If `!merged` and the sender **is** the agent (the abandon handler closed it): finish `"dropped"`, `"closed by the agent while abandoning"` without archiving (step 6 archives). - Handle `implementation_scrapped` rows by resuming the session through the same path existing turn rows use (`ensureSessionLive` region ~line 1496), with prompt from new `buildScrappedTurn` in `conversation-detect.ts`: "@<sender> closed your pull request #P without merging. The implementation is over. Call `forgejo_abandon_implementation` on #<issue> with a summary of what you attempted, what failed and why, the pull request and its branch, and what re-planning should consider. Make no other changes." After the turn settles (any outcome), call `enqueueAbandon(repo, issue, sessionId, false)`: if the agent's tool call already inserted the row the dedupe key makes this a no-op (confirm `insertForgejoTrigger` ignores duplicate dedupe keys — unverified; if it throws instead, catch the unique-constraint error), so the fixed notice of step 6.5 is posted only when the agent did not summarize. - Issue number for the prompt/enqueue: `trigger.scope.number`. 8. **Re-run on the same issue**: no code change beyond the above. After archive, `checkImplementable` no longer sees a usable mapped session, `createCheckout` re-creates `issue-N`, `setForgejoSession` overwrites the mapping, and the old PR is closed so `implementerPrNumbers` of the new session is empty. Verified by test in Verification. ### Critical files & anchors - `apps/server/src/forgejo/dispatcher.ts` — `handleLifecycle` (~1650), `swapInProgressLabels` (~2043), `queuePostMergeFix` (~1273) as the synthetic-row pattern. - `apps/server/src/bridge/session-policy.ts` — `IMPLEMENTER_TOOL_NAMES` (152), `forgejoImplementerPolicy` (350). - `apps/server/src/bridge/in-process.ts` — `tool_execution_end` hook (~986). - `apps/server/src/forgejo/repo-config.ts` — `parseImplementation`. ### Verification Run from `/workspace/omp-deck`: `bun test apps/server/src/forgejo apps/server/src/bridge` and the repo typecheck. New tests: - `repo-config.test.ts`: `implementation: { planning_label: "Status/Planning" }` parses; `planning_label` equal to `progress_label` → the exact error string. - `session-policy.test.ts`: implementer `decidePolicy(IMPL, "forgejo_abandon_implementation", { number: 7 }, "s1")` → `"allow"`; `{ number: 8 }` → deny; conversation policy denies it. - `dispatcher.test.ts` (existing fake API/harness): - abandon row for a session mapped to issue 7 + open PR 12, config `progress_label: "agent/in progress"`, `planning_label: "agent/planning"` → `closedPulls == [12]`, label PUT removes progress and adds planning, session archived, no extra comment posted. - closed-unmerged lifecycle row on PR 12 with sender `alice` → no archive, an `implementation_scrapped` row queued; running it with an agent that does not call the tool → fixed "did not write a summary" comment on #7, labels reset, archived. - same lifecycle row with sender = agent → dropped, not archived. - after abandon, `startImplementer` on #7 succeeds (no 409). ### Assumptions & contingencies - If Forgejo's PATCH pull endpoint is unavailable, close via the issues PATCH `{ state: "closed" }` (PRs share issue routes).