Files
stack/docs/reports/native-kanban-sot/canon-initial-review-no-go.md
jason.woltje 49e8a54105
All checks were successful
ci/woodpecker/push/publish Pipeline was successful
ci/woodpecker/push/ci Pipeline was successful
docs(#751): Publish native Kanban/SOT canon (#752)
2026-07-14 17:08:09 +00:00

358 lines
24 KiB
Markdown
Raw Blame History

This file contains ambiguous Unicode characters
This file contains Unicode characters that might be confused with other characters. If you think that this is intentional, you can safely ignore this warning. Use the Escape button to reveal them.
# Independent Review — Native Kanban/SOT Canon
**Reviewer:** `enhance-sol` (independent of author `planner-sol`)
**Date:** 2026-07-13
**Review mode:** design/contract only; read-only against the staged canon
**Source plan:** `/home/hermes/agent-work/planning/mosaic-native-kanban-sot-plan.md` (`sha256:96ea4fb91436ec9a53f371d27276e27f62ecf817662599ff9152df0db55296e5`)
**Canon reviewed:** every listed artifact under `/home/hermes/agent-work/planning/kanban-canon/`, including the four TypeScript contracts; the author scratchpad was also read as validation context.
## Executive verdict
# NO-GO
The canon is not freeze-ready. I found **8 BLOCKERs**, **7 MAJORs**, and **1 MINOR**. The prose preserves the ratified authority model well, but the frozen types/schema leave concrete fail-closed, approval, fencing, tenant, outage-proposal, migration, and parallelization gaps. Those gaps would force implementation lanes either to invent contract semantics or to ship paths that violate fixed invariants.
### Blocking findings
1. Health/write authorization can be represented as contradictory, stale, or caller-asserted state.
2. Coordinator failures collapse authoritative denial, unknown transport outcome, and version conflict into one permissive shape.
3. Assignment proposals and approval proofs have no authoritative relational binding; lease acquisition accepts a forgeable proof DTO.
4. Fencing uniqueness is present, but monotonic fencing and same-task lease/checkpoint binding are not.
5. Workspace-safe accountable-owner, assignment-principal, and evidence/artifact relationships are not frozen.
6. Attributable post-recovery proposals have neither a canonical table nor command contract.
7. The slice graph starts schema/UI work before prerequisite threat and exact API/DTO freezes and contradicts coder4 lane order.
8. P0 claims a migration map while publishing only generic rules; the concrete N-1 transition from current `origin/main` is absent.
---
## Findings
### KCR-001 — BLOCKER — “Healthy” is not a proof and can be contradictory or stale
**Location**
- `contracts/health-state.v1.ts:21-31``KanbanHealthResponseV1` permits every combination of `state`, `readHealthProven`, and `writeHealthProven`.
- `contracts/mechanical-coordinator.v1.ts:40-49``CoordinatorContextV1` accepts a caller-supplied `healthState` enum only.
- `contracts/mechanical-coordinator.v1.ts:255-293` — every Coordinator operation, including mutating operations, accepts that context.
- `SHARED-CONTRACT.md:171-184` — mutations are allowed only after live PostgreSQL read/write probes.
**Violation**
Fixed invariant 2 / `REQ-SOT-002`: mutations must fail closed unless write health is positively proven. The current type permits `{ state: 'healthy', writeHealthProven: false }`, and the Coordinator mutation boundary can be invoked with a stale or fabricated `{ healthState: 'healthy' }`. A Valkey/client-derived enum could therefore be mistaken for write authorization.
**Minimal fix**
1. Make `KanbanHealthResponseV1` a discriminated union with only these legal combinations: `healthy => read=true/write=true`, `read-only-degraded => read=true/write=false`, and `write-unavailable => read=false/write=false`.
2. Do not accept write authority from a public DTO. Require Gateway/domain code to obtain and revalidate a fresh internal PostgreSQL write-health proof at mutation time (including `checkedAt`, bounded validity/policy revision, and transaction-local enforcement).
3. Split pure evaluation context from mutation context; mutation methods must accept only an unforgeable/internal healthy context or perform the probe themselves.
4. Add negative contract tests for contradictory state, expired proof, Valkey-only liveness, and caller-forged `healthy`.
### KCR-002 — BLOCKER — Coordinator error shape can conflate denial, unknown outcome, and conflict
**Location**
- `contracts/mechanical-coordinator.v1.ts:184-216` — one `CoordinatorFailureV1` allows every code to pair with arbitrary `retryable` and either `requestOutcome` value.
- `contracts/health-state.v1.ts:51-106` — the Gateway health contract correctly distinguishes deliberate denial, transport uncertainty, and version conflict.
- `SHARED-CONTRACT.md:177-216` — frozen client semantics require those cases not to be conflated.
**Violation**
Charter E and `REQ-SOT-002`. The current Coordinator result can legally encode `WRITE_HEALTH_UNPROVEN` as `retryable: true, requestOutcome: 'unknown'`, or `VERSION_CONFLICT` as retryable. That permits blind retry or a false “unknown” outcome after an authoritative fail-closed denial.
**Minimal fix**
Replace `CoordinatorFailureV1` with a discriminated union keyed by code/kind:
- deliberate health denial: `not_applied`, `retryable:false`;
- version conflict: `not_applied`, `retryable:false`, current version;
- stale fence/session/eligibility/approval failures: exact non-retry semantics;
- transport failure: a separate `retryable_transport_error`, `unknown`, same idempotency key.
Reuse or map explicitly to `KanbanMutationFailureV1`, and add exhaustive client tests proving 503 authoritative bodies, 502/504/timeouts, and 409 cannot cross-map.
### KCR-003 — BLOCKER — Approval proof is forgeable and is not linked to the persisted proposal
**Location**
- `contracts/mechanical-coordinator.v1.ts:107-137` — proposal and approval DTOs.
- `contracts/mechanical-coordinator.v1.ts:265-270``acquireApprovedLease` accepts the entire `ApprovalProofV1` by value.
- `contracts/kanban-schema.v1.ts:650-688``task_assignments` has no proposal expiry, task version, session binding, or proposal/approval FK.
- `contracts/kanban-schema.v1.ts:823-863``approval_decisions` can target only a task or mission and has no proposal/assignment relation.
- `SHARED-CONTRACT.md:128-137` — lease acquisition requires authoritative approval under the exact policy revision.
**Violation**
Fixed invariant 5 and `REQ-COORD-002/003`. A caller can construct an `ApprovalProofV1`; the schema cannot prove that it belongs to the proposal, workspace, task version, agent/session, unexpired policy revision, or still-current approval. The DTO state vocabulary (`awaiting_approval | policy_pre_authorized`) also does not map directly to the persisted assignment states (`proposed | approved | ...`).
**Minimal fix**
Persist one authoritative proposal/assignment identity with task version, target agent/session, expiry, state, and policy revision. Add a workspace-aware approval relation to that identity. Change lease acquisition to accept IDs, then reload and lock proposal + approval + task inside PostgreSQL and verify workspace, current version, target session, state, expiry, and policy revision before creating the lease. Freeze one state vocabulary across schema and DTOs.
### KCR-004 — BLOCKER — Fencing is unique but not monotonically increasing; relational binding is incomplete
**Location**
- `contracts/kanban-schema.v1.ts:694-743``task_leases` has positive/unique fencing tokens but no monotonic per-task counter.
- `contracts/kanban-schema.v1.ts:748-784` — checkpoints independently carry task, lease, and fencing token.
- `contracts/kanban-schema.v1.ts:905-910` — token equality is deferred to prose; same-task lease binding is not stated.
- `contracts/mechanical-coordinator.v1.ts:140-175` — worker commands depend on fencing safety.
**Violation**
Fixed invariant 12 / `REQ-COORD-003`. Uniqueness permits token 10 followed by token 9. A lease can reference assignment A while naming task B in the same workspace, and a checkpoint can reference lease A while naming task B. `bigint(..., { mode: 'number' })` also eventually loses integer precision in JavaScript.
**Minimal fix**
Add a durable per-task fencing counter (or equivalent PostgreSQL sequence row) incremented atomically under task lock and use the returned value for every new lease. Add workspace-aware composite constraints tying lease to its exact task+assignment and checkpoint to exact task+lease+fence. Use bigint-safe representation (`bigint`/serialized decimal), and test monotonicity, concurrent claims, stale lower tokens, and mismatched same-workspace IDs.
### KCR-005 — BLOCKER — Hard tenant boundary is not frozen for several polymorphic relationships
**Location**
- `contracts/kanban-schema.v1.ts:315-318` and `475-478` — project/task accountable owners are unvalidated `(kind, text id)` pairs.
- `contracts/kanban-schema.v1.ts:659-663` — assignment principals are unvalidated `(kind, text id)` pairs.
- `contracts/kanban-schema.v1.ts:758` and `841` — checkpoint/evidence artifact relationships are JSON arrays without workspace-aware FKs.
- `SHARED-CONTRACT.md:89-96` — only selected polymorphic checks are delegated to domain transactions; owner/principal/evidence checks are not included.
- `REQUIREMENTS.md:101-108` — every relationship must reject cross-workspace IDs.
**Violation**
Fixed invariant 7 / `REQ-TEN-001` and `REQ-ID-001`. The frozen schema can name a team or agent from another workspace as owner/assignee, and can embed foreign-workspace artifact IDs in checkpoint or approval evidence arrays. A global user ID is also insufficient without active workspace membership validation.
**Minimal fix**
Use workspace-aware owner/assignment join tables or separate nullable user/team/agent columns with exactly-one checks and composite FKs where possible. Model checkpoint/evidence artifact links as workspace-scoped join rows, or freeze explicit transaction checks for every ID. Require active workspace membership for user principals and workspace-agent/session consistency for agent principals. Add DB/repository/API/Coordinator cross-workspace negative tests without existence oracles.
### KCR-006 — BLOCKER — Post-recovery outage proposals have no canonical persistence or command surface
**Location**
- `REQUIREMENTS.md:93-99` — proposal submission and authorized accept/reject are required.
- `SHARED-CONTRACT.md:26-29` — outage notes may return only as authenticated proposals.
- `SHARED-CONTRACT.md:243-267` — the thin command/query contract contains no proposal submit/get/accept/reject operations.
- `contracts/kanban-schema.v1.ts:1-916` — no proposal table captures proposed command, target/version, attribution, lifecycle, or decision.
- `TASKS.md:99-108` — KBN-110 does not own an outage-proposal command path.
**Violation**
Fixed invariant 4 / `REQ-SOT-004`. An implementation lane would have to invent storage or misuse artifacts/approval gates. Either path risks silently applying an outage note or creating shadow state.
**Minimal fix**
Add a workspace-scoped `change_proposals`/`outage_proposals` contract with authenticated proposer, source note digest, target aggregate, expected version, proposed typed command/payload, pending/accepted/rejected state, decision actor/reason/time, idempotency key, and audit linkage. Add explicit submit/query/accept/reject Gateway commands. Acceptance must execute the normal command in a healthy transaction; a proposal itself can never claim, order, satisfy a gate, or mutate the target.
### KCR-007 — BLOCKER — Parallel slice ordering is not freeze-safe and contains a direct lane-order contradiction
**Location**
- `TASKS.md:43-60` — dependency graph makes KBN-010 and KBN-100 siblings.
- `TASKS.md:88-97` — KBN-100 nevertheless depends on KBN-010 threat findings that alter constraints.
- `SHARED-CONTRACT.md:243` and `INDEX.md:44-50` — exact route names/DTO placement remain unresolved.
- `TASKS.md:110-130` — KBN-120/130 depend on a frozen endpoint/DTO contract, while mocks may begin before KBN-110 lands.
- `TASKS.md:145-153` — KBN-200 says lane-serial after KBN-120.
- `TASKS.md:248-254` — wave table runs KBN-200 before KBN-120.
**Violation**
Charter C and the mandatory freeze-before-parallelize gate. Schema can begin before tenant/threat findings are complete; web/CLI consumers have only semantic operations, not exact DTO/endpoint contracts; coder4 has two opposite legal orders. This does not create same-file edits immediately, but it guarantees contract invention or rework across active lanes.
**Minimal fix**
1. Make KBN-010 (or an explicit constraint-impact gate from it) a completed prerequisite of KBN-100.
2. Add a small serialized KBN-105 endpoint/DTO/endpoint-registry freeze, with exact request/response/error DTOs, before KBN-120 and KBN-130 implementation.
3. Choose one coder4 lane order and use it consistently in slice text, graph, and wave table.
4. Name the exact MCP-owned files or assign their Gateway changes to coder3 before coder4 starts.
### KCR-008 — BLOCKER — Claimed P0 migration map is absent; concrete N-1 hazards remain unresolved
**Location**
- `MISSION-MANIFEST.md:153-157` — P0 says to publish a migration map and states the build hold is lifted at line 3.
- `SHARED-CONTRACT.md:101-121` — only generic expand/backfill/contract rules are supplied.
- `contracts/kanban-schema.v1.ts:1-916` — target-state declarations reuse live table names and make target fields required.
- Current foundation evidence: `origin/main:packages/db/src/schema.ts:120-301` has no workspace keys, nullable project/mission links, legacy text status vocabularies, `tasks.tags`, `tasks.assignee`, `tasks.due_date`, mission JSON milestones/config, `mission_tasks.status`, and legacy agent fields.
**Violation**
Charter D / `REQ-MIG-001` and the P0 exit claim. The generic rule is correct, but coder2 lacks the required field-by-field transition map. A direct Drizzle reconciliation could attempt type narrowing/status conversion, add required workspace/project/owner columns too early, or drop legacy columns before N-1 readers and writers are retired.
**Minimal fix**
Publish a concrete current-main delta map before lifting the hold. For each existing table/column, specify expand, backfill, compatibility read/write, switch, and contract release. At minimum cover:
- nullable-first `workspace_id`, required project/owner fields, and workspace backfill;
- legacy task/project/mission status aliases or shadow columns before v1 emission;
- `mission_tasks.status` read retirement and write-source prohibition;
- mapping/retention for tags, assignee, due date, mission description/config/milestones, and agent fields;
- current milestone circular FK ordering;
- empty, production-shape, partial-resume, and rollback/downgrade tests already named in §4.
Explicitly require legacy columns to remain in the unified Drizzle declaration during the expand/N-1 window.
### KCR-009 — MAJOR — Dependency uniqueness permits parallel duplicate edges
**Location**
- `contracts/kanban-schema.v1.ts:561-567` — unique key includes `dependencyType`.
- `SHARED-CONTRACT.md:47-49` — calls for a unique directed edge.
- `REQUIREMENTS.md:142-149` — duplicate edge attempts must fail.
**Violation**
`REQ-DEP-001`. The same predecessor/successor pair can be inserted three times, once per dependency type. That is not a unique directed edge and complicates readiness semantics.
**Minimal fix**
Make `(workspace_id, predecessor_task_id, successor_task_id)` unique independent of type, or explicitly redefine the requirement as one edge per type and freeze deterministic multi-edge completion semantics. The source plan says unique directed edge, so the former is the minimal faithful fix.
### KCR-010 — MAJOR — Same-workspace planning relationships can contradict the project hierarchy
**Location**
- `contracts/kanban-schema.v1.ts:325``projects.currentMilestoneId` has no FK in the declaration.
- `contracts/kanban-schema.v1.ts:427-448` — mission/milestone association checks workspace but not common project.
- `contracts/kanban-schema.v1.ts:490-516` — a tasks project, mission, milestone, and parent only need share a workspace, not a project.
- `contracts/kanban-schema.v1.ts:905-908` — only current milestone is mentioned as a deferred invariant.
**Violation**
`REQ-PLAN-001` and schema correctness. A task in project A can point to a mission/milestone/parent task from project B in the same workspace. A mission can associate a milestone from another project despite having one required project.
**Minimal fix**
Add project-congruent composite keys/FKs (or freeze mandatory transaction checks) for task→mission, task→milestone, task→parent, mission→milestone, and project→current milestone. Add same-workspace/same-project negative tests.
### KCR-011 — MAJOR — Immutable/append-only records can be erased by parent cascades
**Location**
- `contracts/kanban-schema.v1.ts:798-818``task_events` is described as append-only but remains under a workspace cascade.
- `contracts/kanban-schema.v1.ts:911` — only application-role UPDATE/DELETE privilege removal is stated.
- Numerous canonical relationships use `onDelete('cascade')`, including workspace roots and artifact/checkpoint/event owners.
- `REQUIREMENTS.md:41-43` and `154-170` — audit must be append-only, attributable, and reconstructable.
**Violation**
`REQ-AUD-001`. Revoking direct DELETE on `task_events` does not prevent a parent delete from cascading into the audit log. Checkpoints and immutable artifacts also lack explicit append-only privilege/retention semantics.
**Minimal fix**
Use lifecycle/archive states and `RESTRICT` for canonical parent deletion during normal operation. Freeze a separate, audited retention/break-glass purge procedure. Apply INSERT/SELECT-only or equivalent immutability controls to task events, checkpoints, and immutable artifacts, and test that parent deletion cannot silently erase them.
### KCR-012 — MAJOR — Coordinator persistence lacks durable quarantine/retry state and DTO/schema alignment
**Location**
- `contracts/mechanical-coordinator.v1.ts:239-244` — expiry returns `quarantined` IDs.
- `contracts/kanban-schema.v1.ts:457-490` — task has only untyped `retryPolicy` metadata and no quarantine/execution disposition.
- `contracts/mechanical-coordinator.v1.ts:173``evidenceIds` has no corresponding evidence table/type; schema has artifacts.
- `contracts/kanban-schema.v1.ts:479`, `663`, and agent role JSON — specialist roles are free text despite the frozen role vocabulary in `mechanical-coordinator.v1.ts:19-29`.
**Violation**
`REQ-COORD-004` and internal consistency. PostgreSQL cannot deterministically reconstruct why/when a task was quarantined, its bounded retry state, or which typed evidence was submitted. Free-text roles allow the schema and engine to disagree.
**Minimal fix**
Freeze a durable execution/retry/quarantine record (attempt count, next eligibility, terminal reason, actor/policy, timestamps, version) or typed task columns with events. Align `evidenceIds` to artifact IDs or add a real evidence entity. Use one specialist-role enum/check across tasks, assignments, agents/sessions, DTOs, and Coordinator.
### KCR-013 — MAJOR — Thin MVP promises task archive and tag filtering without target-state semantics
**Location**
- `REQUIREMENTS.md:182-193` — users must archive tasks and filter by tags.
- `SHARED-CONTRACT.md:252-267` — mutations include cancel but not archive task.
- `contracts/kanban-schema.v1.ts:457-490` — no task archive field and no typed tags field/table.
- Current `origin/main` already has `tasks.tags`, making omission from the target declaration a migration-loss hazard.
**Violation**
`REQ-UI-001/002` and internal acceptance consistency. “Archive” cannot be implemented without inventing whether it means cancelled, hidden, or soft-deleted; tag filtering has no frozen storage/query contract.
**Minimal fix**
Either remove task archive/tag acceptance from P1, or add explicit non-lifecycle archival semantics (`archived_at/by/reason`) and a workspace-safe tags model/query contract. Preserve/migrate the current tags column until the selected model is live.
### KCR-014 — MAJOR — Recovery contract states critical rules only in comments and has no owning implementation slice
**Location**
- `contracts/recovery-posture.v1.ts:97-147` — exported JSON Schema validates only local field shapes.
- `contracts/recovery-posture.v1.ts:150-156` — PITR/WAL, effective RPO, off-cluster, high-assurance minima, and audit rules are comments only.
- `REQUIREMENTS.md:270-277` — parser rejection of impossible combinations is acceptance-critical.
- `TASKS.md:75-244` — no bounded slice owns recovery config parsing, override audit, backup/WAL setup, or restore/break-glass evidence.
**Violation**
`REQ-REC-001`. A consumer using the advertised JSON Schema can accept weakened high-assurance values, PITR without WAL, or an impossible RPO. The task plan has no lane accountable for closing that acceptance criterion.
**Minimal fix**
Export a normative `validateRecoveryPostureV1`/schema refinement with machine-testable cross-field checks and add a bounded Infra/recovery slice (serialized if it touches shared config) owning config parsing, override audit, mechanism verification, restore test, and break-glass evidence. Recovery config must continue to expose no authority/gate knobs.
### KCR-015 — MAJOR — Pure Coordinator slice cannot implement two frozen methods without persistence access
**Location**
- `contracts/mechanical-coordinator.v1.ts:259-263``explainEligibility` receives only `taskId`, not a structured snapshot.
- `contracts/mechanical-coordinator.v1.ts:289-293``recoverFromPostgres` explicitly reads PostgreSQL.
- `TASKS.md:145-153` — KBN-200 is a pure engine with no SQL, Drizzle, Gateway, or Valkey.
- `TASKS.md:157-164` — persistence belongs to coder3/KBN-210.
**Violation**
Charter C and internal consistency. coder4 cannot implement the frozen port in a pure package without crossing coder3s persistence boundary. If coder3 implements the port instead, KBN-200s acceptance and ownership are misassigned.
**Minimal fix**
Split the contract into a pure decision engine that receives complete immutable snapshots and a persistence/orchestration service port implemented by KBN-210. Move `recoverFromPostgres` and ID-based loading to the adapter/service; make pure explanation accept a snapshot.
### KCR-016 — MINOR — Health denial code/state pairs are not correlated by type
**Location**
- `contracts/health-state.v1.ts:35-61` — either denial code can pair with either degraded state.
- `SHARED-CONTRACT.md:188-190` — prose defines `KANBAN_WRITE_UNAVAILABLE` specifically for `write-unavailable`.
**Violation**
Health contract precision. A client can receive a semantically inconsistent authoritative body even after KCR-001s broader state fix.
**Minimal fix**
Make deliberate denial a two-variant union with exact code/state pairing.
---
## Clean checks / invariants that do hold
The review did **not** find a gap in these areas:
- The canon consistently selects current `mosaicstack/stack` + Drizzle/PostgreSQL and rejects greenfield/Prisma revival.
- Every artifact states PostgreSQL is the sole writable SOT and Valkey/files are non-authoritative.
- Generated `TASKS.md`, `mission.json`, and exports are consistently declared read-only and never import sources. KBN-300s importer is scoped to immutable legacy JSON/Vikunja snapshots, not generated projections.
- Recovery config exposes recovery fields only; it contains no direct fail-open, SOT, Coordinator-authority, or gate-waiver knob.
- The Coordinator interface contains no `createTask`, acceptance-edit, gate-waive, certify, merge, release, or provider-close method. `submitForReview` is type-limited to `in_review`, not `done` or `certified`.
- Certifier is consistently final independent gate with no merge authority.
- The seven canonical task status values match across requirements, shared prose, schema, and Coordinators ready/in-review surfaces.
- One-active-lease partial uniqueness, no-self-edge, outbox aggregate-revision/event-type uniqueness, optimistic task/project/mission/milestone versions, and N-1 test categories are explicitly present.
- The file-tree partition is mostly well separated once the ordering/freeze defects in KCR-007 are corrected.
## Required re-review scope
After remediation, re-review at minimum:
1. health/coordinator discriminated unions and mutation-time health proof;
2. proposal/approval/assignment/lease relational model;
3. monotonic fencing and composite bindings;
4. tenant-safe polymorphic relationships;
5. outage-proposal persistence and commands;
6. concrete current-main migration map;
7. corrected dependency graph and exact API/DTO freeze;
8. recovery validator/owner slice;
9. all schema and DTO vocabulary alignment.
## Overall verdict
**NO-GO — 8 BLOCKERs must be resolved before the v1 contract is frozen or parallel implementation begins.**