lucas/omp-deck

Watch

Fork 0

Plan-issue label failures are silent: report them #271

New issue

Closed

opened 2026-11-13 11:13:08 +01:00 by omp-deck · 0 comments

omp-deck commented

2026-11-13 11:13:08 +01:00

Collaborator

Problem

Starting a "Plan issue" session on a Forgejo issue does not apply
implementation.planning_label from .omp/forgejo.yaml, even though the
repository has that label and the config is on the default branch. The deck
reports nothing: no toast, no message in the session, no comment, nothing in
the UI. The only trace is a log.warn on the server's stdout.

The reported case is lucas/nanoKontroller2 (labels Agent/Ready,
Agent/In Progress, Agent/Planning, Agent/Planned all exist on the
repository; the issue carried Agent/Triage at the time). Why it failed there
is unverified — the repository and the deck's stdout log are both outside
what this session can read. Inspection of the code path leaves exactly these
real failure modes, and they are indistinguishable from the outside:

implementationLabels → api.readRepoFile fails with a non-404 status:
log.warn, returns null, no label is attempted.
implementationLabels → parseRepoConfig fails: log.warn, returns
null. This is all-or-nothing — an error in review:, ci_watchdog:,
system_prompts: or compaction: disables the labels too, while the
implementation: block is perfectly valid.
markIssue → applyLabelDelta returns a non-empty reason (unknown label,
ambiguous label, failed PUT): log.warn, nothing else.

(A 404 on the config file is a fourth exit, but a legitimate one:
parseRepoConfig(null) means "no config, defaults apply", and a repository
that was never onboarded must stay quiet. So must the no-op case where the
label is simply not configured.)

The same silence covers the publish side: approving the plan adds
planned_label and removes planning_label through the same two helpers, and
reports "Plan published to Forgejo" regardless of whether the labels moved.

The dispatcher already solves this for its own label updates — markInProgress
comments on the issue and falls back to notificationService.notify
(dispatcher.ts:2988-3006), and the abandon path collects problems into a
comment (dispatcher.ts:2604-2618). The plan-issue path is the outlier.

The fix is the observability, not a named root cause: after it, the next
occurrence names its own reason. Scope boundary: no Settings-side
config-vs-labels health check, and no change to when a planning session starts
or when an issue is published.

Plan

1. Make `markIssue` report its reason instead of swallowing it

In apps/server/src/forgejo/plan-issue.ts, change the private helper

async function markIssue(api, ref, labels): Promise<void>

to return the failure reason:

/**
 * Apply a lifecycle label change. Returns the failure reason, or "" when the
 * labels were applied, were already as wanted, or none was configured. Never
 * throws and never fails planning: a label is not worth failing over, but
 * callers report what went wrong.
 */
async function markIssue(
	api: DispatchApi,
	ref: { owner: string; repo: string; number: number },
	labels: { add?: readonly (string | null)[]; remove?: readonly (string | null)[] },
): Promise<string>

