docs(core): resolveReviewColumns is the BROAD set — and one of its consumers must NOT migrate onto it - #2750
Merged
Merged
Conversation
…he difference A flaw in the helper I merged in #2730, found by trying to migrate its consumers onto it. One NAME was answering two questions: BROAD "is this card in a lane where review happens?" — every mergeOrchestration lane plus every mergeBlocker/humanReview lane. Safe where over-admission is harmless: notifications, badges, read-only surfaces. This function. NARROW "is this card in THE review lane the engine acts on?" — `resolveLifecycleColumns().review` is `columnsWithFlag(ir, "mergeOrchestration")[0]`, ONE lane, and that 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. `register-task-workflow-routes.ts` keeps its own narrower resolver for exactly that reason (#2723) — its re-engagement moves the card, so admitting a second merge lane is a state change the engine will not agree with. THAT LOCAL COPY IS NOT DRIFT, and this is the correction that matters: I have been arguing for three rounds that the inline copies should converge on this helper. For the notifier they should. For those routes they must NOT — migrating them would reintroduce the over-admission #2723's review round reasoned away. I was about to do exactly that, and only reading both implementations side by side stopped me. No behaviour change. What changes is that the distinction is written down at the helper and pinned by a test, so the next consumer reaching for "the review columns" on a state-changing path can see which one it wants. The test needs a board declaring `mergeOrchestration` TWICE, because no default lineage does and the two answers are identical everywhere else. Mutation: narrowing this helper to the first merge lane — the "consolidation" someone would reasonably attempt — fails it. Verified: 30 trait tests green, core tsc clean, lint clean (0 errors), gate green (487 + 158 + 10 + 71).
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 (2)
📝 WalkthroughWalkthroughThe PR documents the distinction between broad review-column resolution and the narrow lifecycle review lane, and adds a test covering multiple merge lanes. No runtime logic changes are included. ChangesReview lane semantics
Estimated code review effort: 1 (Trivial) | ~5 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 SummaryClarifies the broad-versus-narrow workflow review-column distinction.
Confidence Score: 5/5The PR appears safe to merge because it changes only documentation and regression coverage while accurately preserving existing behavior. The new test directly captures the intended difference between the broad review-column helper and the engine’s narrow lifecycle review lane, and no production logic is modified.
|
| Filename | Overview |
|---|---|
| packages/core/src/workflow-lifecycle-traits.ts | Adds an FNXC architectural comment documenting existing broad review-column semantics without changing runtime behavior. |
| packages/core/src/tests/workflow-lifecycle-traits.test.ts | Adds focused coverage demonstrating that all merge lanes are returned while lifecycle resolution selects only the first. |
Reviews (1): Last reviewed commit: "docs(core): resolveReviewColumns is the ..." | Re-trigger Greptile
gsxdsm
added a commit
that referenced
this pull request
Jul 30, 2026
…d helper (#2734 review) My own code. The producer built `lifecycleColumnSets.review` from `columnsWithFlag(ir, "mergeOrchestration")` alone, so a workflow hosting review on a `humanReview`- or `mergeBlocker`-only lane produced an EMPTY review set — and `applyInReviewEnterEffects` never ran for a card plainly in review, leaving the recovery counters it clears set. Fixed by calling `resolveReviewColumns` (core, merged in #2730), making this the first consumer to use the shared answer instead of a fifth inline union. That is the BROAD set, which is correct here: these hooks ASK the question and move nothing on the answer. A caller that admits and then MOVES wants the narrow single lane — #2750 documents the split at the helper. THE FIRST TEST I WROTE FOR THIS PROVED NOTHING, and it is worth recording because it is the fourth time today. I asserted the HOOK directly, passing `lifecycleColumnSets: { review: ["signoff"] }` by hand — which exercises the hook, already correct, and passes with the producer's bug fully in place. Reverting the producer left it green. The producer is the thing that was wrong, so the test now drives a real move through a real store: a `humanReview`-only lane, seeded `recoveryRetryCount: 3`, asserted cleared after the move. Mutation: reverting the producer fails it. A paired negative keeps the widening from making every column a review lane. Verified: 20 core tests green (2 PG + 18 unit), core tsc clean, lint clean (0 errors), gate green (487 + 158 + 10 + 71), census --strict exit 0.
gsxdsm
added a commit
that referenced
this pull request
Jul 30, 2026
Filed on #2718 but pointing at the guard I converted in #2709, so it is fixed where it lives. `.review` is the single `mergeOrchestration` column, so a board hosting review on a `humanReview`- or `mergeBlocker`-only lane failed this check. `TaskContextMenu` asks by ROLE, so it OFFERED "Bypass failed review" and the store refused it — the operator's only escape from a stranded failed pre-merge step returned a conflict. Both halves were individually reasonable, which is why it took a third party to see them together. Now `resolveReviewColumns` (core, #2730). THE BROAD SET IS RIGHT HERE, and that is a decision rather than a default: this guard REFUSES or PERMITS an operator action and moves nothing, so admitting every lane where review happens cannot send a card anywhere the engine disagrees with. #2750 documents the split — a caller that admits and then MOVES wants the narrow single lane. The message still names the lanes the check actually used, keeping #2709's fix: telling an operator to move to a column their board does not have is worse than refusing. Test: a `humanReview`-only lane passes the guard. Mutation: reverting to the singular `.review` fails it while the existing ten cases stay green — they all run the default board, where the two answers coincide. Lint caught the now-dead `resolveTaskLifecycleColumns` import; removed. Verified: 11 bypass tests green, core tsc clean, lint clean (0 errors), gate green (487 + 158 + 10 + 71), census --strict exit 0.
gsxdsm
added a commit
that referenced
this pull request
Jul 30, 2026
…ixed (#2722 review) I added `resolveReviewColumnsForTask` here to fix a `humanReview`-only lane resolving to nothing — and built it from `mergeBlocker` + `humanReview`, leaving `mergeOrchestration` OUT. So a renamed review lane carrying only the merge trait was excluded, and moving a task there skipped the operator's review notification. Same silent miss, pointed the other way: no error, no log, the card just arrives unannounced. Now `resolveReviewColumns` (core, #2730), which covers all three flags. Both directions go through one definition, so the pair cannot drift apart again — which is the whole reason that helper exists. THE BROAD SET IS RIGHT HERE, and #2750 makes that a decision rather than a default: this path emits a notification and moves nothing, so over-admission costs an extra notification while under-admission costs the operator their signal. A caller that admits and then MOVES wants the narrow single lane. Mutation: restoring the two-flag union fails the new merge-only case while the humanReview-only case stays green — the two halves are only distinguishable together, which is exactly how I shipped one and missed the other. Verified: 54 notification tests green, engine tsc clean, lint clean (0 errors), gate green (487 + 158 + 10 + 71), census --strict exit 0.
gsxdsm
added a commit
that referenced
this pull request
Jul 30, 2026
Filed on #2718 but pointing at the guard I converted in #2709, so it is fixed where it lives. `.review` is the single `mergeOrchestration` column, so a board hosting review on a `humanReview`- or `mergeBlocker`-only lane failed this check. `TaskContextMenu` asks by ROLE, so it OFFERED "Bypass failed review" and the store refused it — the operator's only escape from a stranded failed pre-merge step returned a conflict. Both halves were individually reasonable, which is why it took a third party to see them together. Now `resolveReviewColumns` (core, #2730). THE BROAD SET IS RIGHT HERE, and that is a decision rather than a default: this guard REFUSES or PERMITS an operator action and moves nothing, so admitting every lane where review happens cannot send a card anywhere the engine disagrees with. #2750 documents the split — a caller that admits and then MOVES wants the narrow single lane. The message still names the lanes the check actually used, keeping #2709's fix: telling an operator to move to a column their board does not have is worse than refusing. Test: a `humanReview`-only lane passes the guard. Mutation: reverting to the singular `.review` fails it while the existing ten cases stay green — they all run the default board, where the two answers coincide. Lint caught the now-dead `resolveTaskLifecycleColumns` import; removed. Verified: 11 bypass tests green, core tsc clean, lint clean (0 errors), gate green (487 + 158 + 10 + 71), census --strict exit 0.
gsxdsm
added a commit
that referenced
this pull request
Jul 30, 2026
…ixed (#2722 review) I added `resolveReviewColumnsForTask` here to fix a `humanReview`-only lane resolving to nothing — and built it from `mergeBlocker` + `humanReview`, leaving `mergeOrchestration` OUT. So a renamed review lane carrying only the merge trait was excluded, and moving a task there skipped the operator's review notification. Same silent miss, pointed the other way: no error, no log, the card just arrives unannounced. Now `resolveReviewColumns` (core, #2730), which covers all three flags. Both directions go through one definition, so the pair cannot drift apart again — which is the whole reason that helper exists. THE BROAD SET IS RIGHT HERE, and #2750 makes that a decision rather than a default: this path emits a notification and moves nothing, so over-admission costs an extra notification while under-admission costs the operator their signal. A caller that admits and then MOVES wants the narrow single lane. Mutation: restoring the two-flag union fails the new merge-only case while the humanReview-only case stays green — the two halves are only distinguishable together, which is exactly how I shipped one and missed the other. Verified: 54 notification tests green, engine tsc clean, lint clean (0 errors), gate green (487 + 158 + 10 + 71), census --strict exit 0.
gsxdsm
added a commit
that referenced
this pull request
Jul 30, 2026
…parate review findings (#2761) ## The pattern `workflowMoveMetadata` outlives a task switch, so while the modal is open its flags describe the **previous** task for a render. This component gates editability, the execution-mode replan decision, the intake affordance, the actions menu and the review tab on them. **The guard already existed** at line 961: ```ts const detailFlagsAreForThisTask = workflowMoveMetadata?.taskId === task.id; const detailColumnFlags = detailFlagsAreForThisTask ? workflowMoveMetadata?.currentColumnFlags : undefined; ``` Five call sites read **around** it. Each was found separately — #2744 (review tab), #2696 (`handleDelete` deps), and the four converted here: | line | consumer | |---|---| | 1827 | `canEdit` | | 2147 | execution-mode replan on save | | 2285 | execution-mode replan on mode change | | 3156 | `isIntakeColumn` | | 3782 | actions-menu model | **Six review rounds for one root cause.** Converting them together retires the class instead of paying a round per site — the same arithmetic as the review-lane family, which took #2730 and #2750 to end rather than eight per-file patches. Only the definition line still reads the raw value, which is the point of it. ## Not covered by a new test — stated rather than papered over is internal state populated by a fetch, not a prop, so the stale-flags scenario needs the workflow-metadata request mocked **plus** a task switch mid-render. That is a real test worth writing. It is not a line I can add honestly in passing, and I have shipped four tests today that passed with the bug fully in place — I would rather flag the gap than repeat that. This file's suite also carries **6 pre-existing failures**, confirmed identical on clean by stashing and re-running, which would muddy the signal from a new case. ## What is verified - the narrowing is mechanical and **total** — one remaining raw read, the definition; - pre-existing failure count **unchanged at 6/45** with and without this change; - 106 passed + 5 skipped across both `TaskDetailModal` suites; - dashboard `tsc` clean (`tsconfig.app.json`), lint clean (0 errors), gate green (487 + 158 + 10 + 71). No census movement — this changes which value is read, not whether a column id is compared.
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.
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:
mergeOrchestrationlane + everymergeBlocker/humanReviewlane — this functionresolveLifecycleColumns().review=columnsWithFlag(ir, "mergeOrchestration")[0]— one laneThe narrow answer is what the executor, the scheduler and
project-engineact 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.tsit 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
mergeOrchestrationtwice — 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
tscclean · 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.
Summary by CodeRabbit
Documentation
Tests