bebbdf90835c2cb2ac10beeaf0604d92d51aa7e8
2697 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
gsxdsm
|
bebbdf9083 |
fix(tests): main red — archive restore returns to the ARCHIVED lane now (#2832) (#2847)
## Red on main ``` store-archive-reads > TaskStore archived read parity (PostgreSQL) > rebuilds a missing live row before consuming its archive snapshot AssertionError: expected 'done' to be 'todo' ``` ## The product change is a real fix **#2832** found that `preArchiveColumn` has no database column — it exists on the `Task` type and in the archive snapshot and nowhere else — so the old code fell through to a literal and decided the destination the same way for **every unarchive that has ever run**. On a custom board that meant a card archived mid-implementation came back marked *finished*. That PR flipped its own characterization cases. This one, in a different file, was missed. The fixture creates the card in `done`, so `done` is the answer now. ## I measured a second sample before encoding a rule The behaviour is **narrower than "restores to the lane it came from"**. With the fixture changed to `in-progress`, restore returns **`todo`** — not `in-progress`: | archived from | restored to | |---|---| | `done` | `done` | | `in-progress` | **`todo`** | A terminal lane is preserved; a WIP lane is re-queued. That is plausible product behaviour — a card cannot resume mid-execution after a restore — but it is **not what #2832's summary describes**, so I have flagged it there rather than encoding it here. If re-queueing WIP is deliberate it deserves its own case; if it is not, this snapshot-rebuild path still carries the defect #2832 fixed elsewhere. ## An honest limitation, recorded in the test This assertion is **weaker than it looks and cannot be strengthened here**: `done` is also the complete lane, which is exactly what the pre-#2832 *"no usable history"* branch returned. A card archived from `done` therefore reads identically under both the fixed and the broken implementation. My first attempt "strengthened" it by moving the fixture to `in-progress` — that is what surfaced the second behaviour above, and shipping it would have encoded a rule inferred from two samples. Reverted; the limitation is documented instead. ## Scope Only the line-205 case is touched. Line 218 asserts `todo` for a card genuinely archived from `todo` and still passes — the two are not the same claim. Core **4813 passed / 0 failed** · gate **732 green** · lint clean. Test-only. ## Pre-flight results for the current queue Merged-with-main, engine + core on each: | PR | result | |---|---| | #2822, #2819, #2823, #2818 | only the 2 inherited `workflow-ir-resolver` failures (fixed in #2836, now merged) | | #2805 | clean | | #2828 | inherited only, once this and #2836 land | | #2830 | inherited only | | #2820, #2803, #2808 | **conflict** with main — census baseline; told the owners to regenerate rather than hand-merge | 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
gsxdsm
|
f53c9dbd39 |
fix(core): merge re-enqueue threw on every board with a renamed review column (#2819)
The single most consequential finding from the u12 seam-gate work, picked up because batch-core (#2783) merged without it and it is now unowned. ## The defect `enqueueMergeQueueInTransaction` gates on the task's column being a review column, and takes the board's review columns as an optional trailing argument. - `moves.ts:487` and `moves.ts:1153` — the automatic handoff-to-review path — resolve and pass them. - The public `enqueueMergeQueue` wrapper (`async-merge-coordination.ts:246`), reached through `store.enqueueMergeQueue`, **did not**, so it fell back to `new Set(["in-review"])`. This is not the quiet legacy-id degradation most of these seams produce. The reject branch records `mergeQueue:enqueue-rejected` and **throws** `MergeQueueInvalidColumnError`. Its production callers are `merger.ts:7251` and `self-healing.ts:10329` — so on any board whose review lane is renamed, the merge and recovery re-enqueue paths failed outright while the handoff path kept working. ## Measured, not asserted With the fix reverted, the renamed case fails with the exact predicted error and the controls stay green: ``` × renamed vocabulary: a task in the RENAMED review lane enqueues for merge MergeQueueInvalidColumnError: Task KB-001 is in column 'checking', not 'in-review'; cannot enqueue ✓ default vocabulary: a task in the review lane enqueues for merge ✓ renamed vocabulary: a task in the WIP lane is still REJECTED ✓ default vocabulary: a task in the WIP lane is still REJECTED Tests 1 failed | 3 passed (4) ``` With it: `Tests 4 passed (4)`. ## About the suite **Differential.** The fixture is the builtin coding workflow with only its column ids renamed, so the sole difference between the two runs is vocabulary — a hand-built graph would test the fixture's own transition table as much as the code. It asserts the rename actually landed (`checking` present, `in-review` absent), so a surviving literal cannot pass by luck, and it walks the graph rather than jumping, because moves are transition-validated. **Both negatives included.** A WIP-lane task must still be REJECTED under each vocabulary. Supplying the real columns must not degrade into "every column is a review column", which would let work merge straight out of the WIP lane — the failure mode a careless version of this fix would introduce. ## Why nothing caught it Partial supply. Two of three call sites passed the argument, so a check asking "does SOME caller supply this?" reported the seam as satisfied, and the lifecycle-column census counted the conversion as done. Closing that one-supplier floor in `scripts/check-inert-flag-seams.mjs` (on #2772) is what surfaced it. ## Verification - `pnpm test:gate` green - new suite 4/4; neighbouring merge-queue suites (`taskstore-lifecycle`, `store-in-review-stall`, `runtime-lifecycle-async`) 29/29 - `tsc -p packages/core` 0, lint 0 - changeset included (`@runfusion/fusion` patch) Note: #2772 still carries a TEMPORARY per-call-site exemption for this seam. Once this lands, that exemption's staleness check will fail and I will remove it there — it cannot outlive the fix. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
gsxdsm
|
4c5080c1cf |
fix(tests): main red — the IR fallback is a BRANDED COPY, so identity can't hold (#2836)
## Red on main
```
workflow-ir-resolver > resolveWorkflowIrForTask > falls back to the built-in default when the definition is missing
workflow-ir-resolver > resolveWorkflowIrById > falls back to the canonical IR for an unknown built-in id
AssertionError: expected { version: 'v2', …(6), …(1) } to be { version: 'v2', …(6) }
Received: serializes to the same string
Compared values have no visual difference.
```
That message is the signature of an **identity-only** break, and that is
exactly what it is.
## Why identity can never hold again
Both asserted `toBe` against the exported builtin constant. **#2815**
added `markFellBack`:
```ts
function markFellBack(ir: WorkflowIr): WorkflowIr {
const copy = { ...ir } as WorkflowIr;
Object.defineProperty(copy, FELL_BACK_TO_DEFAULT, { value: true, enumerable: false, configurable: true });
return copy;
}
```
It brands the fallback so a caller can tell a **resolved** workflow from
a **guessed** one — the provenance contract #2618 introduced. Copying
*is* the mechanism, so these two paths cannot return the shared object.
Worth noting: the sibling `toBe` assertions in the same file **still
pass**. The no-selection and throwing-selection paths return the
constant unbranded, so only the two `markFellBack` paths changed — which
is why this presents as two failures rather than five, and why it is a
genuine contract change rather than a blanket refactor.
## Not just loosening to `toEqual`
Swapping `toBe` → `toEqual` alone would delete a real assertion. The
**brand is asserted too**, via the public provenance API rather than by
reaching for the private symbol:
```ts
expect(ir).toEqual(BUILTIN_STEPWISE_FINAL_REVIEW_CODING_WORKFLOW_IR);
const provenance = await resolveWorkflowIrForTaskWithProvenance(store, "t1");
expect(provenance.source).toBe("default");
```
This is **stronger** than the identity check it replaces:
| mutation | result |
|---|---|
| fallback no longer branded | **fails** — `expected 'selection' to be
'default'` |
| fallback returns a different IR entirely | **fails** structurally |
The first is the case the old `toBe` could not articulate: an unbranded
fallback still equals the constant structurally, so a caller asking
*"was this actually resolved?"* would get **yes for a guess** —
precisely the lie #2815 exists to prevent.
Core **4792 passed / 0 failed** (was 2 failed) · gate **732 green** ·
lint clean. Test-only; the resolver is restored clean after the
mutations.
## How it was found
Pre-flighting **#2822, #2819, #2823 and #2818** merged-with-main. All
four reported the *same two* failures — the signature of an inherited
red rather than four independent regressions. Confirmed directly on
`origin/main`. Each of those four is otherwise green (engine 11003
passed on all of them); I have noted that on the PRs.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
||
|
gsxdsm
|
7bf3df9477 |
chore(core): delete the dead merge-blocker guard, and catch exemptions for deleted seams (#2830)
Closes the last of the five findings I reported to batch-core (#2783), which merged without them. This one I held back twice on purpose; the reason is now resolved. ## The dead code `evaluateMergeBlockerGuard` had **exactly one reference in the repo: its own declaration.** Never called, never re-exported from `index.ts`/`index.gate.ts`, never registered as a trait hook. Its `lifecycleColumns` conversion was applied to dead code, and the census counted it as progress. ## Why I would not delete it earlier, and what changed I twice declined this, because no production `"guard"` trait hook is registered anywhere in the codebase — the only `registerTraitHookImpl(..., "guard", ...)` is in a test. If a registration had been dropped, that would be a real product bug and this function would be its evidence, so deleting it would have destroyed the breadcrumb. **It is not missing.** Merge blocking is enforced inline in `task-store/moves.ts` (~645 and ~821) via `getTaskMergeBlocker`, gated on the **resolved trait flags** (`toFacts.flags.complete` + `fromFacts.flags.mergeBlocker`) rather than on column ids — a better implementation than the one being deleted. The logic moved; this function, and a file-header line crediting a never-existent `evaluateDefaultWorkflowGuards` reader, were left behind. The header now records where the guard actually lives, so the next person does not repeat the investigation I just did. Also removed: `GuardVerdict` (its return type, used nowhere else) and the orphaned `getTaskMergeBlocker` import — the latter caught by lint, not by me. ## The gap this exposed The allow-list staleness check only fired for a seam the scan still **finds** — it asks *"is this site supplied now?"*. Delete the declaration and the name is never iterated, so its entry sits in the list forever, exempting nothing and misreporting what is tolerated. I found this by deleting the function and watching the gate stay **silent** about its leftover entry. **Measured:** an `ALLOWED` entry naming a non-existent seam now fails with `no such seam declared any more; remove its ALLOWED entry`; removing the probe returns exit 0. The failure header is reworded, since "the sites are supplied now" no longer covers both reasons. This is the fourth blind spot closed in this check, and like the others it was found by exercising the gate rather than reading it. ## Verification `pnpm test:gate` green · `tsc -p packages/core` 0 · lint 0 · gate now reports **20** seams (was 21 — the drop is this deletion). No changeset: deleting unreachable internal code with no exported surface is behaviour-preserving. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
gsxdsm
|
3603da731f |
fix(core): duplicate markers were never cleared on a board with renamed terminal columns (#2823)
Third unowned finding picked up after batch-core (#2783) merged without addressing its reports. Same class as #2819, opposite failure direction. ## The defect `clearNearDuplicateReferencesTo` runs on every complete/archive/delete of a canonical task and clears the `nearDuplicateOf` markers pointing at it. It first asks `isNearDuplicateCanonicalInactive` whether the canonical really is finished — a safety check, so a live canonical's markers are not cleared out from under an operator. That call omitted the canonical's resolved column flags, so it fell back to the legacy `done`/`archived` ids. On a renamed board a just-completed canonical (`shipped`, `filed`) read as **still active**, the guard early-returned, and the markers were **never cleared**. The flagged duplicates stayed parked behind a user decision that could never arrive — the exact stranding the predicate's own FNXC note says it was written to prevent. ## I had the direction backwards, and it matters My first report of this seam described it as *markers cleared against a live canonical*. That is wrong. The legacy fallback errs toward "still active", so the failure is the opposite: markers that never clear at all. Same seam, same missing argument, entirely different symptom to look for — which is why the direction is worth pinning in a test rather than reasoning about. ## Measured With the fix reverted, exactly one case flips: ``` ✓ default vocabulary: completing the canonical clears the duplicate's marker × renamed vocabulary: completing the canonical clears the duplicate's marker ✓ renamed vocabulary: a canonical still in the WIP lane does NOT clear the marker ✓ default vocabulary: a canonical still in the WIP lane does NOT clear the marker ✓ a soft-deleted canonical clears the marker under a renamed board Tests 1 failed | 4 passed (5) ``` With it: `Tests 5 passed (5)`. ## Both negatives included A canonical still in the WIP lane must **not** clear its duplicates' markers, under each vocabulary. Resolving the real flags must not degrade into "every column is terminal", which would clear markers out from under an operator who has not made the duplicate decision yet. The soft-deleted path is covered too, since that branch never consults column flags at all. ## A fixture trap worth keeping `sourceMetadata` is `jsonb` and must be seeded as an **object**. Seeding a stringified value reads back fine through `getTask` — it parses either shape — while the production query's `source_metadata->>'nearDuplicateOf'` matches nothing. The fixture looks correctly seeded and the code under test can never find the row. My first version had this, and the self-check in the seed helper is what caught it. ## Scope Five of this predicate's six production call sites already resolved flags. This was the sixth, and the one that runs on every archive/complete transition. Not touched here: the same function's SQL predicate excludes duplicates by literal `ne(column, "archived")` / `ne(column, "done")`. That is a per-row question across many rows in one statement, not a one-line supply, so it is flagged rather than guessed at. The five **engine** call sites of this predicate are separately reported on #2785 and surfaced only via #2822. ## Verification `pnpm test:gate` green, new suite 5/5, `tsc -p packages/core` 0, lint 0, changeset included. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
gsxdsm
|
a6af3188df |
fix(core): startup recovery deadlocked on its own per-task lock (#2809)
## The bug `recoverStaleTransitionPendingImpl` runs its whole per-task body inside `store.withTaskLock(id, …)`. On the PostgreSQL arm it then read the task with `store.getTask(id)` — and `getTaskImpl` opens with `store.withTaskLock(id, …)` too. **The per-task lock is non-reentrant.** This codebase states that invariant in prose in two other files: > "nesting inside `withTaskLock` would deadlock since the lock is non-reentrant" — `branch-and-pr-entities.ts:561` > "because the per-task lock is non-reentrant" — `workflow-ops.ts:464` So the sweep waited forever on a lock its own frame was holding. **PostgreSQL-only — which is every production install.** The SQLite arm on the very next line reads through `readTaskFromDb`, a lock-free row read. The backend-mode port swapped only the PostgreSQL arm to `getTask`. The fix restores a lock-free read (`readTaskRow`) on that arm; nothing else changes. ## Why it survived until now The branch is entered **only** when a stale marker names a plugin hook the trait registry still knows (`hasSurvivingPluginHook`). Three nearby cases all miss it: | marker | path | |---|---| | none | the row is never scanned | | only `default-workflow:postCommit` | `hasSurvivingPluginHook` false — marker just cleared | | names an **uninstalled** plugin hook | reconciled away as degraded; nothing survives to re-run | | names a **registered** plugin hook | **reaches the in-lock read → deadlock** | Those first three are what the existing tests cover. The fourth is precisely the state a crash mid-hook leaves behind. All four are asserted in the new suite so the path cannot be re-narrowed and called covered. ## Impact This sweep runs at **startup**. A task left with such a marker deadlocks startup recovery — and because it deadlocks *while holding the task's lock*, that task is also left permanently unlockable. ## How it was found, including a correction By **bisection**, not by reading. An earlier attempt of mine to drive this recovery reported that "the sweep never returns". That was wrong in a way worth recording: the sweep returns fine in three of the four cases, and generalising the one hang to the whole function is what hid the actual trigger across several sessions. Narrowing case by case — empty store, plain task, default-only marker, unknown-hook marker, registered-hook marker — put the fault on one line. ## Verification - **Mutation-verified against the real defect.** With the fix reverted, the regression case fails by name — `recoverStaleTransitionPendingImpl did not settle within 8000ms — deadlock` — while the other three stay green. That is the actual pre-fix behaviour, not a simulation of it. - Every case is **timeboxed** on purpose: a deadlock otherwise surfaces as a suite-level timeout naming no case, which is useless for locating the fault. The deadline is not a flake knob — the fixed code settles in ~150 ms and the broken code never settles, so there is no value in between to tune. - A **vacuity guard** (no markers → scans nothing) so a change that stopped listing marked rows can't leave the other cases green. - `pnpm test:gate` — **exit 0** - full live-PG E2E surface — **152/152** - `pnpm lint` — clean Changeset included (`patch`, category `fix`). 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
gsxdsm
|
109204c590 |
fix: the query class — three sweeps that never ran on a renamed board (#2818)
Three sweeps that **never ran at all** on a renamed board, plus the shared answer the rest of the class needs. Consolidated from three handoff branches so the helper appears once. #2811 merged, so this is my only open PR. `#2800` measured this class and shipped evidence deliberately without conversions: `listTasks({ column: "<literal>" })` filters in the store, so on a renamed board the read returns an **empty array** and the sweep it feeds does nothing. The census scores the comparison *inside* the loop, never the query above it. ## What was broken | file | census count | what actually happened on a renamed board | |---|---|---| | `backlog-pressure-reporter.ts` | **0** | both reads empty, ratio computed as 0/0 — **the alert never fired**, on a board that may be under exactly the pressure it reports | | `stale-task-reporter.ts` | **0** | both reads empty — **no stale-task signal ever raised**, where work is most likely sitting unnoticed | | `restart-recovery-coordinator.ts` | flagged | sweep never ran — **an engine restart left interrupted tasks stuck with no requeue** | Two of the three have a census count of **zero**. They contain no lifecycle comparison at all, so they have never appeared in the backlog, in a per-file list, or in any "N → 0" claim — and were completely inert. **A file at zero is not evidence of anything.** ## The shared answer, and what it is not Every existing resolver answers a **per-task** question. A query has no task in hand, so it needs the project-level one: every column any workflow declares for a role, unioned with the legacy ids so a board mid-rename still finds rows under the old ones. The set is never empty, so a caller cannot accidentally query nothing. The header states what it is **not**: answering a per-card question from the union would mark a card as review because some *other* workflow calls its column review — the flat-set mistake this program has made four times. ## The finding that generalises: the query is rarely the whole defect `stale-task-reporter` **still reported zero after the query was fixed** — `getTaskAgeStalenessSignal` defaults to the legacy pair, so a card the query now returned was refused inside the signal. Converting only the query would have looked like a fix and changed nothing. That is a caveat on #2800's approach, offered as refinement rather than correction: **asserting the query ARGUMENT is right when pinning a known defect** (the outcome is 0 either way) **and insufficient when proving a fix**, because the outcome is the only thing that distinguishes a real conversion from a deeper one. All three conversions here assert outcomes. `restart-recovery` had three layers — query, a redundant re-assertion (deleted; a test pins the `paused` guard it did contribute), and a move destination that was **already** resolved but whose warning comment was stale. A stale warning is its own hazard: it told the next reader a defect existed where none did. ## Verification - helper **8 passed** · three reporter/coordinator suites **29 passed** - `pnpm test:gate` **161 / 13 / 487 / 71** · lint clean · `--strict` exits 0 · four `tsc` targets clean - each conversion revert-proven independently; the failing case is named in each test header ## Two mistakes worth recording **The helper's own test caught a bug in it.** My first draft wrapped the definition loop in one `try`, and `parseWorkflowIr` **validates** rather than parses — one malformed row would have returned legacy-only lanes for *every* workflow, indistinguishable from the bug it exists to fix. Now isolated per definition. **I clobbered the core barrel** by taking `index.ts` wholesale from a handoff branch, dropping two exports `main` had added since; three packages stopped compiling. Taking a file from another branch takes its whole contents, including what is now stale — for a barrel that is nearly always wrong. Re-applied as a single edit on top of `main`. ## Not included `self-healing.ts`'s 49 — actively owned and mid-conversion; an outside refactor there produces conflicting halves of one sweep. `project-engine.ts` (7) and `executor.ts` (2) need their own read of what each sweep does with the rows, which these three are the argument for. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
gsxdsm
|
1824c04584 |
fix(core): restore an archived card to the lane it came from (#2832)
## What Fixes the defect #2824 measured. That PR's characterization cases — merged and asserting the wrong-but-real behaviour — are flipped here to the correct lanes, which is what they were written to do. ## The bug ```ts // archive-lifecycle-2.ts const preArchiveColumn = task.preArchiveColumn ?? "todo"; ``` **`preArchiveColumn` has no database column.** It exists on the `Task` type and in the archive snapshot, and nowhere else — so the in-place restore cannot carry it, `store.getTask(id)` reads a live row that never had it, and the literal decided the destination for **every unarchive that has ever run**. | board | what happened | |---|---| | **default** | `todo` is declared, so the resolver returned it. Restores landed in the queue and **looked right**. | | **custom** | `todo` is declared nowhere, so the resolver took its "no usable history" branch and returned the **complete** lane. A card archived mid-implementation came back marked **finished**. | That coincidence is why this survived **three** separate fixes to `resolveUnarchiveTargetColumnImpl` — a `?? "done"` that invented a column, an `isColumn` legacy-enum gate, and the same gate one function over. Every one was correcting how the resolver interprets a value that never arrived. ## The fix is two halves, and either alone does nothing 1. **Capture** — `taskToArchiveEntryImpl` records `task.column` into the snapshot. That is the last place the original is still in hand, since the entry's own `column` is set to `"archived"` on the line above. 2. **Read** — `unarchiveTaskImpl` reads the **snapshot it already loaded**, not the restored row. I shipped half of this first and watched the destination stay wrong, which is how I found that the field has no row to live on. Mutation matrix: | state | result | |---|---| | both halves | **5/5 pass** | | capture only (read reverted) | **3 fail** | | read only (capture reverted) | **3 fail** | I also tried carrying it through `restoreTaskFromArchive`'s row update — that fails to typecheck, which is the proof that no such column exists and the snapshot is the only source. ## Behaviour changes, deliberately - **Custom boards** — a card returns to the lane it was archived from instead of appearing finished. - **Default board** — a card archived from `done` restored to `todo` under the literal and now restores to `done`. Returning finished work to the queue was the fallback showing through, not a rule anyone chose; the resolver's own branches say a card archived from a declared column goes back to it. ## One expectation of mine was wrong, and the resolver was right I expected a card archived from the review lane to return to **hold**. It returns to the review lane, and that is correct: `.review` is derived from the `mergeOrchestration` flag, **not** from `human-review`. The fixture's review column declares `human-review` + `merge-blocker` only, so it is not a `.review` lane to the resolver — just a declared column with usable history. The case now asserts that with the reasoning attached, so the next reader does not "fix" it back to hold. ## Verification - unarchive suite — **5/5**, mutation matrix above - `pnpm test:gate` — **exit 0** - full live-PG E2E surface — **164/164** - `pnpm lint` — clean Changeset included (`patch`, category `fix`). 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
gsxdsm
|
e5c9ea3870 |
fix(core): resolve the task's own terminal node in the node-override guard (#2812)
## What
Fixes the defect **#2793 measured** but deliberately did not fix. That
PR's two characterization tests are flipped here to assert the correct
behaviour — which is what they were written to do.
## The bug
`updateTask({ nodeId })` passes through `validateNodeOverrideChange`
**twice**, and both calls were wrong in different ways:
| call | how it answered "is this terminal?" |
|---|---|
| `branch-and-pr-entities.ts:568` | `resolveTaskWorkflowIrSync` — the
**default** workflow for every task under PostgreSQL |
| `task-update.ts:53` | no options at all → `defaultIsTerminalNodeId`,
the bare literal `nodeId === "end"` |
On a board whose terminal node is not named `end`, the FN-7641 guard
inverted in both directions:
- an override to a **non-terminal** node that happens to be named `end`
was **rejected** with a merge-proof error about finalizing a card the
operator was not finalizing;
- an override to the board's **real** terminal node was **written
verbatim**, no error, card unadvanced — the silent no-op FN-7641 exists
to prevent.
## The fix
A new `isTaskTerminalNodeIdAsync` resolves the task's own workflow, with
the **identical** literal fail-soft for an unresolvable one. Both call
sites use it — pre-resolved, because `validateNodeOverrideChange`'s
callback is synchronous and it asks the question at most once.
**Nothing forced the sync call at either site**: both frames are already
`async` and already awaiting. That is the same finding as #2809's
review, one file over.
The sync helper is **deleted, not kept as a fallback**. Keeping both
would re-create the half-conversion this program keeps finding — one
caller resolved, one not, and no way to tell from a call site which it
got. `branch-and-pr-entities.ts` also leaves the sync-resolver call-site
allow-list (ratchet green, 3/3), the **second** of the six allow-listed
sites to close.
## Why both halves were needed — and how that is proven
#2793's mutation matrix showed the rejected-`end` case is
**over-determined**: both guards independently called it terminal, so
correcting either one alone changed nothing an operator could see. That
is why fixing only the allow-listed sync site would have looked like
progress and delivered none.
Re-measured here, on the fixed tree:
| state | result |
|---|---|
| both guards fixed | **3/3 pass** |
| inner guard reverted to no-options | **1 fails** |
| outer guard reverted to the literal | **1 fails** |
## Tests
#2793's two cases now assert the fixed behaviour and keep their
reasoning:
- the non-terminal `end` override is **written**, and the card stays in
review — a routing change, not a finalize;
- the real terminal `finish` override is **refused** without merge
proof, **and** the field is not written on the way to refusing.
The fixture-integrity case (`finish` is the end node, `end` is not) is
unchanged — it is what stops both assertions passing for the wrong
reason.
## Verification
- terminal-node suite — **3/3**, both-halves matrix above
- sync-resolver call-site allow-list ratchet — **3/3**
- `pnpm test:gate` — **exit 0**
- full live-PG E2E surface — **151/151**
- `pnpm lint` — clean
Changeset included (`patch`, category `fix`).
🤖 Generated with [Claude Code](https://claude.com/claude-code)
---------
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
||
|
gsxdsm
|
ed83fd6ec3 |
batch-core: node-override guards let a running task be re-routed on a renamed board (45 → 43) (#2821)
## The defect Two guards in `node-override-guard.ts` answered a **role** question with a **column name**: - **`task.column === "in-progress"`** refuses changing a task's node override mid-flight. On a renamed board it never matched, so an operator could re-route a **running** task — precisely what the guard exists to prevent, and the failure is silent because the guard simply returns `allowed: true`. - **`task.column !== "done"`** gates overriding *to* the terminal node. On a renamed board it never matched either, so the override was refused for exactly the tasks that had legitimately reached the end node. Both fail in the direction that looks like normal behaviour rather than an error. ## Why the lanes are injected rather than resolved in place `validateNodeOverrideChange` is **synchronous by design**, and its existing `isTerminalNodeId` option already establishes the pattern: callers with cheap IR access inject, callers without keep a documented literal fallback. **Both production callers now supply the lanes** — `branch-and-pr-entities.ts:594` (which already injected `isTerminalNodeId`) and `task-update.ts:53`. That was the deciding factor: an optional parameter that only tests fill is the inert-injection shape this program keeps finding, where a guard reads as converted, its test passes because the test injects the value, and production keeps the literal. I checked both call sites had a store in scope *before* adding the option. `resolveNodeOverrideLanes` lives beside the guard rather than in the callers, so the two cannot drift about what "executing" and "completed" mean. ## Fallbacks A workflow expressing **no trait on any column** is a v1 upgrade — `synthesizeDefaultColumns` emits `traits: []` everywhere — not a board without these roles, so it keeps the legacy ids. Same for an unresolvable workflow. Both preserve exactly the behaviour the literals already had. ## Verification - **Mutation-verified per guard:** restoring `task.column === "in-progress"` fails a case; restoring `task.column !== "done"` fails a different one. - The suite also pins the paired negative — resolving lanes must not turn the guard into a blanket refusal for a task outside every WIP lane. - `node-override-guard.test.ts` → 27 passed - `pnpm test:gate` → 161 + 487 + 13 + 71 - `--strict` → exit 0; `tsc --noEmit` and `pnpm lint` → 0 errors Census: batch-core scope **45 → 43**; repo total **255**. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **Bug Fixes** - Node overrides now correctly recognize workflow-defined in-progress and completed lanes, including renamed columns. - Override validation falls back safely for legacy or unresolved workflows. - Prevented validation from using stale task-column information during updates. - **Tests** - Added coverage for workflow lane resolution, legacy fallbacks, renamed lanes, and override eligibility. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
gsxdsm
|
bcc8bf17c1 |
fix(core): brand the unregistered built-in workflow fallback (#2815)
> **Corrected after opening.** The first version of this PR claimed the
id cross-check reported `"default"` for *every* authored workflow and
stranded cards in triage recovery forever. That was wrong, and the
correction is below in full. The reachable defect is the branding hole;
removing the id check is correctness, not a live bug fix.
## The bug (reachable)
`resolveWorkflowIrById` has four ways to substitute the default coding
IR. Three brand the result via `markFellBack`. The fourth did not:
```ts
if (isBuiltinWorkflowId(workflowId)) {
const builtin = getBuiltinWorkflow(workflowId);
const ir = builtin?.ir ?? defaultCodingWorkflowIr(); // <- unmarked substitution
```
An id that *looks* built-in but is not registered — a workflow removed
between releases, a typo'd selection — lands there and silently gets the
default coding IR. `resolveWorkflowIrForTaskWithProvenance` then reports
**`source: "selection"`** for it, handing a caller the default board's
graph under the selected workflow's name. That is precisely the lying
signal the API exists to prevent.
Found by review on this PR (thanks — see the thread), not by me.
## The correction to my own claim
I also deleted an id cross-check that ran after the marker check and
reported `"default"` when the resolved IR's `id` differed from the
requested workflow id. I justified that by saying it misfired for every
authored workflow, because `createWorkflowDefinition` stores an IR
verbatim while minting `WF-NNN` separately:
```
store workflow id = WF-001 stored ir.id = custom:prov
PROVENANCE source = default resolved ir.id = custom:prov <- the CORRECT IR, called a guess
```
**That measurement is real but it came from this suite's own fixture.**
Neither `WorkflowIrV1` nor `WorkflowIrV2` declares an `id` field. An
editor-authored workflow carries none, so `resolvedId` is `undefined`
and the check passed it as `"selection"` — the misfire never reached
`triage.ts`'s post-U11 intake recovery, the one production consumer. My
"declined forever, not deferred" claim does not hold.
Removing the check is still right: it is **unreliable** (it interrogates
a property the IR types do not declare, and when one is present it is
the author's id, unrelated to the store-minted row id) and **redundant**
(all four substitutions are now branded, and the marker is checked
first). But it is a cleanup, not a fix — and saying otherwise is how a
narrow change gets backported as a critical one.
## Tests
| case | asserts |
|---|---|
| unregistered **built-in** id | **`source: "default"`** — the reachable
defect |
| missing definition | still `"default"` |
| no selection | still `"default"` |
| IR carrying its own id | `"selection"`, with the id mismatch asserted
explicitly so it can't pass for the wrong reason |
| the resolved IR is the task's own board | contains the renamed hold
column, not a default-board id |
**Mutation-verified:** removing only the branding fails the
unregistered-builtin case and nothing else — which shows it covers that
specific hole rather than overlapping the others. Restoring the id check
fails only the carries-its-own-id case, leaving both fallback cases
green, which shows the deletion did not widen trust.
## Blast radius
`triage.ts:1211` is the only production consumer of the provenance
result across core, engine, dashboard and CLI. Every other `.source ===`
hit is an unrelated field on an unrelated type; the two index hits are
re-exports.
## Verification
- new suite — **5/5**, mutation-verified in both directions
- `pnpm test:gate` — **exit 0**
- `pnpm lint` — clean
Changeset included (`patch`, category `fix`), rewritten to describe the
branding fix rather than the overstated one.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
---------
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
||
|
gsxdsm
|
d2f47acedd |
fix(tests): the core half of #2783's bookkeeping — archived-gate inventory (#2817)
## The 6th red from #2783 #2814 cleared the 5 **engine** reds #2783 left on `main`. This is the sixth, in **core** — I found it after #2814 was already open, and it merged before I could fold this in. ``` archived-column-gate-parity > all three encodings of the archived gate stay in lockstep with the audited inventory AssertionError: TypeScript encoding changed. ``` ## Same cause, same shape as #2786 #2783 converted three more sites off raw `column === "archived"` comparisons and did not update the inventory in the same commit — which the guard's own failure text explicitly asks for. | file | before → after | |---|---| | `async-mission-store.ts` | 2 → 0 | | `task-store/symbol-locks.ts` | 1 → 0 | | `task-store/archive-lifecycle-2.ts` | 2 → 1 | **Verified each is a real conversion, not a dropped gate.** All three now seed a legacy lane set and extend it from the workflow: ```ts const lanes = new Set<string>(["done", "archived"]); … for (const id of columnsWithFlag(ir, "archived")) lanes.add(id); ``` The literal still in `archive-lifecycle-2.ts:46` is `column: "archived"` as a **move destination**, not a gate comparison — the same distinction the planner-lane move targets get. ## Verified NOT a split-brain That is the thing this file exists to catch — TypeScript moving to the resolved role while the SQL sides keep comparing the raw string. The **Drizzle and raw-sql inventories are unchanged and both pass**. Worth stating explicitly because those assertions run *after* the TypeScript one: a plain red tells you nothing about them, so they had to be re-run green to know. ## One thing I nearly got wrong My first edit was a whole-file string replace and it threw on an assertion count. That turned out to be load-bearing: **these paths appear in more than one inventory in this file** (`AUDITED_TS_SITES` and the raw-sql inventory both list `async-mission-store.ts`). An unscoped replace would have silently edited the raw-sql inventory too — making the parity guard agree with itself and defeating the exact cross-encoding check it exists for. The edit is now scoped to `AUDITED_TS_SITES` by line range. ## Evidence - Guard still bites: appending a real `task.column === "archived"` to an audited file → **fails**. - Core **4773 passed / 0 failed** · engine **10988 passed / 0 failed** · gate **732 green** · lint clean. Test-only; `agent-store.ts` restored clean after the mutation. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
gsxdsm
|
6fc98fd6c7 |
the third census-invisible class: 51 hardcoded moveTask destinations, measured — and duplicates never archived on a renamed board (#2808)
A third census-invisible class, measured — plus the two worst instances
fixed.
## The shape
```ts
if (task.column !== "in-review") { … return; } // the census counts THIS
await this.store.moveTask(taskId, "in-progress"); // and cannot see THIS
```
The census is an AST scan for **comparisons**. A `moveTask` destination
is a **call argument**, so no backlog entry ever points at one.
Converting the guard alone is *worse than converting neither*: the
handler starts admitting work on a renamed board and then tries to move
the card into a lane that board may not declare.
This bit twice in one week — #2797 (`branch-worktree` requeued into a
lane that may not exist) and #2807 (a GitHub "changes requested" review
dropped, then a move to a hardcoded `in-progress`). Both times it was
found only because the guard *next to it* happened to be under
conversion. So I went looking.
## Measured
Across `core`/`engine`/`dashboard`/`cli`/`plugins`, excluding
`__tests__`/`*.test.*` and comment lines:
| | count |
| --- | ---: |
| hardcoded `moveTask` destinations in production | **51** |
| …passing `recoveryRehome: true` — **deliberate**, not defects | 22 |
| …plain, rejected on a board that does not declare the target | **29**
|
**The 22 must not be "fixed".** `moves.ts` exempts them on purpose
(#1411): a card stranded in an undeclared column has to stay rescuable
to a legacy safe-landing column, or it can never be recovered at all. A
sweep that converts them deletes the rescue path. That distinction is
the reason this is 29 and not 51, and it is why I measured before
writing.
## Why this got sharper recently
The `workflowHasColumn(workflowIr, toColumn)` rejection used to sit
inside a block gated on `isWorkflowColumnsCompatibilityFlagEnabled` — a
settings key **nothing in production writes** — so it never executed and
the legacy `VALID_TRANSITIONS` table decided instead. U12 hoisted it out
of that dead branch and it is now live, proven on a real store by
`live-move-path-undeclared-target.test.ts`:
```
moveTask(card in "todo" -> "triage") now REJECTS: /Unknown column for this workflow/
```
That changed the failure mode of all 29 from *"silently lands the card
in an undeclared column"* to *"throws"*.
**29 is not a crash count.** Whether a throw surfaces or disappears
depends on whether the caller catches, which is per-site and I did
**not** measure it — the doc says so explicitly rather than letting the
number imply severity it hasn't earned.
## Fixed here: 9 of the 29
`duplicate-intake` and `duplicate-guard` both archive a duplicate. On a
renamed archive lane the move is rejected, so **the duplicate is never
archived and keeps sitting on the operator's board as live work** — and
in `duplicate-guard` the row has already been stamped
`deterministicDuplicateOf`, so it is *marked* a duplicate while
occupying an active lane. Half-applied, which is the same trap as
#2797's branch clear.
Both now resolve the `archived`-trait column from the task's own
workflow through one shared helper, unioned with the legacy id.
**`cli/commands/task-lifecycle`** — `finalizePullRequestMerge` and
`finalizeNoOpMergeTask` both move the card to a hardcoded `"done"`, and
both run `updateTask({ status: null, mergeRetries: 0 })` *first*. On a
rejection the merge has already landed and the bookkeeping is already
cleared while the card never reaches its complete lane: the operator
sees a merged branch, a card still sitting in review, and a reset retry
counter. Same half-applied shape as #2797's branch clear. Both now route
through one resolver so they cannot drift.
**`contamination` / `foreign-only-contamination` (×2) /
`restart-recovery-coordinator`** — four recovery requeues to a hardcoded
`"todo"`, none of them a `recoveryRehome` escape. On a board without
that column the move is rejected and **the recovery never completes** —
the card stays contaminated or stranded, which is precisely the state
these paths exist to clear.
**Consolidation.** `resolveReboundTargetForTask` and
`resolveArchiveTargetForTask` now live beside
`resolveTaskLifecycleColumns` in `workflow-lifecycle-traits`, already
the store-dependent resolution seam. My first pass put the archive
helper inside `duplicate-intake` and had `duplicate-guard` import it
from there — wrong home, and it would have grown a copy per caller as
more sites converted. Seven call sites now share two definitions.
**Plain (non-`recoveryRehome`) destinations: 29 → 21.**
**Coverage on the CLI pair is scoped, and I'd rather say so than imply
more:** the test covers the *resolver*, not the two call sites. Both
enclosing functions are private and reachable only through
`processPullRequest`, which needs a live GitHub surface — exporting them
purely to test wiring is a worse trade than stating what is covered.
Three cases: renamed lane resolves, no-workflow falls back to the legacy
id (which also pins that a default board is byte-identical), and a
throwing lookup falls back.
## Revert result (measured)
| conversion | reverted → |
| --- | --- |
| duplicate archive destination | new case fails — `moveTask` called
with `"archived"` on a board whose archive lane is `boxed` |
| CLI complete-lane resolver | replacing the body with a bare `return
"done"` fails the renamed case |
| both move-target resolvers | replacing either body with a bare return
of its legacy id fails 5 cases across the resolver suite and
`duplicate-guard` |
Each resolver has a **non-vacuous companion** asserting it does *not*
return the legacy id on a renamed board — without it, a resolver
returning any string would pass. The fallback cases are load-bearing
rather than padding: `resolveWorkflowIrForTask` degrades to the built-in
IR rather than throwing, and the built-in rebound/archive lanes *are*
`todo`/`archived`, so those cases also pin that a default board is
byte-identical.
The pre-existing case asserting the legacy `"archived"` passes both
ways, which is exactly why it could not detect this and why the new one
supplies a workflow.
## Ownership note
`packages/core` was `batch-core`'s territory and `packages/cli` was
`batch-cli-plugins`'. Both batches have landed, and this is
newly-discovered work in the class documented here rather than leftover
conversion backlog. Four sites, two shared helpers — happy for either
half to move if those owners would rather carry it.
## Verification
- `pnpm test:gate` — 161 + 487 + 13 + 71, green
- `duplicate-guard` + `duplicate-intake` — 40 passed
- `tsc` on core and engine — clean
- `pnpm lint`, `check:changesets`, census `--strict` — all clean (run
explicitly; a clean `pnpm lint` alone is not evidence the CI Lint check
passes)
<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit
- **Bug Fixes**
- Duplicate tasks are now archived to each workflow’s configured archive
lane.
- Completed tasks are moved to the workflow-specific completion lane,
with a safe fallback for older workflows.
- Recovery and requeue actions now use each workflow’s configured
rebound lane instead of assuming a fixed destination.
- **Documentation**
- Added guidance on avoiding failures caused by hardcoded workflow
destinations and incomplete lifecycle conversions.
- **Tests**
- Added coverage for renamed workflow lanes, fallback behavior,
duplicate archiving, and recovery destinations.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->
---------
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
||
|
gsxdsm
|
240a6be0aa |
fix(core): dependency update deadlocked on a self-blocked task (#2810)
## The bug `updateTaskDependenciesImpl` wraps its whole body in `store.withTaskLock(id, …)`, then reads the current blocker with `readDepTask(task.blockedBy)` → `store.getTask()`. `getTaskImpl` opens with `withTaskLock(id, …)` too, and the per-task lock is **non-reentrant**. So when `blockedBy` is the task's **own id**, the call waits forever on a lock its own frame holds — and holds that lock while doing so, leaving the row permanently unlockable. ## Found by generalising #2809, not by luck #2809 removed one `getTask`-inside-`withTaskLock`. An AST scan for the same shape across `packages/core` and `packages/engine` returned **exactly three sites**: | site | verdict | |---|---| | `lifecycle-ops.ts:1049` | the deadlock fixed in #2809 | | `update-task-deps.ts:233` (`assertTaskExists`) | **safe** — a self-dependency is rejected 15 lines earlier | | `update-task-deps.ts:344` (`readDepTask`) | **this bug** | Both surviving sites carry the same `FNXC:SqliteDualPathCleanup` note — *"In backend mode, readTaskFromDb uses store.db (SQLite) which is unavailable. Replace with async store.getTask() calls."* That port is the common cause across the whole class: it swapped a **lock-free** read for a **lock-acquiring** one. ## Why `blockedBy === id` is reachable The dependencies list rejects self-reference explicitly (*"Task X cannot depend on itself"*) — and that guard is precisely why the sibling `assertTaskExists` read on this same lock is safe, so it is left unchanged. **`blockedBy` has no such guard:** `updateTask({ blockedBy })` accepts the task's own id. The first test asserts that rather than assuming it. The whole regression rests on that state being reachable, so it is proven, not stipulated — and it also pins the asymmetry, so a future guard on `blockedBy` will show up here as a deliberate change. ## The fix Return the in-lock copy already in scope instead of re-reading. One line, no new read path, and **strictly more correct than a re-read**: it is the state this mutation is reasoning about, rather than whatever a concurrent writer left behind. ## Verification - **Mutation-verified against the real defect.** With the fix reverted the regression case fails by name — `updateTaskDependencies did not settle within 8000ms — deadlock` — while the precondition and the ordinary-path cases stay green. That is the actual pre-fix behaviour. - **A differential** covering the ordinary case (blocked by *another* task). Without it, a fix that short-circuited *every* blocker read would pass everything else. - Timeboxed for the same reason as #2809: a deadlock otherwise surfaces as a suite-level timeout naming no case. Not a flake knob — the fixed path settles in ~0.5 s and the broken one never settles. - `pnpm test:gate` — **exit 0** - `pnpm lint` — clean Changeset included (`patch`, category `fix`). Independent of #2809 — different file, no overlap — but the same class, and the scan above is the argument that the class is now closed. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
gsxdsm
|
74cba4b46d |
batch-core: one shared landed-lane helper for the source-issue surfaces (75 → 72) (#2783)
## batch-core continued — the source-issue cluster Follow-on to #2780 (merged). Scope is still `packages/core` + `packages/dashboard/src`. ### The defect Five places asked the same question — *has this task landed?* — and all five compared against the literal `done`: | surface | consequence on a renamed board | |---|---| | GitHub source-issue commenter | never comments on or closes the source issue | | GitLab source-issue commenter | same | | GitLab `closedAt` backfill reconciler | finds nothing, reports a clean scan | | session-diff boundary | finished tasks diff against an already-merged branch | | tracking-comment transition | (already converted; left alone) | The commenters are the sharpest case: they returned **before reading a single setting**, so on a renamed board the feature looked *disabled* rather than broken — an operator checking `githubCommentOnDone` would see it enabled and still get nothing. The backfill is the quietest: `scanned: N, filled: 0` reads as "nothing to do", so the failure was indistinguishable from success. ### The fix One home: `packages/dashboard/src/task-lifecycle-lanes.ts`. Callers now only ask. Five copies of one question is exactly how the halves drift apart — the motivating incident is FN-6115 → FN-6118 → FN-6123, where the same affordance was fixed three times because it lived in two components. This also folds in the duplicate landed-lane helper I had left in `register-session-diff-routes.ts` in the previous PR, which was the sixth copy waiting to happen. Two helpers, and the difference is deliberate: - **`landedColumnsForTask`** — `complete ∪ archived`. Membership, since a board may declare more than one column carrying either role, and `columnsWithFlag(...)[0]` would silently ignore the second. - **`completeColumnsForTask`** — complete only. The GitLab backfill's own FNXC note records that archived tasks live in `archiveDb` and are *intentionally* excluded, so it must not widen to the archived role just because the shared helper offers it. Today it lists with `includeArchived: false` and would see no archived rows either way — but that is an incidental property of the query, not the contract. The test pins the difference so the two are not later "simplified" into one, which would change that caller's behaviour without touching it. Both treat an **empty** resolved set as *unexpressed*, not absent — the v1 hazard: `synthesizeDefaultColumns` upgrades a v1 graph with `traits: []` on every column, so reading empty as "no complete lane" would stop these surfaces firing on every pre-v2 project. The reconciler is two-stage on purpose: the cheap provider and `closedAt` tests run first and reject almost everything, so a workflow read only happens for real candidates, and it shares one IR cache across the scan — one read per distinct workflow rather than per task. ### Census `batch-core` scope **75 → 72**; repo total **338**. ### Verification - `pnpm --filter @fusion/dashboard exec tsc --noEmit -p tsconfig.json` → 0 errors - `pnpm lint` → 0 errors - commenter + reconciler suites → **63 passed**; helper suite → **5 passed** - **Mutation-verified:** making the helper ignore its resolved set fails 1 of 5. --- ## Round 2 — server.ts, chat.ts, and a correction **Census: 75 → 67** across this PR. ### The correction (see the review thread above) My first pass gated the source-issue commenters on `landedColumnsForTask` (`complete ∪ archived`), which **widened** the trigger — `to === "done"` never fired on archival, and the landed set does. Both commenters now use `completeColumnsForTask`, and the unused `hasTaskLanded` wrapper is gone. The ratchet for it is pinned on the **default** board, deliberately: a widening is visible exactly where the legacy names still apply, so no renamed-board fixture would catch it. ### `chat.ts` — three sites, and a pair that had to move together - **Chat verification** required `column === "in-progress"`, so on a renamed board every chat-driven verification was refused with a message naming a column the board does not have. - **The planner refinement pair.** Two separate guards decide this feature: `createSession` *registers* the tool only for a finished task, and the tool's own `execute()` *refuses* a non-finished source. Both compared `done`. Converting only one half would have offered the tool and then had it refuse itself — the half-converted-pair shape. The new test asserts **both** halves in one case (tool present *and* refinement created), and each half reverted independently fails it. Existing `chat-manager` coverage caught neither revert, which is why the case exists rather than relying on the suite that was already there. Complete-only again, not the landed set: an archived task is off the board and is not a refinement source. ### `server.ts` - **Planner-chat retention** — the archival cutoff was a literal, so on a renamed board task-planner chat sessions were retained forever; the rule this listener exists to enforce never fired. Resolved, and awaited inside the existing fire-and-forget chain rather than by making the listener `async` — `task:moved` has synchronous subscribers whose ordering is load-bearing elsewhere, and a chat-row delete is not the right place to introduce a microtask boundary into that emit. - **`isBadgeEligibleTask` — deliberately NOT converted, and marked as backlog.** On a renamed board it is genuinely wrong: an archived card stays badge-eligible, its snapshot is never evicted, and the cache grows for the daemon's lifetime — the exact memory leak the predicate was added to fix, back under a different column name. What blocks it is measured, not assumed: both callers are synchronous `task:updated` / `task:created` listeners whose next statement is documented as *"Update local cache immediately"*, so awaiting lets a second event for the same task interleave between the eligibility check and the cache write. I did **not** add an optional `archivedColumns` parameter, because nothing could fill it — the callers are the sync listeners. That is the inert-injection shape this PR's own review caught twice on #2780: the predicate would read as converted, its test would pass by injecting the value, and production would keep the literal. The unblocking change (a resolved-archived-lane cache on the badge-snapshot scope, keeping the predicate synchronous) is recorded at the site. ### Verification - `tsc --noEmit` → 0 errors; `pnpm lint` → 0 errors - `chat-manager` → 101 passed; commenter/reconciler/helper/badge suites → 55 passed - Mutation-verified per fix, including each half of the refinement pair separately <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Improved task lifecycle handling for renamed workflow lanes, including completed, archived, landed, and in-progress states. * Task lists now exclude completed tasks regardless of the completion lane’s name. * Chat verification and refinement actions now recognize configured workflow lanes. * GitHub and GitLab completion comments trigger only for genuinely completed tasks, not archived tasks. * Knowledge index refreshes and GitLab metadata updates now support custom completion lanes. * **Tests** * Added regression coverage for renamed completion lanes and archived-task behavior. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
gsxdsm
|
e84e9d7f60 |
fix: the caller audit — five unwired parameters, five defects in their callers (#2803)
Seven fixes that were sitting on separate handoff branches with no owner while `main` moved. Consolidated, rebased onto current `main`, and verified **together** rather than only per-branch. The individual branches remain if a subset is preferred. This is the same consolidation that got `batch-core` and #2787 adopted. **Close it if it breaks queue policy** — the branch keeps the work safe either way. ## Where these came from #2787's review found an optional parameter whose production caller never passed it. That is a class, so I ran it against everything I had landed and found five more. **All five turned out to have their real defect in the CALLER, not the parameter** — in four of them the parameter was unreachable: | unwired parameter | what was actually wrong | |---|---| | `blocker-fanout.escalationColumns` | the hold default made the count zero — **no bottleneck warning was emitted at all** | | analytics `columnFlagsByName` | routes never built a map — **0 in-progress / 0 in-review beside correct cost totals** | | `isLegacyAutoMergeStampCandidate` | the read **queried a column a renamed board does not have**, so the backfill iterated nothing | | `rankAssignedTasksForWakeDelta` | `getTasksByAssignedAgent`'s `excludeArchived` used the literal — **archived cards returned as open work** | | `duplicate-intake.columnFlagsByColumnId` | intake could **archive or soft-delete a newly created task** as a duplicate of finished work | The heuristic worth keeping: **an optional parameter no production caller fills is a marker pointing at an unexamined caller.** The census cannot see any of these five — every gate is a `Set`/array literal or a query filter, i.e. a definition rather than a comparison. ## Also included - **`executor.ts`** — the stale-spec guard did the exact thing its own comment forbids: on a renamed board it ran on a LIVE task and pulled it out of execution into replan. `activeMergeStatuses` protected merging cards *by accident*, which is why the symptom looked arbitrary. - **`register-project-routes.ts`** — project health reported **0 active tasks**; its list also still contained `triage`, dead since U11. - **`dashboard/app/utils/taskTiming.ts`** — a **second copy** of `getTotalAgentActiveMs`. Core's was converted; the card chip imports this one, so the census counted the site as done while the rendered number stayed keyed on `"in-progress"`. ## Verification Verified as a set: `pnpm test:gate` **161 / 13 / 487 / 71** · core suites **15 passed** · engine **7** · dashboard **12** · four `tsc` targets clean · lint clean · census `--strict` exits 0. Each fix is revert-proven individually; the specific case that fails is named in each test header. ## Two honesty notes **Three guards here are structural, not behavioural, and say so in their headers.** `sanitizeAgentTaskLinks` is a closure inside `createApiRoutes`; the analytics aggregators need a live `AsyncDataLayer`; the stale-spec guard sits deep inside `execute()`. Each ratchet fails on revert — verified — but none is an end-to-end proof, and the headers state which half they cover. **One of my behavioural test sets would have lied.** The intake-dedup cases drive `findSameAgentDuplicates` directly; I removed the wiring to measure the revert and **they stayed green**, because they pin the predicate and not the caller. That is the exact illusion this audit was chasing, reproduced in my own file. The forward now has its own structural check. ## Deliberately not included `worktree-pool.ts:1205` — it **fails safe** (a missed match protects a branch from cleanup rather than deleting it) and sits in the merger's branch-reaping path where the opposite error destroys work. That deserves its owner's judgement, not a drive-by conversion. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
gsxdsm
|
02d0f80068 |
fix(tests): update the archived-gate inventory after #2780's conversions (#2786)
## New red on main `archived-column-gate-parity.test.ts` fails with **"TypeScript encoding changed"** after batch-core (#2780). It is the only failure in a full `@fusion/core` run (`1 failed / 4748 passed`). ## The conversions are right — this is their missing half #2780 moved four files off raw `column === "archived"` comparisons onto `isTerminalColumnRole`, exactly the intended role-based pattern: | file | before → after | |---|---| | `assigned-task-ranking.ts` | 1 → 0 | | `duplicate-intake.ts` | 1 → 0 | | `near-duplicate-canonical.ts` | 1 → 0 | | `store.ts` | 2 → 1 | (The literal still in `duplicate-intake.ts` is a `moveTask` **destination**, not a gate comparison — a different question, like the planner-lane move targets.) The guard's own failure text asks for the inventory to be updated **in the same commit** as a conversion. That did not happen, so the ratchet went red on main. This PR is only that bookkeeping. ## Verified NOT a split-brain That is the thing this file exists to catch — TypeScript moving to the resolved role while the SQL sides keep comparing the raw string, so on a renamed board one says a task is archived and the others return it as live. The Drizzle and raw-sql inventories are **unchanged and both pass**. Worth stating explicitly because those two assertions run *after* the TypeScript one: a plain red tells you nothing about them, they had to be re-run green to know. ## The guard still bites Appending a real `task.column === "archived"` to an audited file fails it immediately. **Recorded because it nearly fooled me:** my first two mutation attempts *passed*, which looked like a guard blind to new comparisons — a much worse finding than a stale inventory. Both had been inserted at line 2 of a file whose line 1 opens a JSDoc block, so my "code" was comment text and was never compiled. **A mutation that does not compile is not evidence of anything.** Appended at end of file instead, the guard fails on the first run. Gate **732 green** · lint clean. Test-only. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
gsxdsm
|
31b9fafe11 |
fix(tests): the last core red — assert the funnel COUNTS the move, not which stage owns it (#2781)
## What was red
A full `@fusion/core` run on `origin/main` reports **1 failed / 4700
passed**. This is that one: the SDLC funnel case in
`agent-logs-and-monitor.pg.test.ts`.
```ts
expect(result.funnel.stages.find(({ stage }) => stage === "todo")?.entered).toBe(2);
// expected 2, received 0
```
## The move is not lost
Post-U11 the default Planning column is **one** column carrying
`["intake","hold","reset-on-entry"]`. `stageForTraits` prefers the
earliest stage in flow order, so `intake` wins and a move to `todo` is
attributed to the **`triage`** stage. Analytics is working correctly;
the column vocabulary underneath it merged.
## Why not just re-point the assertion
Which stage the merged Planning column *should* report is an open
product question — I flagged it on #2669 while adding the `hold`
mapping, and it is visible to users: **the funnel shows a phantom 100%
drop between Triage and Todo on every default board since U11.**
- Re-pointing the **test** at `"triage"` quietly blesses the phantom
drop.
- Re-pointing the **mapping** retroactively changes how historical
analytics read — not a reversible call.
Neither belongs in a change whose job is clearing a red.
So the assertion now pins what is true under **either** resolution: both
moves are counted exactly once, in the single pre-implementation stage
the Planning column resolves to. When #2669 is decided, this test does
not need rewriting.
## Evidence
**6/6 passed.** Mutation-proved for the failure that actually matters:
| mutation | result |
|---|---|
| unmap every pre-implementation trait (move falls to `OTHER`) |
**fails** — `expected +0 to be 2` |
| unmap `intake` alone (attribution shifts triage → todo) | **passes, by
design** — that is the open question, not a defect |
The second row is the point of the rewrite: the test is indifferent to
the unsettled question and strict about the invariant. It still catches
a dropped, double-counted, or split move.
Gate **732 green** · lint clean. Test-only — no production file touched
(the mutations above were reverted; `git diff` clean).
## Ownership
`packages/core` belongs to the **batch-core** owner under the mega-batch
split. This is fix-forward on a red rather than a conversion, so it is
deliberately confined to one assertion in one test file and touches no
production code — it should not conflict with the batch.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
||
|
gsxdsm
|
8c9b84ae38 |
batch-core: packages/core + dashboard/src lifecycle conversion (129 → 92) (#2780)
## batch-core — `packages/core` + `packages/dashboard/src` Shared branch: two workers are converting into it. Opening the PR because the branch was green with none, and a branch without a PR merges nothing. ### Census Measured with `node scripts/lifecycle-column-census.mjs --json`. | | guards | |---|---| | batch-core scope at branch point | 129 | | batch-core scope now | **92** (51 files) | | repo total now | 358 | Files closed so far: `store.ts` 11→0, `task-merge.ts` 6→0, `live-agent-count.ts` 6→0 (marked, not converted — see #2762), `task-update.ts` 3→0, display-ordering + Wake Delta ranking 5→0, `register-git-github.ts` 4→0. ### The `register-git-github.ts` slice Three PR routes — `pr/create`, `pr/push-branch`, `pr/resolve-conflicts` — plus the `CHANGES_REQUESTED` handler each compared `task.column !== "in-review"`. On a renamed board **none** of them matched, so every PR affordance the dashboard offers was refused for a card sitting in the lane that board calls review, and the refusal named a column that does not exist there. All four now share one helper, `reviewColumnsForTask`, which gets two things right that this program has repeatedly gotten wrong: - **Membership, not a single id.** It takes the broad review set (`mergeOrchestration ∪ mergeBlocker ∪ humanReview`). `resolveLifecycleColumns` returns the *first* column per trait, so a single-id answer silently ignores a board that declares a merge lane **and** a separate human sign-off lane. These guards only refuse or permit — they never move the card — so over-admitting costs nothing while under-admitting refuses a request that should have worked. - **An empty resolved set means UNEXPRESSED, not absent.** `synthesizeDefaultColumns` upgrades a v1 graph by emitting every default column with `traits: []`, so a v1-upgraded workflow resolves to an empty review set while its `in-review` column plainly exists and holds the card. Reading empty as "this board has no review lane" would refuse these routes on **every pre-v2 project** — a worse regression than the one being fixed, and invisible to any v2 test. This is the dashboard twin of the `fn pr create` guard in `packages/cli/src/commands/pr.ts` (#2775). The two surfaces answer the same question and now agree — FN-5893 surface enumeration. ### Testing note: why the seam and not the routes I wrote route-level HTTP tests first and **deleted them**. An express fixture over `registerGitGitHubRoutes` hangs — every case, including the pure refusals, times out at 4s, because registering the router starts background work the fixture never satisfies. Making it run would mean mocking git, the GitHub client, and the pollers: a mock-the-world shell, which is what the project's do-not-add-slow-tests rule (FN-5048) says to avoid in favour of a narrow seam. `reviewColumnsForTask` *is* the narrow seam — it holds the entire decision, and the four call sites now do nothing but ask it and render its answer. Six cases pin it: the renamed lane is returned and `in-review` is not, a two-lane board returns both, a v1-upgraded board falls back, an unresolvable workflow falls back, and the refusal renders lanes an operator can act on. **Mutation-verified, both directions:** reverting the helper to the legacy literal fails 2 of 6; treating an empty set as an answer fails 1 of 6. One fixture bug worth recording, since it would have made the two-lane case vacuous: the trait id is kebab-case `human-review`, not `humanReview`, and the built-in traits must be registered via `import "@fusion/core"` before flags resolve. ### Verification - `pnpm --filter @fusion/dashboard exec tsc --noEmit -p tsconfig.json` → 0 errors - `pnpm lint` → 0 errors - `register-git-github.review-lanes.test.ts` → 6 passed --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
gsxdsm
|
3092c9c2bb |
fleet: live-agent-count.ts 6 → 0 — the no-enrichment fallback, marked not converted (#2762)
Unclaimed file, no overlap with any open fleet PR. Previous PR (#2756) is merged, so this is my one open PR. ## Census | | before | after | |---|---|---| | backlog | 447 | **441** | | reviewed | 38 | 44 | | this file | 6 | **0** | `--strict` exit 0, baseline re-recorded in the same commit. ## Why marked, not converted All six literals sit after a `??` or a `flags ? … :`. Each is reached **only** when the caller supplied no trait flags and no enriched shape — precisely the case `enrichRunningAgentTaskShape` (takes the IR) and `enrichRunningAgentTaskShapeFromFlags` (takes board flags) exist to remove. There is nothing to resolve from there, so the choice is not convert-vs-literal; it is **known legacy answer vs a different guess.** **And the guess is not neutral.** Running and Waiting are *complements* over the same rows: ```ts isWaitingAgentTask = !running && (columnIsIntakeOrHold ?? isLegacyPreImplementationColumn(column)) ``` A card matching neither arm is reported as **neither running nor waiting**, so the footer's queued total silently under-reports it. Guessing "not WIP" or "not review" loses cards from the count; the legacy id at least matches every pre-rename board. That is why this file already carries a `DELIBERATE-LITERAL` marker above `isLegacyPreImplementationColumn` with the same argument — this PR extends it to the three functions holding the remaining fallbacks (`enrichRunningAgentTaskShapeFromFlags`, `terminalKind`, `isRunningAgentTask`). **The fix for a renamed board is at the CALLER** — pass flags, or use the IR-taking enricher. Noted at the site. ## Pattern note for the fleet This is the third file I have taken where `N → 0` is reached by marking rather than converting, and they share a shape worth naming: **a literal after `??` or in the `else` of a `flags ?` ternary is a degraded-mode answer, not an unconverted guard.** The trait path is already there and already correct; the literal is what runs when the trait path has no input. Deleting it does not remove a decision — it substitutes a different one, silently, in exactly the states where nobody is looking (first paint, un-enriched callers, pre-rename data). ## Verification Core typecheck clean · `live-agent-count.test.ts` 11/11 · `--strict` exit 0 · comments only, no behavior change. --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
gsxdsm
|
fdd958efc9 |
fix(test): the archived-gate parity guard could not see an aliased table (red on main) (#2768)
`archived-column-gate-parity` is red on main after #2745 converted three TypeScript sites. Fixing the stale inventory is the small half. **The guard had a hole, and it is the interesting part.** ## The hole The SQL scan required the predicate's receiver to be literally `<x>.tasks`: ```ts ne(schema.project.tasks.column, "archived") // seen const table = schema.project.tasks; ne(table.column, "archived") // INVISIBLE ``` So `branch-group-ops.ts:58` was **never audited**. The inventory claimed six files; seven exist. A parity guard that cannot see one of the encodings reports agreement it never checked — the exact failure mode this file was written to prevent, occurring inside the file itself. ## And that site is not hypothetical `#2745` converted `branch-group-ops`'s **TypeScript** half to the resolved role. Its **SQL** half still compares the raw string. On a board whose archived lane is renamed, the two disagree — one says a task is archived, the other returns it as live. That is the split-brain described in the guard's own failure message, and the guard could not see it. ## The fix Alias bindings (`const <id> = <...>.tasks`) are collected per file in a first pass — first pass because the binding can appear *after* its uses inside nested closures — and accepted as the receiver. `branch-group-ops.ts` joins `AUDITED_SQL_SITES` as **newly visible, not newly written**. Also drops the three TypeScript entries #2745 converted (`blocker-fanout`, `branch-group-ops`, `task-store-helpers`) — that is the red itself. ## Measured, both directions | mutation | result | |---|---| | alias set emptied (the old, alias-blind scan) | **1 failed** — "Drizzle encoding changed" | | product SQL half converted to a resolved lane | **1 failed** — "Drizzle encoding changed" | | as shipped | **2 passed** | The first proves the scanner fix is load-bearing. The second proves the newly-audited site is genuinely *counted*, not merely listed in an inventory. Two earlier mutation attempts produced no output and I discarded them rather than reading them as passes — they had broken the file's syntax, so nothing ran. A mutation that fails to compile proves nothing, and looks identical to a clean run when output is filtered. Gate **726**, core `tsc` clean, lint clean. ## Left for the owner of #2745 Whether `branch-group-ops.ts:58`'s SQL half should now be converted too. The guard's own header explains why the SQL halves cannot simply be converted (`ne(tasks.column, ...)` needs the resolved id as a value, which the call sites do not all have), so this is a real design question rather than a mechanical follow-up — and it is now *visible* and *audited* instead of silently absent. |
||
|
gsxdsm
|
18641ba5d2 |
fleet: the age-staleness hydration site #2746 missed (3rd time in this file), and a blocker that blocked forever (#2749)
## Census | | before | after | |---|---|---| | `packages/core/src/task-age-staleness.ts` | 4 | **0** | | `packages/core/src/blocker-fanout.ts` | 4 | **1** (the marked no-metadata fallback) | | repo backlog | 581 | **573** | Baseline re-recorded; `--strict` exits 0. ## Both were the unconverted sibling in an already-converted file That is the shape this program keeps re-finding, and both files here even carry notes about *previous* P1s on the same question. ### 1. Age staleness never fired `getTaskAgeStalenessSignal` returns `undefined` unless the card is in wip **or** review, then picks its warning/critical thresholds by which of the two it is. Keyed on the literals, a renamed board produced **no age-staleness badge at all**. That is the worst shape a monitoring failure can take: **a missing warning is indistinguishable from health**. Nothing looks broken — "this card has been sitting in progress for a day" simply stopped being said. `reads.ts` already resolves `holdColumn` and `reviewColumn` per row for the sibling signals. Its own comments record a P1 where exactly this role was threaded into a helper but **omitted at both hydration sites** — "same defect, same file, one role over". So this adds the third resolver (`resolveWipColumnForTask`, mirroring the review twin) and threads **both** lanes at **both** sites, off the same per-pass IR cache. The signal's reported `column` deliberately stays on the legacy id: that field is its public shape, which consumers switch on, so renaming it is a separate breaking change rather than part of resolving a guard. ### 2. A blocker that blocked forever `isStaleBlockedByBlocker` decides whether a `blockedBy` marker is stale. Keyed on the literals, a **finished** blocker on a renamed board never read as stale — so the dependent kept its marker permanently and its "waiting on" badge pointed at work that shipped days ago. Every path that clears a stale marker consults this predicate first, so nothing else rescues it. `computeBlockerFanoutMap` — the **only** production caller — already takes `terminalColumns`/`holdColumn`/`classify`, and the file documents two separate P1s about getting this right. The predicate sat on the literals and the call passed nothing. **Both are fixed, and that matters more than it sounds:** converting the predicate alone would have changed *nothing at runtime* while the census scored it as a 4-site win. That is the half-conversion trap, and it is why the wiring gets its own revert proof below. ## Revert proof — each reverted alone | reverted | result | |---|---| | age-staleness lanes → literals | **3 failed** / 8 passed | | blocker predicate → literals | **2 failed** / 9 passed | | fanout **wiring** (`classify` not consulted) | **1 failed** / 10 passed | | none (shipped) | **11 passed** | Each group also carries a paired negative — a non-active lane still raises no staleness signal, and a live blocker is still not stale — so neither fix can degrade into "always fires". ## Verification - 4 core suites (blocker/staleness/age/reads) — **33 passed**, no regressions - new suite — **11 passed** - `pnpm test:gate` — **10 / 158 / 487 / 71** · `pnpm lint` clean · core `tsc --noEmit` **0 errors** ## The 1 remaining `blocker-fanout.ts` keeps one `DELIBERATE-LITERAL`: the no-metadata fallback for an unconverted caller. Deleting it makes an unresolved caller read every blocker as non-terminal, so stale markers would never clear **at all** — strictly worse than the legacy behaviour it would replace. --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
gsxdsm
|
ae4ff9c111 |
test(core): sync workflow resolution always returns the default IR — the proof and a call-site ratchet (#2759)
## A whole class of "conversions" in this program is inert, and the
census scores them as done
`resolveTaskWorkflowIrSync` returns the **default** workflow IR for
**every** task in production. `getTaskWorkflowSelectionImpl` is a
PostgreSQL-cutover stub that returns `undefined` unconditionally
(`workflow-definitions.ts:505` — *"Backend mode cannot synchronously
read PostgreSQL, so return undefined and let the sync reader fall back
to its default"*), so the sync resolver always takes its `!workflowId`
branch. Its return type is **non-optional**, so no caller can detect the
substitution.
**Proven, not argued.** The PG suite binds a task to a workflow whose
lanes are `drafting`/`building`/`checking`/`shipped`, asserts the
**async** reader sees that binding — or the next assertion would be
vacuous — and then shows the sync resolver answering `hold: "todo"` for
the same task. A third case pins the cause directly: the sync selection
reader returns `undefined` while the async one returns the workflow id.
The async resolver on the same task, in the same test, returns the real
lanes. So the remedy for any affected site is always *reach the async
resolver*, never *resolve synchronously and hope*.
### Why this is worse than an unconverted literal
```ts
resolveLifecycleColumns(store.resolveTaskWorkflowIrSync(id))?.hold
```
That **reads** as converted. It resolves an IR, asks for a trait, and
the lifecycle-column census counts it as **progress** — while being
wrong for every custom workflow, silently. A plain `=== "todo"` is
strictly better, because it is at least honest about being a literal.
That is why this needs a guard rather than a comment: the failure mode
is code that *looks right in review*.
### The ratchet earned its place immediately
It allow-lists the six call sites, in the shape this repo already uses
for `engine-no-blocking-shellout` and `check-no-nohup`. On its first run
it corrected the list I had seeded by grep:
- **Found `replan-target.ts`**, which my grep missed — it calls through
an optional-property cast, so no textual search for
`store.resolveTaskWorkflowIrSync` matches it. **Its hazard is the
sharpest of the six:** `resolvePlannerLanes` returns
`resolvedFromWorkflow: true` whenever an IR came back, so on a renamed
board a caller branching on that flag is told the lanes are
workflow-resolved while being handed the **default** ones.
- **Rejected `executor.ts`**, which my grep had matched on a *comment*
with no real call site.
It also carries a completeness case (fails if the scan finds nothing,
rather than passing vacuously) and a staleness case (an entry whose file
stops using the primitive must be removed in the same change, so the
list can't rot into unreviewed permission).
### Verified to fire
Adding a new unlisted call site fails with the offending file named:
```
+ "packages/engine/src/gridlock-detector.ts"
Tests 1 failed | 2 passed (3)
```
My first attempt at that proof landed the probe **inside a block
comment** and the guard correctly reported zero — the methodology was
wrong, not the guard. Worth stating, since a guard I couldn't make fail
is exactly the thing this PR is about.
### Scope
**No source changes.** This pins a fact and guards a primitive. The six
existing sites are deliberately left alone: each needs its own
async-reachability analysis, which is per-site work with real behaviour
risk, not a sweep. `scheduler.ts`'s entry is the honest case — it is
called from synchronous `task:moved` listeners where adding an `await`
would reorder handlers against a synchronous emitter.
### Verification
- new PG suite **3 passed** · new ratchet **3 passed**
- `pnpm test:gate` — **158 / 10 / 487 / 71** · `pnpm lint` clean · core
`tsc --noEmit` **0 errors** · `--strict` exits 0
---------
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
||
|
gsxdsm
|
ea008b4064 |
docs(core): an empty role result has TWO meanings — correcting a rule I applied across three PRs (#2765)
## A correction to a rule I introduced, prompted by #2760 and then measured `synthesizeDefaultColumns` (`workflow-ir.ts`) upgrades a v1 graph by emitting `{ id, name: id, traits: [] }` for the five default ids — **placement only, by design**, with the real trait set living in `BUILTIN_CODING_WORKFLOW_IR`. So a v1-upgraded board arrives at every role resolver looking exactly like a v2 board that declares nothing. Measured on such an IR: | resolver | result | |---|---| | `resolveLifecycleColumns` | `{}` — every role undefined | | `resolveReviewColumns` | `[]` | | `columnsWithFlag(…, "countsTowardWip")` | `[]` | | `resolveTerminalColumns` | `["done","archived"]` — its own legacy fallback saves it | ## The correction I owe I introduced **"resolved and empty means this board declares no such lane"** deliberately across #2731, #2733 and #2734, to fix the *opposite* bug — a legacy fallback masking a genuinely absent lane. I argued for it repeatedly and applied it in several files. It is right for hand-written v2. It is **wrong for the upgrade path**, where it withdraws every role at once. And the two cases are indistinguishable at the call site: both arrive as an empty array. The contrast the test pins is the sharpest evidence — `resolveTerminalColumns` survives the identical IR purely because it never adopted that reading. **The data is the same; the reading is what differs.** ## Affected, named rather than left to be rediscovered - **on `main` today**: `default-workflow-hooks.ts` → `inRole` (from #2734) - **in my open PRs**: the store bypass guard (#2709) and the tracking-state terminal classifier (#2754) Consumers that kept a `length > 0 ? resolved : legacy` guard are unaffected — including the notifier (#2722), which I checked. ## Not fixed here The root repair is for the upgrade to carry real traits instead of placeholders. That changes behaviour for **every persisted v1 workflow**, so it wants its own change with its own measurement — not something slipped into a doc commit. Flagged at the source so the next consumer chooses knowingly. ## Verification 34 trait tests green · core `tsc` clean · lint clean (0 errors) · gate green (487 + 158 + 10 + 71). No production behaviour change; no census movement. |
||
|
gsxdsm
|
86a2b48968 |
fleet: branch-group-ops 6 → 0 — an agent asking for its next task was told there was none (#2739)
Second application of the sync-filter pattern decided in #2737. `branch-group-ops.ts` 6 → **0**. ## The failure `selectNextTaskForAgentImpl` picks an agent's next task by filtering the board for its WIP lane, then its hold lane — both `task.column === "<literal>"`. On a renamed board **both filters match nothing**, so an agent asking for work is told there is none, with its own assigned tasks sitting in the list it just fetched. No error, no log line. The agent idles. `pauseTaskImpl` had the same shape: pausing a running card on a renamed board left its `status` untouched, so the UI kept showing it as working. ## Consumer, not a gate — checked rather than assumed Applying the #2724 test to this file, since it sits closer to the persistence layer than the reconciler did: its **only** SQL predicate is `eq(table.projectId, ...)`. Nothing here compares a column to a literal in SQL, so there is no second encoding of these questions to diverge from. The list arrives from `store.listTasks` and the filters select among rows already in hand. Async predicates were the alternative and would have turned these filter chains into sequential awaits inside the dispatch path. One prefetch, one IR read per distinct workflow, filters stay synchronous. `pauseTaskImpl` resolves for the single task it holds rather than joining a map — different entry point, one id in scope, and a map would have exactly one entry. ## Revert proof | reverted | result | |---|---| | the wip literal | renamed WIP case fails: `expected null to be truthy` | | the hold literal | renamed hold case fails identically | The new test calls the impl **directly** with a store fake resolving a renamed IR. The existing `selectNextTaskForAgent` coverage drives a real store harness, so exercising a renamed vocabulary there means registering a real custom workflow and moving cards through it — heavier than the question, which is only which lane the filters name. The bind evaluator runs for real; only the store is faked. A third case pins that the hold filter keeps its `userPaused` exclusion, so a filter matching every column would not satisfy the other two. **Related coverage checked before writing a new file:** `agent-heartbeat-worktree-renamed-hold.test.ts` covers the requeue **target** on a renamed board, not the dispatcher's **selection** filters — different branch of the same subsystem, so a case added there would have read as duplicate coverage of the wrong thing. ## Verification `pnpm test:gate` **GREEN** (158 + 10 + 487 + 71) · **20 passed** across the routing-policy and new dispatch suites · core `tsc` clean · `pnpm lint` clean · census `--strict` exits 0. 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **Bug Fixes** - Improved agent task selection when workflow lane names or IDs have been renamed. - Agents now correctly resume assigned in-progress or queued tasks across customized workflows. - Prevented agents from selecting tasks paused by users, including on boards with renamed lanes. - Updated task pausing behavior to correctly reflect lifecycle stages beyond default lane names. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
gsxdsm
|
86639f2ce4 |
fleet: planning drain + archive writers 12 → 4 — one stale row starves planning, and a finaliser that wrote an undeclared column (#2742)
**Claimed on #2733 before starting.** `in-process-runtime.ts` + `task-artifacts-ops.ts` — **12 → 4**. ## 1. The planning drain: one stale row stops planning for the whole project FN-8470's own note on this code says it: **one orphan earlier in created_at FIFO prevented every later planning continuation from dispatching.** So on a renamed board the literal terminal pair did not mis-handle one card — an archived or completed card's stale work item read as live, stayed in the due set, and **starved the drain behind it**. The two classifiers take an **optional** terminal set, which is this file's own injection idiom (the specification-complete reaction already takes a `resolveIr` dependency so the pure passes are testable without constructing a runtime that would attach to the real project registry). **Optional is load-bearing:** a *required* parameter would have compiled at every existing caller and then answered "not terminal" for everything. That is the silent direction, and both halves are asserted in the test. ## 2. `moveToDoneImpl` writes `task.column` directly This is the store's own finaliser, not a `moveTask` caller — so its literal is **not** caught by `moveTask`'s unknown-column validation the way every converted call site in this program is. It silently persisted `done` on a board that does not declare it, and then emitted `to: "done"` to every listener. **This is one of the few sites where a literal writes bad state rather than merely failing to act.** A workflow declaring no complete lane now throws instead of inventing one — #2733's rule: a missing field on a resolved struct *is* an answer, and `?? legacy` discards it. ## 3. The unarchive destination — three decisions in four lines, all literal | pre-archive column | lands in | |---|---| | unusable / archived | the **complete** lane | | the **wip** or **review** lane | the **hold** lane (its worktree and session are long gone) | | anything else | back where it was | The second is the expensive one: a card archived *from* the wip lane was restored straight back *into* it **with no worktree**, and the scheduler then counts it as a live holder **occupying a slot**. Made async — its one production caller already is, and the sync alternative is the PostgreSQL no-op documented in #2703. ## Also - **The mission-error requeue** (guard *and* destination in one change): an errored mission task stayed in the wip lane holding a slot, because the guard never matched. - **The planner-chat retention cutoff on archive** — the quiet direction of this defect class: nothing breaks, data that should be deleted simply accumulates, and the only symptom is storage growth nobody attributes to a column name. ## The live defect is not where the census points `reliability-metrics.ts`'s 6 guards are **pure historical readers** over activity-log entries, and **the dashboard does not call them**. The live path is `server.ts`'s `getTaskMovedCountsByDay({ toColumn: "in-review" })` — a **SQL query filter**, the class the census counts separately. So the operator's reliability panel reads zero on a renamed board because of a *query* literal, and converting the six guards the census reports **would change nothing an operator sees**. Converting historical readers also risks reinterpreting past events under today's traits, which is a different decision from converting a live guard — I am not making it inside a vocabulary sweep. Worth generalising for the fleet: **a file's census count and its live exposure are different numbers.** This is the second file where the reported guards are the inert copy and the real one is a query (`executor.ts:5805` was the first). ## Verification `pnpm test:gate` **10 / 158 / 487 / 71** · 31/31 continuation suites · 8/8 archive PG suites · in-process-runtime PG suite green · 5 new cases, **2 red on revert** · `tsc` clean in core and engine · `pnpm lint` clean. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
gsxdsm
|
b0b9d1b373 |
fleet: store.ts 12 → 11 + names the sync-dependency-loop class blocking ~10 sites across 3 clusters (#2709)
Claiming **`packages/core/src/store.ts`** (12). One conversion and a
triage — because **10 of the 12 share a single blocking shape** that is
worth naming once rather than rediscovering per file.
## Census before/after
| | before | after |
|---|---:|---:|
| `store.ts` column guards | **12** | **11** |
Baseline re-recorded; `--strict` exits 0.
## Converted: 1
**1386** — the in-review guard inside `withTaskLock(id, async () => …)`.
Already async, and `this` **is** the store, so
`resolveTaskLifecycleColumns(this, task.id)` resolves the review role
with `in-review` as the fallback. Import added; nothing else in the
method changes.
## The blocking class — 6 sites, and it is not specific to this file
**1772, 1791 ×2, 1874 ×2, 1916, 1917, 1933** all read **another task's**
column — a dependency's, a blocker's, an overlap candidate's — inside
**synchronous callbacks over a prefetched `taskById` map**:
```ts
const unresolvedDeps = (task.dependencies ?? []).filter((depId) => {
const dep = taskById.get(depId);
return dep && !dep.deletedAt && dep.column !== "done" && dep.column !== "archived";
});
```
This is not a substitution. Each dependency may belong to a **different
workflow**, so the role must be resolved *per dep* — N async resolutions
inside a sync `filter`, on a path that deliberately prefetches into a
map precisely to avoid per-item I/O.
Two honest options:
1. **Prefetch lifecycle columns alongside `taskById`** and pass a
resolved map into these predicates. Keeps them synchronous, one
resolution per distinct workflow rather than per dep. This is the one
I'd argue for.
2. Accept per-dep resolution and make the callbacks async — changes the
shape of dependency evaluation.
Both are design changes with real cost, so this is flagged rather than
guessed.
**The same shape appears in at least two other clusters I've worked**:
`TaskDetailModal`'s `overlapBlockerTask.column` (#2696) and
`register-task-workflow-routes`' dependency-summary pair (#2700), both
flagged for this exact reason. **Worth one decision covering all three**
rather than three separate judgement calls by three workers.
## Also flagged: 3
**1610** and **1739** — enclosing-scope async-ness and store access not
established at those points, so not guessed. **1933** belongs to the
sync-filter family above.
## Verification
`pnpm test:gate` **GREEN** (158 + 487 + 10 + 71) · `pnpm lint` clean ·
core `tsc` clean · `--strict` exits 0.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit
* **Bug Fixes**
* Updated failed pre-merge review bypass validation to support custom
workflow boards.
* Tasks can now bypass the step when placed in the board’s configured
review lane.
* Improved error messages to identify the correct review column when
bypassing is not allowed.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->
---------
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
||
|
gsxdsm
|
3066948110 |
fleet: async-comments-attachments.ts NOT converted — the archived gate lives in three encodings (52 sites), pinned by a guard that fails on each (#2724)
Claimed the **async-comments-attachments** cluster (9) and did not convert it. This PR is the evidence for why, as a guard rather than a note — **no production change, census unmoved.** ## What the cluster actually is Every other file in the backlog converts on its own: resolve the task's lifecycle columns, compare against the role. `archived` doesn't, and not by a matter of degree. **Measured in `packages/core` — one rule, three encodings, 52 sites:** | encoding | sites | files | what it decides | |---|---:|---:|---| | TypeScript `=== "archived"` | **37** | 23 | what code does with a row it already has | | Drizzle `ne(tasks.column, 'archived')` | **7** | 6 | which rows a query returns | | raw `` sql`…column != 'archived'` `` | **8** | 5 | same — and invisible to both scans above | Convert only the TypeScript half and a board whose archived lane is renamed **splits**: `getLiveTaskColumn` correctly reports the task archived (it resolved the role) while `readLiveTaskRows` still hands it back as live. A document write is rejected by its gate while its parent is listed as live — a state neither gate alone can produce today. And nothing would catch it. **Every builtin workflow spells that column `archived`**, so all three encodings agree by accident on every board we ship. ## Why the SQL sides aren't just converted too `ne(tasks.column, ...)` needs the resolved id as a **query-build value**, so the IR must be resolved *before* the query — including inside the `for update` document/artifact transactions, which today receive a `db`/`tx` handle and **no store and no workflow reader**. One raw site is a hand-written `SELECT` string, so its comparison isn't even a Drizzle expression that could take a bound value without rewriting the query. The two real options are: thread a resolver into the persistence layer, or **declare `archived` a non-renameable system column** and mark all 52 sites deliberate. Both are decisions with blast radius. Neither is a fleet conversion, so I didn't pick one. ## I was wrong about the shape, and that's the strongest part I wrote the third case as an assertion that **no** raw `sql` template compares a column to `'archived'` — I assumed two encodings. **It failed on the first run with five files.** Nothing in the repo was counting them: they aren't comparisons (invisible to the column census) and aren't `eq`/`ne` calls (invisible to the Drizzle scan). A partial conversion doesn't have to miss one encoding — it can miss two. That case is now an inventory, with a note on why asserting absence was the wrong invariant: an absence assertion has to be deleted by whoever adds the next raw template, and deleting a red guard is how a class of sites stops being tracked. ## Injection proof — all three run | injected | result | |---|---| | convert one TS comparison in `async-comments-attachments.ts` | fails: **"TypeScript encoding changed"** | | remove one Drizzle predicate in `async-lifecycle.ts` | fails: **"Drizzle encoding changed"** | | remove one raw template in `reads.ts` | fails: **"Raw-sql encoding changed"** | Each failure carries the split-brain explanation and the two real options, so the next worker hits the reason rather than a bare count mismatch. ## Two scan bugs I fixed in my own guard - **It audited documentation.** The raw-template scan reported three sites in `async-archive-lineage.ts` that were prose in a JSDoc block. Now comment-stripped through the census's shared `stripComments` rather than a second implementation. - **It missed half the cluster.** Requiring a receiver (`x.column === "archived"`) misses the list paths that hold the value in a bare local (`if (column === null || column === "archived") return []`) — **four of the eight sites in the target file**, measured. The matcher now accepts a bare identifier named `column`. Counts here are per file, not per line: line numbers churn on unrelated edits, and a ratchet that cries wolf gets deleted. ## Verification `pnpm test:gate` **GREEN** (158 + 10 + 487 + 71) · new guard **2/2** · `pnpm lint` clean · core `tsc` clean · census `--strict` exits 0 with the **baseline untouched** — this PR converts nothing and claims nothing. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
gsxdsm
|
7c408ef650 |
fleet: merge path 10 → 2 — a merged PR never advanced its task on a renamed board (#2733)
**Claimed on #2728 before starting.** The merge path: `merge-queue-ops-2.ts` + `merger.ts` — **10 → 2**, both survivors flagged with reasons. ## A merged PR never advanced its task on a renamed board `applyPrMergedTransition` is what moves a card when GitHub reports a PR merged. Every guard in it was a default-lineage literal, and they all failed **in the same direction**: | guard | renamed board | |---|---| | `column === "done"` → skip as already-done | never matched, so a complete card was re-processed | | `column !== "in-review"` → bail `wrong-column` | always matched, so a card **sitting in review** bailed | Net effect: **a PR merged on GitHub never advances its Fusion task.** The operator sees a merged PR whose card sits in review forever — which reads as a broken webhook, so it gets debugged in the wrong place entirely. That is the most expensive property of this defect class: it does not just fail, it misdirects. One snapshot now covers the pre-check, the deliberate **re-read** (a merge can land between checks), and the **move target**. The target is asserted in the test alongside the guards, because converting guards alone would admit the card and then move it to a column the board does not declare. ## merger.ts - **The orphan-stash liveness guard** classified every finished task as unfinished on a renamed board, so orphaned stashes were never cleaned up. Unioned with the legacy ids: too strict here leaves clutter, too loose **discards a stash whose task is still running**, so over-inclusion is the safe direction. - **The worktree-conflict scan** filters by worktree *path* before resolving lanes. The naive order — resolve, then filter — is exactly what made the github-tracking reconciler scan proportional to task history (#2714 review). Lesson transferred rather than re-learned. - The deprecated `aiMergeTask` already-finalized guard. ## Two flagged, not converted **`merge-queue-ops-2`'s sync enqueue guard** runs inside `store.db.transactionImmediate`. A synchronous lane resolution reads `getTaskWorkflowSelectionImpl`, which returns `undefined` **unconditionally in PostgreSQL mode** — so a "conversion" there would drop the census by one and behave exactly as the literal (the finding from #2703). Converting it properly means making the path async or pushing the trait read into SQL: store architecture, not a call site. Left literal **with that note**, so the next worker does not turn it into a false green. `merger.ts`'s last comparison is the same class. ## Pre-existing red, reported not folded **22 failures in `packages/dashboard/src/__tests__/routes-github.test.ts`** — spec revise/rebuild and approve/reject-plan, all asserting moves to **`triage`, the column U11 deleted**. Verified by reverting my diff and re-running: identical 22. Same stale-literal-in-a-test class as the two assertions #2720 fixed, and it is 22 tests pinning a column that does not exist — worth someone owning deliberately rather than as a rider here. ## Verification census **10 → 2** · `pnpm test:gate` **487 / 71** · 23/23 across three merger suites · 4 new cases, **2 red on revert** · `tsc` clean in core and engine · `pnpm lint` clean. ## Also examined and deliberately left alone - **`live-agent-count.ts` (6 guards)** — every literal there is the *documented degradation path* for a task shape that was not enriched, and both production callers already enrich (`useExecutorStats`, `fn project`). Converting them converts nothing; deleting them removes the fallback that fixtures rely on. The invariant that matters is **caller enrichment**, which is not a literal at all. - **`task-merge.ts` (6 guards)** — `getTaskMergeBlocker` is a **pure** function with no store; its callers inject `resolveTask`. Resolving lanes needs a matching injected resolver, which is an interface change across every caller. Also worth a decision first: its dependency check accepts `in-review` as satisfied while the store's `blockedBy` computation (#2720) does not — **two definitions of "dependency satisfied" in one codebase**, and I am not settling that one silently inside a vocabulary sweep. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
gsxdsm
|
ab715cbd39 |
fleet: default-workflow-hooks.ts 7 → 0 — every duration display read ZERO on a renamed board (#2734)
Claiming `default-workflow-hooks.ts` (7 → **0**), verified free against every open PR's diff first. ## Not a vocabulary tidy — three silent zeroes This file's header names it for the default workflow, but the store runs it on the flag-ON path for **every** workflow: the trait registry resolves each hook by **trait id**, not by workflow. `reopen-semantics-by-role.test.ts` already documents that exact hazard for the reopen predicates. The **timing, completion and in-review hooks had the same defect** and were not part of that conversion. On a renamed board, with nothing thrown and nothing logged: - **`applyTimingEffects`** accrues `cumulativeActiveMs` while a card sits in the WIP lane. With the lane named, the exit test never fires — so **no active time is ever accrued**, and `productivity-analytics.ts`, `task-timing.ts` and every duration display read **zero**. - **`applyCompletionTimingEffects`** never stamps `executionCompletedAt`, so a finished card looks unfinished to anything reading that field. - **`applyInReviewEnterEffects`** returns early, leaving the recovery counters set. The file already had the idiom — `ctx.lifecycleColumns`, `planningColumnsOf`, `liveWorkColumnsOf` with `LEGACY_` fallbacks — so this adds no abstraction. One deliberate detail: `applyTimingEffects` resolves the WIP lane **once into a local** rather than reading it twice. The exit test and the re-entry test have to agree about which column is WIP, or a rename makes the accounting count an interval twice, or not at all. ## A test that would have lied to me I wrote the new cases through `applyDefaultWorkflowMoveEffects` first, and **all three failed on the DEFAULT lineage too**. The dispatcher resolves hooks by trait, and neither test IR declares the `timing` trait, so those hooks never ran at all. That failure looks exactly like a conversion bug. Going through the dispatcher would have been testing the trait registry's wiring rather than this change — so the cases call the converted functions directly, and the reason is recorded in the test. ## Revert proof — all three, each naming the renamed lineage | reverted | failure | |---|---| | the `in-progress` literals | `renamed lineage accrued no active time: expected undefined to be 300000` | | the `done` literal | `renamed lineage did not stamp completion: expected undefined to be '2026-07-30T00:00:00.000Z'` | | the `in-review` literal | `renamed lineage kept its recovery counter: expected 3 to be undefined` | Every case runs on **both** lineages and the default one passes either way — which is the point of running it. ## A finding I did not act on **`evaluateMergeBlockerGuard` appears exactly once in the repo — its own definition.** And the file header says it is "implemented as the `evaluateDefaultWorkflowGuards` reader", which does not exist either. The merge-blocker guard hook is **defined and never consulted**. I converted it (trailing optional lifecycle param, matching `DefaultWorkflowMoveContext`) but did not delete it: the header states this file is a deliberate parallel of `store.ts`'s flag-off path so the two can be parity-checked, which makes removing it a scope call for whoever owns that convergence — not something to decide inside a conversion. ## Verification `pnpm test:gate` **GREEN** (158 + 10 + 487 + 71) · **22 passed** across `default-workflow-hooks` + `reopen-semantics-by-role` · core `tsc` clean · `pnpm lint` clean · census `--strict` exits 0. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
gsxdsm
|
61b82a2737 |
fleet: pure lifecycle predicates 17 → 5 — a monitoring signal that went quiet, and a blocker that waited forever (#2745)
**Claimed on #2742 before starting.** Four pure modules — **17 → 5**, every survivor flagged with a reason. All four are **pure functions with no store**, so the fix shape is the injected-set contract established in #2728, not an in-function resolve. ## Three failures that never error | predicate | what a renamed board got | |---|---| | `getTaskAgeStalenessSignal` | `undefined` for **every** card — age-staleness reported nothing | | `isStaleBlockedByBlocker` | "not stale" for a blocker that was finished, paused in review, or retry-exhausted | | `areAllDependenciesDone` | "not satisfied" for a dependency that had landed | The first is the one to sit with: **a monitoring signal that goes quiet is indistinguishable from health.** The board looks fine while cards sit for days, and nobody investigates a metric that isn't alarming. The signal also chose its *threshold pair* by wip-vs-review, so both halves were literal. The second means the blocked card **waited forever**, silently — "not stale" is the answer that produces no event. The third is the **third place** "satisfied" is asked. It now gives the same answer as the store's `blockedBy` computation (#2720) and the merge blocker: complete or archived, unioned with the legacy ids. Three surfaces, one rule — which is exactly why I refused to settle it inside a vocabulary sweep the first two times it came up. ## Optional is load-bearing Both halves are asserted for every predicate: supplying lanes makes a renamed board work, **omitting them preserves every existing caller**. A *required* parameter would have compiled at every call site and then answered "not active" / "not stale" / "not satisfied" for everything. That is the silent direction, and **no type checker catches it** — which is the argument for optional-plus-legacy-default over a clean signature. The restart-recovery classifiers (with-progress / no-progress / merge-active) take the same set, and **the combiner threads it to all three**, so a caller cannot convert the outer question and leave an inner one literal. `isInReviewMissingWorktreeSessionStartFailure` is deliberately untouched — #2728 converts it and duplicating that would conflict. ## The five that remain - **3 are the ternary trait-fallback branches** (`lanes ? … : legacy`) — the documented degradation path the census counts by design, not unconverted guards. I am not marking them `DELIBERATE-LITERAL` to move the number; that marker means "a lifecycle literal reviewed and kept", and mislabelling to flatter a count is how the instrument stops meaning anything. - **`recoverInterruptedRuns`' filter sits behind a `listTasks({ column: "in-progress" })` query.** The query is the live filter, so converting the redundant predicate moves the census and changes nothing an operator sees. **Third file** where the reported guard is the inert copy and the real one is a query. - **`resolveWorkflowBypassGuards` is sync and receives only column strings** — no task, no store. Converting it means adding lanes to `MoveTaskOptions` and threading them from the moves path, which another worker owns. Marked `DELIBERATE-LITERAL` as an explicit hand-off, with the consequence named: on a renamed board the operator's drag out of the wip lane was rejected by the transition validator, so **a card could not be cancelled from the board at all** (AGENTS.md's Move-Task hard-cancel contract). ## Verification `pnpm test:gate` **10 / 158 / 487 / 71** · 9 new cases, **5 red on revert** · 13/13 with the archive PG suite · `tsc` clean in core and engine · `pnpm lint` clean · census **17 → 5**. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
gsxdsm
|
b9f3fb0ff2 |
docs(core): resolveReviewColumns is the BROAD set — and one of its consumers must NOT migrate onto it (#2750)
## A flaw in the helper I merged in #2730 Found by trying to do the migration I had been advocating for three rounds. One **name** was answering two questions: | | question | answer | |---|---|---| | **broad** | "is this card in a lane where review happens?" | every `mergeOrchestration` lane + every `mergeBlocker`/`humanReview` lane — **this function** | | **narrow** | "is this card in *the* review lane the engine acts on?" | `resolveLifecycleColumns().review` = `columnsWithFlag(ir, "mergeOrchestration")[0]` — **one** lane | The narrow answer is what the executor, the scheduler and `project-engine` act on. **A caller that admits on the broad set and then MOVES the card moves cards the engine does not consider in review.** ## The correction I owe I have been arguing across #2722, #2723 and #2728 that the inline review unions should converge on this helper. For the notifier that is right — over-admission there just means an extra notification. For `register-task-workflow-routes.ts` it is **wrong**. That resolver is deliberately narrower (#2723): its re-engagement *moves* the card, so admitting a second merge lane is a state change the engine will not agree with. Its local copy is **not drift from this helper — it is the other question.** Migrating it would reintroduce precisely the over-admission that PR's review round reasoned away. I was about to make that change. Reading both implementations side by side is the only thing that stopped me, and "consolidate the duplicates" would have looked like an obvious cleanup to the next person too. ## What this PR does Nothing to behaviour. It writes the distinction down **at the helper**, where a consumer reaching for "the review columns" will see it, and pins the difference with a test. The test needs a board declaring `mergeOrchestration` **twice** — no default lineage does, which is exactly why the two answers look identical everywhere else and why the conflation survived review. **Mutation: narrowing this helper to the first merge lane — the consolidation someone would reasonably attempt — fails the test.** ## Verification 30 trait tests green · core `tsc` clean · lint clean (0 errors) · gate green (487 + 158 + 10 + 71). No census movement. ## Not done here Migrating the notifier and the CLI copies onto this helper. Those genuinely should converge, but both live in open PRs (#2722, #2728/#2736) with live review threads; switching them under their authors mid-flight is worse than letting them adopt it once this distinction is documented. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Documentation** * Clarified the distinction between broad review-capable lanes and the workflow’s primary review lane. * Documented how review lane selection affects workflow state handling. * **Tests** * Added coverage confirming that review detection includes all matching merge lanes. * Verified lifecycle review selection continues to use only the primary merge lane. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
gsxdsm
|
b6b28c3b0b |
fleet: task-age-staleness 4 → 0 + the claim guard — an agent could claim a finished card, and no card was ever 'stale' (#2746)
Two core clusters. 6 converted, 2 flagged. ## Two silent failures **No card was ever stale.** `task-age-staleness.ts` applies its signal only to the mid-flight and review lanes — a card in a hold or terminal lane is waiting or finished, not stale. Both lanes were named by id, so on a renamed board the signal returned `undefined` for **every** card and the stale-card warning never appeared anywhere on the board. **An agent could claim a finished card.** `claimTaskForAgent`'s terminal guard was `column === "done" || column === "archived"`. On a renamed board neither matched, so the claim **succeeded** and the agent began work on completed output. ## The threshold selectors are a separate literal, and half-converting is worse than neither `task-age-staleness` has two independent uses of `in-progress`: the **lane gate** that decides whether the signal applies, and the **threshold selectors** that pick which warning/critical numbers to measure against. Converting only the gate admits a renamed-WIP card and then measures it against the **review** threshold — a wrong number, silently. Both are converted, and each is revert-proofed on its own: | reverted | result | |---|---| | the lane gate | 3 of the new cases fail (`expected undefined to be defined`) | | the threshold selectors | the threshold case fails — a renamed WIP card gets the review threshold | No new seam for either: the staleness signal already took a `context` object, and its one production caller (`task-store/reads.ts`) already holds a **per-pass IR cache** for precisely this kind of resolution. ## Cost stated rather than hidden The `reads.ts` resolution is **unconditional**, where the hold-column read directly beside it is gated on `task.paused`. That asymmetry is deliberate: the lanes this needs are exactly what decides whether the signal applies at all, so there is no cheaper gate available ahead of it. With the shared per-pass cache that is a struct build per card, not an IR read. ## Flagged and left counted `formatCurrentTaskLine` is a pure formatter over `Pick<Task, "column">` whose output **prints** the column name for a human reader — same class as `github-tracking-comments.ts:165`. It also degrades gracefully: the "(not active — X)" wording is lost on a renamed board, but "(X)" is still accurate, just less specific. Threading a resolution into a string builder to pick a word is the wrong trade. ## The recurring blind spot, fourth time **None of the 12 existing staleness cases could have caught this** — `lifecycle` is optional and they all omit it, so they assert the legacy fallback. Same for the reconciler's 33 (#2737) and `TaskReviewTab`'s 45 (#2744). This is now a consistent property of the optional-flags seam: **the existing suite stays green through the conversion and through a broken one.** Every file in this program needs at least one case that supplies flags, or the conversion is untested in both directions. Worth making an explicit review criterion rather than something each worker rediscovers. ## Verification `pnpm test:gate` **GREEN** (158 + 10 + 487 + 71) · **31 passed** across staleness / routing-policy / dispatch suites · core `tsc` clean · `pnpm lint` clean · census `--strict` exits 0. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
gsxdsm
|
2fe82cb644 |
feat(core): resolveReviewColumns — the shared answer four consumers each invented separately (#2730)
## The pattern `resolveLifecycleColumns().review` is a **single id derived from one flag** (`mergeOrchestration`). The domain is not that shape — a lane can host human review without orchestrating a merge, and a board may declare more than one review lane. So every consumer asking *"is this card in review"* re-derived its own answer, and they drifted: | PR | surface | how it diverged | |---|---|---| | #2713 | routes | terminal columns needed membership; fixed there only | | #2722 | notifier | a `humanReview`-only lane resolved to nothing — the operator's review notification **never fired**, silently | | #2723 | routes | the union was broader than core's single id | | #2728 | CLI | `fn task retry` refused a card `POST /tasks/:id/retry` accepted — the same operator action answering differently per surface | Four files, four patches, **two of them mine** — plus a fifth site inside #2722 that my own first pass missed, which is exactly the enumeration failure I had been pointing out in other people's PRs. Each recurrence cost a review round, and the next consumer would have invented a fifth answer. ## The change ```ts export function resolveReviewColumns(ir: WorkflowIr): string[] ``` **Additive on purpose.** `.review` is untouched, so nothing that reads it changes behaviour. This is the *missing* helper, not a reshaping of the existing one — `.review` stays correct for its own question ("which single lane hosts the merge gate"); this answers the other one ("is this card **already** in a review lane"). **Monotonic**, which #2723's review round argued about at length: a lane carrying **both** `humanReview` and `mergeOrchestration` is included. Adding a trait must never *remove* a lane from this set, or a card stops counting as in review because its column gained an unrelated capability. **Returns empty rather than defaulting to `in-review`.** The legacy fallback belongs to the caller, which knows whether refusing or admitting is the safe direction for its own guard — the notifier and the retry gate fail in opposite directions, and baking one choice in here would make one of them wrong. ## Verification Six tests, both directions: human-review-only lane, multiple lanes, monotonicity, de-duplication, empty result, and agreement with the shipped coding workflow. One asserts the divergence directly — `resolveReviewColumns` finds the lane while `.review` returns `undefined`. **Mutation: dropping the two extra flags fails 2 of 26.** 26 core trait tests green · core `tsc` clean · lint clean · gate green (487 + 158 + 10 + 71). **No census movement** — this adds capability and converts nothing. ## Follow-up, not done here Migrating the four consumers onto it. Each is an open PR with its own review thread, and switching them under their authors mid-flight would be worse than letting them adopt it. The helper is the prerequisite; adoption is theirs. |
||
|
gsxdsm
|
ddba730a59 |
fix(core): 8 reds across 4 files — incl. a real FN-8603 contract violation and a ratchet row pinning deleted code (#2725)
## Measured
Full `@fusion/core` suite: **8 failed / 6 files → 1 failed / 1 file**
(4611 passed). `pnpm typecheck` exit 0 across every package, `pnpm lint`
clean, gate **726**.
The one remaining failure is **not mine to fix** — see the last section.
## Four causes; two are product-side, not test drift
**1. A real FN-8603 contract violation.** `tool-output-budget.ts:116`
had a bare `console.warn`, breaking the rule that production diagnostics
route through the shared logger so severity markers and `FUSION_DEBUG`
gating survive. `log-severity-spam-contract` caught it exactly as
designed. Now `createLogger("tool-output-budget")`, kept at `warn` — an
invalid operator-supplied budget is a real misconfiguration, not routine
chatter.
**2. A ratchet row pinning deleted code.** The manifest pinned a `local
reattached project ${project.id}` demotion in `central-core.ts` whose
call site was deleted by `5ae6332563` ("collapse dead SQLite dual-path
code"). Verified absent from **all** of `packages/core/src`, not merely
moved. A manifest row for deleted code can only ever fail — it ratchets
nothing — so it is removed with that provenance recorded in place.
**3. An intentional settings overlap.** `agentToolOutputMaxChars` now
appears in both scopes. Admitted to the parity list because
`settings-schema.ts:462` states the intent outright: *"Project settings
participate in the existing effective-settings merge, allowing a
project-specific tool-output cap … to override global policy."* Placed
in `GLOBAL_SETTINGS_KEYS` order, as that test requires.
**4. `maxPostReviewFixes` 3 → 10 — the third file pinning the stale 3.**
Driven off the exported `DEFAULT_MAX_POST_REVIEW_FIXES` rather than a
fourth literal copy. That constant exists *because* the declaration
default and two inline `3`s had already drifted apart once; adding
another copy would guarantee a fourth drift.
## duplicate-guard: a narrow seam instead of a rebuilt mock
Its 3 failures were `Cannot read properties of undefined (reading
'projectId')` — the fake modelled the **deleted SQLite path**
(`db.prepare().all()`) and recovered the window by parsing a captured
cutoff string. It broke when the query moved to `asyncLayer` + Drizzle.
Rebuilding a Drizzle chain to recover a number the policy already
returns would be mock-the-world for no gain, so the window policy is now
one exported pure function — `resolveFingerprintWindowMs`, the
**byte-identical** expression — that both the store query and the tests
call. Two side benefits: the ±5s timing tolerance is gone (exact
assertions), and the `Math.max(1, …)` floor now has coverage the old
cutoff-parsing shape could not see.
**Load-bearing, verified by mutation:** restoring the old 5-minute
ceiling fails 3 of them; deleting the floor fails the new case.
## The remaining failure is a deliberately-deferred product decision
`agent-logs-and-monitor.pg.test.ts > aggregateActivityAnalytics …`
expects funnel stage `todo` count 2 and gets 0. This is **already
diagnosed and deferred by another worker**, in
`activity-analytics.ts:604`:
> *"The merged column landing in `triage` while the `todo` stage stays
empty is a SEPARATE and larger question — it makes the funnel show a
phantom 100% drop between Triage and Todo on every default board since
U11 — and it is deliberately not settled here. Changing which stage the
Planning column reports would retroactively alter how historical
analytics read… Flagged for a product decision on PR #2669."*
The merged Planning column carries `["intake","hold","reset-on-entry"]`
and `stageForTraits` prefers the earliest stage, so `intake` wins.
Either fix — remapping the stage, or changing the expectation — silently
settles how historical analytics read. I left it alone rather than pick
a side inside a test-repair PR.
## Two "flaky" files that are NOT flaky — and I nearly mislabelled them
`pg-test-harness-template-concurrency.pg.test.ts` and
`moves-intake-only-hard-cancel.pg.test.ts` each failed in one full-suite
run and not another, which reads as flake and would have earned a
quarantine entry plus a 14-day deletion clock under the standing rule.
Measured in isolation instead:
| File | alongside other PG suites | alone |
|---|---|---|
| `pg-test-harness-template-concurrency` | fails intermittently | **4
passed, 3/3 runs** |
| `moves-intake-only-hard-cancel` | failed once | **2 passed** |
So this is **shared-PG-template contention between concurrently running
suites**, not an inherent flake in either test. Quarantining them would
have started a deletion clock on healthy coverage and hidden a real
harness-parallelism interaction. Flagged for whoever owns the PG
harness; no quarantine entry added.
<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit
* **Bug Fixes**
* Improved duplicate-detection window handling with consistent defaults,
limits, and minimum values.
* Invalid tool output limits now produce standardized warning messages
while preserving fallback behavior.
* Updated settings and workflow validation to accurately reflect
supported configuration defaults and scopes.
* **Tests**
* Strengthened coverage for duplicate-detection windows and
configuration parity.
* Removed an outdated logging severity expectation tied to a
no-longer-applicable message.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->
|
||
|
gsxdsm
|
8f2cddc5fd |
fleet: update-task-deps.ts 7 → 0 — settles "dependency satisfied", and the store was writing a column U11 deleted (#2720)
**Claim announced on #2714 before starting.** `packages/core/src/task-store/update-task-deps.ts` — **7 → 0**. This one settles an open question and turned up a **live bug on the shipped default board**, not just on renamed ones. ## 1. What "dependency satisfied" means — settled, not guessed I flagged this in three files rather than swapping it three times independently (`executor.ts:12325`, `register-task-workflow-routes.ts:3995`, here). The answer has to be the same everywhere or the scheduler and the store disagree about which cards are blocked. Settled in the store, where `blockedBy` is actually written: - **SATISFIED** = the dependency's own board's **complete** or **archived** column. Archived counts: it is finished work the operator filed away, and reading it as unsatisfied blocks every dependent forever with no recourse short of editing the graph. - **NOT review** — a card in review is not done; its branch has not landed. - **Unioned with the legacy ids**, because a row can outlive the column it is stored in. On a renamed board the old literals matched nothing, so **every** dependency read as unresolved and `blockedBy` was pinned to the first one permanently — dependents never unblocked after the work landed. ## 2. The re-specification move was writing a DELETED column — on the default board `hasNewDependencies && column === "todo"` set `column = "triage"`. **U11 (#2515) deleted `triage`**, keeping `todo` as the merged Planning column. Measured, not assumed: ``` resolveDefaultWorkflowIr() columns: todo[intake,hold,reset-on-entry] in-progress[wip,…] in-review[merge,…] done[complete] archived[archived] ``` So the store has been writing a column the shipped board does not declare. And the emitted event hardcoded `from: "todo", to: "triage"` — **`task:moved` is what the GitHub tracking poster, the auto-merge handoff and the executor's listeners react to**, so every subscriber was being told about a column that does not exist. Now the guard reads the hold lane, the target is the intake lane, the log line names the real column, and when intake === hold (the default lineage post-U11) there is **no move and no event** — announcing a move into the column the card already occupies re-runs reset-on-entry effects in every listener. ## 3. Two existing suites taught me more than the conversion did **`refine-duplicate-task.pg.test.ts` proved the union is required, in one run.** My first version compared only the resolved lanes and refused a row sitting in `done` on a board declaring `published`: *"Cannot refine KB-001: task is in 'done', must be in 'published' or 'editorial-review'"*. That row is real, and refusing an operator action on it is worse than accepting one extra column name. Over-inclusion is the safe direction for "may I refine this?" — the same reasoning as the executor's `resolveTerminalColumnsFor`. **Two assertions expected `"triage"`.** They were not protecting behaviour; they were protecting a stale literal that outlived its column. Updated **with the measurement in the file**, because a silently-changed expectation is indistinguishable from a broken one. ## Revert proof 1 of 3 new PostgreSQL cases reddens when the union is removed. Driven through the real store on PostgreSQL because `blockedBy` is persisted and resolution reads the workflow selection from the database — a mocked store would prove neither. ## Verification `pnpm test:gate` **487 / 71** · 13/13 in the two pre-existing dependency/refine suites · 3/3 new · `tsc -p packages/core` clean · `pnpm lint` clean · census `--strict` exit 0 (**7 → 0** for this file). Changeset: none. `@fusion/core` is private, and while item 2 is an operator-visible fix on the default board, it lands as internal behaviour with no API change — say the word if you want one anyway for the release notes. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
gsxdsm
|
8fb7c933d5 |
docs(core): pin the LifecycleColumns arity contract — two shipped bugs came from it (#2721)
Found by **auditing for the pattern** after hitting it twice, rather than waiting for a third instance. ## The contract, previously undocumented `LifecycleColumns` names **one** column per role even when a workflow declares several. Nothing validates that a trait appears at most once — `columnsWithFlag` returns an array and `resolveLifecycleColumns` takes its head. So a workflow may legitimately declare two intake lanes, or split `mergeBlocker` and `humanReview` across a merge lane and a separate sign-off lane, and the struct names only one of each. That makes the fields safe for one question and unsafe for another: | | question | correct? | |---|---|---| | **safe** | *"where should this card **go**?"* | a move target must be exactly one column | | **unsafe** | *"is this card **already** there?"* | membership — use `columnsWithFlag(...).includes(...)` | ## Two shipped bugs, both the unsafe use Both in #2713, both reading like ordinary conversions: - a task in a **second terminal column** was rejected with a 409 - a task in a **human-review lane split from the merge lane** was classified as outside review entirely, suppressing comment re-engagement I fixed the first, did not generalise, and hit the second one review round later. That is the actual failure mode this PR addresses — not the individual bugs, which are already fixed. ## Two call sites left for their owners Named in the doc comment rather than changed here, since they belong to other clusters. Both are correct **only while their workflow declares one column per trait**: - `packages/engine/src/self-healing.ts` — `columns.intake` / `columns.hold` - `packages/core/src/builtin-workflows.ts` — `lifecycle.intake` ## The test asserts the contrast, not just the arity A reader who sees only *"it returns one id"* learns the wrong lesson. The test builds a workflow with **two intake and two complete columns** and shows that exactly one qualifying column **fails an equality check** against the struct while the **membership form gets it right** — the shipped-bug shape, reproduced in miniature. ## Verification `workflow-lifecycle-traits.test.ts` **20 → 22**. `pnpm test:gate` green (10 / 158 / 487 / 71). `pnpm check:lifecycle-columns` exits 0. `tsc -p packages/core/tsconfig.json` clean. `pnpm lint` clean. Docs + test only — no behaviour change, no census movement, no changeset. --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
gsxdsm
|
f6e460acdf |
fleet: moves.ts 15 → 2 (hot path; one hoisted resolution, net one FEWER than before) (#2705)
Claiming **`packages/core/src/task-store/moves.ts`** — 15 guards, largest unclaimed. This is the core move path, every task move in the system, so the conversion is built to add **no work** to it. ## Census before/after | | before | after | |---|---:|---:| | `moves.ts` column guards | **15** | **2** | | repo backlog (this branch vs `origin/main`) | 693 | **679** | Baseline re-recorded; `--strict` exits 0. *(Backlog figures don't compose across my open fleet PRs — each branch carries only its own reductions. 693 → 679 is this branch against main as measured, not a running total.)* ## Zero added cost, and actually one fewer resolution than before `moveTaskInternal` **already** resolves the workflow IR unconditionally at line 400 — the `useWorkflow` gate is gone — and already derived a lifecycle from it ~400 lines later for the trait hooks. So one hoisted `moveLifecycle` immediately after the IR resolution serves all 14 guards, and the later local now **aliases** it instead of resolving a second time. Net effect on the hot path: **one fewer `resolveLifecycleColumns` call than before this PR.** ## Converted: 14 6× `toColumn === "done"` → complete · 4× `fromColumn === "in-review"` → review · 1× `toColumn === "in-review"` → review · 2× `toColumn === "todo"` → hold · 1× `toColumn === "in-progress"` → wip Every site keeps its legacy id as the fallback. `undefined` here means no IR on this path or a v1 column-less IR, and a move must behave **exactly** as before when there is no basis to resolve from — this is the transaction that arbitrates capacity, so "unchanged when unresolvable" is the requirement, not a nicety. ## Flagged, not converted: 1 **Line 309** — `task.column === "archived"` in the handoff-invariant check. It sits in a different function that runs **before** any IR resolution, so converting it would mean *adding* a resolution to a path that currently has none. That is a cost on the handoff path rather than the free reuse everything else here gets, so it wants a deliberate decision rather than my inclusion. ## Verification — the paths that matter, not just typecheck Because this is the move transaction, `tsc` + lint is not sufficient evidence: - **`pnpm test:gate` GREEN** — 158 + 10 + 487 + 71 - `workflow-capacity-invariant` + `move-path-equivalence` **7/7** (the in-transaction capacity gate lives in this file) - `handoff-to-review-atomicity` **4/4** - `store-movement` + `move-task-preserve-status` + `task-move-hard-cancel-ordering` + `transition-pending-and-status-clear` **16/16** - `pnpm lint` clean · core `tsc` clean · `--strict` exits 0 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
gsxdsm
|
af058b0276 |
test(core): two delete suites still modelled the deleted SQLite path (15 red → 0) + a possibly-lost dependents gate (#2697)
## What was red `task-delete-caller-attribution` (13 failed) and `task-delete-nonblocking-cleanup` (2 failed) on clean `origin/main` — **15 failed / 8 passed**. Every case threw the same thing: ``` TypeError: store.deleteTaskBackend is not a function ``` `deleteTaskImpl` / `deleteTaskIfImpl` are now **thin delegators** onto the PostgreSQL backend; the SQLite arms both fakes modelled were deleted (`FNXC:SqliteDualPathCleanup 2026-07-26`). One fake was matching raw SQL strings (`UPDATE tasks SET "column" = 'archived'`). Nothing reached the logic under test. Rewritten onto the PG path using the same three mocked persistence seams `task-delete-notice.test.ts` already uses — that file is green on main, so this is the established pattern here, not a new one. The **real** backend impls are wired in rather than stubbed, so the delegation is what carries the behaviour. ## Measured | Check | Before | After | |---|---|---| | the two files | 15 failed / 8 passed | **21 + 3 passed** | | all 6 core delete/archive files | — | **67 passed** | | `pnpm test:gate` | — | **726 passed** | | `pnpm lint`, core `tsc --noEmit` | — | clean | **Load-bearing, not decorative:** removing the lineage gate from `deleteTaskBackendImpl` makes the gate test fail. My first attempt at that proof was worthless and I caught it — the patch didn't apply because the block appears twice in the file, so the "3 passed" I got back proved nothing. Retargeted to the occurrence inside `deleteTaskBackendImpl` and it failed correctly. ## Deleted rather than repaired `task-delete-nonblocking-cleanup`'s two original cases asserted that delete **schedules branch cleanup**. That behaviour no longer exists: `_scheduleDeleteBranchCleanup` has exactly **one** reference in the tree — its own definition — and the only live `store.cleanupBranchForTask` call is the **archive** path (`archive-lifecycle-2.ts:306`). Reshaping the fake until those passed would have pinned behaviour the product does not have. The file now covers the gates that *did* survive: idempotent re-delete, the lineage-child gate, and exactly one `task:deleted` audit row. The dead `_scheduleDeleteBranchCleanup` is left in place — deleting source is a separate change from fixing tests, and it is already unreferenced so it is inert. Flagged for whoever wants the cleanup. ## FLAGGED, not guessed — a possibly-lost safety gate The removed version asserted `TaskHasDependentsError` when deleting a task that other live tasks depend on. **That error is neither imported nor thrown anywhere in `archive-lifecycle-2.ts`** — the PG backend raises only `TaskHasLineageChildrenError`, `TaskNotFoundError`, and `TaskSelfDeleteError`. Two readings, and it is a product question rather than a test fix: 1. the delete path now **rewrites** dependency references (there is a `removeDependencyReferences` option and a `rewriteDependentsForRemoval` impl) and blocking was dropped deliberately; or 2. the gate was **lost in the cutover**, and a task can now be soft-deleted out from under its dependents. Asserting either shape would encode a guess, so the question is recorded in-file next to the tests. If it is (2), that is a data-integrity regression worth its own fix — I did not want to bury it in a test-repair PR. ## One of my own assumptions, corrected in-file I asserted `deleted.deletedAt` was populated on the returned task and it failed. The PG backend returns the **pre-delete snapshot** — its own comment says so, because the lifecycle emit and the audit row both need the previous column. The comment now records that, so nobody "fixes" the impl to satisfy the wrong expectation. Census unchanged (722) — test files are not scanned. |
||
|
gsxdsm
|
e9e63d8e0f |
consolidate/capacity: --strict was red on main (my #2621), 14 stale baselines, routines seeding a deleted column, worktrees-off audit (#2652)
Capacity unit consolidation. Three coherent themes, small commits inside. ## Census before/after (`node scripts/lifecycle-column-census.mjs`) | | before | after | |---|---:|---:| | triage column guards (the bar) | 10 | **10** | | `--strict` on main | ❌ **RED** | ✅ green | | baseline staleness | 14 files stale | **0** | This branch does **not** move the triage bar — its remaining 10 are moves.ts (dies with the flag), the dashboard cluster, and one deliberate site. It fixes the instrument that measures the bar, plus a live defect the comparison count cannot see. --- ## 1. `--strict` was RED on clean `origin/main`, and it was my fault ``` packages/dashboard/src/routes/register-task-workflow-routes.ts: 22 -> 23 ``` My merged #2621 added a v1-IR pre-WIP fallback answering a greptile P1 and shipped no marker or baseline update, so the program's measuring instrument has been failing on main since it landed. Fixed **at the site** with a `DELIBERATE-LITERAL` marker, not by bumping the baseline. That branch runs only when the IR declares no columns and no nodes, so there is no role to resolve — `resolveLifecycleColumns` returns nothing and the legacy pre-implementation ids are the only pre-WIP signal that exists there. It is *unconvertible*, not unfinished; the sibling `else` two lines down is the trait path for every IR that can answer. A rise that is genuinely correct belongs where a reader will see it. ## 2. The baseline was stale for 14 files — a hole, not cosmetics A stale allowance lets converted guards return while the check stays green. Measured gaps: ``` self-healing.ts allows 126, tree has 111 executor.ts allows 112, tree has 104 moves.ts allows 44, tree has 39 default-workflow-hooks allows 25, tree has 7 mission-feature-sync allows 5, tree has 0 MissionControlPanel allows 4, tree has 0 (+8 more) ``` **Only two of the fourteen are mine.** The other twelve are already-merged conversions by other workers where nobody re-recorded. Re-recorded all fourteen here rather than waiting for twelve PRs, because until it happens the ratchet is not holding the 779 it exists to hold. Flagging it plainly: those drops are other people's work being locked in, not mine being claimed. ## 3. Routines created tasks into the column U11 deleted The routine editor's "Target Column" defaulted to `triage`. That value is submitted as the create step's `taskColumn`, and an **explicit** column bypasses the workflow entry-column resolution added for column-less creates (#2589) — so every routine saved with the untouched default seeded its tasks into a column the board does not declare. Defaulting to `todo` would be the same mistake one column over: a custom workflow declaring no `todo` is seeded into an undeclared column just as surely, because an explicit column overrides entry resolution whatever its value. So the default sends **nothing** and each workflow's own intake resolution decides. The `triage` **option** is removed too, not merely un-defaulted — fixing the initializer alone left the operator able to pick the deleted column one click away, and it was the option labelled "Planning", the name the merged `todo` column now displays. Removing it retires that label inversion as well. Found by scanning **membership** forms rather than comparisons: the comparison census cannot see a `?? "triage"` default, so no count showed this and nobody was looking. Revert-proof — restoring the default fails with *"the default must not name a column at all"*. ## 4. "Worktrees off is INERT" had one unaudited reader The constraint was that `maxWorktrees` become genuinely inert, "not set very high and not skipped by convention". `resolveWorktreeCapacityLimit` returns `null` for that, and its unit tests can only prove the **resolver** is right — they cannot see a second reader, which is the only way the constraint breaks. Audited every `maxWorktrees` read that bounds anything. **Exactly two:** `scheduler.ts` (the admission gate, via the resolver, single call site, optional gate snapshot) and `self-healing.ts`'s `enforceWorktreeCap` — `(settings.maxWorktrees ?? 4) * 2`, a **raw** read. The second is **not a bug** and is left alone: it bounds worktree *directories on disk* and only removes *idle* ones. Worktrees still exist in OFF mode, so that bound must keep applying or idle directories accumulate unbounded. Recorded consequence: in OFF mode the number still governs disk retention while gating no admission — an edge you scoped out. The note says explicitly **not** to unify the two readers: routing hygiene through the resolver returns `null` in OFF mode and silently removes the disk bound, which is a leak dressed as a simplification. New ratchet requires every file bounding on `maxWorktrees` to be named with a reason, and rejects a **stale** allowlist entry. Proven by injecting `active >= (settings.maxWorktrees ?? 4)` into `hybrid-executor.ts`. --- ## Deliberately NOT included - **My own census script.** #2633 landed the canonical one, and it is better than mine — an AST classifier *plus* an independent text classifier with `--compare`, and a baseline that fails on unrecorded **drops** as well as rises. Mine only caught rises. I deleted mine rather than ship a second measuring instrument; three copies of "strip comments" is the drift shape this program keeps paying for, so the worktree ratchet now imports #2633's `stripComments`. - **My TaskContextMenu fix.** Superseded, and by a better answer: main's `isPureIntakeColumn` (intake *without* hold) keeps the merged Planning column shown and suppresses only a bare Ideas capture, which resolves the exact hold-lane objection coderabbit raised against my version. I briefly clobbered that merged work by checking my old file out wholesale, caught it in the diff, and reverted. ## Verification `pnpm lint` clean · core + dashboard `tsc` clean · census suite 23/23 · worktree ratchet 8/8 · RoutineEditor 49/49 · `routes-task-retry-planning-column` 16/16 · `lifecycle-column-census --strict` exits 0. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --- ## Added after review (all four greptile threads were real, and two of them mattered) **The routine fix was half a fix.** `routine-runner.ts:515` *and* `cron-runner.ts:982` both did `column: (step.taskColumn as Column) || "triage"` **after** the step is read, so every routine — including ones saved through the fixed editor — still created tasks into the deleted column. Both now omit it. **The advanced steps editor MANUFACTURED the defect.** `ScheduleStepsEditor.tsx` had three `triage` defaults: the new-step template (`:64`), the per-step initializer (`:95`), and the select still offering it (`:344`). So the path I had *not* fixed produced the bug by default, on fresh data. Template names no column; initializer coerces a persisted `triage`; `triage` removed from the options; empty submits `undefined`. **Four pre-existing tests pinned the defect** and are rewritten to the corrected invariant rather than appeased: | test | asserted | |---|---| | `cron-runner`: "defaults column to triage when taskColumn is not set" | `column: "triage"` | | `ScheduleStepsEditor`: "adds a create-task step..." | `taskColumn` toBe `"triage"` | | `ScheduleStepsEditor`: "allows saving create-task step..." | the legacy column is **resubmitted** | | plus the explicit-column case added beside each, so the fix cannot swallow a deliberate choice | **The allowlist hole was the worst finding.** `AUDITED_BOUNDS` was keyed by FILE, so every bounding expression in an allowlisted file was exempt — a second raw bound in `scheduler.ts` stayed green, the one case that ratchet exists for. Per-expression now, and making it so **immediately surfaced a real second bound the file-level version was hiding** (`maxWorktreesGate.used >= maxWorktreesGate.limit`, safe by construction since the snapshot is `undefined` in OFF mode). Proven by injection. ## Found while re-reading my own deletion, not reported A **rendered tooltip** still named a deleted cap. The "Queued to plan" badge read *"planning starts when a concurrency slot frees up (maxConcurrent / globalMaxConcurrent)"*. The cross-project cap is gone — capacity is two numbers per project — so it told operators their planning waited on a limiter they can no longer find a setting for. Names the surviving dimension only now. ## Coding (Ideas): enforcing #2651 rather than repeating it I took the unowned coding-ideas IR merge, concluded it must not be done, then found **#2651 had already implemented, reverted and documented exactly that** — with better grounding than my own argument. It added no test, so nothing stops the next person reaching the same dead end. So this ships their reasoning as a ratchet, not a second opinion: triage discovery keys on the column's `autoTriage`, so a merged column is either never scanned (cards sit on a bootstrap stub until the **capacity hold** releases them, sending **unplanned** work into in-progress — worse than stalling) or scanning wins and the manual gate is gone. Their scope caveat is kept: `autoTriage` is a general trait field, so only *this preset's* collapse is dead, not manual intake as a concept. The registry does not reject the merged shape, which is why prose was not enough. ## Verification (re-run) `pnpm lint` clean · core + engine + dashboard-app `tsc` clean · `lifecycle-column-census --strict` exits 0 ("every file matches its baseline exactly") · routine-runner 24/24 · cron-runner 156/156 · ScheduleStepsEditor 41/41 · RoutineEditor 49/49 · worktree + coding-ideas 12/12. TaskCard has 2 failures **pre-existing on main** — confirmed identical with my changes stashed. --- ## Bears directly on the closing bar: this PR already removes the 67-guard ratchet slack Measured on current `origin/main` with the census itself: ``` tree total: 787 baseline total: 854 SLACK: 67 FILES ABOVE BASELINE (1): +1 packages/dashboard/src/routes/register-task-workflow-routes.ts (22 -> 23) FILES BELOW BASELINE: 13, totalling 68 unrecorded conversions -18 core/default-workflow-hooks.ts (25->7) -15 engine/self-healing.ts (126->111) -8 engine/executor.ts (112->104) -5 core/task-store/moves.ts (44->39) -5 engine/mission-feature-sync.ts (5->0) -4 core/live-agent-count.ts (10->6) ``` **The slack is not regression — it is 13 files of merged conversions nobody re-recorded**, against exactly **one** rise. This PR re-records the baseline **854 → 782 across 140 files**, which closes it. **And the "+3 that slipped in" is +1, and it is mine.** `register-task-workflow-routes.ts 22 → 23` is the v1-IR pre-WIP fallback my #2621 added; it is justified (that branch runs only when the IR declares no columns or nodes, so there is no role to resolve) but it shipped with no marker and no baseline update — which is why `--strict` has been **red on main since it merged**. Fixed here at the site with a `DELIBERATE-LITERAL` marker rather than by bumping the baseline, because a rise that is genuinely correct belongs where a reader will see it. Sequencing note for the auto-lowering change: if this lands first, that work is purely the mechanism (auto-lower, or fail with tighten instructions) rather than a cleanup, and the two re-records will not collide in the same file. Also worth carrying into that mechanism, from building the same guard here: **`--update` must refuse to RAISE.** An earlier version of mine wrote current counts verbatim, so a developer who added a literal and ran the documented update command locked the regression in as the new ceiling — the mirror of the high-water problem. Lowering can be unattended; raising should be a hand edit with the reason recorded. ## Third piece of residue from my own deletion `updateGlobalConcurrency` in the dashboard API client PUT to `/api/global-concurrency`, a route removed when the machine-wide cap went. Zero callers; the only reference was the `legacy.ts` barrel re-export. Deleted both. `fetchGlobalConcurrency` **survives on purpose** — the GET route remains and serves live utilization telemetry to the footer and Command Center; nothing gates on it. That is the third: after the second raw `maxWorktrees` reader and the "Queued to plan" tooltip. A deletion is not finished when the enforcement goes — the client, the label and the tooltip outlive it. --- ## Re-greened the dashboard API tests: 117 failures on main, ONE root cause These would have polluted the closing verification pass, and nobody owned them. `api()` builds headers via `new Headers(...)` and returns `Object.fromEntries(headers.entries())` — and `Headers.entries()` **lowercases every key**, so the object reaching `fetch` is `content-type`, not `Content-Type`. `ab87d0d80` then added `x-fusion-client: dashboard-ui` for run-audit attribution. Both changes are correct; neither is visible at a call site, so **114 assertions across 7 files** kept asserting the old shape and went red together. Fixed by naming the shape **once** in `app/test/apiRequestHeaders.ts` rather than patching 114 literals — restating a shared fact 114 times is what made a two-line client change look like 117 failures. Deliberately not a loose `objectContaining`: these tests are the only thing pinning that the attribution header is sent *at all*. **117 → 4.** The remaining 4 are unrelated pre-existing CSS failures (`task-detail-modal-tablet-width` ×3, `space-token-defined` ×1) — confirmed identical on clean main with my changes stashed. ### A gap this surfaced, recorded not papered over Three routes failed in the *opposite* direction — they send the old shape because they call `fetch()` **directly**, bypassing `api()`, so they never get the attribution header. `client.ts` claims the opposite: > "Applied once here rather than per-call so no future mutation route has to remember it." That does not hold for a route that bypasses the helper it is applied in. **Measured in `app/api/`: 8 files make direct `fetch()` calls and 7 include mutations (POST/DELETE)** — among them `ai-sessions.ts`'s DELETE, which is the same class as the four-delete incident the header was added for. So the attribution fix has a hole in exactly its motivating case. Not fixed here: routing those onto `api()` is a behaviour change across the API layer and belongs to its owner, not to a test re-green. Those assertions use a separate `API_JSON_HEADERS_NO_ATTRIBUTION` constant so the gap stays **visible** — if a route is later moved onto `api()`, its test fails and points at the note explaining why. --- ## This branch takes the triage bar 10 → 5, and makes `--strict` green `node scripts/lifecycle-column-census.mjs` on this branch reports **triage 5**, against **10** on `origin/main`. The five removed are the ScheduleStepsEditor template/initializer/option and the RoutineEditor default/option — the automation paths that were creating tasks into the deleted column. **`--strict` was also RED on clean main, twice over, and both causes were the same mistake:** a thorough written rationale the tool cannot read, because the marker was not where the census looks. The census reads a comparison node's **leading comments**; a `DELIBERATE-LITERAL` in the JSDoc above the enclosing function or declaration does not reach the comparison inside it. | site | why it is legitimate | why the tool could not see it | |---|---|---| | `columnRoles.ts:80` `isHoldColumnRole` | degrades to `columnId === "todo"` only when a column has **no resolved traits** — identical in kind to `LEGACY_PRE_IMPLEMENTATION_COLUMN_IDS` directly above, which escapes counting only because a Set is a membership form | rationale written, **no marker token** | | `MissionControlPanel.tsx` ×3 | the SDLC funnel **alias table** — maps `to-do`/`ready`/`review`/`shipped` onto one display stage with an explicit `other` bucket, and nothing branches on it | marker in the JSDoc; the comparisons are arrow bodies **inside the array literal**, which it does not reach | The second only surfaced because converting the `triage` stage to a Set removed its count and exposed the siblings — red gate, justification sitting three lines above, unreachable. Both are markers, no behaviour change. Neither is a conversion candidate: resolving the funnel table to traits would **drop the non-column aliases it exists to accept**. **For the auto-lowering work:** the marker-placement rule is now the recurring trap — three instances, three different authors, including me. A marker that does not register is indistinguishable from no marker, and the failure mode is a red gate with a written explanation nobody can act on. If the census accepted a marker anywhere in the enclosing declaration's comments, none of the three would have happened. Baseline re-recorded per the tool's own instruction ("Re-record the baseline in the SAME PR that lowered the count"). <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **Bug Fixes** - Task “Actions” menus no longer appear on bare cards in the Planning column. - Routines, scheduled tasks, and create-task steps now respect each board’s configured workflow intake column instead of using a retired default. - Legacy tasks saved with the retired intake column are migrated to automatic workflow resolution. - Target-column selection now offers only “Automatic (workflow intake)” and “Planning,” removing the obsolete option. - Capacity/planning messaging and related UI tooltip text were clarified; concurrency cap updates are managed per project. - **Tests** - Added/updated coverage for workflow intake resolution, create-task target column behavior (including legacy coercion), capacity safeguards, and API request consistency. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
gsxdsm
|
aa02db5782 |
fleet: scheduler.ts 28 → 27 + make the column-role predicates reachable from the other 80% of the backlog (#2690)
## Census **722 → 721**; `packages/engine/src/scheduler.ts` **28 → 27**. Exactly the one site converted. Baseline re-recorded in the same commit — `--strict` flagged the stale allowance itself, and `in-progress` went 138 → 137. ## The unblocker (commit 1) The role helpers live in `packages/dashboard/app/utils/columnRoles.ts`, a dashboard-**app** module. Measured against the census: | Location | Guards | Share | Helpers importable? | |---|---:|---:|---| | `packages/engine/**` | 316 | 43% | no | | `packages/dashboard/app/**` | 150 | 20% | **yes** | | `packages/core/**` | 148 | 20% | no | | `packages/dashboard/src/**` | 78 | 10% | no | | `packages/cli/**` | 24 | 3% | no | **Only 20% of the backlog can call them at all.** #2685 widens the helper *set* correctly; that is coverage, not location. `packages/core/src/column-roles.ts` is the same flags-first / legacy-id-fallback predicate placed where the other 80% can reach it — core already exports `resolveColumnFlags`, so no new resolution machinery comes with it. **Semantics are mirrored from #2685, not invented**, so the two sets cannot answer the same question differently: `complete` EXCLUDES `archived`; `wip` keys on `countsTowardWip` (the same flag capacity arithmetic uses); `review` accepts `mergeBlocker` OR `humanReview`. One addition — `isTerminalColumnRole` for the `!== "done" && !== "archived"` union, the most repeated shape in the backlog. 10 tests cover both modes of all 8 predicates, including the **degraded no-flags fallback** — the half with no coverage when these lived only in the dashboard app — plus the two cases that prove the predicate does something rather than nothing: a renamed column carrying the right trait answers yes, and a legacy id carrying the WRONG trait answers no. ## A trap every fleet worker converting engine code will hit A new core export must be added to **both** `index.ts` and `index.gate.ts`. The `engine-core` gate project resolves `@fusion/core` to a bundle built from `index.gate.ts` (`scripts/build-engine-core-gate-bundle.mjs`). An export present only in `index.ts` is `undefined` at runtime under the gate: 13 `scheduler-workflow-cutover` tests failed with `isWipColumnRole is not a function`, in a file that does not mock `@fusion/core` at all. The symptom points at the consumer, the cause is the barrel. I nearly mis-attributed this. Baseline first: `scheduler-workflow-cutover` is **42 passed on clean main**, so the 13 were mine — not pre-existing. That measurement is the only reason I looked at the barrel instead of "fixing" the tests. ## The conversion (commit 2) `scheduler.ts:1690`'s `isWipColumnTask` was a hand-rolled copy of `isWipColumnRole` — it stored only `countsTowardWip` as a boolean and re-implemented flags-first-then-legacy-id inline. It now stores the resolved flags object and lets the shared predicate decide. Behaviour is identical in all four states: column present with the flag true or false (flags win), column absent from a resolved IR, and IR resolution failed (both defer to the legacy id). | Check | Result | |---|---| | `scheduler-workflow-cutover` | **42 passed** before and after | | 21 scheduler/capacity/hold-release files | **372 passed** | | `pnpm test:gate` | **726 passed** | | `pnpm lint` | clean | ## Flagged and skipped, not guessed **`scheduler.ts:1736` — a latent legacy-vocabulary defect, not a conversion.** `if (task.column !== "in-progress") continue;` gates the file-scope-lease loop on the literal, ~40 lines below capacity arithmetic that is trait-aware. On a renamed WIP column the loop silently does nothing while capacity counts the same cards correctly. Converting it *changes behaviour* on renamed boards (from wrong to right), which the fleet rules put out of scope — so it is flagged here for whoever owns that fix. It is the same class as U10's six legacy-vocabulary defects. **`hold-release.ts:343`** — already marked `DELIBERATE-LITERAL`. It is the legacy half of FN-5719's dual-accept pair; converting it would make both halves compute the same answer, deleting the compatibility signal *and* its divergence detector while looking like a cleanup. Untouched. **`task-merge.ts:254`** — the documented fallback for callers that have not proven lane identity; trait-aware callers pass `skipColumnIdentityCheck`. Untouched. **The other 14 `scheduler.ts` sites** have no flags in scope (e.g. `isLegacyDependencySatisfied(dep: Task | undefined)`, `shouldHoldActiveFileScopeLease(...)` — task-only pure functions). Threading an IR in changes signatures and call graphs: behaviour change, out of scope. This is why the cluster is 28 → 27 and not 28 → 0, and the reachability measurement behind it is #2687. No changeset: `@fusion/*` are private and no `@runfusion/fusion` behaviour changes. |
||
|
gsxdsm
|
dc50425e98 |
docs: correct 104 future-dated FNXC timestamps across 61 files (#2680)
## What The FNXC convention exists so a reader can place a note against the change that motivated it. A stamp dated *after* the edit landed defeats exactly that. This is program-wide drift, not one author's slip — I contributed to it in my own commits this week, which is how I noticed it. ## Measured, on this tree **104 stamps across 61 files** dated later than the day they were written, from one day ahead to **2026-10-19 (81 days)**: | count | date | count | date | count | date | |---|---|---|---|---|---| | 50 | 2026-07-31 | 6 | 2026-08-05 | 3 | 2026-08-13 | | 17 | 2026-08-01 | 1 | 2026-08-07 | 1 | 2026-08-19 | | 7 | 2026-08-02 | 1 | 2026-08-12 | 2 | 2026-08-26 | | 11 | 2026-08-03 | | | 3 | 2026-10-19 | An earlier number I circulated was ~70. That came from a narrower pathspec and was wrong; **104** is the measurement. ## How Each stamp is rewritten to the date of the commit that introduced **that line**, via per-line `git blame` — deliberately *not* stamped uniformly with today's date. A uniform stamp swaps a wrong date for a different wrong date and flattens the ordering that makes these comments navigable; blame preserves it. Times of day are untouched, and a blame date in the future is clamped rather than trusted. ## Why the verification is listed A docs sweep across 61 files is precisely where a stray edit hides, so the safety claims are mechanical rather than asserted: - every changed line begins with a comment marker — **no code touched**; - **no test asserts an FNXC date later than today**, so no `toContain` assertion on embedded source text can be silently invalidated (several such assertions do exist); - CSS files, which carry several of those assertions, are outside the pathspec. ## Verified lint clean · merge gate green (487 + 158 + 10 + 71) · `census --strict` exit 0 · tsc clean for core, engine, and dashboard (`tsconfig.app.json`). **No behavior change.** Comment text only. ## Not done here A guard preventing recurrence. A check that rejects an FNXC stamp dated after the commit would stop this returning, but it needs a decision about where it runs (lint rule vs. gate) and it is a behavior change to CI — it does not belong riding inside the sweep it would police. |
||
|
gsxdsm
|
30e287a29e |
fix(test): decouple a second logger assertion from log formatting (#2681)
Second instance of the defect fixed in #2675 — found by applying the same attribution pass to the **core** suite's long-red files rather than counting them. ## The defect `withSeverityMarker` (`logger.ts:31`) deliberately wraps every message in a machine-readable severity marker so the TUI log pane can colour by level: an `fnlvl=<level>` marker plus a `[core-merge-policy]` subsystem tag ahead of the real text. This case pinned the raw string with `toHaveBeenCalledWith`, so it broke when that convention landed. It was coupled to log **formatting**, not to the behaviour it exists to check. ## Two instances is a pattern `toHaveBeenCalledWith` on a logger is brittle **by construction** in this codebase, because decorating the message is the logger's entire job. Any assertion pinning an exact logged string will break the next time the format changes — and both instances found so far were long-red, i.e. nobody noticed they had stopped testing anything. Worth a lint rule or a shared helper if a third appears. I have not added one for two instances. ## What is preserved Warn-once semantics and the requirement that the warning names both the legacy value and its replacement are unchanged and still fully asserted. Only the exact-prefix coupling is removed. **Mutation-verified:** deleting the `severityAuditLog.warn` call in `merge-policy.ts` fails with `expected "warn" to be called 1 times, but got 0 times`. A contains-check that passed because it matched nothing would be worse than the brittle assertion it replaces. ## Still red in the core suite, not addressed here Two neighbours in the same cluster, both needing an owner's context rather than a guess: - `settings-parity.test.ts` — `expected [ 'testMode', 'voiceInput', …(15) ] to deeply equal [ …(14) ]`. A settings key now appears in **both** global and project scope without being listed as intentional. That is either a real scoping mistake or a stale allowlist, and the difference matters. - `workflow-ir-settings.test.ts` — `expected 10 to strictly equal 3` on the moved-key catalog. Both are genuine signals, not noise. Flagging rather than guessing, same as the funnel decision on #2674. ## Verification `settings-defaults.test.ts` **39/40 → 40/40**. `pnpm test:gate` green (10 / 158 / 487 / 71). `pnpm check:lifecycle-columns` exits 0. `pnpm lint` clean. Test-only; no changeset. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
gsxdsm
|
bc782d8d92 |
U12: resolve the move-path compatibility flag — trait hooks unconditional, legacy branch deleted (#2655)
U12's headline goal. The raw `experimentalFeatures.workflowColumns` flag gated **every task move**; its six seams are now unconditional and the flag, its last two readers, and the 124-line inline legacy branch are deleted. ## Deleted, not converted The flag-OFF branch goes with the gate. Converting a branch we intended to delete would have left a second definition of every column side effect alive to drift — the defect this program has spent its length removing. Both readers flip in **one commit** because they are not separable: the preflight in `workflow-task-create-ops.ts` computes the `movePolicyPreflight` that `moves.ts` consumes and validates. Un-gating either alone either evaluates workflow move policies — with their plugin-gate side effects — whose result is ignored, or validates against a preflight that was never computed. ## Evidence, not assertion **Equivalence (precondition 1).** `moves-flag-equivalence.test.ts` (commit 1) ran the same journey under both flag states against live PostgreSQL and diffed the persisted row: **identical across 128 fields** plus an equal timing shape, over `todo → in-progress → in-review → todo → in-progress`. Mutation-verified both ways — stamping the flag-ON branch, and diverging the reopen hook, each fail it. **And there was stronger evidence already on main that isn't mine.** U2b's `move-path-equivalence.pg.test.ts` ran *every* scenario once per path and has been green across ~10 of them: `preserveStatus`, `preservePause`, timing accounting, `preserveProgress`, `preserveWorktree`, engine-source rehome, `in-progress → todo`. Two independently built harnesses agreeing is the best evidence this question has had. **The flag was read by nothing in production.** `experimentalFeatures` is global-only and no module writes it, so this path had never run for any project without a stale persisted value. That is also why a green suite was never evidence on its own — both paths were individually valid and only one was live. ## Two claims of mine this PR corrects **1. Seam 2 does not introduce new rejections.** I said in #2639 and in the census that with the flag off there is *no* target validation, so flipping would add refusals. Reproduced the opposite: a move to an undeclared column already rejects on the legacy path with `Invalid transition: … Valid targets: …`. I found it because the discriminator I wrote to prove "the flag is the cause" failed. **2. My first equivalence test proved nothing.** It used `updateSettings`; `experimentalFeatures` is **global-only**, so `getSettingsFast()` filtered the write out and `useWorkflow` was false in *both* runs. Caught by stamping the flag-ON branch and watching the test stay green. It now writes via `updateGlobalSettings` and **asserts the flag took effect** before the journey. U2b's harness carries the same warning independently — `MUST be updateGlobalSettings, NOT updateSettings`. ## The user-visible change Move rejections now report **workflow-resolved** targets instead of the hardcoded legacy adjacency table. Concretely: `Valid targets: in-progress, triage, archived` becomes `Valid targets: archived, in-progress`. That is the fix, not a regression — the legacy table still advertised `triage`, a column the default lineage stopped declaring at #2515, so an operator following the old message was told to move somewhere impossible. Likewise a move *into* `triage` is now refused rather than stranding the card in a column with no trait flags, invisible to every trait-driven sweep until reconciliation re-homes it. `live-move-path-undeclared-target.test.ts` characterised exactly that defect and carried `it.todo("should REFUSE a move into a column the task's workflow does not declare (U2b)")` — **this fulfils it.** ## Test migration | file | change | |---|---| | `move-path-equivalence.pg.test.ts` | deleted — every scenario ran once per path; purpose fully discharged | | `workflow-capacity-invariant.pg.test.ts` | `setPath("inline"\|"hooks")` → `assertMovePathLive()`; the probe is **kept** so capacity cannot pass because moves were broken for an unrelated reason | | `store-movement.pg.test.ts` | asserts the refusal **and** that the legitimate backward move still works, so it reads as a narrowing | | `raw-workflow-columns-flag-census.test.ts` | deleted per its own instructions — it was built to fail in both directions and fired exactly as designed: `expected [] to deeply equal [3 readers]` | | `moves-workflow-flag-seams.test.ts` | deleted — it pinned the six seams this removes | ## Verification Full core suite: **33 failed / 10 files — byte-identical to main's baseline**, with **zero** files failing exclusively on this branch. I measured the baseline by checking out `origin/main` and running the same command, because the first comparison I made was by count alone and would have blamed the flip for 8 files that were already red. `pnpm lint` clean. `pnpm test:gate` green (10 / 132 / 482 / 71). `tsc -p packages/core/tsconfig.json` clean. Core builds. ## Left in place deliberately The `workflowColumns` settings key stays schema-tolerated and is already in `HIDDEN_EXPERIMENTAL_FEATURE_KEYS`, so an upgraded project carrying a stale value renders nothing and loads cleanly. Removing it from the schema would risk rejecting those projects for no benefit now that nothing reads it. --- ## Rebased onto current main — and `triage` reaches ZERO | metric | before | after | |---|---:|---:| | `moves.ts` column guards | 39 | **15** | | repo column total | 745 | **741** | | **`triage` column guards** | 1 | **ABSENT (0)** | `triage` is now absent from `byColumnId` entirely: no unconverted `triage` guard remains anywhere in production source. Combined with #2664 (the last one, in `TaskContextMenu`) this closes bar item 1. The census behaved exactly as designed on the rebase: the flip *deletes* guards, so `--strict` reported `moves.ts: allows 39, tree has 15` rather than leaving a stale allowance, and the re-record lands in this PR's diff. ## Verification on the rebased tree - Full core suite: **33 failed / 10 files — identical to main's baseline**, zero files failing exclusively on this branch (measured by checking out `origin/main` and diffing the failing-file sets, not by comparing counts). - `pnpm test:gate` green (10 / 158 / 487 / 71). - `pnpm check:lifecycle-columns` exits 0. - `pnpm lint` clean, `@fusion/core` builds. ## Two review fixes carried in this PR **P1 — optionless engine moves lost their bypass.** `resolveWorkflowBypassGuardsImpl` did `void moveSource;` — it discarded the resolved parameter and re-read `options?.moveSource`, so `moveTask(id, target)` resolved to `"engine"` at the call site and computed `bypassGuards === false`. Latent while the flag gated validation; with the gate gone, an internal executor/merger/recovery move made without an options object would be judged as a user move. **P2 — the absence signal.** Emitting `workflowId` unconditionally would have stamped `builtin:coding` onto every task with no explicit selection, reporting a fallback as authoritative. Now emits the selection directly, so absent still means "not resolved here". <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **Bug Fixes** - Task moves are now validated against the task’s declared workflow. - Invalid destinations are rejected with a clear error, and tasks remain in their original column. - Valid backward moves continue to work as expected. - Move behavior and lifecycle updates are now handled consistently across workflows. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
gsxdsm
|
b752f9014d |
fix(core): a hold-only column vanished from the SDLC funnel (+ un-red the census on main) (#2674)
Two things, both small, one urgent. ## 1. Hold-only columns disappeared from the funnel `hold` was absent from `TRAIT_TO_STAGE`, so a column whose only pre-implementation trait is `hold` — a renamed board's wait-for-capacity lane — resolved to `OTHER` and vanished from the SDLC funnel. Measured before the fix: ``` stageForTraits(["hold"]) === "other" ``` The default lineage hid it: its Planning column also carries `intake` and `reset-on-entry`, so it always matched something. Only a board that names its wait lane separately was affected — **exactly the custom shape this trait mapping exists to support**. Revert check: removing the entry gives `expected 'other' to be 'todo'`. ## What I deliberately did NOT fix, and why The merged default Planning column carries `["intake","hold","reset-on-entry"]`, and `stageForTraits` prefers the earliest stage in flow order — so `intake` wins and it still resolves to `triage`. The `todo` stage therefore stays empty on every default board since U11, and the funnel shows a **phantom 100% drop between Triage and Todo**. That is a real defect. It is also not a reversible call: changing which stage Planning reports would retroactively alter how historical analytics read. Flagged on #2669 for a product decision. Adding `hold` does not touch it — `intake` still outranks — and a second test **pins the current behaviour** so the larger question gets answered deliberately rather than drifted into by a future edit to this map. ## 2. `check:lifecycle-columns` is RED on pristine `origin/main` — again ``` census exit on pristine main = 1 packages/engine/src/executor.ts: allows 87, tree has 85 ``` `executor.ts` is a file this PR does not touch, so a merge lowered the count without re-recording and the blocking PR check is failing for **every open PR**. The re-record is mechanical and is included here to unblock it — called out explicitly because it is unrelated to the funnel fix and should not ride along unexplained. This is the second time the baseline has gone stale on main this way. The rule works (`--strict` caught it immediately); what is missing is that it caught it *after* the merge. Worth considering whether the census should run on the merge queue rather than only on PR head — otherwise a PR that is green when opened can still land a stale baseline. ## Verification `pnpm test:gate` green (10 / 158 / 487 / 71). `pnpm check:lifecycle-columns` exits 0 after the re-record. `tsc -p packages/core/tsconfig.json` clean. `pnpm lint` clean. `sdlc-funnel-default-columns.test.ts` 8/8. No changeset: the funnel entry is a correctness fix with no user-facing API change, and the baseline re-record is internal. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Improved SDLC funnel classification for hold-only columns, placing them in the Todo stage instead of Other. * Preserved correct Planning column behavior when hold-related traits are combined. * **Tests** * Added coverage for hold-related funnel stage mapping and trait ordering. * Updated lifecycle column census baselines to reflect current results. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
gsxdsm
|
fe7e68bc13 |
fix(core): wedge notifications could never be resolved on PostgreSQL (42P18) (#2669)
Not a U12 change — found while **attributing** the pre-existing live-PG
failures during U12's closing verification, and it turned out to be a
product bug rather than a stale test.
## The defect
`resolveWedgeNotification` builds its UPDATE with:
```ts
jsonb_build_object('status', 'resolved', 'transitionedAt', ${transitionedAt})
```
`jsonb_build_object` is variadic `"any"`, so there is no signature for
PostgreSQL to resolve the bind parameter against. It rejects the
statement at **parse time**:
```
42P18: could not determine data type of parameter $1
```
Parse-time is the important part: this failed on **every call**, not on
unusual data. Wedge notifications could not be resolved at all in
PostgreSQL mode.
Casting the parameter to `::text` fixes it.
## Evidence
- `store-wedge-resolution.pg.test.ts` goes **0/7 → 7/7**. That suite has
been red on `main`.
- **Causally verified, not assumed:** removing the cast reproduces
`42P18` exactly. The fix is the cast, not something incidental to the
edit.
- Checked the rest of `packages/core` for the same shape — this is the
only `jsonb_build_object` call site, so there is no second instance
hiding.
## Why it survived
The failure is in a live-PG suite that was already red, so it read as
part of the ambient noise. I only found it because the closing
verification required me to attribute each failing suite to a cause
rather than count them — and "these 4 fail on main too" is an
attribution of *whose*, not of *what*.
Worth flagging for whoever owns the remaining three
(`agent-logs-and-monitor`, `central-archive-secrets`,
`workflow-settings-project-identity`): the same reasoning applies. A
suite failing on main is not evidence that the code is fine.
## Verification
`pnpm test:gate` green (10 / 158 / 487 / 71). `pnpm
check:lifecycle-columns` exits 0. `tsc -p packages/core/tsconfig.json`
clean. `pnpm lint` clean.
Independent of #2655; either order merges.
---------
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
||
|
gsxdsm
|
a099813e94 |
fix(test): decouple the audit-emitter assertion from log formatting (last of the 4 red PG suites) (#2675)
Last of the four long-red live-PG suites. **This one is a stale test — the only one of the four that is.** ## The cause `withSeverityMarker` (`logger.ts:31`) deliberately wraps every message in a machine-readable severity marker so the TUI log pane can colour by level. The emitted string carries a `fnlvl=warn` marker and a `[core-async-secrets-store]` subsystem tag ahead of the real text. The assertion pinned the raw message with `toHaveBeenCalledWith`, so it broke when that convention landed. It was coupled to log **formatting**, not to the behaviour it exists to check. ## The fix Rewritten to assert what it actually cares about: exactly one warning, whose message **contains** the subsystem-tagged text, carrying the underlying cause. Both halves of the behaviour stay pinned — the `resolves.toMatchObject` above proves the secret is still created when the audit emitter fails, and this proves the failure is surfaced rather than swallowed. **Mutation-verified rather than assumed green:** deleting the `severityAuditLog.warn` call in `async-secrets-store.ts` fails with `expected "warn" to be called 1 times, but got 0 times`. A `stringContaining` assertion that passes because it matches nothing would be worse than the brittle one it replaces. ## The four, complete | suite | verdict | |---|---| | `store-wedge-resolution` | **product bug** — `42P18`, total runtime failure of wedge resolution in PG (#2669) | | `workflow-settings-project-identity` | **stale docs** — resolver contradicted its own documented order (#2671) | | `agent-logs-and-monitor` | **real defect** — funnel mis-bucketing from the U11 merge; half fixed in #2674, half needs a product call | | `central-archive-secrets` | **stale test** — this PR | **Three of four were real problems**, sitting behind "pre-existing, fails on main too". That phrase answers *whose* problem it is, not *what* is wrong. ## Census, again `check:lifecycle-columns` is **still** exiting 1 on `origin/main` — `executor.ts: allows 87, tree has 85` — the same staleness flagged on #2674. Re-recorded here too, because the blocking check stays red for every open PR until some PR carries it, and I do not know which of #2674 / this one lands first. ## Verification `pnpm test:gate` green (10 / 158 / 487 / 71). Suite **14/15 → 15/15**. `pnpm check:lifecycle-columns` exits 0 after the re-record. `pnpm lint` clean. No changeset: test-only plus an internal baseline. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
gsxdsm
|
632d10a9b4 |
fix(engine): a completed-blocked guard was inert on renamed boards — plus one owner for the terminal pair (#2568)
Two commits: a behaviour-preserving extraction, then the behaviour change. ## ⚠️ Stack note worth acting on **#2550 and #2554 both report MERGED, but their content is not on `main`.** They merged into their *base branches*, and the bottom of that stack (**#2544**) is still open. Nothing in this chain has reached `main` yet. Nothing is lost — everything is in `origin/feature/workflow-e2e-merge-rebound`, which is why this PR targets it. But "merged" reads as "landed" and here it doesn't. **Merging #2544 flows the whole chain down.** ## The bug `parkCompletedBlockedTask` opens with *"is this card already finished?"* and answered it with: ```ts if (task.column === "done" || task.column === "archived") return false; ``` On a renamed board neither matches, so **the guard was inert** — and the very next branch (`if (task.column !== "todo")`) would then have **moved a completed card back out of its own terminal column**. A guard that never fires does not fail a test. This one was found by tracing the last ledger site, not by anything going red. ## Why a shared owner, not a local fix `merger-ai`'s `isAlreadyFinalizedColumn` held the **only** copy of the per-role terminal-pair rule — a P1 learned the hard way (PR #2471 review): a per-**set** fallback collapses to one element for a workflow declaring `complete` but no `archived`, silently dropping the archived half of every already-finished check. Executor's guard was the raw literal pair, so **whoever converted it next would have re-made exactly that mistake** — the lesson lived in a comment in another file. Hence `resolveTerminalColumns(ir)` in core: one owner, one place for the rule. ## Evidence, and its limits **Commit 1 (extraction) is proven behaviour-preserving**: `workflow-already-finalized-live-e2e` is unchanged and green through the delegation, and the per-set mutation **still fails** through the shared helper. **Commit 2 (the fix) is unproven at the call site, and I'm labelling it rather than implying otherwise.** `parkCompletedBlockedTask` is private and reached only from inside executor dispatch — I could not drive it end to end. So the shared helper gets its **own** tests, in both partial-role directions, precisely because its other consumer can't vouch for it. The call site is a one-line delegation to a tested function. Weaker evidence than the rest of this unit's work. Saying so, because quietly counting it as proven is the exact failure this unit exists to catch. ## Census 417 → 416. That ratchet (#2557) is a **ceiling**, so it stays green without coordination; lower the pin when convenient. ## Verification - E2E suites 10/10; helper unit tests 5/5 - core + engine `tsc --noEmit` clean - `pnpm test:gate` green (414 + 10 + 71) 🤖 Generated with [Claude Code](https://claude.com/claude-code) --- ## Note on the conflict status (2026-07-31) GitHub reports this PR `CONFLICTING / DIRTY`. **It is not.** Three independent checks: - `git rebase origin/main` on the pushed head reports *"up to date"* and leaves the SHA unchanged — the branch is already on top of main. - `git merge-tree` against the merge base produces **zero** conflict markers. - `origin/main` is unchanged at the commit this was rebased onto. The remote SHA matches the local head, so the push landed. The `mergeable` field is a **stale computation** — it goes stale after a force-push and doesn't always recompute. This branch has now been rebased and force-pushed four times against that cached value. Worth guarding at the source: the auto-retry treats `mergeable` as ground truth, so a stale value generates conflict notices indefinitely. Confirming with a trial rebase or `git merge-tree` before dispatching distinguishes "actually conflicting" from "GitHub hasn't recomputed" — one command, and it ends the loop. Verification on the current head: merge gate green (487 + 158 + 10), engine + core tsc clean, lint clean, 20 tests in the affected suite, zero unresolved threads. --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
gsxdsm
|
8d84cee11e |
fix(core): the workflow-settings identity resolver contradicted its own docs (2 long-red tests) (#2671)
Second of the four long-red live-PG suites, after #2669. This one is **stale documentation making a stale test look like a code bug** — behaviour is unchanged. ## What was wrong `getWorkflowSettingsProjectIdImpl` documented a three-step resolution order: ``` (a) store.asyncLayer?.projectId — central-registry id (PG) (b) store.db.getProjectIdentity()?.id — legacy SQLite identity (c) store.rootDir — last-resort key ``` The code does (a), then returns `rootDir`. **Step (b) was removed** by `FNXC:SqliteDualPathCleanup 2026-07-26-14:15` — but the doc block kept describing it, and a comment three lines above the return still said *"Only the true legacy (non-backend) path consults the SQLite identity"*, which has been false for every caller since. ## Which side was wrong — settled by construction, not judgement In my triage on #2669 I said I would not guess between "the test is stale" and "the code lost a needed branch", because the two have opposite consequences and the stale comments made the intent unreadable from outside. That was the right call then; it is now answerable: `dbImpl` **throws unconditionally and ignores its store argument** (`task-id-integrity.ts:58`): ```ts export function dbImpl(_store: TaskStore): Database { throw new Error("TaskStore.db: SQLite Database is not available in backend mode …"); } ``` There is no mode in which `store.db` yields a usable SQLite handle. Step (b) is unreachable **by construction**, not merely unused — so the code is right and the documentation was wrong. ## Why the tests passed review originally They build a store double whose `getProjectIdentity()` **returns** a value: ```ts db: { getProjectIdentity() { return { id: "legacy_identity_id" }; } } ``` Production cannot produce that shape. The double made an unreachable branch look testable, which is how the assertion survived the cleanup that deleted the branch. Rewritten to the shipped contract. A neighbouring case that already asserted `rootDir` *when the stub throws* was passing all along — the two forms of the same store disagreed inside one file. ## Verification Suite **7/9 → 9/9**. `pnpm test:gate` green (10 / 158 / 487 / 71). `pnpm check:lifecycle-columns` exits 0. `tsc -p packages/core/tsconfig.json` clean. `pnpm lint` clean. No changeset: no behaviour change, and no user-visible effect. ## Remaining from the four - ✅ `store-wedge-resolution` — real product bug, fixed in #2669 - ✅ `workflow-settings-project-identity` — this PR - ⬜ `agent-logs-and-monitor` — `expected +0 to be 2` on an aggregation - ⬜ `central-archive-secrets` — an assertion on `warn` arguments Two of four were real problems hiding behind "pre-existing". The other two are still unruled-out. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
gsxdsm
|
dca20496f4 |
consolidate/u7: plugins to zero + 8 executor rebound guards + resume lanes (supersedes #2607, #2635, #2640) (#2644)
Consolidation branch for U7, per the new one-branch working mode. **Supersedes #2607, #2635, #2640** — the three of my PRs that were stuck on review threads. My other seven (#2602, #2605, #2606, #2611, #2621, #2628, #2633) are green with **zero unresolved threads** and are deliberately left alone for the merge sweep. ## What is in here, file by file | file | change | guards before → after | |---|---|---| | `plugins/…/glasses/src/agent-actions.ts` | gates, destinations and degraded-resolution refusal all resolve from the task's own workflow | 2 → 0 | | `plugins/…/glasses/src/quick-capture.ts` | accepted capture columns come from the board; default no longer names the deleted column | 1 → 0 | | `plugins/…/glasses/src/settings.ts` | quick-capture default was `triage`, the column #2515 removed | (assignment, uncounted) | | `plugins/…/dependency-graph/src/GraphTaskNode.tsx` | redundant column condition deleted | 1 → 0 | | `packages/engine/src/executor.ts` | 8 rebound guards compare the resolved column; 4 resume-eligibility literals share one resolver | 151 → 143 (+4 off-bar) | | `packages/engine/src/__tests__/` | 4 new suites, 26 cases | — | `plugins/` reaches **zero** column guards with this branch. ## The three threads it closes **#2607 — five findings, all mine, all the same rule.** I kept *qualifying* a legacy-id fallback instead of removing it: | attempt | rule | hole review found | |---|---|---| | 1 | fall back to `todo` when the role is missing | moved cards to phantom columns | | 2 | …only if the workflow **declares** `todo` | aliased **review** lane named `todo` | | 3 | …and only if no other role is assigned to it | **traitless** parking column named `todo` | The qualifications were the mistake. Once `resolveLanes` returns a lane set the workflow *has* a column vocabulary, so "no column carries the hold trait" is a complete answer — refuse. `destination()` is two lines now, with no aliasing surface left to qualify. Plus a sixth, which is a genuinely different state: **degraded resolution is indistinguishable from the default board.** `resolveWorkflowIrForTask` is total by design — a missing definition silently returns the *default* coding IR — so a card on a custom board whose definition could not be read resolved to `todo`/`in-progress`. `undefined` lanes cannot express that (it means "no workflow at all", where the legacy ids *are* the answer). The actions now refuse with 409. #2618 would replace this check with resolver provenance; it is not merged, so this does not depend on it. **#2635 — "seven rebound sites remain untested."** Fair; my "same shape" note was an assertion, not coverage. Seven of the eight need a live graph run to reach, so the *shape* is pinned instead: a static check that no guard in front of a rebound move compares against a column literal, with a vacuity case (the same detection run against the original shape) and a match-count floor (≥8), because a guard reporting success on zero matches is worse than no guard. **#2640 — duplicate workflow resolution.** Framed as I/O; it is also a correctness bug. Eligibility and re-entry are two halves of one decision and resolved the workflow separately, so a workflow edit landing between them has the halves reading *different boards*. Now one caller-owned memo per decision — caller-owned because a process-lifetime cache would have to guess when a mid-flight workflow edit invalidates it. ## Behavioural findings, not tidying - **The last-resort recovery for completed-but-stranded work did not exist off the default lineage.** `promotedFromPlannerColumn` was false on a renamed board, so finished work resting in planning was never promoted; the code fell through to a review handoff that role adjacency rejects, and the card stayed stuck with its work complete. - **Rebound guards could not see the column their own move targeted.** U5b converted the move target; the eight `column !== "todo"` checks in front of it were left literal, so on a renamed board the engine moved a card into the column it was already in — and `moveTaskInternal` runs reset-on-entry on every real move, so at the `preserveProgress: false` site it reset step progress a second time. - **The FN-1404 `task:move` audit row was lying**, recording `to: "todo"` while the move target was resolved. A run-audit trail that disagrees with the move it describes is worse than none. Not a comparison, so no census counts it. - **A task interrupted by an engine pause never resumed on a renamed board** (off-bar, `in-review`/`in-progress` literals): four comparisons decided one question and had to agree; two of them disagreed on a renamed board, so re-entry silently never fired. ## Revert proofs, isolated per site | reverted | result | |---|---| | `destination()` back to attempt 3 | 3 of 38 fail | | degraded-resolution refusals removed | 2 of 42 fail | | capture set back to the legacy five | 2 of 3 fail (renamed-board suite) | | forward exclusions → literals | 1 of 14 fails | | missing-wip refusal removed | 2 of 14 fail | | `promotedFromPlannerColumn` → literals | 3 of 7 fail | | promotion target → `"in-progress"` | 3 of 7 fail | | one rebound guard → `!== "todo"` | 1 of 3 fails (static shape) | | resume lanes → legacy trio | 1 of 5 fails | Every conversion is paired with a negative — a forward move, a not-a-planner-lane card, a default-lineage card, an unresolvable workflow — so neither "always fire" nor "never fire" can pass for "resolve the role". ## Commit discipline Twelve commits, each one thing: the code move (`resolvePlannerLanes` out of `triage.ts`) is separate from every behavior change, and each review fix is its own commit with its own revert proof. ## Verification - `pnpm test:gate` **71/71** - 162/162 across the glasses plugin's 19 files; 26/26 across the four new engine suites - engine + glasses typecheck clean; `pnpm lint` clean 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Engine recovery and retries now work correctly with renamed or customized workflow columns. * Tasks in manual-intake columns are no longer automatically planned. * Agent actions and quick capture now respect each board’s declared columns and lifecycle stages. * Awaiting-approval tasks are recognized regardless of their current column. * Command Center SDLC funnel stages now accurately reflect customized workflows. * **Documentation** * Added guidance for safely changing workflow-column logic and interpreting lifecycle-column checks. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |