glasses plugin: the review actions could never resolve a review lane (4 guards + 3 invisible destinations) (#2816)
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)
This commit is contained in:
7
.changeset/glasses-plugin-review-lane.md
Normal file
7
.changeset/glasses-plugin-review-lane.md
Normal file
@@ -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.
|
||||
@@ -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<unknown>;
|
||||
```
|
||||
|
||||
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
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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<AgentActionResult> {
|
||||
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<AgentActionResult> {
|
||||
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<AgentActionResult> {
|
||||
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);
|
||||
}
|
||||
|
||||
|
||||
Reference in New Issue
Block a user