Forgejo: implementer started by a label (plan step 21) #130

New issue

Closed

opened 2026-10-28 12:08:26 +01:00 by omp-deck · 0 comments

omp-deck commented

2026-10-28 12:08:26 +01:00

Collaborator

Problem

Adding the configured intake label to an issue should start an implementer session: the same session the deck's "Implement issue" button starts. Today startImplementer (apps/server/src/forgejo/dispatcher.ts) is the only way in. It requires the hard-coded Agent/Ready, refuses Status/Blocked, refuses (409) when the issue already has a usable session, creates the issue clone, swaps Agent/Ready → Agent/In Progress and runs the first turn. Step 18's label path (issue_label events, only labels the latest timeline change added, sender write access) only recognises issue_handler (labelTriggerFor in repo-config.ts).

Decisions from grilling:

Config: a new implementation section in .omp/forgejo.yaml with intake_label (enables the label path), progress_label (optional) and max_concurrent (default 1).
Label path: the intake label must have been added by the latest change, and the sender needs write access. On start, the intake label is removed and progress_label is added if set.
Manual path: it no longer requires any ready label. It reads the repo config, removes intake_label if present and adds progress_label if set. Without that config it changes no labels. It still refuses Status/Blocked.
Existing session (both paths): a usable forgejo_conversation session is archived and replaced by a new implementer session, and the mapping moves to it. Any other usable session (implementer, pull request) is a refusal.
Concurrency: the per-repo lock from design step 21 is dropped because each issue has its own clone. Instead, max_concurrent limits the first turns that the label path started in a repo. A label row over the limit stays pending and runs when a slot frees. The button is never limited, and its sessions do not count toward the limit.
Defaults on the label path: workspace via matchWorkspaces (the existing gate), base branch null (the remote's default), default model, seed buildImplementerSeed.
Refusals and failures go on the issue as a notice or failure comment.
Unchanged: .omp/commands/implement.md (the chat command). The token-budget gate is out of scope.

Plan

Context

All work is in apps/server/src/forgejo/ (repo-config.ts, dispatcher.ts, and their tests). The checks, archive/replace, checkout, session, label update and first turn are factored into shared private methods. Both startImplementer and a new label branch of the dispatcher call them, so there is exactly one implementation.

Approach

Config (repo-config.ts).
- Add implementation: { intakeLabel: string | null; progressLabel: string | null; maxConcurrent: number } to RepoConfig. DEFAULT_REPO_CONFIG.implementation = { intakeLabel: null, progressLabel: null, maxConcurrent: 1 }.
- Add parseImplementation(doc.implementation), following parseIssueHandler's style:
  - absent/null → the defaults; a non-mapping → error "`implementation` must be a mapping".
  - intake_label and progress_label: optional; a present value must be a non-empty string (trimmed). Errors: "`implementation.intake_label` must be a non-empty string" and the same wording for progress_label.
  - max_concurrent: optional; a present value must be an integer ≥ 1. Error: "`implementation.max_concurrent` must be a positive integer".
- Run these cross-checks in parseRepoConfig after parsing:
  - intake label equals issueHandler.label → error "`implementation.intake_label` must differ from `issue_handler.label`".
  - progress label equals intake label → error "`implementation.progress_label` must differ from `implementation.intake_label`".
  - Both compare exactly.
- Change LabelTrigger to "issue_handler" | "implementation". labelTriggerFor returns "implementation" when config.implementation.intakeLabel === name (issue_handler is checked first; they cannot collide).
- Update the file's header doc comment to describe the implementation section.
Label resolution (dispatcher.ts).
- resolveLabelTrigger returns { label: string; trigger: LabelTrigger; config: RepoConfig } | null. configured now includes config.implementation.intakeLabel when non-null.
- The find accepts any label where labelTriggerFor(config, l) !== null.
- In gate's labeled branch:
  - reason = resolved.trigger === "implementation" ? "implementer-intake" : "issue-labeled".
  - Add "implementer-intake" to ReadyReason and to the label doc comment on Ready.
  - Store resolved.config.implementation.maxConcurrent on Ready as a new field maxConcurrent: number | null (null for every other reason). Set it in the return at the end of gate.
- The existing sender/write-permission check and the workspace NOT_CONNECTED/AMBIGUOUS handling then apply unchanged.
Concurrency slots and deferral (dispatcher.ts).
- Add these fields:
  - private intakeRunning = new Map<string, number>(), keyed by row.repo.toLowerCase().
  - private deferred = new Map<string, StoredForgejoTrigger>(): row id → row.
- At the very end of gate, just before return {...}, when reason === "implementer-intake":
  - If (intakeRunning.get(k) ?? 0) >= maxConcurrent, return the new sentinel "deferred". The row stays pending and is not finished.
  - Otherwise increment the count synchronously. Check and increment run with no await in between, so two drains cannot both take the last slot.
- Change the return types of gate and gateSafely to Ready | "deferred" | null, and update gate's doc comment: "Every null return has finished the row; "deferred" leaves it pending."
- drain: on "deferred", keep the id in gated, set deferred.set(row.id, row) and return. That stops draining this artifact so later rows of the same issue stay behind it (FIFO).
- pump: skip a key when any deferred row has that artifactKey.
- Add private releaseIntakeSlot(repo: string):
  - Decrement the count and delete the entry at 0.
  - For each deferred row whose repo.toLowerCase() matches, delete it from deferred and from gated.
  - Then call this.pump().
- No persistence is needed: after a restart, pending rows re-gate.
Shared implementer core (dispatcher.ts), replacing the body of startImplementer.
- Delete READY_LABEL and IN_PROGRESS_LABEL. Keep BLOCKED_LABEL and hasLabel.
- private async checkImplementable(api, { owner, repo, number }): Promise<{ replace: string | null }>. It throws ImplementerStartError with these messages:
  - mapped session: getForgejoSession, then isUsableTriggerSession. If it is usable and getSessionTrigger(mapped)?.kind === "forgejo_conversation", then replace = mapped. If it is usable with any other kind → 409 `#${number} already has session ${mapped}; archive it to start a new implementer session` (the existing text).
  - Read the issue with the existing 404/502 handling and messages. Then pull request → 422 (existing message), closed → 422 (existing message), hasLabel(labels, BLOCKED_LABEL) → 422 `#${number} is labeled Status/Blocked`.
  - There is no ready-label check any more.
- private async readImplementationConfig(api, owner, repo): Promise<RepoConfig["implementation"]>. It reads REPO_CONFIG_PATH via api.readRepoFile and parses it with parseRepoConfig.
  - Read failure → ImplementerStartError(502, could not read ${REPO_CONFIG_PATH}: ${status} ${message}).
  - Parse failure → ImplementerStartError(422, ${REPO_CONFIG_PATH}: ${error}).
  - Only the manual path uses it; the label path already has the config from the gate.
- private async launchImplementer(opts: { api; workspace; owner; repo; number; baseBranch; model?; replace: string | null; labels: RepoConfig["implementation"] }): Promise<SessionHandle>. Moved from startImplementer:
  - If replace !== null, first await this.archiveAfterIdle(replace, "forgejo_conversation"). Its clone is the same issue-N path that createCheckout re-creates.
  - Checkout (same ImplementerStartError(422, "could not prepare a checkout: …")), then createSession with the forgejo_implementer trigger, removeCheckout on failure, setForgejoSession (overwrites the old mapping), and setName.
  - Finally markInProgress(api, issue, sessionId, labels).
- markInProgress / swapInProgressLabels. Take labels and compute add = progressLabel ? [progressLabel] : [] and remove = intakeLabel ? [intakeLabel] : []. If both are empty, return without any API call.
  - Change the failure comment to `omp-deck could not update this issue's labels: ${reason}\n\nThe implementer session \`${sessionId}\` is running regardless.` and the notification title to `Could not update labels of ${repoFull}#${number}`.
  - Update both doc comments.
- runImplementerTurn. Now returns Promise<TurnOutcome> and still posts its failure comment or notification as today.
- startImplementer. Keeps its signature and existing behavior:
  - the running/pending 409 checks;
  - readImplementationConfig, then checkImplementable;
  - the re-check of running after the awaits, then claiming the key;
  - launchImplementer, releasing the key on error;
  - this.track(this.runImplementerTurn(...).then(() => undefined).finally(release)).
- Update its doc comment: no ready label, a conversation session is replaced, labels come from .omp/forgejo.yaml.
Label path run (dispatcher.ts). In drain, route ready.reason === "implementer-intake" to a new private async runIntake(ready: Ready) instead of runReady:
- The whole body is in try { … } catch (err) { await this.failInternal(row, err) } finally { this.gated.delete(row.id); this.releaseIntakeSlot(row.repo) }.
- if (!claimForgejoTrigger(row.id)) return.
- Run checkImplementable and then launchImplementer. Use workspace = ready.workspace, baseBranch = null and no model, and labels from the config the gate read; carry it on Ready as implementation: RepoConfig["implementation"] | null next to maxConcurrent, or replace maxConcurrent with it.
- Catch ImplementerStartError: await this.postNotice(job, omp-deck can't start an implementer for #${row.number}: ${err.message}), then finishForgejoTrigger(row.id, "dropped", err.message).
- On success: setForgejoTriggerSession(row.id, handle.sessionId), then const outcome = await this.runImplementerTurn(...), awaited here so the artifact key stays held during the first turn. Then finishForgejoTrigger(row.id, outcome === "ok" ? "done" : "failed", outcome === "ok" ? "implementer-intake" : outcome.error). Do not call this.fail, because runImplementerTurn already commented.
- Make Ready.maxConcurrent/implementation the single field implementation so there is one source.

Critical files & anchors

apps/server/src/forgejo/dispatcher.ts: gate (~430–621), drain/pump (~381–411), resolveLabelTrigger (~661), startImplementer → swapInProgressLabels → runImplementerTurn (~1720–1924), archiveAfterIdle (~1644).
apps/server/src/forgejo/repo-config.ts: RepoConfig, parseRepoConfig, labelTriggerFor.
apps/server/src/forgejo/dispatcher.test.ts: describe("ForgejoDispatcher.startImplementer") (~1112). Its harness fakes readRepoFile, labels and sessions; the issue-labeled test is at ~554.

Verification

Run bun test apps/server/src/forgejo from /workspace/omp-deck.

Update existing tests. Delete the Agent/Ready refusal assertions and change "…to Agent/In Progress:" to "omp-deck could not update this issue's labels:". These tests then configure implementation through the harness's repo file.

Add these tests.

In repo-config.test.ts:

implementation parsing: the defaults, each error string, and both cross-checks.
labelTriggerFor returns "implementation" for the intake label.

In dispatcher.test.ts, with the config implementation: {intake_label: "Agent/Ready", progress_label: "Agent/In Progress"}:

An issue_label row adding Agent/Ready by a write-access sender produces:
- one session with trigger forgejo_implementer and baseBranch: null;
- a prompt starting with "Implement Forgejo issue lucas/omp-deck#4";
- labels swapped to [Agent/In Progress];
- the row done with reason "implementer-intake".
A sender without write access → row dropped and no session.
Status/Blocked present → a notice comment containing "can't start an implementer", row dropped, labels untouched.
A usable forgejo_conversation session mapped to #4 (manual and label path) → that session is archived, a new implementer session is mapped, and there is no 409.
A usable forgejo_implementer session → 409 / notice.
Manual path with no config file → the session starts and there is no label PUT. Manual path on an issue without any ready label → the session starts.
max_concurrent (default 1): two label rows on issues #4 and #5 in manual turn mode:
- #5 stays pending with no session while #4's first turn runs;
- after #4's turn completes, #5 runs.
- A button start while #4 runs still succeeds immediately.

Assumptions & contingencies

Should the harness's archiveSession dep not be injectable in the startImplementer setup, inject it the way other lifecycle tests in dispatcher.test.ts do (deps.archiveSession) rather than calling the real bridge.

## Problem Adding the configured intake label to an issue should start an implementer session: the same session the deck's "Implement issue" button starts. Today `startImplementer` (`apps/server/src/forgejo/dispatcher.ts`) is the only way in. It requires the hard-coded `Agent/Ready`, refuses `Status/Blocked`, refuses (409) when the issue already has a usable session, creates the issue clone, swaps `Agent/Ready` → `Agent/In Progress` and runs the first turn. Step 18's label path (`issue_label` events, only labels the latest timeline change added, sender write access) only recognises `issue_handler` (`labelTriggerFor` in `repo-config.ts`). Decisions from grilling: - **Config:** a new `implementation` section in `.omp/forgejo.yaml` with `intake_label` (enables the label path), `progress_label` (optional) and `max_concurrent` (default 1). - **Label path:** the intake label must have been added by the latest change, and the sender needs write access. On start, the intake label is removed and `progress_label` is added if set. - **Manual path:** it no longer requires any ready label. It reads the repo config, removes `intake_label` if present and adds `progress_label` if set. Without that config it changes no labels. It still refuses `Status/Blocked`. - **Existing session (both paths):** a usable `forgejo_conversation` session is archived and replaced by a new implementer session, and the mapping moves to it. Any other usable session (implementer, pull request) is a refusal. - **Concurrency:** the per-repo lock from design step 21 is dropped because each issue has its own clone. Instead, `max_concurrent` limits the first turns that the label path started in a repo. A label row over the limit stays pending and runs when a slot frees. The button is never limited, and its sessions do not count toward the limit. - **Defaults on the label path:** workspace via `matchWorkspaces` (the existing gate), base branch `null` (the remote's default), default model, seed `buildImplementerSeed`. - **Refusals and failures** go on the issue as a notice or failure comment. - **Unchanged:** `.omp/commands/implement.md` (the chat command). The token-budget gate is out of scope. ## Plan ### Context All work is in `apps/server/src/forgejo/` (`repo-config.ts`, `dispatcher.ts`, and their tests). The checks, archive/replace, checkout, session, label update and first turn are factored into shared private methods. Both `startImplementer` and a new label branch of the dispatcher call them, so there is exactly one implementation. ### Approach 1. **Config (`repo-config.ts`).** - Add `implementation: { intakeLabel: string | null; progressLabel: string | null; maxConcurrent: number }` to `RepoConfig`. `DEFAULT_REPO_CONFIG.implementation = { intakeLabel: null, progressLabel: null, maxConcurrent: 1 }`. - Add `parseImplementation(doc.implementation)`, following `parseIssueHandler`'s style: - absent/null → the defaults; a non-mapping → error ``"`implementation` must be a mapping"``. - `intake_label` and `progress_label`: optional; a present value must be a non-empty string (trimmed). Errors: ``"`implementation.intake_label` must be a non-empty string"`` and the same wording for `progress_label`. - `max_concurrent`: optional; a present value must be an integer ≥ 1. Error: ``"`implementation.max_concurrent` must be a positive integer"``. - Run these cross-checks in `parseRepoConfig` after parsing: - intake label equals `issueHandler.label` → error ``"`implementation.intake_label` must differ from `issue_handler.label`"``. - progress label equals intake label → error ``"`implementation.progress_label` must differ from `implementation.intake_label`"``. - Both compare exactly. - Change `LabelTrigger` to `"issue_handler" | "implementation"`. `labelTriggerFor` returns `"implementation"` when `config.implementation.intakeLabel === name` (issue_handler is checked first; they cannot collide). - Update the file's header doc comment to describe the `implementation` section. 2. **Label resolution (`dispatcher.ts`).** - `resolveLabelTrigger` returns `{ label: string; trigger: LabelTrigger; config: RepoConfig } | null`. `configured` now includes `config.implementation.intakeLabel` when non-null. - The `find` accepts any label where `labelTriggerFor(config, l) !== null`. - In `gate`'s `labeled` branch: - `reason = resolved.trigger === "implementation" ? "implementer-intake" : "issue-labeled"`. - Add `"implementer-intake"` to `ReadyReason` and to the `label` doc comment on `Ready`. - Store `resolved.config.implementation.maxConcurrent` on `Ready` as a new field `maxConcurrent: number | null` (null for every other reason). Set it in the `return` at the end of `gate`. - The existing sender/write-permission check and the workspace NOT_CONNECTED/AMBIGUOUS handling then apply unchanged. 3. **Concurrency slots and deferral (`dispatcher.ts`).** - Add these fields: - `private intakeRunning = new Map<string, number>()`, keyed by `row.repo.toLowerCase()`. - `private deferred = new Map<string, StoredForgejoTrigger>()`: row id → row. - At the very end of `gate`, just before `return {...}`, when `reason === "implementer-intake"`: - If `(intakeRunning.get(k) ?? 0) >= maxConcurrent`, return the new sentinel `"deferred"`. The row stays `pending` and is not finished. - Otherwise increment the count synchronously. Check and increment run with no `await` in between, so two drains cannot both take the last slot. - Change the return types of `gate` and `gateSafely` to `Ready | "deferred" | null`, and update `gate`'s doc comment: "Every null return has finished the row; "deferred" leaves it pending." - `drain`: on `"deferred"`, keep the id in `gated`, set `deferred.set(row.id, row)` and `return`. That stops draining this artifact so later rows of the same issue stay behind it (FIFO). - `pump`: skip a key when any `deferred` row has that `artifactKey`. - Add `private releaseIntakeSlot(repo: string)`: - Decrement the count and delete the entry at 0. - For each deferred row whose `repo.toLowerCase()` matches, delete it from `deferred` and from `gated`. - Then call `this.pump()`. - No persistence is needed: after a restart, pending rows re-gate. 4. **Shared implementer core (`dispatcher.ts`), replacing the body of `startImplementer`.** - Delete `READY_LABEL` and `IN_PROGRESS_LABEL`. Keep `BLOCKED_LABEL` and `hasLabel`. - **`private async checkImplementable(api, { owner, repo, number }): Promise<{ replace: string | null }>`.** It throws `ImplementerStartError` with these messages: - mapped session: `getForgejoSession`, then `isUsableTriggerSession`. If it is usable and `getSessionTrigger(mapped)?.kind === "forgejo_conversation"`, then `replace = mapped`. If it is usable with any other kind → 409 `` `#${number} already has session ${mapped}; archive it to start a new implementer session` `` (the existing text). - Read the issue with the existing 404/502 handling and messages. Then pull request → 422 (existing message), closed → 422 (existing message), `hasLabel(labels, BLOCKED_LABEL)` → 422 `` `#${number} is labeled Status/Blocked` ``. - There is no ready-label check any more. - **`private async readImplementationConfig(api, owner, repo): Promise<RepoConfig["implementation"]>`.** It reads `REPO_CONFIG_PATH` via `api.readRepoFile` and parses it with `parseRepoConfig`. - Read failure → `ImplementerStartError(502, `could not read ${REPO_CONFIG_PATH}: ${status} ${message}`)`. - Parse failure → `ImplementerStartError(422, `${REPO_CONFIG_PATH}: ${error}`)`. - Only the manual path uses it; the label path already has the config from the gate. - **`private async launchImplementer(opts: { api; workspace; owner; repo; number; baseBranch; model?; replace: string | null; labels: RepoConfig["implementation"] }): Promise<SessionHandle>`.** Moved from `startImplementer`: - If `replace !== null`, first `await this.archiveAfterIdle(replace, "forgejo_conversation")`. Its clone is the same `issue-N` path that `createCheckout` re-creates. - Checkout (same `ImplementerStartError(422, "could not prepare a checkout: …")`), then `createSession` with the `forgejo_implementer` trigger, `removeCheckout` on failure, `setForgejoSession` (overwrites the old mapping), and `setName`. - Finally `markInProgress(api, issue, sessionId, labels)`. - **`markInProgress` / `swapInProgressLabels`.** Take `labels` and compute `add = progressLabel ? [progressLabel] : []` and `remove = intakeLabel ? [intakeLabel] : []`. If both are empty, return without any API call. - Change the failure comment to `` `omp-deck could not update this issue's labels: ${reason}\n\nThe implementer session \`${sessionId}\` is running regardless.` `` and the notification title to `` `Could not update labels of ${repoFull}#${number}` ``. - Update both doc comments. - **`runImplementerTurn`.** Now returns `Promise<TurnOutcome>` and still posts its failure comment or notification as today. - **`startImplementer`.** Keeps its signature and existing behavior: - the `running`/pending 409 checks; - `readImplementationConfig`, then `checkImplementable`; - the re-check of `running` after the awaits, then claiming the key; - `launchImplementer`, releasing the key on error; - `this.track(this.runImplementerTurn(...).then(() => undefined).finally(release))`. - Update its doc comment: no ready label, a conversation session is replaced, labels come from `.omp/forgejo.yaml`. 5. **Label path run (`dispatcher.ts`).** In `drain`, route `ready.reason === "implementer-intake"` to a new `private async runIntake(ready: Ready)` instead of `runReady`: - The whole body is in `try { … } catch (err) { await this.failInternal(row, err) } finally { this.gated.delete(row.id); this.releaseIntakeSlot(row.repo) }`. - `if (!claimForgejoTrigger(row.id)) return`. - Run `checkImplementable` and then `launchImplementer`. Use `workspace = ready.workspace`, `baseBranch = null` and no `model`, and `labels` from the config the gate read; carry it on `Ready` as `implementation: RepoConfig["implementation"] | null` next to `maxConcurrent`, or replace `maxConcurrent` with it. - Catch `ImplementerStartError`: `await this.postNotice(job, `omp-deck can't start an implementer for #${row.number}: ${err.message}`)`, then `finishForgejoTrigger(row.id, "dropped", err.message)`. - On success: `setForgejoTriggerSession(row.id, handle.sessionId)`, then `const outcome = await this.runImplementerTurn(...)`, awaited here so the artifact key stays held during the first turn. Then `finishForgejoTrigger(row.id, outcome === "ok" ? "done" : "failed", outcome === "ok" ? "implementer-intake" : outcome.error)`. Do not call `this.fail`, because `runImplementerTurn` already commented. - Make `Ready.maxConcurrent`/`implementation` the single field `implementation` so there is one source. ### Critical files & anchors - `apps/server/src/forgejo/dispatcher.ts`: `gate` (~430–621), `drain`/`pump` (~381–411), `resolveLabelTrigger` (~661), `startImplementer` → `swapInProgressLabels` → `runImplementerTurn` (~1720–1924), `archiveAfterIdle` (~1644). - `apps/server/src/forgejo/repo-config.ts`: `RepoConfig`, `parseRepoConfig`, `labelTriggerFor`. - `apps/server/src/forgejo/dispatcher.test.ts`: `describe("ForgejoDispatcher.startImplementer")` (~1112). Its harness fakes `readRepoFile`, labels and sessions; the issue-labeled test is at ~554. ### Verification Run `bun test apps/server/src/forgejo` from `/workspace/omp-deck`. **Update existing tests.** Delete the `Agent/Ready` refusal assertions and change `"…to Agent/In Progress:"` to `"omp-deck could not update this issue's labels:"`. These tests then configure `implementation` through the harness's repo file. **Add these tests.** In `repo-config.test.ts`: - `implementation` parsing: the defaults, each error string, and both cross-checks. - `labelTriggerFor` returns `"implementation"` for the intake label. In `dispatcher.test.ts`, with the config `implementation: {intake_label: "Agent/Ready", progress_label: "Agent/In Progress"}`: - An `issue_label` row adding `Agent/Ready` by a write-access sender produces: - one session with trigger `forgejo_implementer` and `baseBranch: null`; - a prompt starting with `"Implement Forgejo issue lucas/omp-deck#4"`; - labels swapped to `[Agent/In Progress]`; - the row `done` with reason `"implementer-intake"`. - A sender without write access → row `dropped` and no session. - `Status/Blocked` present → a notice comment containing `"can't start an implementer"`, row `dropped`, labels untouched. - A usable `forgejo_conversation` session mapped to #4 (manual and label path) → that session is archived, a new implementer session is mapped, and there is no 409. - A usable `forgejo_implementer` session → 409 / notice. - Manual path with no config file → the session starts and there is no label PUT. Manual path on an issue without any ready label → the session starts. - `max_concurrent` (default 1): two label rows on issues #4 and #5 in `manual` turn mode: - #5 stays `pending` with no session while #4's first turn runs; - after #4's turn completes, #5 runs. - A button start while #4 runs still succeeds immediately. ### Assumptions & contingencies - Should the harness's `archiveSession` dep not be injectable in the startImplementer `setup`, inject it the way other lifecycle tests in `dispatcher.test.ts` do (`deps.archiveSession`) rather than calling the real bridge.