Keep the existing try/catch around applyLabelDelta and the existing
log.warn line verbatim (could not update labels of ${ref.owner}/${ref.repo}#${ref.number}: ${reason});
return reason at the end.

applyLabelDelta (apps/server/src/forgejo/issue-labels.ts:29) already returns
"" for success, for a no-op delta, and for an all-null/empty delta, so the
"nothing configured" case needs no extra branch.

2. Add the notification helper

Still in plan-issue.ts, next to the existing const log = logger("forgejo:plan-issue");:

/** `source` of every notification this module raises. */
const NOTIFY_SOURCE = "forgejo-plan-issue";

Import the singleton the dispatcher uses: import { notificationService } from "../notifications/index.ts";

Add one private helper used by both moments:

/** Report a label/config problem to the deck UI; never throws. */
async function reportProblem(title: string, body: string): Promise<void> {
	try {
		await notificationService.notify({ level: "error", title, body, source: NOTIFY_SOURCE });
	} catch (err) {
		log.warn(`could not report "${title}"`, err);
	}
}

level: "error" is deliberate: NotificationToast auto-dismisses info and
warn after 6s and keeps error/critical until dismissed
(apps/web/src/components/NotificationToast.tsx:16, 42-50), and the user is
navigating into the new session at exactly that moment. No actionUrl, matching
the dispatcher's label notification. The reason string is passed through
verbatim — a long "unknown label … Valid labels: …" clips at line-clamp-3 in
the toast (NotificationToast.tsx:71) and stays complete in the notifications
log in Settings.

No web-side change is needed: notify → BrowserNotificationChannel
(registered in apps/server/src/index.ts) → broadcastBus → the store's
"notification" case → existing toast and Settings log.

3. Report the two config failures

In implementationLabels (plan-issue.ts:25-47), both failure branches
currently do log.warn and return null. Keep the logs, add a report before
each return. The function must learn nothing new — it already has owner,
repo and name:

read failure (!file.ok):
- title: `Could not read ${REPO_CONFIG_PATH} of ${name}`
- body: `${file.status} ${file.message}\n\nThe plan-issue labels of this repository were not applied.`
parse failure (!parsed.ok):
- title: `Invalid ${REPO_CONFIG_PATH} in ${name}`
- body: `${parsed.error}\n\nThe whole file is rejected, so the plan-issue labels of this repository were not applied.`
the catch branch (err): reuse the read-failure title with the error
message as the body.

The 404 path (file.ok with data === null) stays silent: parseRepoConfig
turns it into DEFAULT_REPO_CONFIG, whose label fields are all null, and
markIssue then returns "".

4. Report the label failure at session start

In startPlanIssue (plan-issue.ts:211-214), the call is currently
if (labels) await markIssue(...). Capture the reason and report it:

if (api !== null && binding.issueNumber !== null) {
	const labels = await implementationLabels(api, owner, repo);
	if (labels) {
		const reason = await markIssue(api, { owner, repo, number: binding.issueNumber }, { add: [labels.planningLabel] });
		if (reason !== "") {
			await reportProblem(
				`Could not label ${owner}/${repo}#${binding.issueNumber} as planning`,
				`${reason}\n\nThe planning session started regardless.`,
			);
		}
	}
}

The session still starts whatever happens; the report is awaited only because
notify is async and the surrounding function already awaits label work.

5. Carry the publish-time problem into the approval message

publishPlanIssue (plan-issue.ts:110-138) applies
{ add: [labels.plannedLabel], remove: [labels.planningLabel] } after the issue
write succeeded. A label failure there is not a publish failure: the issue is
written and correct.

In plan-issue.ts, widen the ok-variant:

export type PublishOutcome =
	| { ok: true; url: string | null; number: number; labelProblem?: string }
	| { ok: false; error: string };

In publishPlanIssue, capture markIssue's reason. When it is non-empty,
call reportProblem with title
`Could not label ${b.owner}/${b.repo}#${outcome.number} as planned` and
body `${reason}\n\nThe issue was published; only the label is missing.`,
and include labelProblem: reason in the returned ok-outcome. When it is
empty, return the outcome exactly as today (no labelProblem key).
The inbox update (if (b.inboxId !== null) updateInbox(b.inboxId, { processed: true });)
is unaffected and still runs.
In apps/server/src/bridge/plan-mode-bridge.ts, extend the message builder at
:872-874:

function publishedText(where: string, labelProblem?: string): string {
	const problem = labelProblem
		? ` The issue's planned label could not be applied: ${labelProblem}. Say so in your reply.`
		: "";
	return `Plan published to Forgejo: ${where}.${problem} Plan-issue planning is complete and its planning instructions no longer apply; only finish any post-publish steps they name, then stop. The session stays open: treat any later user message as a normal request.`;
}

Without a problem the string is byte-identical to today's.

Pass the field at both approve sites, which already hold published:
plan-mode-bridge.ts:578-580 (detached approve, session.prompt(publishedText(where), …))
and :773-778 (tool-result approve). Both become
publishedText(where, published.labelProblem).

published is narrowed to the ok-variant at both sites (the !published.ok
branch returned earlier), so the field is available without a cast.

These are the only two publishedText callers; grep -n "publishedText" apps/server/src
must return exactly the definition and those two sites (plus any match in
plan-mode-bridge.test.ts, which asserts on the produced text).

6. Tests

apps/server/src/forgejo/plan-issue.test.ts already has fakeApi({ config }),
LABEL_CONFIG and LABEL_POOL, and a startPlanIssue describe block
(:208-240). Capture notifications with the pattern from
dispatcher.test.ts:600-608:

function recordNotifications(): { title: string; body?: string }[] {
	const seen: { title: string; body?: string }[] = [];
	notificationService.register({ id: "test-recorder", deliver(envelope) { seen.push({ title: envelope.title, body: envelope.body }); } });
	return seen;
}

always paired with notificationService.unregister("test-recorder") in a
finally.

Add, as behavior checks:

start, unknown label: fakeApi with LABEL_CONFIG but a label pool that
lacks agent/planning → startPlanIssue resolves with a session id, no label
PUT is recorded, and exactly one notification whose title contains
as planning and whose body contains unknown label.
start, invalid config: fakeApi({ config: "implementation: {planning_label: x, planned_label: X}" })
(a pair parseRepoConfig rejects, repo-config.test.ts:196-198) → exactly one
notification titled Invalid .omp/forgejo.yaml in o/r, and the session still
starts.
start, nothing configured: fakeApi({ config: "" }) → no notification at
all and no label PUT (guards against turning the legitimate no-op into
noise).
publish, label failure: publishPlanIssue against a pool lacking
agent/planned → outcome stays ok: true with the issue written, carries
labelProblem containing unknown label, and raises one notification.
publish, success: unchanged pool → ok: true with no labelProblem key
and no notification.

In apps/server/src/bridge/plan-mode-bridge.test.ts, the existing
"plan-issue publish on approve" block (:815) asserts the published text. Add
one case whose publish hook returns { ok: true, url: "…", number: 7, labelProblem: "unknown label: agent/planned" }
and assert the prompt text contains planned label could not be applied: unknown label: agent/planned,
and one asserting that an outcome without labelProblem produces the current
text unchanged.

Verification

Working directory /workspace/omp-deck; no env vars or fixtures needed.

bun test apps/server/src/forgejo/plan-issue.test.ts apps/server/src/bridge/plan-mode-bridge.test.ts
— all pass, including the new cases above.
bun test apps/server/src/forgejo/dispatcher.test.ts
— unchanged; proves the shared applyLabelDelta/notification paths were not
disturbed.
bun run typecheck — PublishOutcome and publishedText changed shape, so
this is the check that every caller was migrated.
End-to-end, new behavior, in the running deck: in a workspace whose repo is
on the configured Forgejo instance, temporarily point
implementation.planning_label in that repository's .omp/forgejo.yaml
(default branch) at a label that does not exist, e.g.
planning_label: "Agent/DoesNotExist", then start New session ▾ → Plan
issue on an open issue of that repo. Expected: the session starts and
navigates as before, and a red toast appears bottom-right titled
Could not label <owner>/<repo>#<n> as planning whose body begins
unknown label: Agent/DoesNotExist. Valid labels: …; it stays until
dismissed and is listed in Settings → Notifications. Restore the config
afterwards. On lucas/omp-deck itself this reproduces with no config change
at all, since its configured agent/planning and agent/planned labels do
not exist on the instance — which is a separate, repository-side fact, not
part of this change.

Assumptions & contingencies

The reported lucas/nanoKontroller2 failure is assumed to be one of the three
modes above. If, after this change, starting a plan-issue session there raises
no notification and still applies no label, the labelling code did not run
at all: check that the running server includes commit a690585
(2026-11-10, feat(server): mark planned issues with a configurable planned_label),
and that the session was started through New session ▾ → Plan issue with
an issue seed, which is the only path that calls startPlanIssue.
notificationService.notify is assumed safe to call from startPlanIssue and
publishPlanIssue. It swallows channel errors itself
(notifications/service.ts:72-85); the extra try/catch in reportProblem
exists so a future throwing implementation can never break planning.

## Problem Starting a "Plan issue" session on a Forgejo issue does not apply `implementation.planning_label` from `.omp/forgejo.yaml`, even though the repository has that label and the config is on the default branch. The deck reports nothing: no toast, no message in the session, no comment, nothing in the UI. The only trace is a `log.warn` on the server's stdout. The reported case is `lucas/nanoKontroller2` (labels `Agent/Ready`, `Agent/In Progress`, `Agent/Planning`, `Agent/Planned` all exist on the repository; the issue carried `Agent/Triage` at the time). Why it failed there is **unverified** — the repository and the deck's stdout log are both outside what this session can read. Inspection of the code path leaves exactly these real failure modes, and they are indistinguishable from the outside: 1. `implementationLabels` → `api.readRepoFile` fails with a non-404 status: `log.warn`, returns `null`, no label is attempted. 2. `implementationLabels` → `parseRepoConfig` fails: `log.warn`, returns `null`. This is all-or-nothing — an error in `review:`, `ci_watchdog:`, `system_prompts:` or `compaction:` disables the labels too, while the `implementation:` block is perfectly valid. 3. `markIssue` → `applyLabelDelta` returns a non-empty reason (unknown label, ambiguous label, failed `PUT`): `log.warn`, nothing else. (A 404 on the config file is a fourth exit, but a legitimate one: `parseRepoConfig(null)` means "no config, defaults apply", and a repository that was never onboarded must stay quiet. So must the no-op case where the label is simply not configured.) The same silence covers the publish side: approving the plan adds `planned_label` and removes `planning_label` through the same two helpers, and reports "Plan published to Forgejo" regardless of whether the labels moved. The dispatcher already solves this for its own label updates — `markInProgress` comments on the issue and falls back to `notificationService.notify` (`dispatcher.ts:2988-3006`), and the abandon path collects `problems` into a comment (`dispatcher.ts:2604-2618`). The plan-issue path is the outlier. The fix is the observability, not a named root cause: after it, the next occurrence names its own reason. Scope boundary: no Settings-side config-vs-labels health check, and no change to when a planning session starts or when an issue is published. ## Plan ### 1. Make `markIssue` report its reason instead of swallowing it In `apps/server/src/forgejo/plan-issue.ts`, change the private helper ```ts async function markIssue(api, ref, labels): Promise<void> ``` to return the failure reason: ```ts /** * Apply a lifecycle label change. Returns the failure reason, or "" when the * labels were applied, were already as wanted, or none was configured. Never * throws and never fails planning: a label is not worth failing over, but * callers report what went wrong. */ async function markIssue( api: DispatchApi, ref: { owner: string; repo: string; number: number }, labels: { add?: readonly (string | null)[]; remove?: readonly (string | null)[] }, ): Promise<string> ``` Keep the existing `try`/`catch` around `applyLabelDelta` and the existing `log.warn` line verbatim (`could not update labels of ${ref.owner}/${ref.repo}#${ref.number}: ${reason}`); return `reason` at the end. `applyLabelDelta` (`apps/server/src/forgejo/issue-labels.ts:29`) already returns `""` for success, for a no-op delta, and for an all-null/empty delta, so the "nothing configured" case needs no extra branch. ### 2. Add the notification helper Still in `plan-issue.ts`, next to the existing `const log = logger("forgejo:plan-issue");`: ```ts /** `source` of every notification this module raises. */ const NOTIFY_SOURCE = "forgejo-plan-issue"; ``` Import the singleton the dispatcher uses: `import { notificationService } from "../notifications/index.ts";` Add one private helper used by both moments: ```ts /** Report a label/config problem to the deck UI; never throws. */ async function reportProblem(title: string, body: string): Promise<void> { try { await notificationService.notify({ level: "error", title, body, source: NOTIFY_SOURCE }); } catch (err) { log.warn(`could not report "${title}"`, err); } } ``` `level: "error"` is deliberate: `NotificationToast` auto-dismisses `info` and `warn` after 6s and keeps `error`/`critical` until dismissed (`apps/web/src/components/NotificationToast.tsx:16, 42-50`), and the user is navigating into the new session at exactly that moment. No `actionUrl`, matching the dispatcher's label notification. The reason string is passed through verbatim — a long "unknown label … Valid labels: …" clips at `line-clamp-3` in the toast (`NotificationToast.tsx:71`) and stays complete in the notifications log in Settings. No web-side change is needed: `notify` → `BrowserNotificationChannel` (registered in `apps/server/src/index.ts`) → `broadcastBus` → the store's `"notification"` case → existing toast and Settings log. ### 3. Report the two config failures In `implementationLabels` (`plan-issue.ts:25-47`), both failure branches currently do `log.warn` and `return null`. Keep the logs, add a report before each return. The function must learn nothing new — it already has `owner`, `repo` and `name`: - read failure (`!file.ok`): - title: `` `Could not read ${REPO_CONFIG_PATH} of ${name}` `` - body: `` `${file.status} ${file.message}\n\nThe plan-issue labels of this repository were not applied.` `` - parse failure (`!parsed.ok`): - title: `` `Invalid ${REPO_CONFIG_PATH} in ${name}` `` - body: `` `${parsed.error}\n\nThe whole file is rejected, so the plan-issue labels of this repository were not applied.` `` - the `catch` branch (`err`): reuse the read-failure title with the error message as the body. The 404 path (`file.ok` with `data === null`) stays silent: `parseRepoConfig` turns it into `DEFAULT_REPO_CONFIG`, whose label fields are all `null`, and `markIssue` then returns `""`. ### 4. Report the label failure at session start In `startPlanIssue` (`plan-issue.ts:211-214`), the call is currently `if (labels) await markIssue(...)`. Capture the reason and report it: ```ts if (api !== null && binding.issueNumber !== null) { const labels = await implementationLabels(api, owner, repo); if (labels) { const reason = await markIssue(api, { owner, repo, number: binding.issueNumber }, { add: [labels.planningLabel] }); if (reason !== "") { await reportProblem( `Could not label ${owner}/${repo}#${binding.issueNumber} as planning`, `${reason}\n\nThe planning session started regardless.`, ); } } } ``` The session still starts whatever happens; the report is awaited only because `notify` is async and the surrounding function already awaits label work. ### 5. Carry the publish-time problem into the approval message `publishPlanIssue` (`plan-issue.ts:110-138`) applies `{ add: [labels.plannedLabel], remove: [labels.planningLabel] }` after the issue write succeeded. A label failure there is not a publish failure: the issue is written and correct. - In `plan-issue.ts`, widen the ok-variant: ```ts export type PublishOutcome = | { ok: true; url: string | null; number: number; labelProblem?: string } | { ok: false; error: string }; ``` - In `publishPlanIssue`, capture `markIssue`'s reason. When it is non-empty, call `reportProblem` with title `` `Could not label ${b.owner}/${b.repo}#${outcome.number} as planned` `` and body `` `${reason}\n\nThe issue was published; only the label is missing.` ``, and include `labelProblem: reason` in the returned ok-outcome. When it is empty, return the outcome exactly as today (no `labelProblem` key). The inbox update (`if (b.inboxId !== null) updateInbox(b.inboxId, { processed: true });`) is unaffected and still runs. - In `apps/server/src/bridge/plan-mode-bridge.ts`, extend the message builder at `:872-874`: ```ts function publishedText(where: string, labelProblem?: string): string { const problem = labelProblem ? ` The issue's planned label could not be applied: ${labelProblem}. Say so in your reply.` : ""; return `Plan published to Forgejo: ${where}.${problem} Plan-issue planning is complete and its planning instructions no longer apply; only finish any post-publish steps they name, then stop. The session stays open: treat any later user message as a normal request.`; } ``` Without a problem the string is byte-identical to today's. - Pass the field at both approve sites, which already hold `published`: `plan-mode-bridge.ts:578-580` (detached approve, `session.prompt(publishedText(where), …)`) and `:773-778` (tool-result approve). Both become `publishedText(where, published.labelProblem)`. `published` is narrowed to the ok-variant at both sites (the `!published.ok` branch returned earlier), so the field is available without a cast. These are the only two `publishedText` callers; `grep -n "publishedText" apps/server/src` must return exactly the definition and those two sites (plus any match in `plan-mode-bridge.test.ts`, which asserts on the produced text). ### 6. Tests `apps/server/src/forgejo/plan-issue.test.ts` already has `fakeApi({ config })`, `LABEL_CONFIG` and `LABEL_POOL`, and a `startPlanIssue` describe block (`:208-240`). Capture notifications with the pattern from `dispatcher.test.ts:600-608`: ```ts function recordNotifications(): { title: string; body?: string }[] { const seen: { title: string; body?: string }[] = []; notificationService.register({ id: "test-recorder", deliver(envelope) { seen.push({ title: envelope.title, body: envelope.body }); } }); return seen; } ``` always paired with `notificationService.unregister("test-recorder")` in a `finally`. Add, as behavior checks: - **start, unknown label**: `fakeApi` with `LABEL_CONFIG` but a label pool that lacks `agent/planning` → `startPlanIssue` resolves with a session id, no label `PUT` is recorded, and exactly one notification whose title contains `as planning` and whose body contains `unknown label`. - **start, invalid config**: `fakeApi({ config: "implementation: {planning_label: x, planned_label: X}" })` (a pair `parseRepoConfig` rejects, `repo-config.test.ts:196-198`) → exactly one notification titled `Invalid .omp/forgejo.yaml in o/r`, and the session still starts. - **start, nothing configured**: `fakeApi({ config: "" })` → no notification at all and no label `PUT` (guards against turning the legitimate no-op into noise). - **publish, label failure**: `publishPlanIssue` against a pool lacking `agent/planned` → outcome stays `ok: true` with the issue written, carries `labelProblem` containing `unknown label`, and raises one notification. - **publish, success**: unchanged pool → `ok: true` with no `labelProblem` key and no notification. In `apps/server/src/bridge/plan-mode-bridge.test.ts`, the existing "plan-issue publish on approve" block (`:815`) asserts the published text. Add one case whose publish hook returns `{ ok: true, url: "…", number: 7, labelProblem: "unknown label: agent/planned" }` and assert the prompt text contains `planned label could not be applied: unknown label: agent/planned`, and one asserting that an outcome without `labelProblem` produces the current text unchanged. ## Verification Working directory `/workspace/omp-deck`; no env vars or fixtures needed. 1. `bun test apps/server/src/forgejo/plan-issue.test.ts apps/server/src/bridge/plan-mode-bridge.test.ts` — all pass, including the new cases above. 2. `bun test apps/server/src/forgejo/dispatcher.test.ts` — unchanged; proves the shared `applyLabelDelta`/notification paths were not disturbed. 3. `bun run typecheck` — `PublishOutcome` and `publishedText` changed shape, so this is the check that every caller was migrated. 4. End-to-end, new behavior, in the running deck: in a workspace whose repo is on the configured Forgejo instance, temporarily point `implementation.planning_label` in that repository's `.omp/forgejo.yaml` (default branch) at a label that does not exist, e.g. `planning_label: "Agent/DoesNotExist"`, then start **New session ▾ → Plan issue** on an open issue of that repo. Expected: the session starts and navigates as before, and a red toast appears bottom-right titled `Could not label <owner>/<repo>#<n> as planning` whose body begins `unknown label: Agent/DoesNotExist. Valid labels: …`; it stays until dismissed and is listed in Settings → Notifications. Restore the config afterwards. On `lucas/omp-deck` itself this reproduces with no config change at all, since its configured `agent/planning` and `agent/planned` labels do not exist on the instance — which is a separate, repository-side fact, not part of this change. ## Assumptions & contingencies - The reported `lucas/nanoKontroller2` failure is assumed to be one of the three modes above. If, after this change, starting a plan-issue session there raises **no** notification and still applies no label, the labelling code did not run at all: check that the running server includes commit `a690585` (2026-11-10, `feat(server): mark planned issues with a configurable planned_label`), and that the session was started through **New session ▾ → Plan issue** with an issue seed, which is the only path that calls `startPlanIssue`. - `notificationService.notify` is assumed safe to call from `startPlanIssue` and `publishPlanIssue`. It swallows channel errors itself (`notifications/service.ts:72-85`); the extra `try`/`catch` in `reportProblem` exists so a future throwing implementation can never break planning.