fix(engine): defer validator fail to inconclusive while the linked task is unmerged#1917
Conversation
…sk is unmerged A "fail" verdict from runFeatureValidation is only trustworthy once the linked task's code has actually landed (column done/archived). When the task is still mid-pipeline — an in-review PR, an external merge train, a deferred base sync — the validator judged a checkout that predates the merge and reports the work as missing, minting a duplicate Fix feature (and board task) for code that is about to land. Route that case to handleValidationInconclusive (R21: no Fix feature, run completed as blocked, distinguishable verification_inconclusive event) so a later validation judges the merged code instead. The guard fails open: a missing/unlinked task, an unreadable task store, or an unknown column all keep the existing handleValidationFail path — it can only ever defer a fail on affirmative evidence of an unmerged column, never suppress one on missing data. The vanilla done-triggered flow (scheduler fires processTaskOutcome on toColumn === "done") is unaffected; the guard matters for recovery-path validations and for deployments whose tasks merge through external pipelines. Incident context: a mission validator racing an external merge train failed four features while their tasks were in-review, and the four generated Fix tasks' file-scope leases on hot shared files serialized the entire board. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
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)
📝 WalkthroughWalkthroughThis change modifies ChangesPremerge Guard for Validation Fail Verdict
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 changes validator failure handling when linked task code may not be merged yet. The main changes are:
Confidence Score: 5/5This looks safe to merge.
Important Files Changed
Reviews (2): Last reviewed commit: "Merge branch 'main' into fix/validator-p..." | Re-trigger Greptile |
…the merged code The mission validator runs its read-only judge session with cwd: this.rootDir — the engine's main working copy. When a task's merge landed on the remote or in another worktree and rootDir was never fetched/reset to it, the judge reads PRE-merge files and returns a spurious `fail`. #1917's premerge column guard doesn't catch this: the task column is already `done`, so it falls through to handleValidationFail and mints a bogus Fix Feature. Add a symmetric second guard in the fail branch, after the premerge column check: isValidationWorkspaceStale resolves the task's integration SHA and runs `git merge-base --is-ancestor <sha> HEAD` in rootDir. Only affirmative staleness evidence (exit 1 = NOT an ancestor) defers the fail to inconclusive; exit 0 (ancestor/fresh), a missing SHA, or a bad object (exit 128) all trust the fail. Fail-open: a guard may DEFER a fail, never SUPPRESS one on missing or unreadable data. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…the merged code (#1929) ## Problem The mission validator runs its read-only judge session with `cwd: this.rootDir` — the engine's **main working copy**. When a task's merge landed on the remote (or in another worktree) and `rootDir` was never fetched/reset to that commit, the judge reads **pre-merge files** and returns a spurious `fail`. #1917's premerge column guard does not catch this case: by the time validation runs the task column is already `done`, so execution falls through to `handleValidationFail` and mints a **bogus Fix Feature** for code that is actually correct and merged. ## Fix A symmetric second guard in the `fail` branch, placed **after** the #1917 premerge column check: - `isValidationWorkspaceStale(feature)` resolves the task's integration SHA and runs `git merge-base --is-ancestor <sha> HEAD` in `rootDir`. - **Only affirmative staleness evidence defers.** `--is-ancestor` exit `1` (the SHA is *not* an ancestor of HEAD → the workspace predates the merge) → defer the fail to **inconclusive**, so a later validation judges the merged code. - Every other outcome trusts the fail: exit `0` (ancestor → workspace is fresh), no integration SHA available, or a bad/unknown object (exit `128`). Fail-open doctrine, matching #1917: a guard may only ever **defer** a fail, never **suppress** one on missing or unreadable data. ## Tests Four real-git cases in `mission-execution-loop.test.ts` (skipped when `git` is unavailable): 1. Judged checkout predates the merged commit → fail deferred to inconclusive, no Fix Feature minted, emits `validation:inconclusive`. 2. Merged commit is an ancestor of HEAD (fresh workspace) → normal fail path, Fix Feature minted, emits `validation:failed`. 3. Task carries no integration SHA → fail open (normal fail). 4. Integration SHA is an unknown object (exit 128) → fail open (normal fail). Verified with stash-red/restore-green discipline: with the production guard stashed, case (1) goes red while the three fail-open guardrails stay green — proving case (1) exercises the fix. Full file: 62 passed. `tsc --noEmit`: 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** * Validation failures are now deferred when the workspace appears to be out of date with merged changes, reducing incorrect failure reports. * Tasks with linked work continue to use the usual failure path when the current workspace is up to date. * Staleness checks now avoid masking real validation failures when no merge reference is available or when the reference can’t be verified. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
Problem
MissionExecutionLoop.runFeatureValidationtreats a validator "fail" verdict as authoritative regardless of whether the linked task's code has actually landed. When validation fires while the task is still mid-pipeline — an in-review PR, an external merge train, a deferred base sync — the validator judges a checkout that predates the merge, concludes the feature "is not present", andhandleValidationFailmints a Fix feature for work that is already done.We hit this in production (2026-07-05): a recovery-path validation ran against four features whose implementing tasks were in-review in an external merge pipeline. All four "failed" → four duplicate Fix tasks were created one minute after the real work merged. Worse, the Fix tasks' planned file scopes included hot shared files, so their file-scope leases serialized the entire board until they were manually archived.
Fix
Before dispatching a
failverdict, resolve the linked task's column. If it affirmatively shows the task has not completed (any column other thandone/archived), route the outcome tohandleValidationInconclusive(R21 — completes the run asblocked, logsverification_inconclusive, notifies autopilot, spawns no Fix feature) with a "code not merged yet — validation deferred" reason. A later validation (post-merge recovery pass) judges the real merged code.Fails open by design — the guard may only ever defer a fail, never suppress one on missing data. Missing
taskId, missing task, unreadable store, or unknown column all fall through to the normalhandleValidationFailpath:The vanilla flow is unaffected: the scheduler triggers validation on
toColumn === "done", so by the time a normally-triggered validation runs the task is alreadydoneand the guard is a no-op. Only recovery-path / re-validation runs that race an unmerged task are deferred.Tests
Three new tests in
mission-execution-loop.test.ts(premerge guarddescribe):in-review→ routes to inconclusive: no Fix feature, run completed asblocked,validation:inconclusiveemitted (notvalidation:failed),verification_inconclusivemission event loggeddone→ normal fail path: Fix feature created,validation:failedemittedtaskStore.getTaskrejects → fails open to the normal fail pathnpx vitest run src/__tests__/mission-execution-loop.test.ts: 58/58 green.npx tsc --noEmit: clean. Full engine suite: the 46 failures across 24 files present on my branch fail identically on clean17c4007(verified by re-running the same files on a detached checkout of upstream main) — all pre-existing/environment-dependent, none related to this change.Summary by CodeRabbit