fix(FN-5345): address second-pass review findings
Follow-up to 8e6740468 addressing six review findings, including one real
regression (combined short flags bypass amend detection).
HIGH
- Combined short flags ('-am', '-vm', '-sm', '-aF', ...) now count as
message-supplying tokens in the prepare-commit-msg empty-commit guard.
Previously, an agent could bypass the guard with
git commit --allow-empty -am 'fix --amend handling'
because '-am' did not match the literal '-m' case, so the token loop
continued past the message text and matched the '--amend' substring inside
it. The new pattern -[!-]*[mF]* matches any short combined flag containing
'm' or 'F' while leaving '--amend' (starts with '--') untouched.
Verified locally with two regression tests for '-am' and '-vm' plus one
positive test confirming legitimate '-am' with a real tracked modification
still succeeds.
MEDIUM
- Early empty-own-diff fast-path cleanup no longer uses 'git worktree remove
--force'. We now run 'git status --porcelain --untracked-files=normal'
first; dirty worktrees (or status-check failures) are left alone for the
self-healing sweep to reconcile later. Prevents silent loss of uncommitted
scratch in the no-op finalize path.
- MergeResult.task is now kept in sync with the DB after early-fast-path
cleanup. After 'store.updateTask(taskId, { worktree: null, branch: null })'
succeeds, the in-memory task.worktree/.branch are also cleared to undefined
so the returned result.task does not advertise a removed path or deleted
branch.
LOW
- Branch deletion in the fast-path cleanup only fires when 'task.branch' was
non-null on entry. If the task did not explicitly own a branch on entry,
we never invoke 'git branch -D'; orphan refs are left for
cleanupOrphanedBranches to handle. Prevents deleting a stray ref that
happened to share the canonical name.
- Inverted the empty 'if (poolBypassRequired) {} else { ... }' block in
reacquireReuseIntegrationWorktree to 'if (directReuseEligible) try { ... }'
with the pool-bypass note above it. No behavior change \u2014 just removes the
awkward empty branch and the one-level-deeper indent on the direct-reuse
logic.
Tests
- Full @fusion/engine suite: 448 files / 5881 tests / 9 skipped, all green
- pnpm lint green, pnpm build green
This commit is contained in:
@@ -45,7 +45,7 @@ describe("prepare-commit-msg empty-commit guard (real git, FN-5345/FN-5377)", ()
|
||||
expect(empty.stderr).toContain("refusing empty commit");
|
||||
expect(empty.stderr).toContain("FN-5345/FN-5377");
|
||||
|
||||
// Review-finding regression: a commit message containing the substring
|
||||
// Review-finding regression #1: a commit message containing the substring
|
||||
// '--amend' must NOT trick the parent-cmd tokenized check into allowing
|
||||
// the empty commit. The original glob pattern (*' --amend'*) would have
|
||||
// matched this; the tokenized check rejects it.
|
||||
@@ -56,6 +56,31 @@ describe("prepare-commit-msg empty-commit guard (real git, FN-5345/FN-5377)", ()
|
||||
expect(sneaky.status).not.toBe(0);
|
||||
expect(sneaky.stderr).toContain("refusing empty commit");
|
||||
|
||||
// Review-finding regression #2: combined short flags like '-am', '-vm',
|
||||
// '-sm' must also count as message-supplying tokens, otherwise the
|
||||
// tokenized scan continues past them and hits '--amend' in user-controlled
|
||||
// message text. The combined-short-flag pattern -[!-]*[mF]* catches these
|
||||
// while leaving '--amend' (starts with --) untouched.
|
||||
const sneakyAm = git(
|
||||
worktreeDir,
|
||||
"git commit --allow-empty -am 'feat(FN-5345): fix --amend handling via -am'",
|
||||
);
|
||||
expect(sneakyAm.status).not.toBe(0);
|
||||
expect(sneakyAm.stderr).toContain("refusing empty commit");
|
||||
const sneakyVm = git(
|
||||
worktreeDir,
|
||||
"git commit --allow-empty -vm 'feat(FN-5345): fix --amend handling via -vm'",
|
||||
);
|
||||
expect(sneakyVm.status).not.toBe(0);
|
||||
expect(sneakyVm.stderr).toContain("refusing empty commit");
|
||||
|
||||
// Legitimate combined short flag with -a and a modified TRACKED file:
|
||||
// should succeed (not blocked by the message-flag detection — -a stages
|
||||
// the tracked modification, the resulting commit is non-empty).
|
||||
writeFileSync(join(worktreeDir, "real.txt"), "real-modified\n");
|
||||
const legitAm = git(worktreeDir, "git commit -am 'feat(FN-5345): legit -am commit'");
|
||||
expect(legitAm.status).toBe(0);
|
||||
|
||||
// --amend --no-edit (no staged changes, amend HEAD) is ALLOWED.
|
||||
const amendNoEdit = git(worktreeDir, "git commit --amend --no-edit");
|
||||
expect(amendNoEdit.status).toBe(0);
|
||||
|
||||
@@ -6507,43 +6507,73 @@ async function tryEarlyEmptyOwnDiffFinalize(input: {
|
||||
// FN-5345/FN-5377: best-effort cleanup of the stranded worktree + branch
|
||||
// so .worktrees/ and the branch namespace do not accumulate empty-own-diff
|
||||
// residuals indefinitely. Failures are non-fatal: the task is already done.
|
||||
//
|
||||
// Safety rules:
|
||||
// - FN-4811: never touch a worktree owned by a different task.
|
||||
// - Dirty worktrees are left alone (no --force) so we never silently
|
||||
// discard uncommitted scratch; self-healing's worktree sweep handles
|
||||
// them later.
|
||||
// - Branch deletion only fires when task.branch was non-null on entry
|
||||
// (i.e. the task explicitly owned a branch). If task.branch was null,
|
||||
// `cleanupOrphanedBranches` handles any orphan ref later.
|
||||
let worktreeRemoved = false;
|
||||
let branchDeleted = false;
|
||||
const stranded = task.worktree?.trim();
|
||||
const ownedBranchOnEntry = task.branch?.trim();
|
||||
if (stranded && existsSync(stranded)) {
|
||||
// FN-4811 safety: never remove a worktree currently owned by a different
|
||||
// task. Same-task or unowned paths are eligible.
|
||||
const activeRecord = activeSessionRegistry.lookupByPath(stranded);
|
||||
if (activeRecord && activeRecord.taskId !== taskId) {
|
||||
log.warn(
|
||||
`${taskId}: skipping early-fast-path worktree cleanup — path ${stranded} is owned by ${activeRecord.taskId}`,
|
||||
);
|
||||
} else {
|
||||
let dirty = false;
|
||||
try {
|
||||
await execAsync(
|
||||
`git worktree remove --force ${quoteArg(stranded)}`,
|
||||
{ cwd: projectRootDir, timeout: 30_000 },
|
||||
const { stdout } = await execAsync(
|
||||
`git status --porcelain --untracked-files=normal`,
|
||||
{ cwd: stranded, encoding: "utf-8", timeout: 15_000 },
|
||||
);
|
||||
worktreeRemoved = true;
|
||||
} catch (removeErr) {
|
||||
dirty = stdout.trim().length > 0;
|
||||
} catch (statusErr) {
|
||||
// Treat status failure as "unknown" — fail safe by skipping removal.
|
||||
dirty = true;
|
||||
log.warn(
|
||||
`${taskId}: failed to remove stranded worktree ${stranded} (non-fatal): ${removeErr instanceof Error ? removeErr.message : String(removeErr)}`,
|
||||
`${taskId}: git status check failed for ${stranded}; skipping early-fast-path worktree removal: ${statusErr instanceof Error ? statusErr.message : String(statusErr)}`,
|
||||
);
|
||||
}
|
||||
if (dirty) {
|
||||
log.warn(
|
||||
`${taskId}: skipping early-fast-path worktree cleanup — ${stranded} has uncommitted changes; self-healing sweep will reconcile later`,
|
||||
);
|
||||
} else {
|
||||
try {
|
||||
await execAsync(
|
||||
`git worktree remove ${quoteArg(stranded)}`,
|
||||
{ cwd: projectRootDir, timeout: 30_000 },
|
||||
);
|
||||
worktreeRemoved = true;
|
||||
} catch (removeErr) {
|
||||
log.warn(
|
||||
`${taskId}: failed to remove stranded worktree ${stranded} (non-fatal): ${removeErr instanceof Error ? removeErr.message : String(removeErr)}`,
|
||||
);
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
try {
|
||||
// Branch must be deleted from the project root, not from inside a
|
||||
// worktree that may still be checked out to it.
|
||||
await execAsync(
|
||||
`git branch -D ${quoteArg(branch)}`,
|
||||
{ cwd: projectRootDir, timeout: 30_000 },
|
||||
);
|
||||
branchDeleted = true;
|
||||
} catch (delErr) {
|
||||
log.warn(
|
||||
`${taskId}: failed to delete stranded branch ${branch} (non-fatal): ${delErr instanceof Error ? delErr.message : String(delErr)}`,
|
||||
);
|
||||
if (ownedBranchOnEntry) {
|
||||
try {
|
||||
// Branch must be deleted from the project root, not from inside a
|
||||
// worktree that may still be checked out to it.
|
||||
await execAsync(
|
||||
`git branch -D ${quoteArg(ownedBranchOnEntry)}`,
|
||||
{ cwd: projectRootDir, timeout: 30_000 },
|
||||
);
|
||||
branchDeleted = true;
|
||||
} catch (delErr) {
|
||||
log.warn(
|
||||
`${taskId}: failed to delete stranded branch ${ownedBranchOnEntry} (non-fatal): ${delErr instanceof Error ? delErr.message : String(delErr)}`,
|
||||
);
|
||||
}
|
||||
}
|
||||
if (worktreeRemoved || branchDeleted) {
|
||||
try {
|
||||
@@ -6551,6 +6581,12 @@ async function tryEarlyEmptyOwnDiffFinalize(input: {
|
||||
worktree: worktreeRemoved ? null : task.worktree,
|
||||
branch: branchDeleted ? null : task.branch,
|
||||
});
|
||||
// Keep the in-memory task in sync with the DB so the returned
|
||||
// MergeResult.task does not advertise a removed path / deleted branch.
|
||||
// (updateTask uses null as the "clear this field" sentinel; the
|
||||
// in-memory Task type uses undefined for absent.)
|
||||
if (worktreeRemoved) task.worktree = undefined;
|
||||
if (branchDeleted) task.branch = undefined;
|
||||
} catch (updateErr) {
|
||||
log.warn(
|
||||
`${taskId}: failed to clear worktree/branch pointers after early-fast-path cleanup (non-fatal): ${updateErr instanceof Error ? updateErr.message : String(updateErr)}`,
|
||||
@@ -6732,12 +6768,13 @@ export async function aiMergeTask(
|
||||
// the new path would bypass pool bookkeeping and could collide with
|
||||
// `PoolDoubleLeaseError`.
|
||||
const expectedBranch = task.branch || canonicalFusionBranchName(taskId);
|
||||
const poolBypassRequired = Boolean(options.pool && settings.recycleWorktrees);
|
||||
try {
|
||||
if (poolBypassRequired) {
|
||||
// Pool semantics require acquireTaskWorktree; skip direct-reuse.
|
||||
} else {
|
||||
const { stdout: porcelain } = await execAsync(
|
||||
// FN-4954: when a worktree pool is attached and recycling is enabled, pool
|
||||
// semantics REQUIRE going through `acquireTaskWorktree` so `WorktreePool`'s
|
||||
// lease bookkeeping stays consistent. Skip the direct-reuse shortcut here
|
||||
// and fall through to the existing acquisition path.
|
||||
const directReuseEligible = !(options.pool && settings.recycleWorktrees);
|
||||
if (directReuseEligible) try {
|
||||
const { stdout: porcelain } = await execAsync(
|
||||
`git worktree list --porcelain`,
|
||||
{ cwd: projectRootDir, encoding: "utf-8", timeout: 30_000 },
|
||||
);
|
||||
@@ -6848,7 +6885,6 @@ export async function aiMergeTask(
|
||||
});
|
||||
return;
|
||||
}
|
||||
}
|
||||
} catch (listErr) {
|
||||
mergerLog.warn(
|
||||
`${taskId}: git worktree list consult failed before reacquire; proceeding with fresh creation: ${listErr instanceof Error ? listErr.message : String(listErr)}`,
|
||||
|
||||
@@ -139,7 +139,14 @@ fi
|
||||
for tok in $PARENT_CMD; do
|
||||
case "$tok" in
|
||||
-m|-F|--message|--file|--message=*|--file=*)
|
||||
# Message args start here; everything after this is user-controlled.
|
||||
# Long-form message args; everything after this is user-controlled.
|
||||
break
|
||||
;;
|
||||
-[!-]*[mF]*)
|
||||
# Combined short flag containing 'm' or 'F' (e.g. -am, -vm, -sm, -aF).
|
||||
# First char is '-', second is NOT '-' (so '--amend' does not match),
|
||||
# and the cluster contains a message-supplying letter. Everything after
|
||||
# this token is user-controlled message text.
|
||||
break
|
||||
;;
|
||||
--amend)
|
||||
|
||||
Reference in New Issue
Block a user