Merge 137-kanban-guards-and-watcher-branches -> main #2

Merged

Mark merged 4 commits from 137-kanban-guards-and-watcher-branches into main

2026-09-12 16:51:50 +00:00

Mark commented

2026-09-12 16:50:57 +00:00

Owner

Auto-opened by batch merger to land 137-kanban-guards-and-watcher-branches into main.

Mark added 4 commits

2026-09-12 16:50:57 +00:00

Harden webhook→kanban transition guards 606505ec44

Implements the four-guard state model (plan §1, Vikunja #137,
kanban t_c657a9cc) at the Mark/kanban transition layer:

* Add terminal + owner columns to the tasks schema with persistence
  (already on the branch from t_d11f22e0 — extended here with the
  guard wiring that consumes them).
* Add setTaskFlags() storage method and PATCH /api/tasks/:id/flags
  route for non-transition flag mutation (already on the branch —
  this commit extends with the transition-guard wiring).
* Add transition_guards.ts: pure-logic guard module with typed
  TransitionGuardError. Rules enforced:
    1. Any transition into a done state (validated, archived) sets
       terminal=1.
    2. Any transition into a running state (executing, executed) by
       the watcher sets owner=watcher.
    3. Any transition out of a done state is rejected with
       kind=done_is_terminal, regardless of source webhook / caller.
    4. Any transition into a running state from a non-watcher is
       rejected with kind=foreign_running_write. Missing/empty
       caller → kind=missing_caller_for_running.
* Wire the guard into storage.transitionTask() — runs BEFORE the
  ALLOWED_TRANSITIONS check so a webhook targeting a done-state exit
  surfaces as the typed 409 guard error, not a generic 400.
* Wire the guard's flag updates into the same UPDATE as the state
  transition so the terminal latch and owner claim are atomic.
* Surface TransitionGuardError at the HTTP layer as 409 (Conflict)
  with kind + task_id + from_state + to_state in the body. Other
  errors stay 400.

Acceptance tests (card body, all four covered):
  (a) done→active rejected          — storage.test.ts (a) tests
  (b) non-watcher→running rejected   — storage.test.ts (b) tests
  (c) watcher→running sets owner     — storage.test.ts (c) test
  (d) any→done sets terminal=true    — storage.test.ts (d) tests

Plus 23 pure-logic guard tests in transition_guards.test.ts, plus
HTTP surface tests (409 vs 400 precedence) in routes.test.ts.

Verification: 116 tests pass (was 80). Migration test exercises
the on-disk runner against a legacy DB and confirms safe defaults.

Sibling work tracked separately:
- t_383734ad (Refactor watcher loop into three named branches)
- t_7fd87e14 (verifier — opens PR after this lands)

Add four-guard integration tests + watcher abstraction + kanban_unblock surface 9db150aead

Implements the four named test cases from the parent design task spec
(plan §1 / Vikunja#137, kanban t_fc944a5a):

  (a) webhook→done is terminal across N watcher ticks
      - Done card never re-arms across 5 sweep ticks: not in watch set,
        no events dispatched, terminal=true persisted, updated_at unchanged.
      - Mirrors the spec's rule 1 (done is done) and rule 4 (no
        re-watching of done cards).

  (b) blocked card ignores every event except kanban_unblock
      - Iterates the full KANBAN_EVENT_CATALOG; only kanban_unblock
        re-subscribes the card. Every other event is a no-op on the
        watch set and the storage row.
      - Storage-layer mirror: transitionTask() rejects blocked→active
        with a typed error referencing kanbanUnblock(); kanbanUnblock()
        is the SOLE handler that flips blocked→assigned.
      - kanbanUnblock() also rejects non-blocked cards with a typed
        error explaining the precondition.

  (c) running rejects transitions from any non-watcher caller
      - Forged HTTP: caller=webhook → executing rejected with
        kind=foreign_running_write.
      - Other-lane identities (dispatcher, janitor, webhook, user,
        operator, forged-http-attacker) all rejected.
      - Missing/null/empty caller rejected with kind=missing_caller_for_running.
      - Same-shape call from caller=watcher succeeds and sets owner=watcher.
      - Both running destinations (executing AND executed) covered.

  (d) after a sweep, done cards do not appear in the watch list
      - Mixed done/active/blocked fixture; sweep drops done, drops
        blocked, dispatches watcher-owned running, rejects foreign-owned
        running (defence-in-depth).
      - Done cards never re-subscribe on subsequent sweeps (5 ticks).
      - kanban_unblock re-subscribes a previously-dropped blocked card
        (the only re-add path).

