Phase A3: the in-txn capacity invariant does NOT hold — root-caused, ratcheted, not fixed - #2469
Merged
Merged
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds a Postgres Vitest suite that probes workflow-column execution paths, fills the ChangesWorkflow capacity invariant coverage
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
Greptile SummaryAdds a PostgreSQL integration-test ratchet documenting the currently broken workflow-capacity invariant.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains.
|
| Filename | Overview |
|---|---|
| packages/core/src/tests/postgres/workflow-capacity-invariant.pg.test.ts | Adds four PostgreSQL cases that distinguish the path gate and pool-sentinel mismatch while ratcheting the intended capacity invariant. |
Reviews (2): Last reviewed commit: "test(core): ground truth for the in-txn ..." | Re-trigger Greptile
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/core/src/__tests__/postgres/workflow-capacity-invariant.pg.test.ts (1)
1-46: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCondense the FNXC header.
This 45-line implementation narrative is not concise and includes fragile source-line references. Retain the dated behavioral contract here; keep the detailed diagnosis outside the test source.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/__tests__/postgres/workflow-capacity-invariant.pg.test.ts` around lines 1 - 46, Condense the header comment above the tests to retain only the dated behavioral contract and the essential statement that the suite captures current capacity-enforcement behavior. Remove the detailed R1/R2 diagnosis, source-line references, implementation narrative, and operational blast-radius discussion; leave the test cases and their assertions unchanged.Sources: Coding guidelines, Learnings
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/core/src/__tests__/postgres/workflow-capacity-invariant.pg.test.ts`:
- Around line 184-199: Add coverage for the same expected-failure capacity
invariant through the “inline” path as well as “hooks”, preferably by
parameterizing the existing test over both paths. Ensure each case calls setPath
with its path and verifies capacity-exhausted rejection with secondColumn
remaining “todo”.
---
Nitpick comments:
In `@packages/core/src/__tests__/postgres/workflow-capacity-invariant.pg.test.ts`:
- Around line 1-46: Condense the header comment above the tests to retain only
the dated behavioral contract and the essential statement that the suite
captures current capacity-enforcement behavior. Remove the detailed R1/R2
diagnosis, source-line references, implementation narrative, and operational
blast-radius discussion; leave the test cases and their assertions unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 4559a4bb-fef8-4c6a-883f-7d8065d59c56
📒 Files selected for processing (1)
packages/core/src/__tests__/postgres/workflow-capacity-invariant.pg.test.ts
…NOT hold (Phase A3)
Phase A3. `workflow-capacity.ts` states enforcement "runs INSIDE
moveTaskInternal's transaction and is NEVER bypassable". Phase A2 observed a
second move accepted into a full wip column. This establishes which is true, by
test. The A2 observation was REAL, not a harness artifact.
The invariant does not hold for default-workflow tasks, for two independent
reasons, each proven separately.
R1 — POOL-ID SENTINEL MISMATCH (the defect). moves.ts:319 resolves the pool for
a task with no workflow selection as `?? "builtin:coding"`, while
countActiveInCapacitySlotAsyncImpl buckets no-selection rows under
`?? DEFAULT_WORKFLOW_POOL_ID` ("__default-workflow__"). The check therefore asks
for occupants of a pool nothing is ever bucketed into: the count is always 0 and
the limit cannot bind. The SECOND enforcement point, the hold/release sweep at
hold-release.ts:116, uses DEFAULT_WORKFLOW_POOL_ID and is correct — so the two
points disagree about pool identity, which is exactly what the module docstring
claims is impossible ("the two enforcement points can never disagree").
R2 — THE useWorkflow GATE. The whole block sits inside
`if (useWorkflow && workflowIr && fromColumn !== toColumn)` (moves.ts:921), and
useWorkflow reads the raw experimentalFeatures.workflowColumns key nothing sets.
On the LIVE path the check cannot run at all, so R1 is latent there and becomes
reachable the moment A2 converges onto the flag-ON side.
The third case is the DISCRIMINATOR and is what makes this a diagnosis rather
than a guess: giving both tasks an explicit `builtin:coding` selection changes
nothing but the sentinel agreement, and the rejection appears with the typed
`capacity-exhausted` code. That rules out "capacity is simply not wired".
The fourth case is the INVARIANT RATCHET, marked `it.fails`: it states the
invariant as written, so today the body fails and the marker keeps CI green
while honestly recording the breach — and the moment the sentinel is fixed the
marker starts failing, forcing whoever lands the fix to flip it and the two
DEFECT expectations. That is "a test that fails if the invariant is broken" in
the only shape that does not park a permanently-red test in CI.
Both-path forcing reuses A2's assertPathActive probe. Without it this suite
would silently run one path twice and report a tautology, which is how A2's
first two harness bugs produced sixteen false passes between them.
NO FIX HERE, deliberately. Fixing the sentinel is a one-line change but it turns
store-level capacity enforcement ON for every default-workflow task the moment
the paths converge; the graph column boundary parks runs on capacity-exhausted,
so that is a scheduling behavior change across every project. Blast radius is
reported for an operator decision first, per the unit's step 4.
gsxdsm
force-pushed
the
feature/workflow-capacity-ground-truth
branch
from
July 27, 2026 20:52
575d01f to
80a72d5
Compare
gsxdsm
added a commit
that referenced
this pull request
Jul 27, 2026
…guards, red-green) (#2479) **Stacked on #2469** → #2468 → #2467. Base is `feature/workflow-capacity-ground-truth`. This is **slice B1 of Phase B, not all of Phase B.** Sizing escalation sent separately; the census is below. ## Why this is a slice Measured census of code lines referencing a lifecycle column literal (comments excluded): | Unit | Files | Sites | |---|---|---:| | U4 | `self-healing.ts` | 203 | | U5 | `executor.ts` 171, `scheduler.ts` 55, `replan-target.ts` 20, `merger-ai.ts` 5, `hold-release.ts` 4, `mesh-lease-manager.ts` 4, `task-agent-sync.ts` 3 | 262 | | U6 | `moves.ts` 34, `default-workflow-hooks.ts` 13, `board-config.ts` 9, `blocker-fanout.ts` 6, `task-priority.ts` 5, `dependency-blocked-todo-report.ts` 2, `stale-paused-todo.ts` 1 | 70 | | | **Total** | **535** | The plan's "~207" counts the guard category only. Under the phase's non-negotiable rule — a test that **fails before** conversion, per guard — that is ~200 red-green cycles. Doing it as one sweep would reproduce exactly the failure this phase exists to prevent: converted guards nobody proved still fire. `moves.ts` and `default-workflow-hooks.ts` stay **parked** per the dispatch constraint (move-path convergence and the pool-id sentinel are on an operator decision). ## Guards converted (4), each red-green Every case below was written **first** and observed failing against the literal implementation. | Module | Guard | Before → After | |---|---|---| | `stale-paused-todo.ts` | stall detection | `column !== "todo"` → resolved **hold** column | | `blocker-fanout.ts` | active | `ACTIVE_COLUMNS.has(col)` → `!terminalColumns.has(col)` | | `blocker-fanout.ts` | hold-wait metric | `col === "todo"` → resolved **hold** column | | `task-priority.ts` | unblock active | `UNBLOCK_ACTIVE_COLUMNS` **deleted**, folded into the terminal set | Three of the seven new cases are **regression floors** that pass before and after. One of them earned its keep immediately: it failed on my own fixture (`activeCount` vs the public `totalCount`), catching a bad test rather than bad code — which is the point of asserting the default path alongside the renamed one. ### The `task-priority` finding `UNBLOCK_ACTIVE_COLUMNS` and `DONE_COLUMNS` encoded **one concept twice**, two lines apart, and disagreed for any custom column: dependency counting treated a `drafting` card as unmet (correct) while the active check treated it as inactive (wrong), zeroing the blocker's unblock weight. The enumeration wasn't just legacy-shaped — it contradicted its own neighbour. ##⚠️ Behavior change, not a pure refactor Inverting active from enumeration to exclusion means **a card in a column that is neither terminal nor in the legacy enum now counts as active where it previously did not.** That is the plan's stated intent, but it is a real change for any project already using a custom column — **Coding (Ideas)' `ideas` column is the in-tree case.** Fan-out counts and unblock weights for such cards will rise. ## Verification - Four affected suites green (45 tests), each conversion observed red→green. - `pnpm lint`, `tsc --noEmit` (core) green. **Not verified / not done, stated plainly:** - **Call sites are not wired.** These modules now *accept* resolved roles; every parameter still defaults to the legacy set, so at the call sites the vocabulary is unchanged. A caller that cannot resolve a workflow keeps literal behavior. Threading `resolveLifecycleColumns` through `reads.ts` and `self-healing.ts` is follow-on work — until then the guards are *convertible*, not *converted end-to-end*. - `dependency-blocked-todo-report.ts` and `board-config.ts` are untouched in this slice. - 19 core-suite failures exist on this branch; all confirmed **pre-existing** by stashing and re-running on a clean tree (`duplicate-guard`, `log-severity-spam-contract`, `settings-parity`, `task-delete-caller-attribution`, `settings-defaults`). 🤖 Generated with [Claude Code](https://claude.com/claude-code) --- **Supersedes #2470**, which GitHub force-closed when its base branch was deleted by the merge of #2469 and refuses to reopen. Same head branch, same commits (rebased onto `main`), now based on `main` directly. The two P1 review threads on #2470 were resolved there — one of them with a correction noting the threading half landed in code that was subsequently deleted as a dead feature in #2477. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Dependency and blocker reports now correctly recognize custom hold, active, and terminal workflow columns. * Blockers in renamed terminal columns are no longer incorrectly reported as active. * Stale paused-task badges and self-healing now work with workflow-specific hold columns. * Mixed boards with different workflow column names are handled consistently. * Existing default workflow behavior remains compatible, including fallback handling when workflow details cannot be resolved. * **Enhancements** * Reporting and task-priority calculations now support configurable single or multiple hold and terminal columns. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stacked on #2468, which is stacked on #2467. Base is
feature/workflow-move-path-convergence.Answer: the A2 observation was real. The documented invariant is broken. Not a false alarm, not a harness artifact.
workflow-capacity.tsstates enforcement "runs INSIDEmoveTaskInternal's transaction and is NEVER bypassable (not a guard — runs regardless of bypassGuards/recoveryRehome/moveSource)". It does not hold for default-workflow tasks, for two independent reasons.R1 — Pool-id sentinel mismatch (the defect)
moves.ts:319(in-txn check, asks)?? "builtin:coding"countActiveInCapacitySlotAsyncImpl(answers)?? DEFAULT_WORKFLOW_POOL_ID→"__default-workflow__"hold-release.ts:116(sweep, second enforcement point)?? DEFAULT_WORKFLOW_POOL_ID✅The check asks for occupants of a pool that no occupant is ever bucketed into, so the count comes back
0and the limit can never bind. The sweep is correct, so the two enforcement points disagree about pool identity — precisely what the module docstring says is impossible ("the two enforcement points can never disagree on what a limit is, only on the live count").Note the shape of the bug: it is not a missing check. The check runs, queries correctly, and returns a confidently wrong answer.
R2 — The
useWorkflowgateThe whole block sits inside
if (useWorkflow && workflowIr && fromColumn !== toColumn)(moves.ts:921), anduseWorkflowreads the rawexperimentalFeatures.workflowColumnskey nothing in production sets. On the live path the check cannot run at all, so R1 is latent today and becomes reachable the moment A2 converges onto the flag-ON side.How this was established, not guessed
Three of my assumptions failed earlier in this program, so this one is pinned by a discriminating experiment rather than a code reading:
builtin:codingcapacity-exhaustedThe third case changes nothing but sentinel agreement. That rules out "capacity is simply not wired" and isolates the cause to the mismatch.
Both-path forcing reuses A2's
assertPathActiveprobe. Without it this suite would silently run one path twice and report a tautology — the failure mode that produced sixteen false passes across A2's two harness bugs.The deliverable: an invariant ratchet
The three cases above assert today's wrong behavior, so on their own they would let the defect live forever. A fourth case states the invariant as written and is marked
it.fails:it.failspasses; CI stays green while honestly recording the breach;it.failsfails, forcing whoever lands the fix to flip it and the twoDEFECT:expectations.That is "a test that fails if the invariant is broken" in the only shape that does not park a permanently-red test in CI.
Blast radius (step 4) — why I did not fix it
The fix is one line: make
moves.ts:319useDEFAULT_WORKFLOW_POOL_ID, matching the sweep. The consequences are not one line.Today: zero.
useWorkflowis false everywhere, so the corrected check still cannot run on the live path. The sentinel fix is safe to land in isolation.At A2 convergence: every default-workflow task move into
in-progressbecomes capacity-checked againstmaxConcurrent(default 2), for the first time. Affected movers:capacity-exhaustedand parks the run. Runs that previously proceeded would begin suspending — this is a scheduling behavior change across every project, not an error path.executor.ts,project-engine.ts,pr-comment-handler.tseach move tasks intoin-progressand would begin seeing a rejection they have never seen.maxConcurrent.Scheduler-side admission (
maxConcurrent) is a separate, still-live control, so this is not "capacity is unenforced today" — it is "the store-level check the graph and promote paths are written against returns 0 and never binds".Recommendation: land the sentinel fix on its own (provably inert today), with the ratchet flipped in the same commit, before A2 convergence — so convergence does not simultaneously switch paths and switch on a previously-dead enforcement.
Verification
4 cases green against real PostgreSQL (3 passed + 1 expected-fail), path flip proven live on every case.
pnpm lintandtsc --noEmitgreen.Follow-up: both "not verified" items are now answered
Recorded here rather than left as open questions, since this is where anyone investigating capacity will look.
1. Does the sync/SQLite counter carry the same mismatch? — YES, identically, but it is unreachable.
countActiveInCapacitySlotSyncImpl(project-store-ops.ts:767) buckets rows the same way as the async one:So it disagrees with
moves.ts:319in exactly the same way. However its only caller is the publicTaskStore.countActiveInCapacitySlotSyncwrapper, which has no in-repo caller at all — it is dead API surface. The mismatch is real but currently unreachable, which makes it a landmine for whoever wires it up rather than an active defect. Fixing the sentinel should fix both call sites together.2. Are custom workflows with an explicit numeric
limitaffected? — NO, and this is already proven by the discriminator above.The mismatch fires only when the selection row is absent — that is what the
??fallback is for. A custom workflow necessarily has a selection row; that is what makes it custom. The DISCRIMINATOR case adds an explicit selection and the rejection appears, which is exactly the custom-workflow shape. So the defect is scoped to no-selection (default-workflow) tasks only.The explicit-numeric-
limitquestion turns out to be orthogonal:resolveColumnBudgetKeyreturningcol:${columnId}decides which columns share a budget, not which pool id is passed to the counter. It does not interact with the sentinel at all.Net effect on blast radius: narrower than first stated. Only default-workflow (no-selection) tasks slip the limit today. Custom-workflow tasks are already enforced — meaning the fix does not switch enforcement on for them, it only closes the gap for the default workflow.
🤖 Generated with Claude Code
Summary by CodeRabbit