-
Notifications
You must be signed in to change notification settings - Fork 108
fix(engine): self-heal failed in-review cards whose PR merged on the remote #1922
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
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 | ||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -1970,6 +1970,39 @@ export class SelfHealingManager { | |||||||||||||||||||||||||||||||||||||
| return findAlreadyMergedTaskCommit(input); | ||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| /** | ||||||||||||||||||||||||||||||||||||||
| * Best-effort refresh of the remote-tracking base ref so the already-merged | ||||||||||||||||||||||||||||||||||||||
| * evidence detector can see a squash that landed on the remote after this | ||||||||||||||||||||||||||||||||||||||
| * process last fetched. Returns the `origin/<base>` ref to re-run the detector | ||||||||||||||||||||||||||||||||||||||
| * against, or null when there is nothing fresher to prove against. | ||||||||||||||||||||||||||||||||||||||
| * | ||||||||||||||||||||||||||||||||||||||
| * Fail-closed: a fetch error (offline / auth / no remote) is swallowed and we | ||||||||||||||||||||||||||||||||||||||
| * still attempt to resolve the (possibly stale) remote-tracking ref; if even | ||||||||||||||||||||||||||||||||||||||
| * that is absent we return null and the caller leaves the card untouched. | ||||||||||||||||||||||||||||||||||||||
| */ | ||||||||||||||||||||||||||||||||||||||
| private async refreshRemoteBaseRef(baseBranch: string): Promise<string | null> { | ||||||||||||||||||||||||||||||||||||||
| // Already a remote ref — nothing local to refresh. | ||||||||||||||||||||||||||||||||||||||
| if (baseBranch.startsWith("origin/")) return null; | ||||||||||||||||||||||||||||||||||||||
| const remoteRef = `origin/${baseBranch}`; | ||||||||||||||||||||||||||||||||||||||
| try { | ||||||||||||||||||||||||||||||||||||||
| await execAsync(`git fetch origin ${shellQuote(baseBranch)}`, { | ||||||||||||||||||||||||||||||||||||||
| cwd: this.options.rootDir, | ||||||||||||||||||||||||||||||||||||||
| timeout: 60_000, | ||||||||||||||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||||||||||||||
| } catch { | ||||||||||||||||||||||||||||||||||||||
| // Swallow: fall through to the existing remote-tracking ref if present. | ||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||
| try { | ||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+1987
to
+1995
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
Suggested change
|
||||||||||||||||||||||||||||||||||||||
| await execAsync(`git rev-parse --verify ${shellQuote(remoteRef)}`, { | ||||||||||||||||||||||||||||||||||||||
| cwd: this.options.rootDir, | ||||||||||||||||||||||||||||||||||||||
| timeout: 30_000, | ||||||||||||||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||||||||||||||
| return remoteRef; | ||||||||||||||||||||||||||||||||||||||
| } catch { | ||||||||||||||||||||||||||||||||||||||
| return null; | ||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| private async readCommitTaskOwnership(sha: string, taskId: string, lineageId?: string) { | ||||||||||||||||||||||||||||||||||||||
| const { stdout } = await execAsync(`git show -s --format=%s%x1f%b ${shellQuote(sha)}`, { | ||||||||||||||||||||||||||||||||||||||
| cwd: this.options.rootDir, | ||||||||||||||||||||||||||||||||||||||
|
|
@@ -8635,14 +8668,37 @@ export class SelfHealingManager { | |||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| const landed = await this.findAlreadyMergedTaskCommit({ | ||||||||||||||||||||||||||||||||||||||
| let landed = await this.findAlreadyMergedTaskCommit({ | ||||||||||||||||||||||||||||||||||||||
| taskId: task.id, | ||||||||||||||||||||||||||||||||||||||
| lineageId: task.lineageId, | ||||||||||||||||||||||||||||||||||||||
| repoDir: this.options.rootDir, | ||||||||||||||||||||||||||||||||||||||
| baseBranch, | ||||||||||||||||||||||||||||||||||||||
| taskBranch: task.branch, | ||||||||||||||||||||||||||||||||||||||
| baseCommitSha: task.baseCommitSha, | ||||||||||||||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||||||||||||||
| if (!landed && getPrimaryPrInfo(task)) { | ||||||||||||||||||||||||||||||||||||||
| // Fetch-then-prove: the LOCAL base ref can be stale. When a PR merged | ||||||||||||||||||||||||||||||||||||||
| // on the remote (human / merge-train squash) but this process never | ||||||||||||||||||||||||||||||||||||||
| // fetched, the owned commit is absent from the local base branch, so | ||||||||||||||||||||||||||||||||||||||
| // the detector finds nothing and the failed card holds its file-scope | ||||||||||||||||||||||||||||||||||||||
| // lease forever. Best-effort refresh the remote-tracking base ref and | ||||||||||||||||||||||||||||||||||||||
| // re-run the SAME evidence detector against it. The owned-commit proof | ||||||||||||||||||||||||||||||||||||||
| // (and every foreign-ownership guard inside the detector) still gates | ||||||||||||||||||||||||||||||||||||||
| // the heal, so this only un-wedges a genuinely-merged task — it never | ||||||||||||||||||||||||||||||||||||||
| // phantom-finalizes on unproven state. Gated on a recorded PR: no PR | ||||||||||||||||||||||||||||||||||||||
| // ⇒ nothing could have merged remotely ⇒ no fetch. | ||||||||||||||||||||||||||||||||||||||
| const refreshedBaseRef = await this.refreshRemoteBaseRef(baseBranch); | ||||||||||||||||||||||||||||||||||||||
| if (refreshedBaseRef) { | ||||||||||||||||||||||||||||||||||||||
| landed = await this.findAlreadyMergedTaskCommit({ | ||||||||||||||||||||||||||||||||||||||
| taskId: task.id, | ||||||||||||||||||||||||||||||||||||||
| lineageId: task.lineageId, | ||||||||||||||||||||||||||||||||||||||
| repoDir: this.options.rootDir, | ||||||||||||||||||||||||||||||||||||||
| baseBranch: refreshedBaseRef, | ||||||||||||||||||||||||||||||||||||||
| taskBranch: task.branch, | ||||||||||||||||||||||||||||||||||||||
| baseCommitSha: task.baseCommitSha, | ||||||||||||||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||
| if (!landed) continue; | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| const mergeDetails: MergeDetails = { | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
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.
When
git fetch origin <base>fails, this branch still falls through and returns any existingorigin/<base>ref. If that local tracking ref is stale but contains an old owned squash commit, the recovery path can prove against stale evidence, move the failed review task todone, markmergeConfirmed: true, and remove the worktree even though the current remote base was never checked. Returnnullon fetch failure so destructive recovery only runs after a successful refresh.