-
Notifications
You must be signed in to change notification settings - Fork 108
fix(engine): defer validator fail when the judged workspace predates the merged code #1929
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -36,6 +36,17 @@ import { createLogger } from "./logger.js"; | |
| import { createFallbackModelObserver } from "./fallback-model-observer.js"; | ||
| import { resolveMcpServersForStore } from "./mcp-resolution.js"; | ||
| import { createRunAuditor, generateSyntheticRunId } from "./run-audit.js"; | ||
| import { exec } from "node:child_process"; | ||
| import { promisify } from "node:util"; | ||
|
|
||
| const execAsync = promisify(exec); | ||
|
|
||
| /** Shell-quote a single argument for a `git` invocation (mirror of the local | ||
| * helper in branch-conflicts.ts — kept local rather than shared per repo | ||
| * convention). */ | ||
| function quoteShellArg(value: string): string { | ||
| return `'${value.replace(/'/g, `'\\''`)}'`; | ||
| } | ||
|
|
||
| /** Logger for the mission execution loop subsystem. */ | ||
| export const loopLog = createLogger("mission-loop"); | ||
|
|
@@ -512,6 +523,18 @@ export class MissionExecutionLoop extends EventEmitter { | |
| run.id, | ||
| `linked task ${feature.taskId} is still "${premergeColumn}" (code not merged yet) — validation deferred`, | ||
| ); | ||
| } else if (await this.isValidationWorkspaceStale(feature)) { | ||
| // Even when the task column shows "done", the judge session ran in | ||
| // this.rootDir — if that working copy never fetched/reset to the | ||
| // merged commit (merge landed on remote or another worktree), the | ||
| // validator read PRE-merge files → a spurious fail. Defer instead of | ||
| // minting a bogus Fix Feature; a later validation judges the merged | ||
| // code. Only affirmative staleness evidence defers (fail-open). | ||
| await this.handleValidationInconclusive( | ||
| feature.id, | ||
| run.id, | ||
| `validation workspace predates the merged code for ${feature.taskId} — validation deferred`, | ||
| ); | ||
| } else { | ||
| await this.handleValidationFail(feature.id, run.id, result); | ||
| } | ||
|
|
@@ -547,6 +570,30 @@ export class MissionExecutionLoop extends EventEmitter { | |
| return column; | ||
| } | ||
|
|
||
| /** | ||
| * Affirmative-evidence check that the judged checkout (this.rootDir HEAD) | ||
| * predates the linked task's merged code. True ONLY when the integration SHA | ||
| * is resolvable AND is NOT an ancestor of rootDir HEAD. Fails open (returns | ||
| * false = trust the fail) on unresolvable SHA / unknown object / any git | ||
| * error — the guard may only ever defer a fail, never suppress one. | ||
| */ | ||
| private async isValidationWorkspaceStale(feature: MissionFeature): Promise<boolean> { | ||
| const integrationSha = await this.resolveIntegrationSha(feature); | ||
| if (!integrationSha) return false; // no evidence → trust the fail | ||
|
Comment on lines
+581
to
+582
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When a done task only carries the modeled task SHA field, |
||
| try { | ||
| await execAsync(`git merge-base --is-ancestor ${quoteShellArg(integrationSha)} HEAD`, { | ||
| cwd: this.rootDir, | ||
| timeout: 30_000, | ||
| }); | ||
| return false; // exit 0 → ancestor → workspace is fresh | ||
| } catch (err) { | ||
| // `--is-ancestor` exits 1 = NOT an ancestor (affirmatively stale); exit | ||
| // 128 = bad object / not a repo (unknown → fail-open). execAsync's thrown | ||
| // error carries `.code` = process exit code. ONLY code === 1 defers. | ||
| return (err as { code?: number })?.code === 1; | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Run the validation AI session for a feature. | ||
| * | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
git init --initial-branch -b flag introduced version💡 Result:
The --initial-branch and -b flags for the git init command were introduced in Git version 2.28.0 [1][2][3]. These options allow users to specify the name of the initial branch in a newly created repository [4][5]. If these flags are not used, Git defaults to the name defined by the init.defaultBranch configuration variable, or to master if that variable is not set [5][6].
Citations:
🏁 Script executed:
Repository: Runfusion/Fusion
Length of output: 50372
🏁 Script executed:
Repository: Runfusion/Fusion
Length of output: 13456
🏁 Script executed:
Repository: Runfusion/Fusion
Length of output: 5149
🏁 Script executed:
Repository: Runfusion/Fusion
Length of output: 6239
Avoid
git init -b mainwithout a Git-version gatehasGitonly checks that git is installed, so these fixtures still run on Git < 2.28 andgit init -b mainwill fail. Gate on the git version or initialize normally and rename the branch afterward.♻️ Version-agnostic branch setup
📝 Committable suggestion
🤖 Prompt for AI Agents