fix(engine): defer validator fail to inconclusive while the linked task is unmerged (#1917)
## Problem
`MissionExecutionLoop.runFeatureValidation` treats 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", and `handleValidationFail` mints
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 `fail` verdict, resolve the linked task's column.
If it affirmatively shows the task has **not** completed (any column
other than `done`/`archived`), route the outcome to
`handleValidationInconclusive` (R21 — completes the run as `blocked`,
logs `verification_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 normal
`handleValidationFail` path:
```ts
private async getPremergeTaskColumn(taskId: string | undefined): Promise<string | null> {
if (!taskId) return null;
const linkedTask = await this.taskStore.getTask(taskId).catch(() => null);
const column = linkedTask?.column;
if (!column || column === "done" || column === "archived") return null;
return column;
}
```
The vanilla flow is unaffected: the scheduler triggers validation on
`toColumn === "done"`, so by the time a normally-triggered validation
runs the task is already `done` and 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 guard`
describe):
1. fail verdict + linked task `in-review` → routes to inconclusive: no
Fix feature, run completed as `blocked`, `validation:inconclusive`
emitted (not `validation:failed`), `verification_inconclusive` mission
event logged
2. fail verdict + linked task `done` → normal fail path: Fix feature
created, `validation:failed` emitted
3. fail verdict + `taskStore.getTask` rejects → fails open to the normal
fail path
`npx 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 clean
`17c4007`** (verified by re-running the same files on a detached
checkout of upstream main) — all pre-existing/environment-dependent,
none related to this change.
<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit
* **Bug Fixes**
* Validation failures now account for whether the linked task is
actually merged. If the task is still in progress, the result is marked
as inconclusive instead of creating a fix flow.
* Added clearer handling when task details can’t be read, so normal
failure behavior still applies.
* Improved validation status reporting and event logging for merged vs.
unmerged task states.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->
This commit is contained in:
@@ -1878,6 +1878,142 @@ describe("MissionExecutionLoop", () => {
|
||||
});
|
||||
});
|
||||
|
||||
// ── premerge guard (fail verdict while the linked task is unmerged) ──────
|
||||
|
||||
describe("premerge guard", () => {
|
||||
function primeFailVerdict() {
|
||||
const failResponse = JSON.stringify({
|
||||
status: "fail",
|
||||
assertions: [{ assertionId: "CA-1", passed: false, message: "Failed", expected: "ok", actual: "not ok" }],
|
||||
summary: "Assertion failed",
|
||||
});
|
||||
mockSessionHolder.session.state.messages = [
|
||||
{ role: "user", content: "Validate this" },
|
||||
{ role: "assistant", content: failResponse },
|
||||
];
|
||||
}
|
||||
|
||||
function primeFeature() {
|
||||
const feature = createMockFeature({
|
||||
loopState: "implementing",
|
||||
taskId: "FN-001",
|
||||
id: "F-001",
|
||||
implementationAttemptCount: 1,
|
||||
});
|
||||
missionStore._setFeature(feature);
|
||||
missionStore.getFeatureByTaskId = vi.fn().mockReturnValue(feature);
|
||||
missionStore.listAssertionsForFeature = vi.fn().mockReturnValue(makeAssertions(1));
|
||||
return feature;
|
||||
}
|
||||
|
||||
it("should defer a fail to inconclusive while the linked task is not done/archived", async () => {
|
||||
primeFeature();
|
||||
primeFailVerdict();
|
||||
|
||||
// The linked task is still in review — its code has not merged yet, so
|
||||
// the validator judged a checkout that predates the work.
|
||||
taskStore._setTask({ id: "FN-001", title: "Test", description: "Implementation", log: [], column: "in-review" });
|
||||
|
||||
loop = new MissionExecutionLoop({
|
||||
taskStore: taskStore as any,
|
||||
missionStore: missionStore as any,
|
||||
rootDir: "/tmp",
|
||||
});
|
||||
const emitSpy = vi.spyOn(loop, "emit");
|
||||
loop.start();
|
||||
|
||||
await loop.processTaskOutcome("FN-001");
|
||||
|
||||
// Routed to the inconclusive outcome — no Fix Feature minted (R21)
|
||||
expect(missionStore.createGeneratedFixFeature).not.toHaveBeenCalled();
|
||||
expect(missionStore.completeValidatorRun).toHaveBeenCalledWith(
|
||||
expect.any(String),
|
||||
"blocked",
|
||||
expect.stringContaining("code not merged yet"),
|
||||
);
|
||||
expect(emitSpy).toHaveBeenCalledWith(
|
||||
"validation:inconclusive",
|
||||
expect.objectContaining({
|
||||
featureId: "F-001",
|
||||
reason: expect.stringContaining('"in-review"'),
|
||||
}),
|
||||
);
|
||||
expect(emitSpy).not.toHaveBeenCalledWith("validation:failed", expect.anything());
|
||||
|
||||
// The infra-failure marker keeps deferred fails separable from real ones
|
||||
expect(missionStore.logMissionEvent).toHaveBeenCalledWith(
|
||||
expect.any(String),
|
||||
"warning",
|
||||
expect.stringContaining("Verification inconclusive"),
|
||||
expect.objectContaining({
|
||||
code: "verification_inconclusive",
|
||||
outcome: "inconclusive",
|
||||
infraFailure: true,
|
||||
}),
|
||||
);
|
||||
});
|
||||
|
||||
it("should run the normal fail path once the linked task is done", async () => {
|
||||
primeFeature();
|
||||
primeFailVerdict();
|
||||
|
||||
taskStore._setTask({ id: "FN-001", title: "Test", description: "Implementation", log: [], column: "done" });
|
||||
|
||||
loop = new MissionExecutionLoop({
|
||||
taskStore: taskStore as any,
|
||||
missionStore: missionStore as any,
|
||||
rootDir: "/tmp",
|
||||
});
|
||||
const emitSpy = vi.spyOn(loop, "emit");
|
||||
loop.start();
|
||||
|
||||
await loop.processTaskOutcome("FN-001");
|
||||
|
||||
expect(missionStore.createGeneratedFixFeature).toHaveBeenCalledWith(
|
||||
"F-001",
|
||||
expect.any(String),
|
||||
expect.arrayContaining(["CA-1"]),
|
||||
expect.any(String),
|
||||
);
|
||||
expect(emitSpy).toHaveBeenCalledWith(
|
||||
"validation:failed",
|
||||
expect.objectContaining({ featureId: "F-001" }),
|
||||
);
|
||||
expect(emitSpy).not.toHaveBeenCalledWith("validation:inconclusive", expect.anything());
|
||||
});
|
||||
|
||||
it("should fail open (normal fail path) when the linked task cannot be read", async () => {
|
||||
primeFeature();
|
||||
|
||||
loop = new MissionExecutionLoop({
|
||||
taskStore: taskStore as any,
|
||||
missionStore: missionStore as any,
|
||||
rootDir: "/tmp",
|
||||
});
|
||||
// Bypass the AI session (runValidation also reads the task, without a
|
||||
// catch) so the rejecting getTask below only exercises the guard.
|
||||
vi.spyOn(loop as any, "runValidation").mockResolvedValue({
|
||||
status: "fail",
|
||||
assertions: [{ assertionId: "CA-1", passed: false, message: "Failed", expected: "ok", actual: "not ok" }],
|
||||
summary: "Assertion failed",
|
||||
});
|
||||
taskStore.getTask = vi.fn().mockRejectedValue(new Error("store unavailable"));
|
||||
|
||||
const emitSpy = vi.spyOn(loop, "emit");
|
||||
loop.start();
|
||||
|
||||
await loop.processTaskOutcome("FN-001");
|
||||
|
||||
// Unknown task state must never suppress a fail — only defer on
|
||||
// affirmative evidence of an unmerged column.
|
||||
expect(missionStore.createGeneratedFixFeature).toHaveBeenCalled();
|
||||
expect(emitSpy).toHaveBeenCalledWith(
|
||||
"validation:failed",
|
||||
expect.objectContaining({ featureId: "F-001" }),
|
||||
);
|
||||
});
|
||||
});
|
||||
|
||||
// ── handleValidationBlocked ───────────────────────────────────────────────
|
||||
|
||||
describe("handleValidationBlocked", () => {
|
||||
|
||||
@@ -498,7 +498,23 @@ export class MissionExecutionLoop extends EventEmitter {
|
||||
if (result.status === "pass") {
|
||||
await this.handleValidationPass(feature.id, run.id, result.summary);
|
||||
} else if (result.status === "fail") {
|
||||
await this.handleValidationFail(feature.id, run.id, result);
|
||||
// A "fail" verdict is only trustworthy once the linked task's code has
|
||||
// actually landed (done/archived). If the task is still mid-pipeline
|
||||
// (in-review PR, external merge train, deferred base sync), the
|
||||
// validator judged a checkout that predates the merge — route to the
|
||||
// inconclusive outcome (R21, no Fix Feature) and let a later validation
|
||||
// judge the merged code. Missing task / unknown column falls through to
|
||||
// the normal fail handling (defer only on affirmative evidence).
|
||||
const premergeColumn = await this.getPremergeTaskColumn(feature.taskId);
|
||||
if (premergeColumn) {
|
||||
await this.handleValidationInconclusive(
|
||||
feature.id,
|
||||
run.id,
|
||||
`linked task ${feature.taskId} is still "${premergeColumn}" (code not merged yet) — validation deferred`,
|
||||
);
|
||||
} else {
|
||||
await this.handleValidationFail(feature.id, run.id, result);
|
||||
}
|
||||
} else if (result.status === "inconclusive") {
|
||||
// R21 — "verification could not run" is distinct from "behavior observed
|
||||
// wrong". An infra-driven inconclusive (no isolating backend, timeout,
|
||||
@@ -516,6 +532,21 @@ export class MissionExecutionLoop extends EventEmitter {
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* Resolve the linked task's column when it affirmatively shows the task has
|
||||
* NOT completed yet (any column other than "done"/"archived"). Returns null
|
||||
* when the task is completed, missing, unlinked, or unreadable — i.e. every
|
||||
* case where a fail verdict should be trusted. Fails open on purpose: the
|
||||
* guard may only ever defer a fail, never suppress one on missing data.
|
||||
*/
|
||||
private async getPremergeTaskColumn(taskId: string | undefined): Promise<string | null> {
|
||||
if (!taskId) return null;
|
||||
const linkedTask = await this.taskStore.getTask(taskId).catch(() => null);
|
||||
const column = linkedTask?.column;
|
||||
if (!column || column === "done" || column === "archived") return null;
|
||||
return column;
|
||||
}
|
||||
|
||||
/**
|
||||
* Run the validation AI session for a feature.
|
||||
*
|
||||
|
||||
Reference in New Issue
Block a user