Mark planned issues with a configurable `planned_label` #248

New issue

Closed

opened 2026-11-10 21:15:11 +01:00 by omp-deck · 0 comments

omp-deck commented

2026-11-10 21:15:11 +01:00

Collaborator

Problem

.omp/forgejo.yaml already names the labels that steer the issue lifecycle under implementation: intake_label (adding it starts an implementer), progress_label (added on start, removed on abandon) and planning_label (added on abandon). Nothing marks the planning stage itself: an issue being planned by a plan-issue session looks identical to one nobody has touched, and an issue whose plan is finished looks identical to one still being written.

Add an optional implementation.planned_label. A plan-issue session started on an issue marks it with planning_label; when the approved plan is written to the issue, planned_label replaces planning_label. Both labels are opt-in: with neither configured, behavior is exactly as today. An abandoned implementation, which already returns the issue to planning_label, additionally drops the now-stale planned_label.

Plan

1. Shared label application helper

ForgejoDispatcher.swapLabels (apps/server/src/forgejo/dispatcher.ts:2792) is the only code that applies a label delta through the API, and it is private; plan-issue sessions need the same behavior. Move it out, widening both sides to lists (the abandon path must now remove two labels).

Create apps/server/src/forgejo/issue-labels.ts:

/**
 * Applying a label delta to an issue through the API: the counterpart of
 * `planLabelDelta`, which only computes one. Shared by the dispatcher and
 * plan-issue sessions.
 */

import type { DispatchApi } from "./dispatch-api.ts";
import { labelKey } from "./repo-config.ts";
import { type LabelRef, planLabelDelta } from "./tools/labels.ts";

export function canonicalLabelName(name: string, pool: readonly LabelRef[]): string;

/**
 * Adds and removes the named labels in one atomic replace; null and duplicate
 * names are ignored. Returns the failure reason, or "" on success and when the
 * delta is empty or a no-op.
 */
export async function applyLabelDelta(
	api: DispatchApi,
	{ owner, repo, number }: { owner: string; repo: string; number: number },
	labels: { add?: readonly (string | null)[]; remove?: readonly (string | null)[] },
): Promise<string>;

Move canonicalLabelName (dispatcher.ts:223, together with its doc comment at :218-222) into the new file unchanged and export it; delete it from dispatcher.ts. Its only uses are inside the moved swapLabels body.
applyLabelDelta's body is swapLabels's body (dispatcher.ts:2797-2813) with the two single-name arrays replaced by (labels.add ?? []).filter((n): n is string => typeof n === "string" && n.trim() !== "") and the same for remove; keep the comment explaining the re-read before the replace-all PUT. planLabelDelta already deduplicates names case-insensitively and treats adding a present label or removing an absent one as a no-op, so no extra guarding is needed.
Delete swapLabels from dispatcher.ts and rewire its two callsites:
- dispatcher.ts:2764 (markInProgress) → applyLabelDelta(api, issue, { add: [labels.progressLabel], remove: [labels.intakeLabel] }).
- dispatcher.ts:2414 (abandon) → applyLabelDelta(api, { owner, repo, number }, { add: [planningLabel], remove: [progressLabel, plannedLabel] }), destructuring plannedLabel alongside the other two at dispatcher.ts:2412. Keep the surrounding try/catch and the problems.push("could not update the labels: …") reporting exactly as it is.

2. Config field

In apps/server/src/forgejo/repo-config.ts:

Add plannedLabel: string | null to RepoConfig["implementation"] (after planningLabel) and plannedLabel: null to DEFAULT_REPO_CONFIG.implementation.
In parseImplementation, add planned_label to the labels object and to the ["intake_label", "progress_label", "planning_label"] loop key list, and map it to plannedLabel in the returned value. The existing loop already produces the right error (`implementation.planned_label` must be a non-empty string) and trims.
In parseRepoConfig, after the existing planning_label check (:158-160), add:

	if (impl.plannedLabel !== null && impl.planningLabel !== null && sameLabel(impl.plannedLabel, impl.planningLabel)) {
		return { ok: false, error: "`implementation.planned_label` must differ from `implementation.planning_label`" };
	}

Only this pair is checked: planned_label equal to intake_label is legal and meaningful ("plan done" and "ready to implement" can be one label), mirroring the existing allowance of intake_label == planning_label (repo-config.test.ts:190).

Update the module doc comment (repo-config.ts:16-22): state that a plan-issue session started on an issue adds implementation.planning_label to it, and that writing the approved plan to the issue adds implementation.planned_label and removes planning_label; extend the abandon sentence to say the progress label and the planned label are removed.

3. Labels on plan-issue start and publish

In apps/server/src/forgejo/plan-issue.ts, add two module-private helpers (the file currently never reads the repo config):

/** The `implementation` labels from `.omp/forgejo.yaml`, or null when it cannot be read or parsed (logged). */
async function implementationLabels(
	api: DispatchApi,
	owner: string,
	repo: string,
): Promise<RepoConfig["implementation"] | null>;

/** Apply a lifecycle label change; failures are logged and otherwise ignored, a label is never worth failing over. */
async function markIssue(
	api: DispatchApi,
	ref: { owner: string; repo: string; number: number },
	labels: { add?: readonly (string | null)[]; remove?: readonly (string | null)[] },
): Promise<void>;

implementationLabels reads REPO_CONFIG_PATH via api.readRepoFile, parses with parseRepoConfig, and returns parsed.config.implementation; on a failed read or parse it logs log.warn and returns null. Copy the structure of loadRepoSystemPrompt (apps/server/src/forgejo/repo-system-prompt.ts:30-54), including its try/catch around the whole body — a broken config must never break planning. Do not call createDispatchApi() here: both callers already hold an api.
markIssue returns immediately when every name in add/remove is null, calls applyLabelDelta, and logs log.warn(\could not update labels of ${owner}/${repo}#${number}: ${reason}`)when it returns a non-empty reason or throws. Nothing is posted on the issue and no notification is raised: unlike the dispatcher'smarkInProgress`, these sites run with a human attached to the session.

In startPlanIssue:

Hoist the api used by the issue branch so it is visible after session creation: declare let api: DispatchApi | null = null; before the if (seed.kind === "issue") branch and assign it there instead of the current local const api.
After await handle.setPlanMode(true) and before the kickoff void handle.prompt(...), add:

	if (api !== null && binding.issueNumber !== null) {
		const labels = await implementationLabels(api, owner, repo);
		if (labels) await markIssue(api, { owner, repo, number: binding.issueNumber }, { add: [labels.planningLabel] });
	}

Placing it after session creation means a start that is refused (PlanIssueStartError) never labels the issue. Inbox and free-text seeds have no issue at this point and are skipped.

In publishPlanIssue, after the write succeeded and outcome is set, before the b.inboxId update:

	const labels = await implementationLabels(api, b.owner, b.repo);
	if (labels) {
		await markIssue(
			api,
			{ owner: b.owner, repo: b.repo, number: outcome.number },
			{ add: [labels.plannedLabel], remove: [labels.planningLabel] },
		);
	}

This covers both seed kinds: createIssue (dispatch-api.ts:51) accepts only {title, body}, so a newly created issue is labelled by the same call; removing planning_label from it is a no-op. The publish outcome is unaffected by a label failure — the plan is on the issue either way, and PublishOutcome / plan-mode-bridge.ts stay unchanged.

Update the publishPlanIssue doc comment (plan-issue.ts:58-62) and the module doc comment to mention the label changes.

4. Protect the new label from the agent

In apps/server/src/bridge/session-policy.ts, add implementation.plannedLabel to the protectedNames list (:289-292, next to implementation.planningLabel). It steers automation exactly like its siblings, so the agent's forgejo_update_issue_labels must not set or clear it by hand.

5. This repository's config

In .omp/forgejo.yaml, under implementation (after progress_label, before max_concurrent):

  planning_label: agent/planning
  planned_label: agent/planned

Whether labels named agent/planning and agent/planned exist in lucas/omp-deck on the Forgejo instance is unverified — confirm first; if they do not, every plan-issue start and publish logs a label warning until they are created, and nothing else breaks.

6. Tests

apps/server/src/forgejo/repo-config.test.ts: add plannedLabel: null to the defaults assertion (:150-153) and planned_label to the full-parse case (:158-168); add expect(error("implementation: {planning_label: x, planned_label: X}")).toBe("implementation.planned_labelmust differ fromimplementation.planning_label") and expect(parseRepoConfig("implementation: {intake_label: x, planned_label: x}").ok).toBe(true) to the validation test.
apps/server/src/forgejo/plan-issue.test.ts: extend fakeApi (:59) with readRepoFile (returning configurable YAML text), readIssue (returning the issue's current labels), listLabels (the repo's label pool) and replaceIssueLabels (recording the id list). Then assert, for a config with planning_label: agent/planning and planned_label: agent/planned:
- issue seed #7 currently carrying agent/planning → after publishPlanIssue the recorded PUT holds the id of agent/planned and not that of agent/planning;
- inbox seed → the created issue #42 gets the agent/planned id, and the inbox item is still marked processed;
- a failing replaceIssueLabels (or an unknown planned_label name) still returns {ok: true, …} and still marks the inbox item processed;
- no config file (readRepoFile → {ok:true,data:null}) → no replaceIssueLabels call at all.
apps/server/src/forgejo/plan-issue.test.ts: add a startPlanIssue case with a stub bridge — createSession returning { sessionId: "s1", setName: async () => {}, setPlanMode: async () => {}, prompt: async () => {} } cast as unknown as AgentBridge, matching the file's existing as unknown as DispatchApi style — asserting that an issue seed triggers a PUT adding agent/planning, and that an inbox seed triggers none.
apps/server/src/forgejo/dispatcher.test.ts: in the abandon suite (:3494), add planned_label: Agent/Planned to CONFIG and assert the replace-all PUT after an abandon drops both the progress and planned label ids while adding the planning one.

Verification

From the repository root:

bun test apps/server/src/forgejo/repo-config.test.ts apps/server/src/forgejo/plan-issue.test.ts apps/server/src/forgejo/dispatcher.test.ts apps/server/src/bridge/session-policy.test.ts
bun run typecheck

End-to-end check of the new behavior without a live Forgejo, as a throwaway script (delete it afterwards) under apps/server:

Build a fake DispatchApi whose readRepoFile returns "implementation:\n planning_label: agent/planning\n planned_label: agent/planned\n", whose listLabels returns [{id:1,name:"agent/planning"},{id:2,name:"agent/planned"}], whose readIssue returns issue #7 with labels [{id:1,name:"agent/planning"}], and which records replaceIssueLabels calls.
Call publishPlanIssue({workspaceId:"w",owner:"o",repo:"r",issueNumber:7,inboxId:null}, "# T\n\n## Problem\nx\n\n## Plan\ny\n", api).
Expected: the function returns {ok:true, url:…, number:7}, exactly one updateIssueBody call, and exactly one replaceIssueLabels call with [2] — agent/planned added, agent/planning gone.

Assumptions & contingencies

The labels agent/planning and agent/planned are expected to exist in lucas/omp-deck on the Forgejo instance (unverified). If they do not, the code is still correct — the label call fails and is logged — but to keep the repo's own config clean, create the labels in Forgejo rather than reverting step 5.
If dispatcher.test.ts's abandon suite turns out to assert label ids through a path that the list-valued remove changes beyond the planned_label id (for example an ordering assertion), keep applyLabelDelta's output ordering identical to today's swapLabels (planLabelDelta preserves the current order and appends additions) and adjust only the expected id set.

## Problem `.omp/forgejo.yaml` already names the labels that steer the issue lifecycle under `implementation`: `intake_label` (adding it starts an implementer), `progress_label` (added on start, removed on abandon) and `planning_label` (added on abandon). Nothing marks the planning stage itself: an issue being planned by a plan-issue session looks identical to one nobody has touched, and an issue whose plan is finished looks identical to one still being written. Add an optional `implementation.planned_label`. A plan-issue session started on an issue marks it with `planning_label`; when the approved plan is written to the issue, `planned_label` replaces `planning_label`. Both labels are opt-in: with neither configured, behavior is exactly as today. An abandoned implementation, which already returns the issue to `planning_label`, additionally drops the now-stale `planned_label`. ## Plan ### 1. Shared label application helper `ForgejoDispatcher.swapLabels` (`apps/server/src/forgejo/dispatcher.ts:2792`) is the only code that applies a label delta through the API, and it is private; plan-issue sessions need the same behavior. Move it out, widening both sides to lists (the abandon path must now remove two labels). Create `apps/server/src/forgejo/issue-labels.ts`: ```ts /** * Applying a label delta to an issue through the API: the counterpart of * `planLabelDelta`, which only computes one. Shared by the dispatcher and * plan-issue sessions. */ import type { DispatchApi } from "./dispatch-api.ts"; import { labelKey } from "./repo-config.ts"; import { type LabelRef, planLabelDelta } from "./tools/labels.ts"; export function canonicalLabelName(name: string, pool: readonly LabelRef[]): string; /** * Adds and removes the named labels in one atomic replace; null and duplicate * names are ignored. Returns the failure reason, or "" on success and when the * delta is empty or a no-op. */ export async function applyLabelDelta( api: DispatchApi, { owner, repo, number }: { owner: string; repo: string; number: number }, labels: { add?: readonly (string | null)[]; remove?: readonly (string | null)[] }, ): Promise<string>; ``` - Move `canonicalLabelName` (`dispatcher.ts:223`, together with its doc comment at `:218-222`) into the new file unchanged and export it; delete it from `dispatcher.ts`. Its only uses are inside the moved `swapLabels` body. - `applyLabelDelta`'s body is `swapLabels`'s body (`dispatcher.ts:2797-2813`) with the two single-name arrays replaced by `(labels.add ?? []).filter((n): n is string => typeof n === "string" && n.trim() !== "")` and the same for `remove`; keep the comment explaining the re-read before the replace-all PUT. `planLabelDelta` already deduplicates names case-insensitively and treats adding a present label or removing an absent one as a no-op, so no extra guarding is needed. - Delete `swapLabels` from `dispatcher.ts` and rewire its two callsites: - `dispatcher.ts:2764` (`markInProgress`) → `applyLabelDelta(api, issue, { add: [labels.progressLabel], remove: [labels.intakeLabel] })`. - `dispatcher.ts:2414` (abandon) → `applyLabelDelta(api, { owner, repo, number }, { add: [planningLabel], remove: [progressLabel, plannedLabel] })`, destructuring `plannedLabel` alongside the other two at `dispatcher.ts:2412`. Keep the surrounding `try`/`catch` and the `problems.push("could not update the labels: …")` reporting exactly as it is. ### 2. Config field In `apps/server/src/forgejo/repo-config.ts`: - Add `plannedLabel: string | null` to `RepoConfig["implementation"]` (after `planningLabel`) and `plannedLabel: null` to `DEFAULT_REPO_CONFIG.implementation`. - In `parseImplementation`, add `planned_label` to the `labels` object and to the `["intake_label", "progress_label", "planning_label"]` loop key list, and map it to `plannedLabel` in the returned value. The existing loop already produces the right error (`` `implementation.planned_label` must be a non-empty string ``) and trims. - In `parseRepoConfig`, after the existing `planning_label` check (`:158-160`), add: ```ts if (impl.plannedLabel !== null && impl.planningLabel !== null && sameLabel(impl.plannedLabel, impl.planningLabel)) { return { ok: false, error: "`implementation.planned_label` must differ from `implementation.planning_label`" }; } ``` Only this pair is checked: `planned_label` equal to `intake_label` is legal and meaningful ("plan done" and "ready to implement" can be one label), mirroring the existing allowance of `intake_label == planning_label` (`repo-config.test.ts:190`). - Update the module doc comment (`repo-config.ts:16-22`): state that a plan-issue session started on an issue adds `implementation.planning_label` to it, and that writing the approved plan to the issue adds `implementation.planned_label` and removes `planning_label`; extend the abandon sentence to say the progress label *and* the planned label are removed. ### 3. Labels on plan-issue start and publish In `apps/server/src/forgejo/plan-issue.ts`, add two module-private helpers (the file currently never reads the repo config): ```ts /** The `implementation` labels from `.omp/forgejo.yaml`, or null when it cannot be read or parsed (logged). */ async function implementationLabels( api: DispatchApi, owner: string, repo: string, ): Promise<RepoConfig["implementation"] | null>; /** Apply a lifecycle label change; failures are logged and otherwise ignored, a label is never worth failing over. */ async function markIssue( api: DispatchApi, ref: { owner: string; repo: string; number: number }, labels: { add?: readonly (string | null)[]; remove?: readonly (string | null)[] }, ): Promise<void>; ``` - `implementationLabels` reads `REPO_CONFIG_PATH` via `api.readRepoFile`, parses with `parseRepoConfig`, and returns `parsed.config.implementation`; on a failed read or parse it logs `log.warn` and returns null. Copy the structure of `loadRepoSystemPrompt` (`apps/server/src/forgejo/repo-system-prompt.ts:30-54`), including its `try`/`catch` around the whole body — a broken config must never break planning. Do not call `createDispatchApi()` here: both callers already hold an api. - `markIssue` returns immediately when every name in `add`/`remove` is null, calls `applyLabelDelta`, and logs `log.warn(\`could not update labels of ${owner}/${repo}#${number}: ${reason}\`)` when it returns a non-empty reason or throws. Nothing is posted on the issue and no notification is raised: unlike the dispatcher's `markInProgress`, these sites run with a human attached to the session. In `startPlanIssue`: - Hoist the api used by the issue branch so it is visible after session creation: declare `let api: DispatchApi | null = null;` before the `if (seed.kind === "issue")` branch and assign it there instead of the current local `const api`. - After `await handle.setPlanMode(true)` and before the kickoff `void handle.prompt(...)`, add: ```ts if (api !== null && binding.issueNumber !== null) { const labels = await implementationLabels(api, owner, repo); if (labels) await markIssue(api, { owner, repo, number: binding.issueNumber }, { add: [labels.planningLabel] }); } ``` Placing it after session creation means a start that is refused (`PlanIssueStartError`) never labels the issue. Inbox and free-text seeds have no issue at this point and are skipped. In `publishPlanIssue`, after the write succeeded and `outcome` is set, before the `b.inboxId` update: ```ts const labels = await implementationLabels(api, b.owner, b.repo); if (labels) { await markIssue( api, { owner: b.owner, repo: b.repo, number: outcome.number }, { add: [labels.plannedLabel], remove: [labels.planningLabel] }, ); } ``` This covers both seed kinds: `createIssue` (`dispatch-api.ts:51`) accepts only `{title, body}`, so a newly created issue is labelled by the same call; removing `planning_label` from it is a no-op. The publish outcome is unaffected by a label failure — the plan is on the issue either way, and `PublishOutcome` / `plan-mode-bridge.ts` stay unchanged. - Update the `publishPlanIssue` doc comment (`plan-issue.ts:58-62`) and the module doc comment to mention the label changes. ### 4. Protect the new label from the agent In `apps/server/src/bridge/session-policy.ts`, add `implementation.plannedLabel` to the `protectedNames` list (`:289-292`, next to `implementation.planningLabel`). It steers automation exactly like its siblings, so the agent's `forgejo_update_issue_labels` must not set or clear it by hand. ### 5. This repository's config In `.omp/forgejo.yaml`, under `implementation` (after `progress_label`, before `max_concurrent`): ```yaml planning_label: agent/planning planned_label: agent/planned ``` Whether labels named `agent/planning` and `agent/planned` exist in `lucas/omp-deck` on the Forgejo instance is *unverified — confirm first*; if they do not, every plan-issue start and publish logs a label warning until they are created, and nothing else breaks. ### 6. Tests - `apps/server/src/forgejo/repo-config.test.ts`: add `plannedLabel: null` to the defaults assertion (`:150-153`) and `planned_label` to the full-parse case (`:158-168`); add `expect(error("implementation: {planning_label: x, planned_label: X}")).toBe("`implementation.planned_label` must differ from `implementation.planning_label`")` and `expect(parseRepoConfig("implementation: {intake_label: x, planned_label: x}").ok).toBe(true)` to the validation test. - `apps/server/src/forgejo/plan-issue.test.ts`: extend `fakeApi` (`:59`) with `readRepoFile` (returning configurable YAML text), `readIssue` (returning the issue's current labels), `listLabels` (the repo's label pool) and `replaceIssueLabels` (recording the id list). Then assert, for a config with `planning_label: agent/planning` and `planned_label: agent/planned`: - issue seed #7 currently carrying `agent/planning` → after `publishPlanIssue` the recorded PUT holds the id of `agent/planned` and not that of `agent/planning`; - inbox seed → the created issue #42 gets the `agent/planned` id, and the inbox item is still marked processed; - a failing `replaceIssueLabels` (or an unknown `planned_label` name) still returns `{ok: true, …}` and still marks the inbox item processed; - no config file (`readRepoFile` → `{ok:true,data:null}`) → no `replaceIssueLabels` call at all. - `apps/server/src/forgejo/plan-issue.test.ts`: add a `startPlanIssue` case with a stub bridge — `createSession` returning `{ sessionId: "s1", setName: async () => {}, setPlanMode: async () => {}, prompt: async () => {} }` cast `as unknown as AgentBridge`, matching the file's existing `as unknown as DispatchApi` style — asserting that an issue seed triggers a PUT adding `agent/planning`, and that an inbox seed triggers none. - `apps/server/src/forgejo/dispatcher.test.ts`: in the abandon suite (`:3494`), add `planned_label: Agent/Planned` to `CONFIG` and assert the replace-all PUT after an abandon drops both the progress and planned label ids while adding the planning one. ## Verification From the repository root: - `bun test apps/server/src/forgejo/repo-config.test.ts apps/server/src/forgejo/plan-issue.test.ts apps/server/src/forgejo/dispatcher.test.ts apps/server/src/bridge/session-policy.test.ts` - `bun run typecheck` End-to-end check of the new behavior without a live Forgejo, as a throwaway script (delete it afterwards) under `apps/server`: 1. Build a fake `DispatchApi` whose `readRepoFile` returns `"implementation:\n planning_label: agent/planning\n planned_label: agent/planned\n"`, whose `listLabels` returns `[{id:1,name:"agent/planning"},{id:2,name:"agent/planned"}]`, whose `readIssue` returns issue #7 with labels `[{id:1,name:"agent/planning"}]`, and which records `replaceIssueLabels` calls. 2. Call `publishPlanIssue({workspaceId:"w",owner:"o",repo:"r",issueNumber:7,inboxId:null}, "# T\n\n## Problem\nx\n\n## Plan\ny\n", api)`. 3. Expected: the function returns `{ok:true, url:…, number:7}`, exactly one `updateIssueBody` call, and exactly one `replaceIssueLabels` call with `[2]` — `agent/planned` added, `agent/planning` gone. ## Assumptions & contingencies - The labels `agent/planning` and `agent/planned` are expected to exist in `lucas/omp-deck` on the Forgejo instance (unverified). If they do not, the code is still correct — the label call fails and is logged — but to keep the repo's own config clean, create the labels in Forgejo rather than reverting step 5. - If `dispatcher.test.ts`'s abandon suite turns out to assert label ids through a path that the list-valued `remove` changes beyond the `planned_label` id (for example an ordering assertion), keep `applyLabelDelta`'s output ordering identical to today's `swapLabels` (`planLabelDelta` preserves the current order and appends additions) and adjust only the expected id set.