engine: finalization parked ALREADY-MERGED work as failed on a renamed board - #2964
Merged
Conversation
…enamed board The worst symptom in this family: the branch landed, and the board says the task failed. project-engine's merge-confirmed finalization spread the task's REAL column into getTaskHardMergeBlocker with no `reviewColumns`, so the identity check ran against the literal `in-review`. On a renamed board it returned `task is in 'signoff', must be in 'in-review'` and the caller parked the card `failed` with "Merge confirmed but finalization blocked". Its sibling recovery path in auto-merge-finalization.ts had ALREADY solved this, by passing the review-eligible sentinel instead of the card's own column — with the rationale recorded at that site. project-engine simply never got the same treatment. Rather than spell the sentinel a second time, it is now exported once as REVIEW_ELIGIBLE_SENTINEL_COLUMN next to the helper whose contract defines it, and both recovery paths use it. Two sites independently spelling a magic value is how one of them came to be missing it. This also answers the census, which flagged the new literal — correctly. Its guidance (mine, from #2909) is to hoist a deliberate literal into a declaration where the DELIBERATE-LITERAL marker actually attaches, rather than leaving it mid-expression where the marker is silently ignored. That is what the shared constant does. Revert measured: passing the card's own renamed column instead of the sentinel reproduces the shipped string, which the second test pins. Fusion-Task-Id: KB-SELF-HEALING-QUERIES
Contributor
|
Warning Review limit reached
Next review available in: 30 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 (7)
📝 WalkthroughWalkthroughThe core package exports ChangesRenamed-board finalization
Estimated code review effort: 2 (Simple) | ~10 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
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/task-merge.ts`:
- Around line 363-377: Update the contract comment above
REVIEW_ELIGIBLE_SENTINEL_COLUMN to begin with the required FNXC:Area-of-product
yyyy-MM-dd-hh:mm prefix, using the existing decision timestamp and an
appropriate product area; retain the DELIBERATE-LITERAL marker and the remaining
explanation unchanged.
In `@packages/engine/src/project-engine.ts`:
- Around line 3588-3602: Normalize the task status before the sentinel blocker
check used by getTaskHardMergeBlocker: in the recovery path around
REVIEW_ELIGIBLE_SENTINEL_COLUMN, clear queued in addition to merging and
merging-pr, preferably reusing the existing transient-status normalization. Add
a renamed-review-lane test covering a merge-confirmed task with status queued
and preserve the successful finalization outcome.
🪄 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: b7608417-dc10-46bf-ac99-badba6a5612a
📒 Files selected for processing (7)
.changeset/finalize-parks-merged-work-failed.mdpackages/core/src/index.gate.tspackages/core/src/index.tspackages/core/src/task-merge.tspackages/engine/src/__tests__/finalize-merge-confirmed-renamed-lane.test.tspackages/engine/src/auto-merge-finalization.tspackages/engine/src/project-engine.ts
…s drop Three findings from the #2964 review: 1. queued was not cleared before the sentinel blocker check in project-engine.ts, while auto-merge-finalization.ts cleared it. queued BLOCKS (SCHEDULER_TRANSIENT_STATUSES), so a merge-confirmed card the scheduler had queued was parked failed -- the same already-landed-work-parked-failed bug, one layer down. Fixed by extracting clearMergeConfirmedTransientStatus so the two paths cannot diverge a third time, matching how this change already de-duplicated the review-eligible sentinel. 2. The DELIBERATE-LITERAL sentinel comment lacked the FNXC prefix AGENTS.md requires. Marker detection is a substring search over ancestor comments, so the prefix does not disturb it -- verified via census --strict. 3. Not reported by either reviewer: census --strict already failed on this branch. Replacing the literal with the sentinel dropped auto-merge-finalization.ts 1 -> 0 without re-recording the baseline, leaving an allowance open for regrowth. Mutation-verified: removing queued from the set fails exactly the queued case (7 pass -> 1 fail). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Collaborator
Author
|
Both review threads addressed in 1.
|
gsxdsm
added a commit
that referenced
this pull request
Jul 31, 2026
THE GAP THAT LET THREE DEFECTS THROUGH IN ONE DAY. unwired-lane-parameter.mjs catches a parameter that reaches NO caller. It is deliberately satisfied by a mention anywhere, so PARTIAL wiring is invisible to it: #2956 getInReviewStallReason wired at 0 of 4 call sites, siblings wired #2963 both merge entry points unwired — merging IMPOSSIBLE on a renamed board #2964 merge-confirmed finalization unwired — already-landed work parked `failed` Every one was a fix that added an optional parameter without the call-site sweep that has to follow it. The guard was green throughout. A CENSUS, NOT A GUARD, and the distinction is the point. Auditing the sites this finds showed FOUR of seven were legitimately unwired: skipColumnIdentityCheck callers have proven lane identity by a stronger means, a sentinel-column caller wants the check satisfied by construction, and a dead export has no caller to wire. Failing on those is ~57% false positives — and the sibling guard's own header says why that is worse than a miss: it teaches people to disable the check. So it ratchets like the lifecycle census: 36 known sites across 20 files, may only shrink. A NEW unwired caller raises the count and fails; wiring one lowers it and re-records. Detection is AST-based (TypeScript), not regex: find exported functions accepting a lane-named argument — directly or as an options-bag member — then find call sites passing none of them. The ad-hoc regex scan I used during the audit produced false negatives on multi-line calls, which is exactly how a caller gets missed. Verified to FAIL on the recurrence: injecting one new unwired caller into self-healing.ts reports `9 unwired now, baseline allows 8` and exits 1. A ratchet that cannot fail is worse than none, so that was measured rather than assumed. Fusion-Task-Id: KB-SELF-HEALING-QUERIES
gsxdsm
added a commit
that referenced
this pull request
Jul 31, 2026
**This is the gap that let three defects reach `main` in one day.** `unwired-lane-parameter.mjs` catches a parameter that reaches **no** caller. It is deliberately satisfied by a mention *anywhere*, so **partial** wiring is invisible to it: | | | | --- | --- | | #2956 | `getInReviewStallReason` wired at **0 of 4** call sites while both siblings were wired | | #2963 | both merge entry points unwired — merging was **impossible** on a renamed board | | #2964 | merge-confirmed finalization unwired — **already-landed work parked `failed`** | Every one was a fix that added an optional parameter without the call-site sweep that has to follow it. The existing guard was green throughout, correctly by its own contract. ## A census, not a guard — and that distinction is the whole design Auditing the sites this finds showed **four of seven were legitimately unwired**: `skipColumnIdentityCheck` callers have already proven lane identity by a stronger means, a sentinel-column caller wants the identity check satisfied by construction, and a dead export has no caller to wire at all. A check that failed on those is ~57% false positives. The sibling guard's own header says why that is worse than a miss — *"it teaches people to disable the check"* — and I agree, so this does not do it. Instead it ratchets like the lifecycle census: **36 known unwired sites across 20 files**, allowed to shrink and not to grow. A new unwired caller raises the count and fails; wiring one lowers it and re-records. The recurrence — adding a caller that forgets the lane answer — is precisely what gets caught, and the legitimate sites cost one baseline line each instead of a permanently red gate. ## Detection is AST-based, deliberately It finds exported functions accepting a lane-named argument — directly *or* as an options-bag member — then finds call sites passing none of them. Not regex: the ad-hoc scan I used during the audit produced false negatives on multi-line calls, which is exactly how a caller gets missed in the first place. Using a heuristic to police a defect caused by a heuristic seemed like a poor trade. ## Verified to fail on the recurrence A ratchet that cannot fail is worse than none, so this was measured rather than assumed. Injecting one new unwired caller into `self-healing.ts`: ``` [check-lane-wiring] call sites not passing a resolved lane argument INCREASED: packages/engine/src/self-healing.ts: 9 unwired now, baseline allows 8 ``` exit 1, naming the file and the delta. ## Placement Runs as a named `check:lane-wiring` step in `pr-checks.yml` beside the lifecycle, SQL, inert-seam and FNXC ratchets — same convention, same failure ergonomics, ~1s. Note the baseline records today's state, which still includes the #2963/#2964 sites because those fixes have not merged yet. When they land the count drops and the baseline is re-recorded downward — the ratchet working as intended rather than a conflict. ## Verification `pnpm test:gate` 161 + 487 + 13 + 71; `tsc` engine clean; lint, lifecycle census `--strict`, FNXC gate, and the new check all clean.
gsxdsm
added a commit
that referenced
this pull request
Jul 31, 2026
THE FAILURE MODE WORTH DISTINGUISHING: the rest of this family went QUIET on a
renamed board. This one SHOUTED.
getInReviewStallReason satisfied its own lane check from `context.reviewColumns`,
then called getTaskMergeBlocker WITHOUT them. That helper re-ran its column-identity
check against the literal `in-review` and returned
task is in 'signoff', must be in 'in-review'
for a perfectly healthy card — which was surfaced as `{ code: "merge-blocker" }`. So
on a renamed board every in-review card was flagged as stalled, each citing a lane
the board does not have. That is how a signal stops being read at all.
SECOND SYMPTOM, found by the revert rather than by reading: on a GENUINELY failed
card the identity message wins over the real one, so the operator saw the bogus
column complaint instead of "task is marked 'failed': merge verification failed".
It did not only invent stalls; it masked the true reason for real ones.
The outer question was resolved and the inner one was not — the same
half-conversion the helper's own comment records for moves.ts, and #2963/#2964
fixed for the merge paths. Last one in the family.
Fixture note: `paused` is deliberately NOT the genuine-stall case. An earlier guard
returns undefined for a paused card before the merge blocker is consulted, so that
case would pass whether or not the lanes are forwarded.
Reverts measured: restoring the unforwarded call fails 2 of 3 — the healthy card
reports a merge-blocker stall, and the failed card reports the wrong reason.
Fusion-Task-Id: KB-SELF-HEALING-QUERIES
gsxdsm
added a commit
that referenced
this pull request
Jul 31, 2026
) **The failure mode worth distinguishing: the rest of this family went quiet on a renamed board. This one shouted.** `getInReviewStallReason` satisfied its **own** lane check from `context.reviewColumns` — then called `getTaskMergeBlocker` **without** them. That helper re-ran its column-identity check against the literal `in-review` and returned, for a perfectly healthy card: ``` task is in 'signoff', must be in 'in-review' ``` …which was surfaced as `{ code: "merge-blocker" }`. **Every in-review card on a renamed board was flagged as stalled**, each citing a lane the board does not have. That is how a signal stops being read at all. ## A second symptom, found by the revert rather than by reading On a **genuinely failed** card, the identity message wins over the real one. The operator saw the bogus column complaint instead of `task is marked 'failed': merge verification failed`. So it did not only invent stalls — it **masked the true reason for real ones**. I would not have noticed that from the diff; it showed up because the revert run asserted on the reason text. ## Same shape, last one in the family The outer question was resolved and the inner one was not — the half-conversion the helper's own comment records for `moves.ts`, and #2963/#2964 fixed for the merge entry points. This is the last site the audit turned up where the lane answer was already in scope and simply not forwarded. ## Revert results | | reverted → | | --- | --- | | the unforwarded call (what ships today) | **2 of 3 fail** — healthy card reports a merge-blocker stall; failed card reports the wrong reason | **Fixture note worth keeping:** `paused` is deliberately *not* the genuine-stall case. An earlier guard returns `undefined` for a paused card before the merge blocker is ever consulted, so that case would pass whether or not the lanes are forwarded — the vacuous shape this series has produced eight times. ## Verification `pnpm test:gate` 161 + 487 + 13 + 71; `@fusion/core` full suite **4878 passed** (457 files); `tsc` core clean; lint, lifecycle census `--strict`, FNXC gate, changesets all clean.
gsxdsm
added a commit
that referenced
this pull request
Jul 31, 2026
…er was asked about `in-review` (#2976) ## PR merges silently never ran on a renamed board `processPullRequestMergeTask` called its injected blocker with the task alone: ```ts if (getTaskMergeBlocker(task)) return "skipped"; ``` So `options.reviewColumns` was undefined and the blocker's identity check fell back to `task.column === "in-review"`. On a board whose merge lane is named anything else it returns: ``` task is in 'checking', must be in 'in-review' ``` …which is truthy, so this function returns `"skipped"`. **Silently and permanently** — nothing logs, nothing fails, the PR simply never merges. `daemon.ts`, `serve.ts` and `dashboard.ts` all drain PR merges through here, making this a third instance of the #2963/#2964 class ("merge entry points unwired — merging was impossible on a renamed board"). Found via the baseline #2966 shipped: `packages/cli/src/commands/task-lifecycle.ts` was a known-unwired call site in it. ## Narrow resolution, deliberately `resolveReviewColumns` is the **broad** set, and its own FNXC note warns that a caller which admits on it *and then moves the card* will act on cards the engine does not consider in review. This function merges and moves to the complete lane — a state-changing admission — so it uses `resolveMergeOrchestrationColumn`, the single lane the engine acts on. That matches how `moves.ts` wires the same call. Degradation is unchanged in both directions: `resolveWorkflowIrForTask` substitutes the default IR rather than throwing, so a default board resolves `in-review` and behaves identically; a v1-upgraded IR resolves every role empty and keeps the documented legacy literal (covered by a test). ## One shape choice worth flagging The option is always **passed** and conditionally **valued**: ```ts getTaskMergeBlocker(task, { reviewColumns: mergeLane ? new Set([mergeLane]) : undefined }) ``` rather than making the whole argument conditional. These are identical at runtime — the blocker treats an undefined `reviewColumns` exactly as it treats absent options — but **only this shape is visible to `lane-wiring-census.mjs`**, which matches an object-literal argument and cannot see a ternary. I wrote the ternary first, and the gate still reported the site as unwired; wiring a gate cannot check is how this defect survived in the first place. The gate then confirmed the fix and asked for the baseline in the same commit: ``` [check-lane-wiring] unwired call sites decreased: packages/cli/src/commands/task-lifecycle.ts: 1 -> 0 ``` Baseline re-recorded 9 → 8 in this commit, so the allowance cannot be regrown into. ## Revert proof **There was no test for this function at all** — that is why it went unnoticed. Restoring only `task-lifecycle.ts`: ``` AssertionError: expected "vi.fn()" to be called with arguments: [ ObjectContaining{…}, …(1) ] AssertionError: expected 'skipped' not to be 'skipped' AssertionError: expected "vi.fn()" to be called with arguments: [ ObjectContaining{…}, undefined ] Tests 3 failed | 1 passed (4) ``` The one case that passes both ways is "still skips a card that is not in any merge lane" — it guards against over-admission rather than proving the fix, and I am not claiming it as coverage of the defect. ## Verification (measured) - new suite **4/4**; with `pr-automerge-cleanup` **9 passed / 2 files** - `tsc --noEmit`, `eslint` — clean - `check-lane-wiring` (8, none added), `lifecycle-column-census --strict`, `check-sql-column-literals`, `check-fnxc-future-dates` — green **Changeset added** (`patch`). `packages/cli` is the published `@runfusion/fusion` and this changes user-facing merge behaviour, so AGENTS.md requires one. My first pass hedged and left it to a maintainer — that was wrong, the rule is not discretionary, and it is now in the branch.
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.
The worst symptom in this family: the branch landed, and the board says the task failed.
project-engine's merge-confirmed finalization spread the task's real column intogetTaskHardMergeBlockerwith noreviewColumns, so the identity check ran against the literalin-review. On a renamed board it returnedtask is in 'signoff', must be in 'in-review', and the caller parked the card:For work that had already merged.
Its sibling had already solved this
auto-merge-finalization.tspasses the review-eligible sentinel instead of the card's own column, with the reasoning recorded at that site:getTaskHardMergeBlockerasks "is this card blocked by anything other than where it sits?", and its callers are recovery paths for landed work that a graph crash can leave resting in any column.project-enginesimply never got the same treatment.One name instead of two spellings
Rather than write the sentinel a second time, it is exported once as
REVIEW_ELIGIBLE_SENTINEL_COLUMNnext to the helper whose contract gives it meaning, and both recovery paths use it. Two sites independently spelling a magic value is how one of them came to be missing it — that is the actual root cause here, not the literal itself.This also answers the census, which flagged the new literal — correctly. Its guidance (which I wrote, in #2909) is to hoist a deliberate literal into a declaration, where a
DELIBERATE-LITERALmarker actually attaches, instead of leaving it mid-expression where the marker is silently ignored. The shared constant is exactly that, and it lowersauto-merge-finalization's literal count too.Revert result
The middle test asserts that string deliberately — it is what landed in
task.error, so a regression reports what the operator would actually have seen. A third case checks the sentinel does not suppress genuine blockers: incomplete steps still block finalization in any lane.These drive the helper directly; reaching
project-engine's finalization end to end needs a live engine, a merge run and a real repo, while the defect is entirely in what the blocker is asked.Verification
pnpm test:gate161 + 487 + 13 + 71;project-engine+auto-merge-finalization+ the new suite, 207;tscclean on core and engine; lint, census--strict, FNXC gate, changesets all clean.Summary by CodeRabbit
Bug Fixes
Tests