Skip to content

Commit 89e2059

Browse files
committed
fix: harden ensureTaskFilesCommitted merge-tree path and rename param (Sage review)
Two fixes from Sage code review of TP-163: 1. merge-tree output validation: validate the tree SHA matches a 40-hex OID pattern before passing it to commit-tree. git merge-tree --write-tree exits 0 on clean merge but could produce unexpected output in edge cases. An invalid SHA would cause commit-tree to fail with a confusing error. 2. executeWave param renamed baseBranch -> orchBranch: the parameter is always the orch branch name (batchState.orchBranch at all call sites), not the user's working branch. The old name caused confusion in the new ensureTaskFilesCommitted code that explicitly handles orch branch updates. Workspace multi-repo gap (Sage finding #1) tracked in #479 — requires per-repo grouping of wave tasks and is a larger change.
1 parent 3e9d65c commit 89e2059

1 file changed

Lines changed: 41 additions & 31 deletions

File tree

extensions/taskplane/execution.ts

Lines changed: 41 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -1536,38 +1536,48 @@ export function ensureTaskFilesCommitted(
15361536
repoRoot,
15371537
);
15381538
if (mergeTreeRes.ok) {
1539-
// First line of stdout is the merged tree SHA
1539+
// First line of stdout is the merged tree SHA.
1540+
// git merge-tree --write-tree exits 0 on clean merge, non-zero on conflicts.
1541+
// Since it exited 0, the tree should be conflict-free, but validate
1542+
// the SHA looks like a valid 40-hex OID before using it.
15401543
const mergedTree = mergeTreeRes.stdout.trim().split("\n")[0];
1541-
const mergeCommitMsg = `merge: include staged task files for wave ${waveIndex} into orch branch`;
1542-
const commitTreeRes = runGit(
1543-
["commit-tree", mergedTree, "-p", orchTip, "-p", newHead, "-m", mergeCommitMsg],
1544-
repoRoot,
1545-
);
1546-
if (commitTreeRes.ok) {
1547-
const mergeCommitSha = commitTreeRes.stdout.trim();
1548-
const refUpdateRes = runGit(
1549-
["update-ref", `refs/heads/${orchBranch}`, mergeCommitSha, orchTip],
1544+
if (!/^[0-9a-f]{40}$/i.test(mergedTree)) {
1545+
execLog("wave", `W${waveIndex}`, `warning: merge-tree returned unexpected output (non-fatal)`, {
1546+
orchBranch,
1547+
output: mergedTree.slice(0, 60),
1548+
});
1549+
} else {
1550+
const mergeCommitMsg = `merge: include staged task files for wave ${waveIndex} into orch branch`;
1551+
const commitTreeRes = runGit(
1552+
["commit-tree", mergedTree, "-p", orchTip, "-p", newHead, "-m", mergeCommitMsg],
15501553
repoRoot,
15511554
);
1552-
if (refUpdateRes.ok) {
1553-
execLog("wave", `W${waveIndex}`, `merged staging commit into orch branch (non-FF wave)`, {
1554-
orchBranch,
1555-
orchTip: orchTip.slice(0, 8),
1556-
newHead: newHead.slice(0, 8),
1557-
mergeCommit: mergeCommitSha.slice(0, 8),
1558-
});
1555+
if (commitTreeRes.ok) {
1556+
const mergeCommitSha = commitTreeRes.stdout.trim();
1557+
const refUpdateRes = runGit(
1558+
["update-ref", `refs/heads/${orchBranch}`, mergeCommitSha, orchTip],
1559+
repoRoot,
1560+
);
1561+
if (refUpdateRes.ok) {
1562+
execLog("wave", `W${waveIndex}`, `merged staging commit into orch branch (non-FF wave)`, {
1563+
orchBranch,
1564+
orchTip: orchTip.slice(0, 8),
1565+
newHead: newHead.slice(0, 8),
1566+
mergeCommit: mergeCommitSha.slice(0, 8),
1567+
});
1568+
} else {
1569+
execLog("wave", `W${waveIndex}`, `warning: failed to update orch branch ref after merge-tree (non-fatal)`, {
1570+
orchBranch,
1571+
error: refUpdateRes.stderr,
1572+
});
1573+
}
15591574
} else {
1560-
execLog("wave", `W${waveIndex}`, `warning: failed to update orch branch ref after merge-tree (non-fatal)`, {
1575+
execLog("wave", `W${waveIndex}`, `warning: failed to create merge commit for orch branch (non-fatal)`, {
15611576
orchBranch,
1562-
error: refUpdateRes.stderr,
1577+
error: commitTreeRes.stderr,
15631578
});
15641579
}
1565-
} else {
1566-
execLog("wave", `W${waveIndex}`, `warning: failed to create merge commit for orch branch (non-fatal)`, {
1567-
orchBranch,
1568-
error: commitTreeRes.stderr,
1569-
});
1570-
}
1580+
} // end valid tree SHA
15711581
} else {
15721582
execLog("wave", `W${waveIndex}`, `warning: failed to compute merge-tree for orch branch (non-fatal; requires git ≥ 2.38)`, {
15731583
orchBranch,
@@ -1621,7 +1631,7 @@ export function ensureTaskFilesCommitted(
16211631
* @param batchId - Batch ID for naming
16221632
* @param pauseSignal - Shared pause signal (mutated by stop-wave policy)
16231633
* @param dependencyGraph - Dependency graph for computing transitive dependents
1624-
* @param baseBranch - Branch to base worktrees on (captured at batch start)
1634+
* @param orchBranch - Orch branch to base worktrees on (and to update after staging commits)
16251635
* @param onMonitorUpdate - Optional callback for dashboard updates during monitoring
16261636
* @param onLanesAllocated - Optional callback fired after lane allocation succeeds
16271637
* @param workspaceConfig - Workspace configuration for repo routing (null/undefined = repo mode)
@@ -1646,7 +1656,7 @@ export async function executeWave(
16461656
batchId: string,
16471657
pauseSignal: { paused: boolean },
16481658
dependencyGraph: DependencyGraph,
1649-
baseBranch: string,
1659+
orchBranch: string,
16501660
onMonitorUpdate?: MonitorUpdateCallback,
16511661
onLanesAllocated?: (lanes: AllocatedLane[]) => void,
16521662
workspaceConfig?: WorkspaceConfig | null,
@@ -1668,10 +1678,10 @@ export async function executeWave(
16681678
// Task folders may contain untracked files (PROMPT.md, STATUS.md) that
16691679
// won't appear in worktrees unless committed. Stage and commit them now,
16701680
// before worktree creation, so workers can find their TASK_AUTOSTART paths.
1671-
// Pass baseBranch (the orch branch) so the staging commit is also
1672-
// reflected in the orch branch before worktrees are allocated from it.
1681+
// Pass orchBranch so the staging commit is reflected in the orch branch
1682+
// before worktrees are allocated from it.
16731683
try {
1674-
ensureTaskFilesCommitted(waveTasks, pending, repoRoot, waveIndex, baseBranch);
1684+
ensureTaskFilesCommitted(waveTasks, pending, repoRoot, waveIndex, orchBranch);
16751685
} catch (err: unknown) {
16761686
const errMsg = err instanceof Error ? err.message : String(err);
16771687
execLog("wave", `W${waveIndex}`, `task file commit failed: ${errMsg}`);
@@ -1695,7 +1705,7 @@ export async function executeWave(
16951705
}
16961706

16971707
// ── Stage 1: Allocate lanes ──────────────────────────────────
1698-
const allocResult = allocateLanes(waveTasks, pending, config, repoRoot, batchId, baseBranch, workspaceConfig);
1708+
const allocResult = allocateLanes(waveTasks, pending, config, repoRoot, batchId, orchBranch, workspaceConfig);
16991709

17001710
if (!allocResult.success) {
17011711
const errMsg = allocResult.error?.message || "Unknown allocation failure";

0 commit comments

Comments
 (0)