Implement-issue button: server enforces Agent/Ready gate and moves issue to Agent/In Progress #83

New issue

Closed

opened 2026-10-26 15:38:57 +01:00 by omp-deck · 0 comments

omp-deck commented

2026-10-26 15:38:57 +01:00

Collaborator

Problem

The Agent/Ready / Status/Blocked label check and the Agent/Ready → Agent/In Progress transition currently exist only as instructions to the agent (.omp/commands/implement.md steps 3–4), which agents don't follow reliably. The server-started implementer (the "Implement issue" button in the web UI → POST /api/forgejo/implement → ForgejoDispatcher.startImplementer) does no label handling at all, and its agent has no label tool.

When a session is started via the "Implement issue" button, the server itself must perform the checks and the label change:

Refuse to start (HTTP 422, no session) unless the issue has Agent/Ready and does not have Status/Blocked. Label names match case-insensitively after trimming.
After the session is created successfully, swap Agent/Ready → Agent/In Progress. If the swap fails, the session still runs; the failure is logged, posted as a comment on the issue, and — if that comment also fails — raised as a deck notification.

The manual /implement chat command keeps its own instructions unchanged. The implementer's system context is not changed.

Plan

Approach

Steps 1–2 keep the tree building and existing tests passing; step 3 adds the behavior.

1. Make label fetching usable outside tools (apps/server/src/forgejo/tools/labels.ts)

Change fetchAvailableLabels(ctx: ForgejoToolContext, signal?) to fetchAvailableLabels(ctx: Pick<ForgejoToolContext, "client" | "owner" | "repo">, signal?). The body only uses those three fields, so existing callers in tools/issues.ts and tools/prs.ts stay valid unchanged.

2. Extend DispatchApi (apps/server/src/forgejo/dispatch-api.ts)

IssueSummary gains labels: LabelRef[] (LabelRef = { id: number; name: string }, import the type from ./tools/labels.ts). Update its doc comment to "Title, state and labels of issue/PR number…".
RawIssue gains labels?: { id: number; name: string }[] | null; readIssue maps it to labels: (raw.labels ?? []).map((l) => ({ id: l.id, name: l.name })).
Add two methods to the interface and to createDispatchApi:
- listLabels(owner: string, repo: string, signal?: AbortSignal): Promise<ApiResult<LabelRef[]>> — doc: "Labels usable on issues of the repo: its own plus its org's." Implementation: fetchAvailableLabels({ client, owner, repo }, signal).
- replaceIssueLabels(owner: string, repo: string, number: number, ids: number[], signal?: AbortSignal): Promise<ApiResult<unknown>> — doc: "Replace all labels of issue number with ids in one request." Implementation: client.json<unknown>("PUT", repoApiPath(owner, repo, "issues", String(number), "labels"), { body: { labels: ids }, signal }) (same endpoint/body the forgejo_update_issue_labels tool uses in tools/issues.ts).
In apps/server/src/forgejo/dispatcher.test.ts, update FakeApi so it compiles: add availableLabels: LabelRef[] (default: [{ id: 1, name: "Agent/Ready" }, { id: 2, name: "Agent/In Progress" }, { id: 3, name: "Status/Blocked" }]), listLabelsResult: ApiResult<LabelRef[]> | null = null (when set, listLabels returns it instead), labelPuts: Array<{ number: number; ids: number[] }>, and putStatus: number | null = null (when set, replaceIssueLabels fails with that status). replaceIssueLabels records the put and, on success, rewrites issues.get(number).labels to the matching availableLabels entries. Change the OPEN constant in the startImplementer describe block to include labels: [{ id: 1, name: "Agent/Ready" }].

3. Gate and transition in ForgejoDispatcher.startImplementer (apps/server/src/forgejo/dispatcher.ts)

