fix(engine): defer validator fail when the judged workspace predates the merged code#1929
Open
flexi767 wants to merge 1 commit into
Open
fix(engine): defer validator fail when the judged workspace predates the merged code#1929flexi767 wants to merge 1 commit into
flexi767 wants to merge 1 commit into
Conversation
…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`. Runfusion#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>
Contributor
📝 WalkthroughWalkthroughAdds a git-ancestry-based staleness check to the validation fail path in mission-execution-loop.ts, deferring validation to an inconclusive outcome when the workspace HEAD predates a task's merged integration commit. Adds corresponding tests using real git fixtures. ChangesStale-workspace premerge guard
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant MissionLoop as mission-execution-loop
participant Git as git CLI
participant Handler as Validation Handler
MissionLoop->>MissionLoop: processTaskOutcome receives fail verdict
MissionLoop->>MissionLoop: getPremergeTaskColumn check
MissionLoop->>MissionLoop: isValidationWorkspaceStale(rootDir, integrationSha)
MissionLoop->>Git: git merge-base --is-ancestor integrationSha HEAD
Git-->>MissionLoop: exit code
alt exit code 1 (workspace stale)
MissionLoop->>Handler: defer to inconclusive path
else ancestor confirmed or unresolvable
MissionLoop->>Handler: proceed with normal fail path
end
Possibly related PRs
🚥 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 |
Contributor
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+581
to
+582
| const integrationSha = await this.resolveIntegrationSha(feature); | ||
| if (!integrationSha) return false; // no evidence → trust the fail |
Contributor
There was a problem hiding this comment.
When a done task only carries the modeled task SHA field, resolveIntegrationSha returns no SHA because it looks for duck-typed integrationSha or baseCommit fields. That makes this new guard return false, so the stale-workspace fail still enters handleValidationFail and mints the bogus Fix Feature this change is meant to defer.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/engine/src/__tests__/mission-execution-loop.test.ts`:
- Around line 486-496: The test fixture setup in initGitRepo uses git init -b
main without checking the installed Git version, so it can fail on older Git
releases. Update the repo initialization in initGitRepo to be version-agnostic
by either gating the main-branch initialization on a Git version check or
initializing the repo normally and renaming the branch afterward, while keeping
the rest of the fixture setup unchanged.
🪄 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: b9f12b0b-95a4-478b-bf7f-2d8b3b23e292
📒 Files selected for processing (2)
packages/engine/src/__tests__/mission-execution-loop.test.tspackages/engine/src/mission-execution-loop.ts
Comment on lines
+486
to
+496
| /** A throwaway git repo with one initial commit; HEAD on `main`. */ | ||
| function initGitRepo(): string { | ||
| const repo = mkdtempSync(join(tmpdir(), "fn-stale-ws-")); | ||
| git(repo, "git init -b main"); | ||
| git(repo, 'git config user.email "test@example.com"'); | ||
| git(repo, 'git config user.name "Test User"'); | ||
| git(repo, "git config commit.gpgsign false"); | ||
| writeFileSync(join(repo, "foo.ts"), "line1\n"); | ||
| git(repo, "git add foo.ts && git commit -m init"); | ||
| return repo; | ||
| } |
Contributor
There was a problem hiding this comment.
📐 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:
- 1: https://stackoverflow.com/questions/42871542/how-can-i-create-a-git-repository-with-the-default-branch-name-other-than-maste
- 2: https://stackoverflow.com/questions/64787301/git-init-b-branch-name-command-in-terminal-is-throwing-an-unknown-switch
- 3: Fix git init -b being unavailable for older versions composer/composer#9387
- 4: git/git@32ba12d
- 5: https://git-scm.com/docs/git-init.html
- 6: https://github.com/git/git/blob/75555676ad3908b0f847a9ae154c35e12114c82f/Documentation/git-init.txt
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the test file around the reported lines.
sed -n '440,530p' packages/engine/src/__tests__/mission-execution-loop.test.ts
# Find the helper used to gate git-dependent tests.
rg -n "hasGit|git init -b main|initial-branch|branch -M main" packages/engine/src/__tests__ packages/engine/src -S
# Locate the hasGit implementation.
fd -a "hasGit" packages/engine/src packages/engine/src/__tests__Repository: Runfusion/Fusion
Length of output: 50372
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Search for any explicit Git version checks in the test suite or helpers.
rg -n "git version|--version|2\.28|initialBranch|init\.defaultBranch" packages/engine/src -S
# If there's a shared test utility, inspect it for Git capability checks.
rg -n "function hasGit|const hasGit|export .*hasGit" . -SRepository: Runfusion/Fusion
Length of output: 13456
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find every call site of initGitRepo in the file.
rg -n "initGitRepo\(" packages/engine/src/__tests__/mission-execution-loop.test.ts -n -S
# Read the surrounding test blocks for those call sites.
python3 - <<'PY'
from pathlib import Path
p = Path("packages/engine/src/__tests__/mission-execution-loop.test.ts")
lines = p.read_text().splitlines()
targets = []
for i,l in enumerate(lines, start=1):
if "initGitRepo(" in l:
targets.append(i)
for t in targets:
start = max(1, t-25)
end = min(len(lines), t+35)
print(f"\n--- lines {start}-{end} around {t} ---")
for n in range(start, end+1):
print(f"{n:4d}: {lines[n-1]}")
PYRepository: Runfusion/Fusion
Length of output: 5149
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the skip gating around the real-git tests that use initGitRepo.
sed -n '2030,2185p' packages/engine/src/__tests__/mission-execution-loop.test.tsRepository: Runfusion/Fusion
Length of output: 6239
Avoid git init -b main without a Git-version gate
hasGit only checks that git is installed, so these fixtures still run on Git < 2.28 and git init -b main will fail. Gate on the git version or initialize normally and rename the branch afterward.
♻️ Version-agnostic branch setup
- git(repo, "git init -b main");
+ git(repo, "git init");
git(repo, 'git config user.email "test@example.com"');
git(repo, 'git config user.name "Test User"');
git(repo, "git config commit.gpgsign false");
writeFileSync(join(repo, "foo.ts"), "line1\n");
git(repo, "git add foo.ts && git commit -m init");
+ git(repo, "git branch -M main");📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| /** A throwaway git repo with one initial commit; HEAD on `main`. */ | |
| function initGitRepo(): string { | |
| const repo = mkdtempSync(join(tmpdir(), "fn-stale-ws-")); | |
| git(repo, "git init -b main"); | |
| git(repo, 'git config user.email "test@example.com"'); | |
| git(repo, 'git config user.name "Test User"'); | |
| git(repo, "git config commit.gpgsign false"); | |
| writeFileSync(join(repo, "foo.ts"), "line1\n"); | |
| git(repo, "git add foo.ts && git commit -m init"); | |
| return repo; | |
| } | |
| /** A throwaway git repo with one initial commit; HEAD on `main`. */ | |
| function initGitRepo(): string { | |
| const repo = mkdtempSync(join(tmpdir(), "fn-stale-ws-")); | |
| git(repo, "git init"); | |
| git(repo, 'git config user.email "test@example.com"'); | |
| git(repo, 'git config user.name "Test User"'); | |
| git(repo, "git config commit.gpgsign false"); | |
| writeFileSync(join(repo, "foo.ts"), "line1\n"); | |
| git(repo, "git add foo.ts && git commit -m init"); | |
| git(repo, "git branch -M main"); | |
| return repo; | |
| } |
🤖 Prompt for 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.
In `@packages/engine/src/__tests__/mission-execution-loop.test.ts` around lines
486 - 496, The test fixture setup in initGitRepo uses git init -b main without
checking the installed Git version, so it can fail on older Git releases. Update
the repo initialization in initGitRepo to be version-agnostic by either gating
the main-branch initialization on a Git version check or initializing the repo
normally and renaming the branch afterward, while keeping the rest of the
fixture setup unchanged.
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
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.
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) androotDirwas never fetched/reset to that commit, the judge reads pre-merge files and returns a spuriousfail.#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 tohandleValidationFailand mints a bogus Fix Feature for code that is actually correct and merged.Fix
A symmetric second guard in the
failbranch, placed after the #1917 premerge column check:isValidationWorkspaceStale(feature)resolves the task's integration SHA and runsgit merge-base --is-ancestor <sha> HEADinrootDir.--is-ancestorexit1(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.0(ancestor → workspace is fresh), no integration SHA available, or a bad/unknown object (exit128).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 whengitis unavailable):validation:inconclusive.validation:failed.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
Summary by CodeRabbit