simplify(todos): make work_item_approvals the only approval storage - #111
Merged
Merged
Conversation
The eight `approval_*` columns on `work_items` had been write-dead since Todos v2 slice 4 — every read already resolved through `work_item_approvals`, but the columns stayed, `rowToWorkItem` still mapped them, and three of its eleven approval fields were knowingly-wrong hardcoded defaults. Drop them. - `migrate.ts`: freeze the pre-drop shape as `V2_APPROVAL_WORK_ITEMS_TABLE_DDL` (the `V1_WORK_ITEMS_TABLE_DDL` precedent) and delete the columns from the canonical DDL, which cascades into `verifyCurrentWorkItemSchema`. A database matching the frozen shape classifies "current" and heals in the existing boot transaction: create the additive tables, copy the columns off-row, then `ALTER TABLE ... DROP COLUMN` each. `backfillWorkItemApprovals` takes its source table so the v1 rebuild reads `work_items_v1_legacy` before the drop, and it stops running on every boot — it is a one-shot rebuild step now. - `store.ts`: `rowToWorkItem` returns a module-local `WorkItemRowBase` that omits all eleven approval fields, so `overlayApproval` is the only producer of them and a read path that skips hydration no longer typechecks. - Fix the prose the removal falsifies (frozen / dual-read / `approval_state`). Payload field names and values are untouched; the golden parity fixtures and pins are byte-identical, only their seeding mechanism changed.
hristo2612
force-pushed
the
simplify/PLA-48-drop-frozen-approval-columns
branch
from
August 3, 2026 10:06
764a0aa to
c1a1e43
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Selected area
Todo approval storage in
packages/jinn/src/work-items/— thework_itemstable's eightapproval_*columns and the read seam instore.tsthat still mapped them ontoWorkItem.Evidence of over-engineering / blended concerns
Todos v2 slice 4 moved approvals into a dedicated
work_item_approvalstable but left the oldcolumns in place behind a "dual-read window" that was never closed:
approval_*columns since slice 4; every read already resolved through
work_item_approvals. The columnssurvived only as shadow state, plus a
backfillWorkItemApprovalscall that re-ran on everygateway boot to keep copying from a table nobody wrote.
rowToWorkItemreturned a fullWorkItem, populating theeleven approval fields from those dead columns — including three knowingly-wrong hardcoded
defaults. Any read path that forgot to call
hydrateApprovalscompiled fine and returned anall-NULL approval state that looked authoritative. The correctness of the approval surface
rested on every call site remembering an opt-in step.
store.ts(:28, :274-281) andthe
requestApprovaldoc inapprovals.tsdescribed the columns as live, frozen, ordual-read.
Fixed constraint budget
netLineDeltafilesTouchednewFilesmaxFileLinesQualitative locks: exactly one new public export (
V2_APPROVAL_WORK_ITEMS_TABLE_DDL); onlymigrate.tsmay end larger than base (the frozen legacy recognizer DDL has no deletablecounterpart); no new dependencies, config options, or single-caller abstractions;
packages/web/**,approval-rows.ts,approval-authority.ts,workflow-todo-binding.tsandall payload field names untouched.
Measured budget (verbatim output)
Command from
PLAN.md, run at HEAD764a0aa36a34d4b00950daad5bffe70ceb819396:Per file:
Every touched file ends at or below its base line count except
migrate.ts(1132 → 1168), theone file the budget explicitly allowed to grow.
What was deleted / clarified
Deleted
approval_*columns from the canonicalWORK_ITEMS_TABLE_DDL, which cascades intoREQUIRED_TABLE_SQL/verifyCurrentWorkItemSchemaautomatically. Fresh databases never getthem; existing ones drop them via
ALTER TABLE work_items DROP COLUMN.backfillWorkItemApprovalscall. The backfill is now a one-shot step insideeach rebuild path, parameterized by source table (
work_itemspre-drop for the v2 legacy heal,work_items_v1_legacyfor the v1 rebuild).rowToWorkItem, its threewrong hardcoded defaults, and the frozen-columns test helpers (
rawColumns,expectColumnsFrozenNull) whose covered behaviour is deleted along with the columns.dual-read.
Clarified
rowToWorkItemnow returns a module-localWorkItemRowBase = Omit<WorkItem, …approval fields>.overlayApprovalis the only producer of the eleven approval fields, so a read path thatskips hydration is a type error rather than a silent all-NULL authority bug. No new export,
no runtime cost, no
as WorkItemcast anywhere in the diff.V2_APPROVAL_WORK_ITEMS_TABLE_DDL(the existingV1_WORK_ITEMS_TABLE_DDLprecedent) so a legacy database is recognized and classifies"current"— healable at boot inside the existing single immediate transaction, so backfilland drops commit atomically or not at all. No new preflight state.
Compatibility. Public payload field names and values are unchanged. The golden
work-item-approval-parityFIXTURES and pinned payloads are byte-identical; only their seedingmechanism changed.
Test results
Run in the worktree at HEAD
764a0aa3, after the final commit:pnpm typecheck— 2 tasks successful (@jinn/web,jinn-cli), clean.pnpm test— 309 test files passed, 3829 tests passed, 1 skipped, 0 failed.pnpm build— 2 tasks successful; web bundle built,dist/compiled, assets synced.pnpm lint— the repo defines no lint task, so this executes zero tasks (noted by theverifier, not treated as a passing gate).
Independently re-verified in round 2 by
jinn-verifierwith the turbo cache forced off, plus acolumn-death grep across
packages/jinn/srcandpackages/web/srcreturning empty.Known gap: the migration was exercised through the test suite only (fresh-database, v2-legacy
fixture, and v1 fixture upgrade paths), not against a live gateway booting on a real
pre-PLA-48 home.
Todo: PLA-48