Add module constants: const READY_LABEL = "Agent/Ready";, const IN_PROGRESS_LABEL = "Agent/In Progress";, const BLOCKED_LABEL = "Status/Blocked"; and a private helper hasLabel(labels: readonly LabelRef[], name: string): boolean comparing l.name.trim().toLowerCase() === name.toLowerCase().
Precheck: directly after the existing isPull and state !== "open" checks (before the second this.running.has(key) check and this.running.add(key)):
- not hasLabel(issue.data.labels, READY_LABEL) → throw new ImplementerStartError(422, #${number} is not labeled ${READY_LABEL}).
- hasLabel(issue.data.labels, BLOCKED_LABEL) → throw new ImplementerStartError(422, #${number} is labeled ${BLOCKED_LABEL}).
- Ready check first, so an issue missing Ready and having Blocked reports the Ready message.
Transition: add private async markInProgress(api: DispatchApi, issue: { owner: string; repo: string; number: number }, sessionId: string): Promise<void> that never throws. In startImplementer, call await this.markInProgress(api, { owner, repo, number }, handle.sessionId) after the existing setName try/catch and before this.track(this.runImplementerTurn(...)). It runs only when session creation succeeded (failed checkout/createSession paths throw earlier and touch no labels). Steps:
1. Re-read the issue with api.readIssue (fresh labels, so the replace-all PUT does not drop labels added since the precheck) and fetch api.listLabels(owner, repo). Either failing → reason `${status} ${message}`.
2. planLabelDelta({ current: issue.labels, available, add: [IN_PROGRESS_LABEL], remove: [READY_LABEL] }) (reuse from ./tools/labels.ts). !ok → reason = plan.error (e.g. unknown Agent/In Progress). noop → return.
3. api.replaceIssueLabels(owner, repo, number, plan.nextIds); failure → reason `${status} ${message}`.
4. On any reason: log.warn(could not mark ${repoFull}#${number} in progress: ${reason}); post api.postComment(owner, repo, number, omp-deck could not move this issue to ${IN_PROGRESS_LABEL}: ${reason}\n\nThe implementer session `${sessionId}` is running regardless.); if the post fails, log.warn it and await notificationService.notify({ level: "error", title: Could not mark ${repoFull}#${number} in progress, body: ${reason}\n\nDeck session: `${sessionId}`, source: INBOX_SOURCE }) — same pattern as the failure branch of runImplementerTurn. Wrap the whole method body in try/catch so an unexpected throw is logged and treated as a reason too; it must not reach the caller (the running key is already claimed and the session exists).

Critical files & anchors

apps/server/src/forgejo/dispatcher.ts — startImplementer (~L826–909): precheck after the #N is closed check; markInProgress call between setName and this.track(...). runImplementerTurn (~L941–955) is the comment→notification fallback pattern to copy.
apps/server/src/forgejo/dispatch-api.ts — IssueSummary, RawIssue, createDispatchApi.
apps/server/src/forgejo/tools/labels.ts — planLabelDelta, fetchAvailableLabels, LabelRef.
apps/server/src/forgejo/dispatcher.test.ts — FakeApi (~L74) and describe("ForgejoDispatcher.startImplementer") (~L865).

Verification

Add these tests to describe("ForgejoDispatcher.startImplementer") in apps/server/src/forgejo/dispatcher.test.ts:

Issue without Agent/Ready → refusal(...) equals { status: 422, message: "#4 is not labeled Agent/Ready" }; issue with Agent/Ready + status/blocked (case/whitespace variant) → { status: 422, message: "#4 is labeled Status/Blocked" }; in both, h.sessions is empty and h.api.labelPuts is empty.
Happy path (extend the existing "creates a mapped implementer session…" test): after whenIdle, h.api.labelPuts equals [{ number: 4, ids: [2] }] and issue 4's labels are [{ id: 2, name: "Agent/In Progress" }]; an extra label on the issue (e.g. { id: 9, name: "bug" }) is preserved in the put ids.
Swap failure (putStatus = 500): startImplementer still resolves with sess-1, the turn still runs (handle.prompts length 1), and h.api.posted contains one comment on #4 starting with omp-deck could not move this issue to Agent/In Progress:.
Agent/In Progress missing from availableLabels: same as above, comment contains unknown label.
Failed checkout (existing test) additionally asserts h.api.labelPuts is empty.

Run from /workspace/omp-deck: bun test apps/server/src/forgejo and bun run typecheck.

Manual end-to-end (needs a configured Forgejo + registered workspace): in the web UI open "Implement issue" for an issue without Agent/Ready → modal shows #N is not labeled Agent/Ready and no session appears; add Agent/Ready, retry → session opens and the issue on Forgejo shows Agent/In Progress instead of Agent/Ready.

## Problem The `Agent/Ready` / `Status/Blocked` label check and the `Agent/Ready` → `Agent/In Progress` transition currently exist only as instructions to the agent (`.omp/commands/implement.md` steps 3–4), which agents don't follow reliably. The server-started implementer (the "Implement issue" button in the web UI → `POST /api/forgejo/implement` → `ForgejoDispatcher.startImplementer`) does no label handling at all, and its agent has no label tool. When a session is started via the "Implement issue" button, the server itself must perform the checks and the label change: - Refuse to start (HTTP 422, no session) unless the issue has `Agent/Ready` and does not have `Status/Blocked`. Label names match case-insensitively after trimming. - After the session is created successfully, swap `Agent/Ready` → `Agent/In Progress`. If the swap fails, the session still runs; the failure is logged, posted as a comment on the issue, and — if that comment also fails — raised as a deck notification. The manual `/implement` chat command keeps its own instructions unchanged. The implementer's system context is not changed. ## Plan ### Approach Steps 1–2 keep the tree building and existing tests passing; step 3 adds the behavior. **1. Make label fetching usable outside tools** (`apps/server/src/forgejo/tools/labels.ts`) - Change `fetchAvailableLabels(ctx: ForgejoToolContext, signal?)` to `fetchAvailableLabels(ctx: Pick<ForgejoToolContext, "client" | "owner" | "repo">, signal?)`. The body only uses those three fields, so existing callers in `tools/issues.ts` and `tools/prs.ts` stay valid unchanged. **2. Extend `DispatchApi`** (`apps/server/src/forgejo/dispatch-api.ts`) - `IssueSummary` gains `labels: LabelRef[]` (`LabelRef` = `{ id: number; name: string }`, import the type from `./tools/labels.ts`). Update its doc comment to "Title, state and labels of issue/PR `number`…". - `RawIssue` gains `labels?: { id: number; name: string }[] | null`; `readIssue` maps it to `labels: (raw.labels ?? []).map((l) => ({ id: l.id, name: l.name }))`. - Add two methods to the interface and to `createDispatchApi`: - `listLabels(owner: string, repo: string, signal?: AbortSignal): Promise<ApiResult<LabelRef[]>>` — doc: "Labels usable on issues of the repo: its own plus its org's." Implementation: `fetchAvailableLabels({ client, owner, repo }, signal)`. - `replaceIssueLabels(owner: string, repo: string, number: number, ids: number[], signal?: AbortSignal): Promise<ApiResult<unknown>>` — doc: "Replace all labels of issue `number` with `ids` in one request." Implementation: `client.json<unknown>("PUT", repoApiPath(owner, repo, "issues", String(number), "labels"), { body: { labels: ids }, signal })` (same endpoint/body the `forgejo_update_issue_labels` tool uses in `tools/issues.ts`). - In `apps/server/src/forgejo/dispatcher.test.ts`, update `FakeApi` so it compiles: add `availableLabels: LabelRef[]` (default: `[{ id: 1, name: "Agent/Ready" }, { id: 2, name: "Agent/In Progress" }, { id: 3, name: "Status/Blocked" }]`), `listLabelsResult: ApiResult<LabelRef[]> | null = null` (when set, `listLabels` returns it instead), `labelPuts: Array<{ number: number; ids: number[] }>`, and `putStatus: number | null = null` (when set, `replaceIssueLabels` fails with that status). `replaceIssueLabels` records the put and, on success, rewrites `issues.get(number).labels` to the matching `availableLabels` entries. Change the `OPEN` constant in the `startImplementer` describe block to include `labels: [{ id: 1, name: "Agent/Ready" }]`. **3. Gate and transition in `ForgejoDispatcher.startImplementer`** (`apps/server/src/forgejo/dispatcher.ts`) - Add module constants: `const READY_LABEL = "Agent/Ready";`, `const IN_PROGRESS_LABEL = "Agent/In Progress";`, `const BLOCKED_LABEL = "Status/Blocked";` and a private helper `hasLabel(labels: readonly LabelRef[], name: string): boolean` comparing `l.name.trim().toLowerCase() === name.toLowerCase()`. - **Precheck**: directly after the existing `isPull` and `state !== "open"` checks (before the second `this.running.has(key)` check and `this.running.add(key)`): - not `hasLabel(issue.data.labels, READY_LABEL)` → `throw new ImplementerStartError(422, `#${number} is not labeled ${READY_LABEL}`)`. - `hasLabel(issue.data.labels, BLOCKED_LABEL)` → `throw new ImplementerStartError(422, `#${number} is labeled ${BLOCKED_LABEL}`)`. - Ready check first, so an issue missing Ready and having Blocked reports the Ready message. - **Transition**: add `private async markInProgress(api: DispatchApi, issue: { owner: string; repo: string; number: number }, sessionId: string): Promise<void>` that never throws. In `startImplementer`, call `await this.markInProgress(api, { owner, repo, number }, handle.sessionId)` after the existing `setName` try/catch and before `this.track(this.runImplementerTurn(...))`. It runs only when session creation succeeded (failed checkout/createSession paths throw earlier and touch no labels). Steps: 1. Re-read the issue with `api.readIssue` (fresh labels, so the replace-all PUT does not drop labels added since the precheck) and fetch `api.listLabels(owner, repo)`. Either failing → reason `` `${status} ${message}` ``. 2. `planLabelDelta({ current: issue.labels, available, add: [IN_PROGRESS_LABEL], remove: [READY_LABEL] })` (reuse from `./tools/labels.ts`). `!ok` → reason = `plan.error` (e.g. unknown `Agent/In Progress`). `noop` → return. 3. `api.replaceIssueLabels(owner, repo, number, plan.nextIds)`; failure → reason `` `${status} ${message}` ``. 4. On any reason: `log.warn(`could not mark ${repoFull}#${number} in progress: ${reason}`)`; post `api.postComment(owner, repo, number, `omp-deck could not move this issue to ${IN_PROGRESS_LABEL}: ${reason}\n\nThe implementer session \`${sessionId}\` is running regardless.`)`; if the post fails, `log.warn` it and `await notificationService.notify({ level: "error", title: `Could not mark ${repoFull}#${number} in progress`, body: `${reason}\n\nDeck session: \`${sessionId}\``, source: INBOX_SOURCE })` — same pattern as the failure branch of `runImplementerTurn`. Wrap the whole method body in try/catch so an unexpected throw is logged and treated as a reason too; it must not reach the caller (the `running` key is already claimed and the session exists). ### Critical files & anchors - `apps/server/src/forgejo/dispatcher.ts` — `startImplementer` (~L826–909): precheck after the `#N is closed` check; `markInProgress` call between `setName` and `this.track(...)`. `runImplementerTurn` (~L941–955) is the comment→notification fallback pattern to copy. - `apps/server/src/forgejo/dispatch-api.ts` — `IssueSummary`, `RawIssue`, `createDispatchApi`. - `apps/server/src/forgejo/tools/labels.ts` — `planLabelDelta`, `fetchAvailableLabels`, `LabelRef`. - `apps/server/src/forgejo/dispatcher.test.ts` — `FakeApi` (~L74) and `describe("ForgejoDispatcher.startImplementer")` (~L865). ### Verification Add these tests to `describe("ForgejoDispatcher.startImplementer")` in `apps/server/src/forgejo/dispatcher.test.ts`: - Issue without `Agent/Ready` → `refusal(...)` equals `{ status: 422, message: "#4 is not labeled Agent/Ready" }`; issue with `Agent/Ready` + `status/blocked ` (case/whitespace variant) → `{ status: 422, message: "#4 is labeled Status/Blocked" }`; in both, `h.sessions` is empty and `h.api.labelPuts` is empty. - Happy path (extend the existing "creates a mapped implementer session…" test): after `whenIdle`, `h.api.labelPuts` equals `[{ number: 4, ids: [2] }]` and issue 4's labels are `[{ id: 2, name: "Agent/In Progress" }]`; an extra label on the issue (e.g. `{ id: 9, name: "bug" }`) is preserved in the put ids. - Swap failure (`putStatus = 500`): `startImplementer` still resolves with `sess-1`, the turn still runs (`handle.prompts` length 1), and `h.api.posted` contains one comment on #4 starting with `omp-deck could not move this issue to Agent/In Progress:`. - `Agent/In Progress` missing from `availableLabels`: same as above, comment contains `unknown label`. - Failed checkout (existing test) additionally asserts `h.api.labelPuts` is empty. Run from `/workspace/omp-deck`: `bun test apps/server/src/forgejo` and `bun run typecheck`. Manual end-to-end (needs a configured Forgejo + registered workspace): in the web UI open "Implement issue" for an issue without `Agent/Ready` → modal shows `#N is not labeled Agent/Ready` and no session appears; add `Agent/Ready`, retry → session opens and the issue on Forgejo shows `Agent/In Progress` instead of `Agent/Ready`.