fix(FN-4811): scope-leak guard always allows .changeset/ paths
The [scope-leak] reviewLevel=N enforcement=warn warning was firing on many in-progress tasks for off-scope .changeset/FN-XXXX-*.md files (the production signature on FN-4789, FN-4801, FN-4818 \u2014 their branches all contained .changeset/FN-4811-*.md files from the in-progress fix stack). By convention every task may add its own changeset entry under .changeset/ per AGENTS.md 'Finalizing Changes' section, so .changeset/ files are now treated as always-allowed by the scope-leak guard regardless of the task's declared file scope. Cross-task changeset leakage is still caught by stronger downstream guards (file-scope invariant at squash, post-merge audit) at much higher signal-to-noise. This change only suppresses the noisy per-execution warning that was flooding logs without adding any defensive value. Adds a new exported helper isAlwaysAllowedScopeLeakPath() so the allowlist surface is easy to extend. Test coverage in scope-leak-changeset-allowlist.test.ts. Also (incorporated from interrupted merge state): loosens the executing-task-lock.test.ts assertion that one losing-instance store sees zero work-log entries rather than the brittle exact-count of mockedCreateFnAgent invocations (the no-fn_task_done retry path can fire on the winning instance, so the count varies). Fusion-Task-Id: FN-4811
This commit is contained in:
@@ -1,17 +1,23 @@
|
||||
/**
|
||||
* FN-4811 follow-up (FN-4809 production reproduction):
|
||||
*
|
||||
* After commit 82f80e72f added a per-instance `this.executing.add()` synchronous
|
||||
* claim, production STILL produced two execute() invocations for the same task
|
||||
* ID that both reached "Executor detected stale merge state" (executor.ts:2661)
|
||||
* After 82f80e72f's per-instance `this.executing.add()` synchronous claim,
|
||||
* production STILL produced two execute() invocations for the same task ID
|
||||
* that both reached "Executor detected stale merge state" (executor.ts:2661)
|
||||
* and both generated runIds within 1 second of each other (y2nb + 9gde for
|
||||
* FN-4809 at 02:48:17–18 UTC). The only viable explanation is that there is
|
||||
* more than one `TaskExecutor` instance in the process (engine restart race,
|
||||
* multi-project hybrid runtime, or test-helper-style code creating a second
|
||||
* instance).
|
||||
* more than one `TaskExecutor` instance in the process (e.g., engine restart
|
||||
* race, multi-project hybrid runtime, or test-helper-style code creating a
|
||||
* second instance).
|
||||
*
|
||||
* The fix is a process-wide singleton `executingTaskLock` in
|
||||
* `active-session-registry.ts`. This test covers the contract directly.
|
||||
* `active-session-registry.ts`. This test covers the contract directly:
|
||||
*
|
||||
* - Two distinct `TaskExecutor` instances calling `execute()` for the same
|
||||
* task ID. Only one should actually run — the other must bail at the
|
||||
* process-wide claim.
|
||||
* - The lock is released when execute() completes, so a subsequent
|
||||
* execute() on either instance is allowed.
|
||||
*/
|
||||
import { describe, it, expect, vi, beforeEach } from "vitest";
|
||||
import "../executor-test-helpers.js";
|
||||
@@ -79,8 +85,16 @@ describe("FN-4811 follow-up (FN-4809): process-wide executingTaskLock", () => {
|
||||
expect(resultA.status).toBe("fulfilled");
|
||||
expect(resultB.status).toBe("fulfilled");
|
||||
|
||||
// Exactly one store should have received work-related log entries — the
|
||||
// losing instance bailed at the process-wide claim before any work began.
|
||||
// Critical: exactly one instance progresses into real work; the other bails
|
||||
// at the process-wide claim before any work begins. Before this fix, each
|
||||
// instance had its own `executing` Set, so both proceeded past the per-instance
|
||||
// guard and both created agent sessions.
|
||||
//
|
||||
// The exact createFnAgent count depends on the retry loop (mocked prompt never
|
||||
// calls fn_task_done, so the no-fn_task_done retry path may fire), so we don't
|
||||
// assert exact-1. The invariant we DO assert is that exactly one of the two
|
||||
// stores received work-related log entries — the other store stayed completely
|
||||
// untouched because its execute() bailed at the lock claim.
|
||||
const aLogCount = (storeA.logEntry as any).mock.calls.length;
|
||||
const bLogCount = (storeB.logEntry as any).mock.calls.length;
|
||||
expect((aLogCount > 0) !== (bLogCount > 0)).toBe(true);
|
||||
|
||||
@@ -0,0 +1,29 @@
|
||||
/**
|
||||
* FN-4811 follow-up: `.changeset/` files are always allowed by the scope-leak guard,
|
||||
* regardless of the task's declared file scope. By convention every task may add its
|
||||
* own changeset entry under `.changeset/` (per AGENTS.md "Finalizing Changes"), and
|
||||
* cross-task changeset leakage is caught by stronger downstream guards (file-scope
|
||||
* invariant at squash, post-merge audit) — so the per-execution scope-leak warning
|
||||
* doesn't need to flag them.
|
||||
*/
|
||||
import { describe, it, expect } from "vitest";
|
||||
import { isAlwaysAllowedScopeLeakPath } from "../executor.js";
|
||||
|
||||
describe("FN-4811 follow-up: scope-leak always-allowed paths", () => {
|
||||
it("treats .changeset/*.md files as always allowed", () => {
|
||||
expect(isAlwaysAllowedScopeLeakPath(".changeset/FN-4811-fix.md")).toBe(true);
|
||||
expect(isAlwaysAllowedScopeLeakPath(".changeset/fn-4811-fix.md")).toBe(true);
|
||||
expect(isAlwaysAllowedScopeLeakPath(".changeset/config.json")).toBe(true);
|
||||
});
|
||||
|
||||
it("treats nested .changeset/ paths as always allowed", () => {
|
||||
expect(isAlwaysAllowedScopeLeakPath(".changeset/nested/dir/file.md")).toBe(true);
|
||||
});
|
||||
|
||||
it("does NOT match arbitrary paths", () => {
|
||||
expect(isAlwaysAllowedScopeLeakPath("packages/engine/src/executor.ts")).toBe(false);
|
||||
expect(isAlwaysAllowedScopeLeakPath("README.md")).toBe(false);
|
||||
expect(isAlwaysAllowedScopeLeakPath(".changesetlike.md")).toBe(false);
|
||||
expect(isAlwaysAllowedScopeLeakPath("docs/.changeset/x.md")).toBe(false);
|
||||
});
|
||||
});
|
||||
@@ -231,6 +231,18 @@ export function extractReferencedPathsFromWorkflowFeedback(feedback: string): st
|
||||
return extracted;
|
||||
}
|
||||
|
||||
/**
|
||||
* FN-4811 follow-up: paths the scope-leak guard never flags, regardless of declared
|
||||
* scope. These are file types every task may legitimately touch as part of standard
|
||||
* delivery (e.g., `.changeset/` per AGENTS.md's "Finalizing Changes" section).
|
||||
* Cross-task contamination of these paths is caught by stronger guards downstream
|
||||
* (file-scope invariant at squash commit, branch-tip checks, post-merge audit).
|
||||
*/
|
||||
export function isAlwaysAllowedScopeLeakPath(filePath: string): boolean {
|
||||
const normalizedPath = normalizeWorkflowScopePath(filePath);
|
||||
return normalizedPath.startsWith(".changeset/");
|
||||
}
|
||||
|
||||
export function workflowPathMatchesDeclaredScope(filePath: string, scopePatterns: readonly string[]): boolean {
|
||||
const normalizedPath = normalizeWorkflowScopePath(filePath);
|
||||
for (const rawPattern of scopePatterns) {
|
||||
@@ -2573,21 +2585,24 @@ export class TaskExecutor {
|
||||
* as-is. Branches remain task-scoped (`fusion/{task-id}`).
|
||||
*/
|
||||
async execute(task: Task): Promise<void> {
|
||||
// FN-4811 follow-up (FN-4809/FN-4814/FN-4811 production failure): claim a
|
||||
// FN-4811 follow-up (FN-4814/FN-4809/FN-4811 production failure): claim a
|
||||
// PROCESS-WIDE lock synchronously before any other work. Per-instance
|
||||
// `this.executing` was insufficient in production because two execute()
|
||||
// invocations for the same task ID still both reached "Executor detected
|
||||
// stale merge state" (executor.ts:2661) and both generated runIds — the only
|
||||
// viable explanation is multiple TaskExecutor instances in the same process
|
||||
// (engine restart race, multi-project hybrid runtime, etc.). The only
|
||||
// fully-reliable guard is a singleton lock shared across all instances.
|
||||
// stale merge state" (executor.ts:2661) and both generated runIds — producing
|
||||
// duplicate "Worktree created at /..." log entries within the same second.
|
||||
// The only fully-reliable guard is a singleton lock shared across all
|
||||
// TaskExecutor instances in the same process (e.g., engine restart race,
|
||||
// multi-project hybrid runtime, etc.). This is `executingTaskLock` in
|
||||
// active-session-registry.ts, a module-level Set.
|
||||
const claimed = executingTaskLock.tryClaim(task.id);
|
||||
executorLog.log(`execute() called for ${task.id} (claimed=${claimed}, perInstanceExecuting=${this.executing.has(task.id)})`);
|
||||
if (!claimed) return;
|
||||
|
||||
// Maintain the per-instance Set too, for back-compat with all the existing
|
||||
// `this.executing.has()` checks throughout the file (handler gates,
|
||||
// stuck-detector, resumeTaskForAgent, etc.).
|
||||
// stuck-detector, resumeTaskForAgent, etc.). Per-instance state stays
|
||||
// consistent with the process-wide lock.
|
||||
this.executing.add(task.id);
|
||||
|
||||
const assignedAgentId = task.assignedAgentId;
|
||||
@@ -5206,7 +5221,13 @@ export class TaskExecutor {
|
||||
}
|
||||
|
||||
const offScopeFiles = touchedFiles
|
||||
.filter((filePath) => !workflowPathMatchesDeclaredScope(filePath, declaredScope));
|
||||
.filter((filePath) => !workflowPathMatchesDeclaredScope(filePath, declaredScope))
|
||||
// FN-4811 follow-up: by convention every task may add its own changeset entry
|
||||
// under `.changeset/`, so changeset files are always considered in-scope and
|
||||
// never flagged by the scope-leak guard. The file-scope invariant at squash and
|
||||
// the broader contamination guards still catch cross-task changeset leakage at
|
||||
// a higher signal-to-noise ratio than the per-execution scope-leak warning.
|
||||
.filter((filePath) => !isAlwaysAllowedScopeLeakPath(filePath));
|
||||
if (offScopeFiles.length === 0) {
|
||||
return { blocked: false };
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user