fix(web): preserve executor fields across kanban task cache merges (#2705)
Co-authored-by: ayattara <alassane.yattara@savoirfairelinux.com> Co-authored-by: Carlos Florencio <carlosmflorencio@gmail.com>
This commit is contained in:
@@ -0,0 +1,143 @@
|
||||
---
|
||||
spec: docs/specs/kanban-task-executor-cache-staleness/spec.md
|
||||
created: 2026-08-15
|
||||
status: completed
|
||||
---
|
||||
|
||||
# Implementation Plan: Kanban task cache preserves executor fields across merges
|
||||
|
||||
## Overview
|
||||
|
||||
Add an executor-fields preserve guard to the hydration merge and a
|
||||
request-start freshness guard to the workflow snapshot merge. Hydration uses
|
||||
`preserveOmittedExecutorFields` when an equal-or-newer incoming task omits its
|
||||
executor bundle. Snapshot reconciliation compares the current task with the
|
||||
task captured at request start, then copies the complete current bundle when a
|
||||
live change or in-flight task creation makes the response stale. This mirrors
|
||||
the existing `preservePrimaryExecutorFields` guard used by the WebSocket
|
||||
handler (`apps/web/lib/ws/handlers/tasks.ts`) while keeping HTTP authority
|
||||
rules separate from raw WebSocket presence checks.
|
||||
|
||||
## Root cause
|
||||
|
||||
`primary_executor_*`/`is_remote_executor` are `omitempty` fields derived per
|
||||
read from the primary session's executor binding
|
||||
(`apps/backend/internal/task/service/service_events.go:566-582`,
|
||||
`apps/backend/internal/task/dto/dto.go:162-164,180`); binding an executor only
|
||||
updates the `task_sessions` row, never the owning task's `updated_at`
|
||||
(`apps/backend/internal/task/repository/sqlite/session.go:741-743,907-921`).
|
||||
`mergeKanbanTasks` compares timestamps with `incomingTime >= existingTime` and,
|
||||
on that branch, replaces the cached task object wholesale
|
||||
(`apps/web/lib/state/hydration/hydrator.ts:47-49`) with no gap-fill — unlike
|
||||
its `backfillServerDerivedFields` helper, which only runs on the opposite
|
||||
(incoming-is-older) branch and only covers five dependency-projection fields,
|
||||
not the executor fields. `use-all-workflow-snapshots.ts`'s snapshot merge
|
||||
(`apps/web/hooks/domains/kanban/use-all-workflow-snapshots.ts:60-83`) preserves
|
||||
`primarySessionId`/`primarySessionState`/`autopilot`/`statusSummary` when a
|
||||
fresh response omits them but has no equivalent for the executor fields. A
|
||||
stale or equal-timestamp response landing after a `task.updated` WebSocket
|
||||
event already populated the executor fields therefore blanks them back out.
|
||||
|
||||
`isRemoteExecutor` cannot be gap-filled with a plain per-field `undefined`
|
||||
check: `toKanbanTask` maps it with `?? false`
|
||||
(`apps/web/lib/kanban/map-task.ts:199`), so the mapped value is never
|
||||
`undefined`, unlike its three sibling fields (`?? undefined`). Both fixed sites
|
||||
still use `primaryExecutorType` as the bundle-presence signal when they need to
|
||||
recognize an omitted response. The snapshot merge also compares the full
|
||||
current bundle with the request-start bundle, so an explicit stale value cannot
|
||||
override a live clear.
|
||||
|
||||
## Frontend
|
||||
|
||||
### Executor-fields preserve helper
|
||||
|
||||
- In `apps/web/lib/kanban/map-task.ts`, add and export small helpers,
|
||||
`preserveOmittedExecutorFields(merged: KanbanTask, existing: KanbanTask):
|
||||
void`, that copies `primaryExecutorId`, `primaryExecutorType`,
|
||||
`primaryExecutorName`, and `isRemoteExecutor` from `existing` onto `merged`
|
||||
only when `merged.primaryExecutorType === undefined`. This is the shared home
|
||||
both merge sites import from, matching the existing "single publisher /
|
||||
single mapper" convention already documented on `TaskLike` in that file.
|
||||
- Export `copyPrimaryExecutorFields` for snapshot reconciliation. This helper
|
||||
copies all four fields, including `undefined` and `false`, when the live
|
||||
cache is newer than the HTTP response.
|
||||
- Do not change `toKanbanTask`'s existing `is_remote_executor ?? false`
|
||||
mapping or the pinned `"defaults isRemoteExecutor to false when missing"`
|
||||
test in `apps/web/lib/kanban/map-task.test.ts` — out of scope per the spec's
|
||||
constraints.
|
||||
|
||||
### `mergeKanbanTasks` wholesale-replace branch
|
||||
|
||||
- In `apps/web/lib/state/hydration/hydrator.ts`, in the `incomingTime >=
|
||||
existingTime` branch of `mergeKanbanTasks`, before assigning
|
||||
`draftTasks[idx] = incoming`, call
|
||||
`preserveOmittedExecutorFields(incoming, existing)` so the mutation lands on
|
||||
the object that becomes the new cached task.
|
||||
- Leave the `incomingTime < existingTime` branch and
|
||||
`backfillServerDerivedFields` unchanged; this fix is scoped to the
|
||||
wholesale-replace branch's regression only.
|
||||
|
||||
### Workflow snapshot merge
|
||||
|
||||
- In `apps/web/hooks/domains/kanban/use-all-workflow-snapshots.ts`, capture the
|
||||
task at request start and compare it with the current cached task.
|
||||
- Copy the current executor bundle when the task changed after request start or
|
||||
when the task first appeared during the request. Let a full response win
|
||||
when no live executor change occurred.
|
||||
|
||||
## Tests
|
||||
|
||||
- **`mergeKanbanTasks` preserves known executor fields on a same/newer-timestamp
|
||||
merge that omits them:** add a test to
|
||||
`apps/web/lib/state/hydration/hydrator-kanban-tasks.test.ts` that hydrates a task with all
|
||||
four executor fields populated, then hydrates again with the same `id`, an
|
||||
equal `updatedAt`, and no executor fields on the incoming task; assert the
|
||||
merged task in `draft.kanban.tasks` still has the original executor field
|
||||
values.
|
||||
- **`mergeKanbanTasks` still adopts a legitimately different executor value:**
|
||||
add a test in the same file asserting that when the second hydration's
|
||||
incoming task carries a different `primaryExecutorType` (and the other three
|
||||
fields), the merged task adopts the new values rather than keeping the old
|
||||
ones.
|
||||
- **Snapshot merge preserves known executor fields when a fresh response omits
|
||||
them:** add a test to
|
||||
`apps/web/hooks/domains/kanban/use-all-workflow-snapshots.test.ts`, modeled
|
||||
on the existing `"preserves a cached autopilot marker when a fresh snapshot
|
||||
omits it"` test, asserting the merged snapshot task keeps its cached
|
||||
`primaryExecutorId`/`primaryExecutorType`/`primaryExecutorName`/
|
||||
`isRemoteExecutor` when the fresh response's task omits all four.
|
||||
- **Snapshot merge preserves a live clear over an explicit stale bundle:** add
|
||||
a deferred-response test where the request starts with executor A, a live
|
||||
event clears it, and the response returns executor A.
|
||||
- **Snapshot merge preserves a task added during the request:** add a
|
||||
deferred-response test where the task is absent at request start, a live
|
||||
event adds executor B, and the response omits executor fields.
|
||||
- **`preserveOmittedExecutorFields` unit coverage:** add direct test cases in
|
||||
`apps/web/lib/kanban/map-task.test.ts` (or a colocated test if the helper
|
||||
moves) covering: incoming omits all four fields (preserve fires), incoming
|
||||
carries a real `primaryExecutorType` (preserve does not fire, incoming wins).
|
||||
|
||||
## Task Breakdown
|
||||
|
||||
- **Wave 1** (sequential; task 02 imports the helper task 01 adds):
|
||||
- `task-01-preserve-helper-and-hydrator-fix.md` — add
|
||||
`preserveOmittedExecutorFields` and wire it into `mergeKanbanTasks`.
|
||||
- `task-02-snapshot-merge-fix.md` — wire the same helper into
|
||||
`use-all-workflow-snapshots.ts`.
|
||||
|
||||
## Verification
|
||||
|
||||
```bash
|
||||
cd apps/web
|
||||
pnpm exec vitest run lib/state/hydration/hydrator.test.ts lib/state/hydration/hydrator-kanban-tasks.test.ts lib/kanban/map-task.test.ts hooks/domains/kanban/use-all-workflow-snapshots.test.ts
|
||||
pnpm run typecheck
|
||||
```
|
||||
|
||||
## Risks
|
||||
|
||||
- Gating omission detection on `primaryExecutorType` is a deliberate
|
||||
workaround for `toKanbanTask`'s lossy `?? false` mapping.
|
||||
- Snapshot reconciliation copies the complete current bundle when a live
|
||||
change is proven. This includes explicit clears and tasks added in flight.
|
||||
- Hydration keeps the mapped-shape limitation because it has no request-start
|
||||
snapshot or raw payload presence data.
|
||||
+140
@@ -0,0 +1,140 @@
|
||||
---
|
||||
id: "01-preserve-helper-and-hydrator-fix"
|
||||
title: "Add executor-fields preserve helper and fix mergeKanbanTasks"
|
||||
status: done
|
||||
wave: 1
|
||||
depends_on: []
|
||||
plan: "plan.md"
|
||||
spec: "../../specs/kanban-task-executor-cache-staleness/spec.md"
|
||||
---
|
||||
|
||||
# Task 01: Add executor-fields preserve helper and fix mergeKanbanTasks
|
||||
|
||||
Add a shared `preserveOmittedExecutorFields` helper and wire it into
|
||||
`mergeKanbanTasks`'s wholesale-replace branch so a same/newer-timestamp merge
|
||||
that omits the executor fields does not blank out an already-known executor
|
||||
binding in the kanban cache.
|
||||
|
||||
## Acceptance
|
||||
|
||||
- `apps/web/lib/kanban/map-task.ts` exports
|
||||
`preserveOmittedExecutorFields(merged: KanbanTask, existing: KanbanTask):
|
||||
void` that copies `primaryExecutorId`, `primaryExecutorType`,
|
||||
`primaryExecutorName`, and `isRemoteExecutor` from `existing` onto `merged`
|
||||
only when `merged.primaryExecutorType === undefined`.
|
||||
- `mergeKanbanTasks`'s `incomingTime >= existingTime` branch in
|
||||
`apps/web/lib/state/hydration/hydrator.ts` calls this helper before
|
||||
`draftTasks[idx] = incoming`, so a merge that omits the executor fields keeps
|
||||
the cached values instead of losing them.
|
||||
- A merge whose incoming task legitimately carries a different
|
||||
`primaryExecutorType` still adopts the new value(s) — the helper is a
|
||||
gap-fill, not a sticky override.
|
||||
- `toKanbanTask`'s `is_remote_executor ?? false` mapping and the pinned
|
||||
`"defaults isRemoteExecutor to false when missing"` test in
|
||||
`apps/web/lib/kanban/map-task.test.ts` are unchanged.
|
||||
|
||||
## Files likely touched
|
||||
|
||||
- `apps/web/lib/kanban/map-task.ts`
|
||||
- `apps/web/lib/kanban/map-task.test.ts`
|
||||
- `apps/web/lib/state/hydration/hydrator.ts`
|
||||
- `apps/web/lib/state/hydration/hydrator.test.ts`
|
||||
|
||||
## Dependencies
|
||||
|
||||
None.
|
||||
|
||||
## Parallelism
|
||||
|
||||
`sequential`. Task 02 imports the helper this task adds.
|
||||
|
||||
## Inputs
|
||||
|
||||
- Spec sections **Desired behavior**, **Regression scenarios**, and
|
||||
**Constraints**.
|
||||
- `apps/web/lib/ws/handlers/tasks.ts:40-61` (`preservePrimaryExecutorFields`) as
|
||||
the reference behavior for the WebSocket path — do not modify this file; it
|
||||
already handles its own (raw-payload) input shape correctly.
|
||||
- `apps/web/lib/state/hydration/hydrator.ts`'s existing
|
||||
`backfillServerDerivedFields` / `SERVER_DERIVED_TASK_FIELDS` as the existing
|
||||
in-file precedent for a gap-fill-only guard.
|
||||
|
||||
## TDD sequence
|
||||
|
||||
1. In `apps/web/lib/state/hydration/hydrator.test.ts`, add a failing test:
|
||||
hydrate a task with all four executor fields populated via `hydrateState`,
|
||||
then hydrate again with the same `id`, an equal `updatedAt`, and no executor
|
||||
fields on the incoming task payload; assert the resulting
|
||||
`draft.kanban.tasks` entry still has the original executor field values.
|
||||
Confirm it fails (the fields come back `undefined`/`false`) before touching
|
||||
production code.
|
||||
2. Add a second test asserting that when the second hydration's incoming task
|
||||
carries a genuinely different `primaryExecutorType` (plus its sibling
|
||||
fields), the merged task adopts the new values.
|
||||
3. Add direct unit tests for `preserveOmittedExecutorFields` in
|
||||
`apps/web/lib/kanban/map-task.test.ts`: incoming omits all four fields
|
||||
(preserve fires); incoming carries a real `primaryExecutorType` (preserve
|
||||
does not fire).
|
||||
4. Implement `preserveOmittedExecutorFields` in `map-task.ts` and wire it into
|
||||
`mergeKanbanTasks` in `hydrator.ts`. Keep the change minimal — no changes to
|
||||
`toKanbanTask` itself.
|
||||
5. Re-run the tests from step 1-3 and confirm they pass.
|
||||
|
||||
## Verification
|
||||
|
||||
```bash
|
||||
cd apps/web
|
||||
pnpm exec vitest run lib/state/hydration/hydrator.test.ts lib/state/hydration/hydrator-kanban-tasks.test.ts lib/kanban/map-task.test.ts
|
||||
pnpm run typecheck
|
||||
```
|
||||
|
||||
## Risks
|
||||
|
||||
- Gating on `primaryExecutorType` rather than `isRemoteExecutor` itself is
|
||||
intentional (see plan **Risks**); do not "simplify" this to a per-field
|
||||
`undefined` loop, since that silently fails to preserve `isRemoteExecutor`
|
||||
(it is never `undefined` post-mapping).
|
||||
- Do not extend the preserve to distinguish an explicit executor clear from an
|
||||
omitted field — that disambiguation is out of scope (see spec **Constraints**
|
||||
and **Out of scope**).
|
||||
|
||||
## Output contract
|
||||
|
||||
Report the RED failure, GREEN result, exact files changed, and any deviation
|
||||
from this task file. Record every exact command and outcome in `## Results`.
|
||||
|
||||
## Results
|
||||
|
||||
Added `preserveOmittedExecutorFields` (exported from `apps/web/lib/kanban/map-task.ts`,
|
||||
gated on `primaryExecutorType`'s own `undefined`-ness per the plan's rationale)
|
||||
and wired it into `mergeKanbanTasks`'s wholesale-replace branch in
|
||||
`apps/web/lib/state/hydration/hydrator.ts`, right before `draftTasks[idx] =
|
||||
incoming`.
|
||||
|
||||
- RED: `pnpm exec vitest run lib/kanban/map-task.test.ts` — 2 new tests failed
|
||||
with `TypeError: preserveOmittedExecutorFields is not a function`. Then
|
||||
`pnpm exec vitest run lib/state/hydration/hydrator.test.ts` — the
|
||||
same-timestamp-omits-executor-fields test failed, asserting the merged task
|
||||
lost its executor fields (matched the bug exactly); the
|
||||
legitimately-different-value test passed even before the fix, as expected.
|
||||
- GREEN: same two commands after implementing the helper and wiring it in —
|
||||
all tests passed.
|
||||
- Deviation from the task file: the new `hydrator.test.ts` tests were moved
|
||||
into a new sibling file `apps/web/lib/state/hydration/hydrator-kanban-tasks.test.ts`
|
||||
instead of staying inline, because adding them to `hydrator.test.ts` pushed
|
||||
it from 649 to 663 lines, over the repo's 600-line file limit (already
|
||||
pre-existing at 649, worsened by the addition). Similarly, the new
|
||||
`map-task.test.ts` `describe("preserveOmittedExecutorFields", ...)` block
|
||||
was made a top-level sibling describe instead of nesting inside `"toKanbanTask
|
||||
— state normalization"`, which had pushed that arrow function past the
|
||||
100-line function limit.
|
||||
- Full verification: `pnpm exec vitest run lib/kanban/map-task.test.ts
|
||||
lib/state/hydration/hydrator.test.ts lib/state/hydration/hydrator-kanban-tasks.test.ts`
|
||||
— 48 passed. `pnpm exec eslint lib/kanban/map-task.ts lib/kanban/map-task.test.ts
|
||||
lib/state/hydration/hydrator.ts lib/state/hydration/hydrator.test.ts
|
||||
lib/state/hydration/hydrator-kanban-tasks.test.ts` — clean, no warnings.
|
||||
`pnpm run typecheck` — clean.
|
||||
|
||||
Files changed: `apps/web/lib/kanban/map-task.ts`, `apps/web/lib/kanban/map-task.test.ts`,
|
||||
`apps/web/lib/state/hydration/hydrator.ts`, `apps/web/lib/state/hydration/hydrator.test.ts`
|
||||
(tests moved out), `apps/web/lib/state/hydration/hydrator-kanban-tasks.test.ts` (new).
|
||||
@@ -0,0 +1,104 @@
|
||||
---
|
||||
id: "02-snapshot-merge-fix"
|
||||
title: "Preserve executor fields in the workflow snapshot merge"
|
||||
status: done
|
||||
wave: 1
|
||||
depends_on: ["01-preserve-helper-and-hydrator-fix"]
|
||||
plan: "plan.md"
|
||||
spec: "../../specs/kanban-task-executor-cache-staleness/spec.md"
|
||||
---
|
||||
|
||||
# Task 02: Preserve executor fields in the workflow snapshot merge
|
||||
|
||||
Wire request-start executor freshness into `useAllWorkflowSnapshots`'s
|
||||
per-task snapshot merge. A stale workflow response must not blank out or
|
||||
restore an executor binding changed by a live event in `kanbanMulti.snapshots`.
|
||||
|
||||
## Acceptance
|
||||
|
||||
- In `apps/web/hooks/domains/kanban/use-all-workflow-snapshots.ts`'s
|
||||
`fetchAndWriteSnapshot`, compare the current task with its request-start
|
||||
task before applying the HTTP response.
|
||||
- Copy all four current executor fields when the task changed after request
|
||||
start or first appeared during the request.
|
||||
- A fresh snapshot response whose task omits all four executor fields keeps the
|
||||
cached task's executor field values in the merged snapshot.
|
||||
- A fresh snapshot response whose task carries a genuinely different
|
||||
`primaryExecutorType` (plus its sibling fields) still adopts the new values.
|
||||
|
||||
## Files likely touched
|
||||
|
||||
- `apps/web/hooks/domains/kanban/use-all-workflow-snapshots.ts`
|
||||
- `apps/web/hooks/domains/kanban/use-all-workflow-snapshots.test.ts`
|
||||
|
||||
## Dependencies
|
||||
|
||||
Task 01 (`preserveOmittedExecutorFields` must exist in `map-task.ts` first).
|
||||
|
||||
## Parallelism
|
||||
|
||||
`sequential`.
|
||||
|
||||
## Inputs
|
||||
|
||||
- Spec sections **Desired behavior** and **Regression scenarios**.
|
||||
- The existing `"preserves a cached autopilot marker when a fresh snapshot
|
||||
omits it"` and `"keeps an explicit false autopilot value from the fresh
|
||||
snapshot"` tests in `use-all-workflow-snapshots.test.ts` as the pattern to
|
||||
follow (mock `fetchWorkflowSnapshot`, seed `kanbanMulti.snapshots`, assert on
|
||||
`mockSetWorkflowSnapshot`'s recorded call).
|
||||
|
||||
## TDD sequence
|
||||
|
||||
1. Add a failing deferred-response test where a live detach clears the cached
|
||||
executor and the stale response returns the old explicit executor bundle.
|
||||
2. Add a failing deferred-response test where a task first appears during the
|
||||
request with live executor data and the stale response omits that data.
|
||||
3. Add a complete-copy helper and wire it into the snapshot merge.
|
||||
4. Re-run the tests from steps 1-2 and confirm they pass.
|
||||
|
||||
## Verification
|
||||
|
||||
```bash
|
||||
cd apps/web
|
||||
pnpm exec vitest run hooks/domains/kanban/use-all-workflow-snapshots.test.ts hooks/domains/kanban/use-all-workflow-snapshots-inflight.test.ts
|
||||
pnpm run typecheck
|
||||
```
|
||||
|
||||
## Risks
|
||||
|
||||
- `mapSnapshotTask` returns `null` for tasks outside `stepIds`; make sure the
|
||||
new test's task is on a step included in the mocked snapshot's `steps` so it
|
||||
survives the existing filter before the preserve logic is reached.
|
||||
|
||||
## Output contract
|
||||
|
||||
Report the RED failure, GREEN result, exact files changed, and any deviation
|
||||
from this task file. Record every exact command and outcome in `## Results`.
|
||||
|
||||
## Results
|
||||
|
||||
The merge now uses request-start freshness for the complete executor bundle.
|
||||
When the live cache changes during the fetch, or when a task appears while the
|
||||
fetch is in flight, the current bundle wins. When it does not change, the
|
||||
snapshot remains authoritative and can clear the executor.
|
||||
|
||||
- RED: `pnpm exec vitest run
|
||||
hooks/domains/kanban/use-all-workflow-snapshots.test.ts` — both new race
|
||||
regressions failed. A stale snapshot restored the old executor after a live
|
||||
detach, and a task added during the fetch lost its live executor.
|
||||
- GREEN: the same command after the fix — all 20 tests passed.
|
||||
- Focused verification: `pnpm exec vitest run
|
||||
lib/kanban/map-task.test.ts lib/state/hydration/hydrator.test.ts
|
||||
lib/state/hydration/hydrator-kanban-tasks.test.ts
|
||||
hooks/domains/kanban/use-all-workflow-snapshots.test.ts
|
||||
hooks/domains/kanban/use-all-workflow-snapshots-inflight.test.ts` — 80
|
||||
tests passed.
|
||||
- `pnpm exec eslint` passed for all touched TypeScript files. `pnpm run
|
||||
typecheck` passed.
|
||||
- Updated the spec and implementation plan to document the request-start
|
||||
freshness rule and the separate hydration gap-fill rule.
|
||||
|
||||
Files changed: `apps/web/hooks/domains/kanban/use-all-workflow-snapshots.ts`,
|
||||
`apps/web/hooks/domains/kanban/use-all-workflow-snapshots.test.ts`,
|
||||
`apps/web/lib/kanban/map-task.ts`, and the related spec and plan files.
|
||||
@@ -304,6 +304,7 @@ System pages (Radarr/Sonarr-style) for status, disk usage, database maintenance,
|
||||
| [quick-chat-idle-dot](quick-chat-idle-dot/spec.md) | draft |
|
||||
| [native-code-review](native-code-review/spec.md) | building |
|
||||
| [missing-task-route-recovery](missing-task-route-recovery/spec.md) | draft |
|
||||
| [kanban-task-executor-cache-staleness](kanban-task-executor-cache-staleness/spec.md) | draft |
|
||||
| [browser-inspect-annotations-save](browser-inspect-annotations-save/spec.md) | shipped |
|
||||
| [automations-pr-merged-trigger](automations-pr-merged-trigger/spec.md) | draft |
|
||||
| [automation-runs-delete-all-by-status](automation-runs-delete-all-by-status/spec.md) | draft |
|
||||
|
||||
@@ -0,0 +1,140 @@
|
||||
---
|
||||
title: Kanban task cache preserves executor fields across merges
|
||||
status: draft
|
||||
created: 2026-08-15
|
||||
owner: kandev
|
||||
---
|
||||
|
||||
# Kanban task cache preserves executor fields across merges
|
||||
|
||||
## Problem
|
||||
|
||||
The kanban task cache learns a task's primary executor (`primaryExecutorId`,
|
||||
`primaryExecutorType`, `primaryExecutorName`, `isRemoteExecutor`) from whichever
|
||||
task payload arrives with it populated. `primary_executor_*` are `omitempty` on
|
||||
the backend task DTO and are derived per read from the primary session's
|
||||
executor binding (`apps/backend/internal/task/dto/dto.go:162-164,180`,
|
||||
`apps/backend/internal/task/service/service_events.go:566-582`); binding an
|
||||
executor updates only the `task_sessions` row, not the owning task's
|
||||
`updated_at` (`apps/backend/internal/task/repository/sqlite/session.go:741-743,
|
||||
907-921`).
|
||||
|
||||
Two of the three client code paths that merge a fresh task payload into the
|
||||
cache already guard the resulting gap explicitly — an `omitempty` field can be
|
||||
absent on a payload that is otherwise the newest or an equal-timestamp
|
||||
duplicate, and an absent key must not erase an already-known value:
|
||||
|
||||
- `apps/web/lib/ws/handlers/tasks.ts:40-61` (`preservePrimaryExecutorFields`,
|
||||
keyed off raw WebSocket payload key presence).
|
||||
- `apps/web/lib/state/hydration/hydrator.ts`'s `backfillServerDerivedFields`
|
||||
guards five dependency-projection fields the same way, but only on its
|
||||
"incoming is older" branch, and does not cover executor fields.
|
||||
|
||||
Two merge sites have no such guard at all for the executor fields:
|
||||
|
||||
- `apps/web/lib/state/hydration/hydrator.ts`'s `mergeKanbanTasks`, on the
|
||||
"incoming is newer-or-equal" branch, replaces the cached task object
|
||||
wholesale (`draftTasks[idx] = incoming`) with no gap-fill.
|
||||
- `apps/web/hooks/domains/kanban/use-all-workflow-snapshots.ts`'s per-workflow
|
||||
snapshot merge preserves `primarySessionId`, `primarySessionState`,
|
||||
`autopilot`, and `statusSummary` when a fresh response omits them, but has no
|
||||
equivalent guard for any of the four executor fields.
|
||||
|
||||
Because `mergeKanbanTasks` compares `updatedAt` with `>=` (not `>`), and
|
||||
because an executor binding never bumps the task row's `updated_at`, a stale or
|
||||
same-timestamp response that lands after a `task.updated` WebSocket event
|
||||
already populated the executor fields silently blanks them back out in the
|
||||
cache. Kanban card executor badges, the task switcher, and
|
||||
`getWorkspaceSourceCapabilities`-gated affordances (e.g. the workspace "Add
|
||||
folder" action, which needs a known non-`undefined` executor type to render)
|
||||
regress to their unknown-executor state even though the executor is already
|
||||
known.
|
||||
|
||||
`isRemoteExecutor` needs its own handling: `toKanbanTask`
|
||||
(`apps/web/lib/kanban/map-task.ts:199`) maps the wire's `is_remote_executor`
|
||||
with `?? false`, so the mapped field is never `undefined` — unlike its three
|
||||
sibling executor fields, which map with `?? undefined`. A plain
|
||||
"preserve-when-`undefined`" rule therefore cannot detect that
|
||||
`isRemoteExecutor` was actually omitted from a given payload. `is_remote_executor`
|
||||
is only ever emitted alongside `primary_executor_type` (both are derived from
|
||||
the same executor snapshot; see `service_events.go:566-582` and
|
||||
`dto.go:775-782`), so `primaryExecutorType`'s own `undefined`-ness is the
|
||||
reliable signal for whether the whole four-field executor bundle was omitted
|
||||
from a given payload.
|
||||
|
||||
## Desired behavior
|
||||
|
||||
- When `mergeKanbanTasks` takes the "incoming is newer-or-equal" branch for a
|
||||
task already present in the cache, and the incoming task's
|
||||
`primaryExecutorType` is `undefined`, the merged task keeps the existing
|
||||
cached `primaryExecutorId`, `primaryExecutorType`, `primaryExecutorName`, and
|
||||
`isRemoteExecutor` values instead of losing them to the replace.
|
||||
- When the incoming task's `primaryExecutorType` is defined (a real value, from
|
||||
a payload that legitimately carries executor state), the incoming value(s)
|
||||
win outright — this is not a sticky/permanent preserve, only a gap-fill for a
|
||||
payload that omits the field.
|
||||
- In `use-all-workflow-snapshots.ts`, a current cached executor bundle wins
|
||||
when it changed after the snapshot request started, even when the response
|
||||
contains an older explicit executor bundle.
|
||||
- When the executor bundle did not change after the request started, the full
|
||||
snapshot is authoritative. An omitted executor bundle can clear the cache.
|
||||
- When a task first appears while the request is active, its current cached
|
||||
executor bundle wins over a response captured before that live event.
|
||||
- Every other field's existing merge behavior (timestamp comparison, the
|
||||
dependency-projection backfill on the older-incoming branch, unrelated
|
||||
fields' wholesale replace) is unchanged.
|
||||
|
||||
## Regression scenarios
|
||||
|
||||
- **GIVEN** a cached kanban task whose executor is already known (all four
|
||||
executor fields populated), **WHEN** a `mergeKanbanTasks` merge receives an
|
||||
incoming task with the same `id`, an `updatedAt` equal to or newer than the
|
||||
cached value, and no `primary_executor_*`/`is_remote_executor` keys in its
|
||||
source payload, **THEN** the merged task keeps its previously known executor
|
||||
fields instead of losing them to the wholesale replace.
|
||||
- **GIVEN** the same cached task, **WHEN** a `mergeKanbanTasks` merge receives
|
||||
an incoming task at or after the cached `updatedAt` whose payload legitimately
|
||||
carries a different executor type, **THEN** the merged task adopts the new
|
||||
executor value(s); the preserve never overrides a real value.
|
||||
- **GIVEN** a workflow snapshot response whose task list omits the executor
|
||||
fields for a task the cache already knows the executor for, **WHEN**
|
||||
`useAllWorkflowSnapshots` merges that response, **THEN** the merged snapshot
|
||||
task keeps the cached executor fields.
|
||||
- **GIVEN** a cached task whose executor is known at request start, **WHEN** a
|
||||
live event clears the executor and a stale response returns the old executor
|
||||
bundle, **THEN** the merged task stays clear.
|
||||
- **GIVEN** a task that is absent at request start, **WHEN** a live event adds
|
||||
the task with executor data and a stale response omits that data, **THEN**
|
||||
the merged task keeps the live executor bundle.
|
||||
|
||||
## Constraints
|
||||
|
||||
- No backend, API, or persistence contract changes; this is a client-side cache
|
||||
merge correctness fix using data already delivered on the wire.
|
||||
- Do not change `toKanbanTask`'s existing `is_remote_executor ?? false`
|
||||
mapping or its pinned `map-task.test.ts` contract test — the merge-layer fix
|
||||
works around that mapping's loss of "omitted" information for
|
||||
`isRemoteExecutor` by gating on `primaryExecutorType`'s own `undefined`-ness
|
||||
instead, rather than changing the shared mapper's output contract.
|
||||
- The snapshot merge uses the request-start cache as its freshness boundary.
|
||||
It copies the complete current executor bundle when the task changed after
|
||||
request start or first appeared during the request. It does not use omission
|
||||
alone as proof of a stale response.
|
||||
- The hydration merge still uses the mapped `KanbanTask` shape. It cannot
|
||||
distinguish an explicit clear from an omitted key in that path, so it keeps
|
||||
the existing gap-fill rule and its known limitation.
|
||||
|
||||
## Out of scope
|
||||
|
||||
- Changing `toKanbanTask` / `map-task.ts`'s field mapping.
|
||||
- Adding a "session detached" disambiguation signal to the mapped `KanbanTask`
|
||||
shape or to either merge site.
|
||||
- Any change to `apps/web/lib/ws/handlers/tasks.ts`'s existing
|
||||
`preservePrimaryExecutorFields`, which already handles the WebSocket path
|
||||
correctly.
|
||||
- The `getWorkspaceSourceCapabilities` disabled/hidden-state UI copy itself;
|
||||
this fix addresses the cache staleness that feeds it, not its rendering.
|
||||
|
||||
## Implementation plans
|
||||
|
||||
- [Kanban task cache preserves executor fields across merges](../../plans/kanban-task-executor-cache-staleness/plan.md)
|
||||
Reference in New Issue
Block a user