From ed6d54485bc802117d2f488cc68bfc7bba6cec3c Mon Sep 17 00:00:00 2001 From: gsxdsm Date: Thu, 30 Jul 2026 12:55:38 -0700 Subject: [PATCH] glasses plugin: the review actions could never resolve a review lane (4 guards + 3 invisible destinations) (#2816) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four agent actions still keyed on literals, with three census-invisible `moveTask` destinations between them. `agent-actions.ts` already had `laneContext`/`destination` from an earlier partial conversion — these were simply never migrated. ## Census | file | main | here | | --- | ---: | ---: | | `plugins/fusion-plugin-even-realities-glasses/src/agent-actions.ts` | 4 | **0** | Plus 3 hardcoded `moveTask` destinations the census cannot see (`requestReview` → `in-review`, `returnToAgent` → `todo`, `retryTask` → `todo`). ## The real finding: this plugin could never resolve a review lane `resolveLifecycleColumns` keys its `review` role on the **`mergeOrchestration` trait alone**. A board whose review column carries only `merge-blocker` and/or `human-review` — the common custom shape, since `merge` is opt-in — resolves **no review lane at all**. So every review-gated action here (`requestReview`, `acceptReview`, `returnToAgent`, `retryTask`) compared against `undefined` and **refused every card**, and `requestReview` had nowhere to move one. This is not a regression from converting them; it is why they *could not* be converted with `lanes.review` as-is. Converting the four guards without noticing would have shipped four actions that fail closed on exactly the boards this program exists to support — a conversion that looks complete, passes its suite, and makes the plugin useless on a custom board. **Widened in `laneContext`, not in the shared resolver.** `resolveLifecycleColumns` is consumed well beyond this plugin, and its `review` role deliberately means "the merge-orchestration column" for the merge queue. The gap is already recorded in `notification-renamed-lifecycle-columns.test.ts` and in #2807 — reconciling the two definitions is a core-level decision, not one to take from a plugin. `mergeBlocker` is preferred over `humanReview` because a card cannot leave a merge-blocking column until the gate clears, which is the closer analogue of the legacy `in-review`. ## The suite caught an over-reach of mine My first version put a blanket `if (degraded) conflict(...)` at the top of `retryTask`, which broke a pinned invariant the test names outright: **"a degraded workflow does not block retries that move nothing."** The status-only retry just clears fields; refusing it because the workflow could not be read breaks a recovery that needs no lane at all. Degraded now blocks only the branches that actually **move**. Same reasoning applied to `acceptReview`, which also moves nothing. The existing `startWork` convention — conflict on degraded — is right precisely *because* it moves. ## Ordering `returnToAgent` and `retryTask` now resolve their destination **before** the field clear. Both cleared first, so a rejected move left the assignee and status — or the worktree, branch and base refs — nulled with the card exactly where it was. That is the fifth instance of this half-applied shape in the audit, and it is rule 3 in the class doc. ## Revert results (measured, each independently) | conversion | reverted → | | --- | --- | | `requestReview` destination | 1 failed — moves to the literal `in-review`, which this workflow does not declare | | `returnToAgent` destination | 1 failed — moves to the literal `todo`, same | Plus a non-vacuous companion: a renamed card *not* in the wip lane must still be refused by `requestReview`, so a gate admitting everything would not pass. ## Verification - Plugin suite — **186/186** - `pnpm test:gate` — 161 + 487 + 13 + 71, green - `tsc` on the plugin — clean - `pnpm lint`, `check:changesets`, census `--strict` — all clean (run explicitly) --- .changeset/glasses-plugin-review-lane.md | 7 ++ ...etask-destinations-are-census-invisible.md | 12 +++ .../src/__tests__/agent-actions.test.ts | 56 ++++++++++++ .../src/agent-actions.ts | 87 +++++++++++++++++-- 4 files changed, 153 insertions(+), 9 deletions(-) create mode 100644 .changeset/glasses-plugin-review-lane.md diff --git a/.changeset/glasses-plugin-review-lane.md b/.changeset/glasses-plugin-review-lane.md new file mode 100644 index 0000000000..0551646f2c --- /dev/null +++ b/.changeset/glasses-plugin-review-lane.md @@ -0,0 +1,7 @@ +--- +"@runfusion/fusion": patch +--- + +summary: Glasses plugin review actions now work on boards whose review column is renamed. +category: fix +dev: `requestReview`/`acceptReview`/`returnToAgent`/`retryTask` gated on literal columns and moved to literal destinations. Their review lane also could not resolve at all, because `resolveLifecycleColumns` keys `review` on `mergeOrchestration` alone; `laneContext` now widens to `mergeBlocker`/`humanReview` when that role is absent. diff --git a/docs/solutions/architecture-patterns/hardcoded-movetask-destinations-are-census-invisible.md b/docs/solutions/architecture-patterns/hardcoded-movetask-destinations-are-census-invisible.md index b2347fa48f..e4a9efd0e0 100644 --- a/docs/solutions/architecture-patterns/hardcoded-movetask-destinations-are-census-invisible.md +++ b/docs/solutions/architecture-patterns/hardcoded-movetask-destinations-are-census-invisible.md @@ -121,6 +121,18 @@ an undeclared column — invisible to every trait-driven sweep until reconciliat throws. Whether that surfaces or disappears depends entirely on whether the caller catches, which is per-site and is **not** measured here — do not read "29" as "29 crashes". +## The one remaining non-blocked site, and why it is not converted + +`packages/dashboard/app/utils/appLifecycle.ts:245` — the CLI-session **cancel** action, `deps.moveTask(session.id, "todo")`. Its dependency type pins the literal in the signature itself: + +```ts +moveTask: (id: string, column: "todo") => Promise; +``` + +Converting it needs the resolved hold lane threaded from the caller (`App.tsx#handleCliAction`), and there is **no resolved-columns map in that scope** — the app-side convention is an optional `columnFlagsById` passed down, as `deriveStatsFromTasks` does, and App does not have one here. + +Adding the parameter without wiring it would be an **unwired parameter**, which is precisely the anti-pattern the caller audit (#2803) removed five of. So this is left for the dashboard-app owner, who can introduce the map at the same time. One site, recorded rather than half-done. + ## What to do 1. **Convert the pair or neither.** When a census entry sits in a function that also performs a diff --git a/plugins/fusion-plugin-even-realities-glasses/src/__tests__/agent-actions.test.ts b/plugins/fusion-plugin-even-realities-glasses/src/__tests__/agent-actions.test.ts index 53df974300..d971abfa8e 100644 --- a/plugins/fusion-plugin-even-realities-glasses/src/__tests__/agent-actions.test.ts +++ b/plugins/fusion-plugin-even-realities-glasses/src/__tests__/agent-actions.test.ts @@ -369,6 +369,62 @@ describe("resolved lanes drive destinations, not just gates", () => { expect(deps.moveTask).toHaveBeenCalledWith("FN-1", "backlog"); }); + /* + FNXC:PluginLifecycleColumns 2026-07-30-22:50 (census-invisible moveTask destinations): + The four actions this suite had not yet reached. Same lesson as the note above — the census counts + the GATE and cannot see the DESTINATION — applied to `requestReview`, `acceptReview`, `returnToAgent` + and `retryTask`. + + REVERT CHECKS, measured (each independently): + - requestReview gate -> refuses the renamed card outright ("request-review not allowed in + column=building"). + - requestReview dest -> moves to the literal `in-review`, which this workflow does not declare. + - returnToAgent dest -> moves to the literal `todo`, same. + - retryTask gate/dest -> refuses, or rebounds to a lane the board does not have. + */ + it("requestReview gates on the renamed WIP lane and moves to the renamed REVIEW lane", async () => { + const deps = createResolvingDeps(makeTask({ column: "building", status: null }), renamedIr); + + await requestReview({ taskId: "FN-1" }, deps as never); + + expect(deps.moveTask).toHaveBeenCalledWith("FN-1", "checking"); + expect(deps.moveTask).not.toHaveBeenCalledWith("FN-1", "in-review"); + }); + + it("requestReview still refuses a renamed card that is not in the wip lane", async () => { + /* Non-vacuous: without this a gate admitting every column would satisfy the case above. */ + const deps = createResolvingDeps(makeTask({ column: "backlog", status: null }), renamedIr); + + await expect(requestReview({ taskId: "FN-1" }, deps as never)).rejects.toThrow(/request-review not allowed/); + expect(deps.moveTask).not.toHaveBeenCalled(); + }); + + it("acceptReview gates on the renamed REVIEW lane", async () => { + const deps = createResolvingDeps(makeTask({ column: "checking", status: null }), renamedIr); + + await acceptReview({ taskId: "FN-1" }, deps as never); + + expect(deps.updateTask).toHaveBeenCalledWith("FN-1", { status: null, assigneeUserId: null }); + }); + + it("returnToAgent gates on the renamed REVIEW lane and rebounds to the renamed HOLD lane", async () => { + const deps = createResolvingDeps(makeTask({ column: "checking", status: null }), renamedIr); + + await returnToAgent({ taskId: "FN-1" }, deps as never); + + expect(deps.moveTask).toHaveBeenCalledWith("FN-1", "backlog"); + expect(deps.moveTask).not.toHaveBeenCalledWith("FN-1", "todo"); + }); + + it("retryTask rebounds a failed renamed card to the renamed HOLD lane", async () => { + const deps = createResolvingDeps(makeTask({ column: "building", status: "failed" }), renamedIr); + + await retryTask({ taskId: "FN-1" }, deps as never); + + expect(deps.moveTask).toHaveBeenCalledWith("FN-1", "backlog"); + expect(deps.moveTask).not.toHaveBeenCalledWith("FN-1", "todo"); + }); + it("REFUSES a card in a legacy-named column the workflow assigns to REVIEW", async () => { /* The aliasing case. Unscoped, `todo` counted as a planning lane and startWork would diff --git a/plugins/fusion-plugin-even-realities-glasses/src/agent-actions.ts b/plugins/fusion-plugin-even-realities-glasses/src/agent-actions.ts index e73580434c..fdbaaf81d9 100644 --- a/plugins/fusion-plugin-even-realities-glasses/src/agent-actions.ts +++ b/plugins/fusion-plugin-even-realities-glasses/src/agent-actions.ts @@ -1,5 +1,5 @@ import type { PluginContext } from "@fusion/plugin-sdk"; -import { isBuiltinWorkflowId, resolveLifecycleColumns, resolveWorkflowIrById, resolveWorkflowIrForTask } from "@fusion/core"; +import { columnsWithFlag, isBuiltinWorkflowId, resolveLifecycleColumns, resolveWorkflowIrById, resolveWorkflowIrForTask } from "@fusion/core"; import { taskToCard, type GlassesCard } from "./cards.js"; import { GlassesInputError } from "./quick-capture.js"; @@ -207,7 +207,31 @@ async function laneContext( } const roles = snapshotIr ? resolveLifecycleColumns(snapshotIr as never) : undefined; - const lanes: Lanes | undefined = roles ?? undefined; + let lanes: Lanes | undefined = roles ?? undefined; + /* + FNXC:PluginLifecycleColumns 2026-07-30-23:10 (the review lane this plugin could never resolve): + `resolveLifecycleColumns` keys its `review` role on the `mergeOrchestration` trait ALONE. A board whose + review column carries only `merge-blocker` and/or `human-review` — the common custom shape, since + `merge` is opt-in — therefore resolves NO review lane at all. + + Every review-gated action here (`requestReview`, `acceptReview`, `returnToAgent`, `retryTask`) compares + against that lane, so on such a board they refused every card, and `requestReview` had nowhere to move + one. Not a regression from converting them: it is why they could not be converted with + `lanes.review` as-is. + + Widened HERE rather than in the shared resolver: `resolveLifecycleColumns` is consumed well beyond this + plugin and its `review` role deliberately means "the merge-orchestration column" for the merge queue. + The gap is recorded in `notification-renamed-lifecycle-columns.test.ts` and in PR #2807; reconciling the + two definitions is a core-level decision, not one to make from a plugin. + + `mergeBlocker` first, then `humanReview` — a card cannot leave a merge-blocking column without the + merge gate clearing, which is the closer analogue of the legacy `in-review`. + */ + if (lanes && lanes.review === undefined && snapshotIr) { + const widenedReview = columnsWithFlag(snapshotIr as never, "mergeBlocker")[0] + ?? columnsWithFlag(snapshotIr as never, "humanReview")[0]; + if (widenedReview) lanes = { ...lanes, review: widenedReview }; + } const declared = new Set( Object.values(lanes ?? {}).filter((value): value is string => typeof value === "string"), ); @@ -277,10 +301,21 @@ export async function startWork(input: AgentActionInput, deps: AgentActionDeps): export async function requestReview(input: AgentActionInput, deps: AgentActionDeps): Promise { const taskId = normalizeTaskId(input.taskId); const task = await getTaskOrThrow(deps.taskStore, taskId); - if (task.column !== "in-progress") { + /* + FNXC:PluginLifecycleColumns 2026-07-30-22:50 (census-invisible moveTask destinations): + Both halves, from ONE snapshot — the same rule `startWork` above already follows. The GATE was the + literal `in-progress` (counted by the census) and the DESTINATION was the literal `in-review` (a call + argument, so invisible to it). On a renamed board the gate refused every card, and had it not, the + move would have targeted a lane the board may not declare. + */ + const { lanes: reviewLanes, degraded: reviewDegraded } = await laneContext(deps.taskStore, taskId); + if (reviewDegraded) conflict("request-review", task); + if (String(task.column) !== destination(reviewLanes, "wip")) { conflict("request-review", task); } - await deps.taskStore.moveTask(taskId, "in-review"); + const reviewTarget = destination(reviewLanes, "review"); + if (!reviewTarget) conflict("request-review", task); + await deps.taskStore.moveTask(taskId, reviewTarget); return toResult(deps.taskStore, taskId); } @@ -305,7 +340,11 @@ export async function approvePlan(input: AgentActionInput, deps: AgentActionDeps export async function acceptReview(input: AgentActionInput, deps: AgentActionDeps): Promise { const taskId = normalizeTaskId(input.taskId); const task = await getTaskOrThrow(deps.taskStore, taskId); - if (task.column !== "in-review") { + /* FNXC:PluginLifecycleColumns 2026-07-30-22:50: review lane by ROLE; on a renamed board this refused + every card. No destination and no degraded-conflict: acceptReview only clears fields, and the same + "do not block an action that moves nothing" rule the retry cases pin applies here. */ + const { lanes: acceptLanes } = await laneContext(deps.taskStore, taskId); + if (String(task.column) !== destination(acceptLanes, "review")) { conflict("accept-review", task); } await deps.taskStore.updateTask(taskId, { status: null, assigneeUserId: null }); @@ -315,15 +354,25 @@ export async function acceptReview(input: AgentActionInput, deps: AgentActionDep export async function returnToAgent(input: AgentActionInput, deps: AgentActionDeps): Promise { const taskId = normalizeTaskId(input.taskId); const task = await getTaskOrThrow(deps.taskStore, taskId); - if (task.column !== "in-review") { + /* + FNXC:PluginLifecycleColumns 2026-07-30-22:50 (census-invisible moveTask destinations): + Gate AND destination from one snapshot. The destination is resolved BEFORE the field clear below: + `updateTask` used to run first, so a rejected move left the assignee and status cleared with the card + still in review — the half-applied shape this audit keeps finding. + */ + const { lanes: returnLanes, degraded: returnDegraded } = await laneContext(deps.taskStore, taskId); + if (returnDegraded) conflict("return-to-agent", task); + if (String(task.column) !== destination(returnLanes, "review")) { conflict("return-to-agent", task); } + const returnTarget = destination(returnLanes, "hold"); + if (!returnTarget) conflict("return-to-agent", task); await deps.taskStore.updateTask(taskId, { assigneeUserId: null, status: null, assignedAgentId: null, }); - await deps.taskStore.moveTask(taskId, "todo"); + await deps.taskStore.moveTask(taskId, returnTarget); return toResult(deps.taskStore, taskId); } @@ -331,7 +380,18 @@ export async function retryTask(input: AgentActionInput, deps: AgentActionDeps): const taskId = normalizeTaskId(input.taskId); const task = await getTaskOrThrow(deps.taskStore, taskId); - if (task.column === "in-review" && RETRYABLE_FAILURE_STATUSES.has(String(task.status))) { + /* + FNXC:PluginLifecycleColumns 2026-07-30-22:50: review lane by ROLE, resolved once and reused by the + rebound below so the gate and the destination cannot disagree. + + NO degraded-conflict here, unlike `startWork`. The suite pins "a degraded workflow does not block + retries that move nothing": the status-only retry above just clears fields, and refusing it because + the workflow could not be read would break a recovery that needs no lane at all. `destination()` + already falls back to the legacy ids when lanes are unresolved, which is the behaviour those cases + assert. Only the branch that actually MOVES needs a resolved target. + */ + const { lanes: retryGateLanes } = await laneContext(deps.taskStore, taskId); + if (String(task.column) === destination(retryGateLanes, "review") && RETRYABLE_FAILURE_STATUSES.has(String(task.status))) { await deps.taskStore.updateTask(taskId, { status: null, error: null, stuckKillCount: 0, mergeRetries: 0 }); return toResult(deps.taskStore, taskId); } @@ -366,6 +426,15 @@ export async function retryTask(input: AgentActionInput, deps: AgentActionDeps): } if (RETRYABLE_FAILURE_STATUSES.has(String(task.status))) { + /* + FNXC:PluginLifecycleColumns 2026-07-30-22:50 (census-invisible moveTask destinations): + Destination resolved BEFORE the field clear. The clear used to run first, so a rejected move left the + worktree, branch and base refs nulled with the card exactly where it was — the retry destroyed the + pointers back to the work and then did not requeue. Reuses the snapshot taken at the top of this + action so the gate and the destination cannot disagree. + */ + const retryTarget = destination(retryGateLanes, "hold"); + if (!retryTarget) conflict("retry", task); // Intentional v1 limitation: omits dashboard retry step-reset/branch-inspection behavior. await deps.taskStore.updateTask(taskId, { status: null, @@ -378,7 +447,7 @@ export async function retryTask(input: AgentActionInput, deps: AgentActionDeps): recoveryRetryCount: null, nextRecoveryAt: null, }); - await deps.taskStore.moveTask(taskId, "todo"); + await deps.taskStore.moveTask(taskId, retryTarget); return toResult(deps.taskStore, taskId); }