From 7b68f205014cecaf2ccb3667ca1dd4ea77de7dad Mon Sep 17 00:00:00 2001 From: gsxdsm Date: Thu, 30 Jul 2026 19:12:26 -0700 Subject: [PATCH] batch(docs): fold the three workflow-learnings / annotation PRs into one (#2942) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Family batch — replaces #2926, #2892, #2887 Per the consolidation directive: the u9/e2e **docs family**, folded into one branch and one CI run. Three PRs, five commits, **five files, comment and markdown only**. | folded PR | commits | |---|---| | #2892 `docs/union-vs-per-task` | the project union and the per-task answer are not ranked; date correction | | #2926 `docs/date-my-measured-claims` | date the measured claims (one was wrong); date the grep-vs-AST measurement in the SQL gate header | | #2887 `docs/archived-state-literals` | mark the three archived STATE literals as deliberate | Cherry-picked in original order with authorship preserved; all five applied clean, no conflicts. ## Scope is provably comment-only ``` docs/solutions/workflow-learnings/lifecycle-conversions-that-score-as-wins.md docs/solutions/workflow-learnings/project-union-versus-per-task-lanes.md packages/core/src/task-store/async-maintenance.ts ← FNXC DELIBERATE-LITERAL annotation packages/core/src/task-store/workflow-definitions.ts ← FNXC DELIBERATE-LITERAL annotation scripts/check-sql-column-literals.mjs ← header prose only ``` Every added line in `packages/` and `scripts/` is inside a comment — checked by filtering the diff for declarations, conditionals and returns, which returns nothing. The two core files gain `DELIBERATE-LITERAL` markers explaining that `'archived'` is a **state** marker there, not a lane: the sweep collects rows Fusion itself archived or soft-deleted, so widening to the resolved archived set would pull live cards into a cleanup pass. ## Verification (scoped, per the directive — not the full suite) - `pnpm lint` — clean - `check-sql-column-literals` — exit 0 (the file it annotates) - `check:lifecycle-columns` — exit 0 (the markers it adds are census-visible) - `sync-workflow-ir-callsite-allowlist.test.ts` — 3/3 ## A correction worth recording Mid-fold I saw a changeset, `self-healing.ts` and a test file in `git diff origin/main..HEAD` and nearly reported the batch as impure. They were **main's own commits** — `origin/main` advanced between branch creation and the diff, so the comparison was against a stale base. Rebasing onto current `main` reduced it to the five files above. Worth flagging for anyone else folding a family today: with `main` moving this fast, diff the branch **after** rebasing or the file list will lie to you. ## Closing the originals #2926, #2892 and #2887 are superseded by this and are being closed. I hold no PRs of my own in this family — all mine merged — so this fold is on behalf of the family rather than a rollup of my own work. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5 (1M context) --- ...ifecycle-conversions-that-score-as-wins.md | 19 +++-- .../project-union-versus-per-task-lanes.md | 80 +++++++++++++++++++ .../core/src/task-store/async-maintenance.ts | 7 ++ .../src/task-store/workflow-definitions.ts | 20 +++++ scripts/check-sql-column-literals.mjs | 6 +- 5 files changed, 125 insertions(+), 7 deletions(-) create mode 100644 docs/solutions/workflow-learnings/project-union-versus-per-task-lanes.md diff --git a/docs/solutions/workflow-learnings/lifecycle-conversions-that-score-as-wins.md b/docs/solutions/workflow-learnings/lifecycle-conversions-that-score-as-wins.md index 1d12057f8c..c7952420b6 100644 --- a/docs/solutions/workflow-learnings/lifecycle-conversions-that-score-as-wins.md +++ b/docs/solutions/workflow-learnings/lifecycle-conversions-that-score-as-wins.md @@ -152,7 +152,8 @@ hands in.** If your fixture passes the lane name, you have tested the consumer. computes that argument in production, and whether they compute it or spell it. MEASURED, so nobody builds the wrong instrument: an AST probe for a call argument -`{ column: "" }` finds **79 sites** across `packages/`. It flags the two real `moves.ts` +`{ column: "" }` found **79 sites** across `packages/` when measured on 2026-07-30 (42 as +of 2026-07-31, after the analytics conversions landed — the ratio is the point, not the total). It flags the two real `moves.ts` offenders, but most of the rest are legitimate — `set({ column: "archived" })` writing the archive state, `listTasks({ column: "todo" })` filtering a query. A blocking gate on this shape needs a curated list of consumers that interpret the column as a ROLE (as opposed to storing or filtering @@ -167,7 +168,8 @@ a plausible "the census cannot see this" theory; each was measured. **Membership tests with inline literals** — `["done","archived"].includes(task.column)`. The census counts binary comparisons only, so this shape would be invisible. **Zero sites.** Nobody writes it. -**Named legacy-id collections** — `const X = ["done","archived"]` then `X.has(col)`. **48 declarations**, +**Named legacy-id collections** — `const X = ["done","archived"]` then `X.has(col)`. **48 declarations** +measured 2026-07-30, 49 on 2026-07-31; and the raw count is misleading: on inspection they are all either - the intentional fallback vocabulary the role helpers degrade to (`LEGACY_TERMINAL_COLUMNS`, @@ -182,9 +184,16 @@ would have been the same mistake as gating on `{ column: "" }` call arg mostly legitimate): a number that looks like a work list and is not. **The rule:** measure a candidate surface, then inspect a sample, before reporting it OR instrumenting -it. A raw count is a hypothesis. The SQL surface survived this test — 14 sites, all genuinely -vocabulary-bound — and got a ratchet. These two did not, and got nothing, which is the correct -outcome and cost an hour to establish rather than a wrong gate to maintain. +it. A raw count is a hypothesis. The SQL surface survived this test and got a ratchet; these two did +not, and got nothing, which is the correct outcome and cost an hour to establish rather than a wrong +gate to maintain. + +CORRECTION, 2026-07-31. This paragraph used to read "the SQL surface survived this test — 14 sites, +all genuinely vocabulary-bound". Both halves were wrong within a day. The population was 31 once the +scanner stopped missing Drizzle-templated queries, and hand-reading split it into roughly fourteen +lane-bound sites, eleven `archived` comparisons (three of them CORRECT as literals — they read a +state, not a lane), and one in dead code. Writing "all genuinely vocabulary-bound" was the same +mistake this section warns about, committed in the sentence that warns about it. ## Guards start catching other people's work, not just yours diff --git a/docs/solutions/workflow-learnings/project-union-versus-per-task-lanes.md b/docs/solutions/workflow-learnings/project-union-versus-per-task-lanes.md new file mode 100644 index 0000000000..85a115ea48 --- /dev/null +++ b/docs/solutions/workflow-learnings/project-union-versus-per-task-lanes.md @@ -0,0 +1,80 @@ +--- +category: workflow-learnings +module: packages/core/src/project-lane-vocabulary +tags: [lifecycle-columns, resolved-lanes, arity, project-union, per-task] +problem_type: design-decision +applies_when: choosing between resolveProjectColumnsForRoles and a per-task lane resolution +--- + +# The project union and the per-task answer are not ranked + +Recorded 2026-07-30, after the same reviewer finding arrived on five PRs in one day and got **four +different correct answers**. The instinct the program had built by then — "a union is the lazy option, +resolve per task" — is wrong often enough to be worth writing down, because acting on it shipped a +regression. + +`resolveProjectColumnsForRoles(store, roles)` returns every column any workflow in the project gives +the role, unioned with the legacy ids. A per-task resolution answers for one card's own board. Neither +is the better tool. + +## The question that decides it + +Not "can I resolve per task?" — usually you can. It is: + +> **What does over-inclusion cost here, and what does a missing selection cost?** + +Those are the two failure directions, and they are asymmetric in a way that changes per call site. + +## The four answers, with what made each one right + +| site | answer | why | +|---|---|---| +| `awaitingPlanning` board enrichment | **union** | over-inclusion costs one extra file read; the per-card predicate still answers per card, so a widened candidate set cannot produce a wrong badge | +| glasses completion notifier | **narrow** | a notification is an outward action with no downstream narrowing — over-inclusion is a wrong notification reaching a user | +| `workflow-analytics` completions | **narrow, feasible** | the rows carry a workflow id (`GROUP BY`), so the keyed answer was available and the union was simply the wrong arity | +| `team-analytics` totals | **union, accepted** | per-task means one workflow read per task and moving an indexed `COUNT` into JS; the cost is a number an operator reads, recorded rather than paid | +| archived-document guards | **union, after trying narrow** | see below | + +## The one that reversed the instinct + +`comments-ops.ts` guards whether a card's documents are read-only. Both callers have `taskId`, so the +keyed answer looked available and the arity argument said take it. + +Switching to `resolveWorkflowIrForTask` **broke a passing renamed-lane test**: a card in a renamed +archived lane stopped being read-only. The per-task resolver needs the task's own workflow +**selection**, and with none recorded it degrades to the built-in IR, whose archived lane is +`archived`. The project union needs no selection — which is exactly why it caught the renamed card. + +The failure modes were not the same size: + +- **union**: a live card loses document writes, and only where two workflows reuse one id with + different traits — a configuration nobody has reported. +- **per-task**: every renamed-lane card with no recorded selection silently becomes *writable while + archived* — the defect the guard exists to prevent, with a test proving it. + +Reverted. The correct fix is **per-task with a union fallback when the selection is absent** — narrow +when the card can answer, broad when it cannot — which needs "no selection" distinguished from +"selection resolved to the default", i.e. the provenance form. + +## Rules that fall out of it + +1. **A union is not a shortcut.** It is the right answer wherever a card cannot be assumed to have a + resolvable workflow, because it does not depend on one. +2. **Ask what happens with no selection before converting.** `resolveWorkflowIrForTask` does not fail, + it *substitutes* the built-in board — so a per-task conversion silently answers with the wrong + vocabulary exactly where the renamed-board bug lives. +3. **Over-inclusion is free for a read, costly for an action, and misleading for a metric.** Query, + candidate filter, anything narrowed downstream: union. Notification, move, write gate: narrow. + Aggregate shown to a human: narrow if the rows are keyed, otherwise accept and record. +4. **Swapping one defect for a bigger one drops a guard count and looks like progress.** That is the + failure mode `lifecycle-conversions-that-score-as-wins.md` catalogues; this is the same trap + reached through a correct-sounding principle rather than a careless edit. + +## Also unresolved, and related + +`resolveProjectColumnsForRoles` seeds only the legacy ids for a project whose workflows declare **no** +lifecycle traits at all, so a hand-authored v2 board with renamed columns and no traits contributes +nothing and its cards are invisible to every sweep that queries by role. The three-state rule at +project scope. The fix cannot be a changed default for the reason above — sweeps want the widening and +the aggregators do not — so it wants an opt-in +(`{ untraitedProject: "declared-columns" }`). Recorded at three call sites in `self-healing.ts`. diff --git a/packages/core/src/task-store/async-maintenance.ts b/packages/core/src/task-store/async-maintenance.ts index 87488adf8d..dc71acad59 100644 --- a/packages/core/src/task-store/async-maintenance.ts +++ b/packages/core/src/task-store/async-maintenance.ts @@ -37,6 +37,13 @@ export async function pruneAgentLogFilesAsync( severityAuditLog.warn("[fusion] PostgreSQL agent-log-file pruning is using the legacy unscoped project sentinel because asyncLayer.projectId is missing"); } const projectId = boundProjectId || "__legacy_unscoped__"; + /* + FNXC:WorkflowResolvedColumns 2026-07-30-22:12 DELIBERATE-LITERAL: + `'archived'` is the STATE marker here, not a lane. This sweep collects rows Fusion itself archived + or soft-deleted; a card merely sitting in a workflow's archived-TRAIT lane is live work and must not + be collected. Widening to the resolved archived set would pull real cards into a cleanup pass. + See #2839 for why the live-VIEW exclusions that share this literal are a different question. + */ const rows = (await layer.db.execute( sql`SELECT id FROM project.tasks WHERE project_id = ${projectId} AND (deleted_at IS NOT NULL OR "column" = 'archived')`, )) as unknown as Array<{ id: string }>; diff --git a/packages/core/src/task-store/workflow-definitions.ts b/packages/core/src/task-store/workflow-definitions.ts index ed3fd615b0..096d5a8f6a 100644 --- a/packages/core/src/task-store/workflow-definitions.ts +++ b/packages/core/src/task-store/workflow-definitions.ts @@ -117,6 +117,20 @@ export function migrateLegacyArchiveEntriesToArchiveDbImpl(store: TaskStore): vo } export async function migrateActiveArchivedTasksToArchiveDbImpl(store: TaskStore): Promise { + /* + FNXC:WorkflowResolvedColumns 2026-07-30-22:10 DELIBERATE-LITERAL: + `'archived'` here is the STATE marker, not a board lane, and must NOT be widened to the resolved + archived columns. + + This finds live rows that Fusion's own archive path stamped (`archive-lifecycle-2.ts` and + `serialization.ts` both hardcode `column: "archived"`) so they can be migrated into the archive + DB. A workflow may also declare an archived-TRAIT lane under any id — `resolveLifecycleColumns` + resolves it, and a card can be moved there — but such a card was never archived by Fusion, has no + archive-store row, and migrating it would move live work out of the board. + + So the resolved set is the wrong question at this site even though it is the right one for the + live-view exclusions that share this literal. See issue #2839 for the split. + */ const rows = store.db.prepare(`SELECT * FROM tasks WHERE "column" = 'archived'`).all() as unknown as TaskRow[]; if (rows.length === 0) { return; @@ -906,6 +920,12 @@ export function pruneAgentLogFilesImpl(store: TaskStore, retentionDays: number): if (!Number.isFinite(retentionDays) || retentionDays <= 0) { return { prunedFiles: 0, prunedEntries: 0, freedBytes: 0 }; } + /* + FNXC:WorkflowResolvedColumns 2026-07-30-22:14 DELIBERATE-LITERAL: + STATE marker again, same reasoning as migrateActiveArchivedTasksToArchiveDbImpl above: this prunes + agent-log files for rows Fusion archived or soft-deleted. A card in a workflow's archived-TRAIT + lane is live work whose logs must survive, so the resolved set would delete data here. + */ // Only prune JSONL files for tasks that are no longer active (soft-deleted or archived) const inactiveTaskIds = new Set( ( diff --git a/scripts/check-sql-column-literals.mjs b/scripts/check-sql-column-literals.mjs index f1ac10ca29..81aa981510 100644 --- a/scripts/check-sql-column-literals.mjs +++ b/scripts/check-sql-column-literals.mjs @@ -23,8 +23,10 @@ baseline records per-file counts, a new file or a higher count fails, and a LOWE the baseline is ratcheted down as sites are migrated rather than silently drifting. COMMENTS ARE NOT MATCHED, and that is the whole reason this is AST-based. A line-oriented grep for -the same pattern reports 37 hits, 25 of which are prose quoting `column === "done"` in an explanatory -note. A guard with a 68% false-positive rate teaches its readers to skip it, and this repo already +the same pattern reported 37 hits when this was written (2026-07-30), 25 of them prose quoting +`column === "done"` in an explanatory note. Re-measured 2026-07-31: 38 grep hits against 20 real ones. +The totals drift as conversions land and comments do not — the RATIO is the argument, and it has held +at roughly half. A guard with a 68% false-positive rate teaches its readers to skip it, and this repo already learned that lesson the expensive way. Comments are not AST nodes, so walking string and template literals cannot match them at all. */