New code:
  - server/watcher.ts: the in-process Watcher class with Sweep() and
    deliverKanbanEvent(). Three branches per the spec: drop_done,
    drop_blocked, dispatch_watcher_process. Foreign-owner running cards
    are rejected with WatcherError (defence-in-depth).
  - server/storage.ts: kanbanUnblock() storage method (sole handler for
    blocked→assigned). transitionTask() now rejects blocked→active with
    a typed error mentioning kanbanUnblock() so the gate is enforced at
    the storage layer in addition to the guard layer.
  - server/__tests__/four_guards_integration.test.ts: 13 new tests
    covering all four named cases end-to-end.

Test summary: 129 passed (was 116); +13 tests, no regressions.

Refs: kanban t_fc944a5a, plan §1 / Vikunja#137

Fix HTTP trust boundary for four-guard state model (t_c657a9cc) 53f5565621

Round-1 review flagged that the HTTP routes trusted the body's
self-attested caller field: a forged request that posts
{"to_state":"executing","caller":"watcher"} slipped past the guard
with owner=watcher written to the card. Same gap on PATCH /flags
which accepted owner from the body with no auth.

The four-guard state model is correct at the guard layer. The defect
was upstream: the HTTP boundary had no notion of caller identity and
trusted the body's self-attestation. This commit closes that
boundary.

Fix:
- shared/schema.ts: drop caller from insertTransitionSchema. The body
  field is now silently stripped by zod (unknown keys are ignored,
  not rejected, so the change is non-breaking for clients).
- New server/principal.ts: deriveCallerFromRequest(req) reads the
  caller identity from the X-Internal-Actor header (closed set:
  "watcher" only today) and defaults to "unknown".
  deriveOwnerFromRequest applies the same rule to PATCH /flags'
  owner field. The module documents the remaining auth gap
  explicitly so the next reader does not mistake "header-derived
  caller" for "authenticated caller".
- server/routes.ts:
  * POST /api/tasks/:task_id/transition: caller is now derived from
    the header, not from parsed.caller.
  * PATCH /api/tasks/:task_id/flags: owner is server-derived (or
    undefined - body field is dropped regardless of value).
- server/storage.ts: replace the dynamic await import(...) of
  transition_guards (style nit from the review) with a top-level
  ESM import - no circular dep risk, easier to mock and audit.

Tests (24 new tests, 153 total):
- server/__tests__/principal.test.ts (16 tests): pure-logic coverage
  of the trust-boundary helper - missing header, empty/whitespace
  value, comma-joined duplicate (header smuggling), unknown
  principal, array path, case-different values (closed-set equality,
  not normalized), trimming.
- server/__tests__/routes.test.ts (8 new tests in 2 new describe
  blocks): forged-but-lies attack (body claims caller=watcher, route
  must pass caller=unknown to storage), default-deny without header,
  legitimate watcher via header, forged X-Internal-Actor value, body
  caller field ignored even with a valid header, PATCH /flags owner
  dropped without header, PATCH /flags owner set via header (body
  field ignored), PATCH /flags owner with bad type silently dropped.

Acceptance from the reviewer's required correction list - all
addressed:
1. Server-controlled caller identity at HTTP boundary. Yes.
2. Storage-layer caller parameter preserved. Yes.
3. Forged-but-lies test that the route rejects body-supplied
   caller=watcher. Yes (explicit assertion: expect(caller).toBe("unknown")
   inside the mock).
4. PATCH /flags fix: owner is server-derived; body field dropped. Yes.

Verification: 153/153 tests pass (was 129); typecheck clean (3
pre-existing multer strictness errors on origin/main unchanged).

Refactor watcher loop into three named branches (t_383734ad) a0bc081d9a

Refactor server/watcher.ts Sweep() into three named branch methods:
  - branch_done(card)      -> drop_done
  - branch_blocked(card)   -> drop_blocked
  - branch_running(card)   -> dispatch_watcher_process / abort

Branch 3 (running) now asserts card.owner === watcher at entry. Assertion
failure throws WatcherError{kind:'foreign_running_write'} which sweep()
catches per-card, so the tick for that card ABORTS (per card-body spec)
without silently continuing. Other cards in the same tick still process.

Test surface expanded with a new (e) describe block:
  - failing input (owner != watcher) -> assertion fires,
    abort_running_owner_assertion action emitted, watch set untouched
  - mixed-tick abort isolation: failing card's tick aborts but the
    good card in the same tick dispatches normally
  - null owner also triggers the assertion (defence-in-depth)
  - happy path (owner == watcher) dispatches without throwing

Acceptance:
  1. Three named branches clearly visible (branch_done/blocked/running
     at server/watcher.ts:137/150/170; sweep() routes by name at
     218/223/231).
  2. _watchSet.add happens ONLY in branch_running (line 181) and inside
     the 'kanban_unblock' arm of deliverKanbanEvent (line 287) — no
     timer, no sweep, no other event path may re-add a blocked card.
  3. Running-tick owner assertion: 4 new tests pin the failing-input
     surface.

Tests: 157/157 vitest pass (was 153; +4 new).
tsc: pre-existing routes.ts:22,218,231 errors unchanged; 0 new.