refactor(cutover 1/3): core — IR-driven lifecycle foundation#2341
Conversation
|
Warning Review limit reached
Next review available in: 27 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThis change establishes workflow IR as the lifecycle authority, adding trait-based transition policies, default workflow resolution, review leases, workflow pin persistence, legacy-row adoption, schema safeguards, review-level creation presets, and extensive validation and regression coverage. ChangesWorkflow lifecycle cutover
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant TaskStore
participant WorkflowResolver
participant TransitionPolicy
participant WorkflowDatabase
TaskStore->>WorkflowResolver: resolve task workflow IR
WorkflowResolver-->>TaskStore: resolved IR and column traits
TaskStore->>TransitionPolicy: evaluate transition invariants
TransitionPolicy-->>TaskStore: allow or rejection
TaskStore->>WorkflowDatabase: persist move and workflow metadata
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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 |
Greptile SummaryThis PR lands the first third of the IR-driven lifecycle cutover — the core foundation layer for
Confidence Score: 4/5Safe to merge with one fix: detectWorkflowDrift in workflow-ir-resolver.ts does not check the pinned (entry-time) column against the current column list when the node itself survives. No recovery sweep calls this function yet in this PR, so there is no immediate runtime breakage, but the defect will be inherited by parts 2/3 where the consumers land. The drift-detection infrastructure is laid here but its consumers arrive in part 2/3. The pin.columnId field's stated purpose — catching a deleted column even when the node survives — is not implemented: the check always resolves to node.column (the current column), so pin.columnId is unreachable. Parts 2/3 will depend on this function behaving correctly. Everything else — the transition-policy module, legacy adoption sweep, pooled-capacity enforcement, builtin-workflow column fix, and schema migration — is sound and well-tested. packages/core/src/workflow-ir-resolver.ts — detectWorkflowDrift needs to check pin.columnId against the current columns array independently of node.column. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
subgraph "KTD-3: IR Pin & Drift Detection"
A[Task enters node] --> B[computeWorkflowIrPin\nirHash + columnId stored]
B --> C[workflowIrPin persisted to DB]
C --> D{Restart / recovery}
D --> E[detectWorkflowDrift\ncurrent IR vs pin]
E -->|hash matches| F[No drift — resume]
E -->|node deleted| G[park: reconcile-workflow-drift]
E -->|column deleted| G
E -->|hash differs, structure OK| F
end
subgraph "KTD-5: Shared Transition Policy"
H[moveTaskInternalImpl] --> I[resolveTransitionColumnFacts\nfrom + to flags]
I --> J[evaluateTransitionInvariants\nmerge-blocker + terminal-reentry]
J -->|allow| K{capacity check\nenforcePooledColumnCapacity}
J -->|reject| L[TransitionRejectionError]
K -->|budget OK| M[move committed]
K -->|exhausted| L
end
subgraph "KTD-8: Legacy Adoption Sweep"
N[store open] --> O{drained marker?}
O -->|yes| P[skip sweep]
O -->|no| Q[paginate listTasks]
Q --> R[planLegacyAdoption\nfor each row]
R -->|skip / already-adopted| Q
R -->|resume-graph / clear| S[updateTask status=null]
R -->|park-paused| T[updateTask paused=true]
R -->|preserve + backfill| U[updateTask enabledWorkflowSteps]
S & T & U --> V{mutationPlanned?}
V -->|no| W[write drained marker]
V -->|yes| Q
end
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
subgraph "KTD-3: IR Pin & Drift Detection"
A[Task enters node] --> B[computeWorkflowIrPin\nirHash + columnId stored]
B --> C[workflowIrPin persisted to DB]
C --> D{Restart / recovery}
D --> E[detectWorkflowDrift\ncurrent IR vs pin]
E -->|hash matches| F[No drift — resume]
E -->|node deleted| G[park: reconcile-workflow-drift]
E -->|column deleted| G
E -->|hash differs, structure OK| F
end
subgraph "KTD-5: Shared Transition Policy"
H[moveTaskInternalImpl] --> I[resolveTransitionColumnFacts\nfrom + to flags]
I --> J[evaluateTransitionInvariants\nmerge-blocker + terminal-reentry]
J -->|allow| K{capacity check\nenforcePooledColumnCapacity}
J -->|reject| L[TransitionRejectionError]
K -->|budget OK| M[move committed]
K -->|exhausted| L
end
subgraph "KTD-8: Legacy Adoption Sweep"
N[store open] --> O{drained marker?}
O -->|yes| P[skip sweep]
O -->|no| Q[paginate listTasks]
Q --> R[planLegacyAdoption\nfor each row]
R -->|skip / already-adopted| Q
R -->|resume-graph / clear| S[updateTask status=null]
R -->|park-paused| T[updateTask paused=true]
R -->|preserve + backfill| U[updateTask enabledWorkflowSteps]
S & T & U --> V{mutationPlanned?}
V -->|no| W[write drained marker]
V -->|yes| Q
end
Reviews (4): Last reviewed commit: "Address PR review feedback round 3 (#234..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
- dedupe workflow_ir_pin_column_id in postgres-health EXPECTED_PROJECT_COLUMNS - legacy-adoption: 'paused' removed from preserve-comment (task pause is a boolean field, never a status; documented) - getTaskMergeBlocker gains explicit skipColumnIdentityCheck option; KTD-5 validator passes the REAL task instead of spoofing column:'in-review' - status-write census recursively scans core+engine+dashboard src with precision-hardened patterns (9 false-positive classes excluded) and a >100-file vacuity floor Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Transition policy + shared validator, IR validation hardening, CAS review leases, capacity budgets, lifecycle traits, IR pin/drift, review-level preset, legacy adoption + migration 0026, builtin workflow fixes (core side), and the cutover plan + handoff docs. Stack: 1/3 (base main); engine cutover is 2/3; dashboard + changesets top out in #2335. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- dedupe workflow_ir_pin_column_id in postgres-health EXPECTED_PROJECT_COLUMNS - legacy-adoption: 'paused' removed from preserve-comment (task pause is a boolean field, never a status; documented) - getTaskMergeBlocker gains explicit skipColumnIdentityCheck option; KTD-5 validator passes the REAL task instead of spoofing column:'in-review' - status-write census recursively scans core+engine+dashboard src with precision-hardened patterns (9 false-positive classes excluded) and a >100-file vacuity floor Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…laimed 0026) Rebased onto main's 0026_bigint_counters (#2331): our migration renumbers to 0027, SCHEMA_BASELINE_VERSION bumps to 0027, applier re-spliced onto main's reworked file, fixture/probe versions updated. Note: main's tip itself fails the automation project-isolation upgrade test (bigint ALTER vs legacy 0000 fixture missing token_usage_input_tokens) — verified on detached origin/main; not ours. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
612bc71 to
419d464
Compare
|
Rebased onto latest main and resolved the conflict: main's #2331 claimed migration 0026 (bigint counters), so this PR's IR-pin/legacy-adoption migration renumbered to 0027 with
🤖 Generated with Claude Code |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/core/src/task-store/lifecycle-ops.ts (1)
256-268: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winAdoption sweep is gated behind unrelated steps' success.
adoptLegacyTaskRowsOnOpen(store)shares this try block withrunWorkflowColumnsIntegrityPass()andrecoverStaleTransitionPending(). If either of those two throws, the catch swallows it and adoption never runs this boot cycle — even thoughadoptLegacyTaskRowsOnOpenalready has its own internal try/catch and is designed to be independently safe. Given the file's stated goal ("every pre-cutover task row must wake OWNED — no silently frozen rows"), the sweep should not be incidentally skipped by an unrelated failure.♻️ Suggested fix: give the adoption call its own try boundary
try { await store.runWorkflowColumnsIntegrityPass(); // `#1401`: recover any transitionPending markers stranded by a crash // between the in-txn write and the post-commit clear (they otherwise // permanently inflate capacity counts for their target column). await store.recoverStaleTransitionPending(); - await adoptLegacyTaskRowsOnOpen(store); } catch (err) { storeLog.warn("workflowColumns integrity pass failed during init", { phase: "init:workflow-columns-integrity", error: err instanceof Error ? err.message : String(err), }); } + await adoptLegacyTaskRowsOnOpen(store);🤖 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/task-store/lifecycle-ops.ts` around lines 256 - 268, Separate adoptLegacyTaskRowsOnOpen(store) from the try block containing runWorkflowColumnsIntegrityPass() and recoverStaleTransitionPending(). Give the integrity and recovery operations their existing warning boundary, then invoke adoption under its own try boundary so failures in either unrelated step cannot skip the adoption sweep, while preserving its internal error handling.
🧹 Nitpick comments (2)
packages/core/src/workflow-capacity.ts (1)
175-185: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSimplify redundant array checks.
Because
targetKeyis truthy at this stage, we are mathematically guaranteed thatresolveColumnBudgetKeysuccessfully locatedtargetColumnusingfindColumn, which already asserts that(ir as WorkflowIrV2).columnsis an array.Consequently,
columnsis always iterable andsetwill always contain at leasttargetColumn, making the defensive checks and the ternary fallback safely removable.♻️ Proposed refactor
export function resolveWipBudgetColumns(ir: WorkflowIr, targetColumn: string): string[] { const targetKey = resolveColumnBudgetKey(ir, targetColumn); if (!targetKey) return []; - const v2 = ir as WorkflowIrV2; - const columns = Array.isArray(v2.columns) ? v2.columns : []; const set: string[] = []; - for (const c of columns) { + for (const c of (ir as WorkflowIrV2).columns) { if (resolveColumnBudgetKey(ir, c.id) === targetKey) set.push(c.id); } - return set.length > 0 ? set : [targetColumn]; + return set; }🤖 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/workflow-capacity.ts` around lines 175 - 185, In resolveWipBudgetColumns, rely on the successful targetKey lookup to use (ir as WorkflowIrV2).columns directly, removing the redundant Array.isArray fallback. Since targetColumn is guaranteed to be included, return the collected set directly and remove the length check and targetColumn fallback.packages/core/src/task-store/lifecycle-ops.ts (1)
271-343: 🚀 Performance & Scalability | 🔵 TrivialFull active-task sweep runs on every store open indefinitely, not just during the cutover window.
adoptLegacyTaskRowsOnOpenpaginates through every active task (500/page) on every single store open, forever — there's no completion marker to short-circuit the sweep once no legacy rows remain. For deployments with large task counts or frequent restarts, this adds a permanent, unbounded (in wall-clock terms) startup cost that scales with total active task count rather than with the shrinking legacy backlog. The offset-pagination rationale documented above is sound for correctness, but nothing here caps the steady-state cost once adoption is fully drained project-wide.Consider persisting a project-level "legacy adoption complete" marker (set once a full sweep completes with zero adoptions) to skip the sweep entirely on subsequent opens, re-arming it only if needed.
🤖 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/task-store/lifecycle-ops.ts` around lines 271 - 343, Update adoptLegacyTaskRowsOnOpen to persist and check a project-level legacy-adoption completion marker before paginating; skip the sweep when the marker is set, and set it only after a full sweep completes with zero adoptions. Preserve the existing pagination, per-task adoption, and failure handling, and re-arm or avoid marking completion when the sweep fails or adopts rows.
🤖 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/index.gate.ts`:
- Around line 2219-2242: Add the runtime export of StaleBinarySchemaError and
assertBinaryNotOlderThanDatabase to the gate barrel index.gate.ts, mirroring
their existing exports from index.ts. Place them with the other gate-safe barrel
exports and preserve their original source module and export behavior so the
engine-core bundle exposes both symbols.
In `@packages/core/src/task-store/workflow-definitions.ts`:
- Around line 468-482: Wrap the default IR returned by the custom-workflow
fallback branches in store.applyBuiltInPromptOverridesSync, matching the
!workflowId path. Update both the !row branch and the database-read catch
branch, using DEFAULT_WORKFLOW_ID with resolveDefaultWorkflowIr(), while leaving
successful custom-workflow parsing unchanged.
---
Outside diff comments:
In `@packages/core/src/task-store/lifecycle-ops.ts`:
- Around line 256-268: Separate adoptLegacyTaskRowsOnOpen(store) from the try
block containing runWorkflowColumnsIntegrityPass() and
recoverStaleTransitionPending(). Give the integrity and recovery operations
their existing warning boundary, then invoke adoption under its own try boundary
so failures in either unrelated step cannot skip the adoption sweep, while
preserving its internal error handling.
---
Nitpick comments:
In `@packages/core/src/task-store/lifecycle-ops.ts`:
- Around line 271-343: Update adoptLegacyTaskRowsOnOpen to persist and check a
project-level legacy-adoption completion marker before paginating; skip the
sweep when the marker is set, and set it only after a full sweep completes with
zero adoptions. Preserve the existing pagination, per-task adoption, and failure
handling, and re-arm or avoid marking completion when the sweep fails or adopts
rows.
In `@packages/core/src/workflow-capacity.ts`:
- Around line 175-185: In resolveWipBudgetColumns, rely on the successful
targetKey lookup to use (ir as WorkflowIrV2).columns directly, removing the
redundant Array.isArray fallback. Since targetColumn is guaranteed to be
included, return the collected set directly and remove the length check and
targetColumn fallback.
🪄 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: 56c2b67d-0998-4909-a670-1b39fc8b83d9
📒 Files selected for processing (47)
docs/plans/2026-07-18-001-refactor-ir-driven-lifecycle-cutover-plan.mddocs/plans/2026-07-19-001-u5e-executecore-lift-handoff.mddocs/plans/2026-07-19-002-u5e-remaining-deletions-handoff.mddocs/plans/2026-07-19-003-u5f-workflow-aware-flip-handoff.mddocs/plans/2026-07-19-004-u5g-flip-residual-handoff.mddocs/plans/2026-07-19-005-u10-flip-residual-handoff.mddocs/plans/2026-07-19-006-u10b-post-flip-handoff.mdpackages/core/src/__tests__/custom-review-lane-merge-blocker.test.tspackages/core/src/__tests__/legacy-adoption.test.tspackages/core/src/__tests__/no-selection-default-workflow-move.test.tspackages/core/src/__tests__/postgres/schema-applier.test.tspackages/core/src/__tests__/review-level-preset.test.tspackages/core/src/__tests__/workflow-ir-validation.test.tspackages/core/src/__tests__/workflow-lifecycle-traits.test.tspackages/core/src/builtin-workflows.tspackages/core/src/index.gate.tspackages/core/src/index.tspackages/core/src/legacy-adoption.tspackages/core/src/postgres/index.tspackages/core/src/postgres/migrations/0000_initial.sqlpackages/core/src/postgres/migrations/0027_workflow_ir_pin_and_legacy_adoption.sqlpackages/core/src/postgres/postgres-health.tspackages/core/src/postgres/schema-applier.tspackages/core/src/postgres/schema/project.tspackages/core/src/review-level-preset.tspackages/core/src/store.tspackages/core/src/task-merge.tspackages/core/src/task-store/__tests__/transition-validator.test.tspackages/core/src/task-store/lifecycle-ops.tspackages/core/src/task-store/moves.tspackages/core/src/task-store/persistence.tspackages/core/src/task-store/remaining-ops-2.tspackages/core/src/task-store/remaining-ops-6.tspackages/core/src/task-store/serialization.tspackages/core/src/task-store/task-creation.tspackages/core/src/task-store/task-row-mappers.tspackages/core/src/task-store/task-update.tspackages/core/src/task-store/workflow-definitions.tspackages/core/src/types.tspackages/core/src/types/board.tspackages/core/src/types/workflow-steps.tspackages/core/src/workflow-capacity.tspackages/core/src/workflow-ir-resolver.tspackages/core/src/workflow-ir.tspackages/core/src/workflow-lifecycle-traits.tspackages/core/src/workflow-step-results.tspackages/core/src/workflow-transition-policy.ts
- mirror StaleBinarySchemaError + assertBinaryNotOlderThanDatabase into index.gate.ts (gate-barrel sync rule) - deleted-workflow and read-error fallbacks now apply built-in prompt overrides like the no-selection branch Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- adoption sweep gets its own try boundary (unrelated integrity-pass throws can no longer skip it) - resolveWipBudgetColumns: provably-dead defensive fallbacks removed - durable 'legacy-adoption-drained' marker short-circuits the drain sweep once complete (non-numeric by design — the stale-binary guard ignores unparseable versions; userPaused rows withhold the marker so they stay adoptable after unpause; read failure fails open toward sweeping) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Re: CodeRabbit review body round 3 (inline posts failed; findings addressed in 9c5e71c):
Fixed: the adoption call runs on its own boundary now — it's internally fail-soft, so a bare await is safe, and unrelated integrity-pass throws can no longer skip it.
Fixed: dropped the provably-dead
Fixed: a durable non-numeric 🤖 Generated with Claude Code |
…on deleted (#2342) Part **2 of 3** of the IR-driven lifecycle cutover (stacked on #2341; top is #2335). **Scope (80 files, packages/engine + cli/pi skill docs + AGENTS/architecture):** graph-driven column moves via the column-boundary controller (R1), single-mover scheduler/hold-release trait cutover (KTD-2/KTD-9), trait re-keyed self-healing + merger with the R7b confirmed-merge-must-finalize guarantee, graph-exclusive Plan Review with leased dedup (R4/R5), the executeCore body-lift — zero legacy re-entry — with fn_review_step + interceptor machinery deleted and tombstone-ratcheted (R9), builtin workflow runtime fixes (missing hold handler, unseamed-node column inheritance, no-merge completion mover), the 6-column benchmark acceptance suite (11 tests) + 12-builtin lifecycle sweep (94 assertions), and the executor test-harness modernization. Also retires core's interpreter-cutover scaffolding whose last consumer (the authoritative driver) dies here. **Merge order:** #2341 → this → #2335. After #2341 merges, retarget this to main. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
1 participant
Part 1 of 3 of the IR-driven lifecycle cutover (split from #2335 to fit review-tool file limits; plan: docs/plans/2026-07-18-001-refactor-ir-driven-lifecycle-cutover-plan.md, included here).
Scope (48 files, packages/core + docs/plans): shared transition policy + validator (KTD-5), IR validation hardening incl. the benchmark capability floor, CAS review leases (KTD-4), pooled WIP capacity budgets (KTD-9), lifecycle-trait helpers, durable IR pin/drift detection (KTD-3), review-level creation-time preset, legacy adoption module + census + migration 0026 + stale-binary guard (KTD-8), core-side builtin workflow fixes (single default-IR authority, no-merge complete-column support).
Note:
workflow-cutover.ts(interpreter parity scaffolding) stays alive in this PR — its last consumer dies in part 2/3, which retires it.Merge order: this PR → #TBD-2 (engine) → #2335 (dashboard/top).
🤖 Generated with Claude Code
Summary by CodeRabbit
reviewLevelpresets to automatically configure enabled workflow steps.