feat: add three guardrails against out-of-scope agent deletions
1. Scoping rules in agent prompts: executor, triage template, and reviewer now explicitly forbid deleting/gutting modules, settings, interfaces, exports, or test files outside the task's declared File Scope. Reviewer will REVISE if out-of-scope removals are detected. 2. Pre-merge diffstat scope check: merger.ts validates the git diffstat against the task's PROMPT.md File Scope before merging. Large deletions outside scope are logged as warnings on the task (soft guardrail). 3. Changeset requirement for feature removal: triage template now requires a .changeset/ entry when removing existing functionality. Executor and reviewer enforce this requirement. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -139,6 +139,10 @@ model, read-only access) to independently assess your work.
|
|||||||
- Follow the "Do NOT" section strictly
|
- Follow the "Do NOT" section strictly
|
||||||
- If you find work outside the task's scope, use \`task_create\`
|
- If you find work outside the task's scope, use \`task_create\`
|
||||||
- Update documentation listed in "Must Update" and check "Check If Affected"
|
- Update documentation listed in "Must Update" and check "Check If Affected"
|
||||||
|
- NEVER delete, remove, or gut modules, interfaces, settings, exports, or test files outside your File Scope
|
||||||
|
- NEVER remove features as "cleanup" — if something seems unused, create a task for investigation instead
|
||||||
|
- Removing code is acceptable ONLY when it is explicitly part of your task's mission
|
||||||
|
- If you remove existing functionality, you MUST create a changeset in \`.changeset/\` explaining the removal and rationale
|
||||||
|
|
||||||
## Completion
|
## Completion
|
||||||
After all steps are done, tests pass, and docs are updated:
|
After all steps are done, tests pass, and docs are updated:
|
||||||
|
|||||||
@@ -39,6 +39,9 @@ import {
|
|||||||
resolveTrivialWhitespace,
|
resolveTrivialWhitespace,
|
||||||
LOCKFILE_PATTERNS,
|
LOCKFILE_PATTERNS,
|
||||||
GENERATED_PATTERNS,
|
GENERATED_PATTERNS,
|
||||||
|
parseDiffStat,
|
||||||
|
extractFileScope,
|
||||||
|
validateDiffScope,
|
||||||
type ConflictCategory,
|
type ConflictCategory,
|
||||||
} from "./merger.js";
|
} from "./merger.js";
|
||||||
import { createKbAgent } from "./pi.js";
|
import { createKbAgent } from "./pi.js";
|
||||||
@@ -1960,3 +1963,172 @@ describe("aiMergeTask — build verification", () => {
|
|||||||
expect(store.moveTask).toHaveBeenCalledWith("FN-050", "done");
|
expect(store.moveTask).toHaveBeenCalledWith("FN-050", "done");
|
||||||
});
|
});
|
||||||
});
|
});
|
||||||
|
|
||||||
|
// ── Pre-merge diffstat scope validation tests ────────────────────────
|
||||||
|
|
||||||
|
describe("parseDiffStat", () => {
|
||||||
|
it("parses standard diffstat output", () => {
|
||||||
|
const stat = [
|
||||||
|
" packages/core/src/types.ts | 9 ++--",
|
||||||
|
" packages/engine/src/notifier.ts | 46 +-----",
|
||||||
|
" 2 files changed, 10 insertions(+), 45 deletions(-)",
|
||||||
|
].join("\n");
|
||||||
|
|
||||||
|
const entries = parseDiffStat(stat);
|
||||||
|
expect(entries).toHaveLength(2);
|
||||||
|
expect(entries[0].file).toBe("packages/core/src/types.ts");
|
||||||
|
// Rounding may shift total by ±1, so check approximate range
|
||||||
|
expect(entries[0].insertions + entries[0].deletions).toBeGreaterThanOrEqual(9);
|
||||||
|
expect(entries[0].insertions + entries[0].deletions).toBeLessThanOrEqual(10);
|
||||||
|
expect(entries[1].file).toBe("packages/engine/src/notifier.ts");
|
||||||
|
expect(entries[1].deletions).toBeGreaterThan(entries[1].insertions);
|
||||||
|
});
|
||||||
|
|
||||||
|
it("handles pure-deletion lines", () => {
|
||||||
|
const stat = " packages/engine/src/usage.ts | 527 ---";
|
||||||
|
const entries = parseDiffStat(stat);
|
||||||
|
expect(entries).toHaveLength(1);
|
||||||
|
expect(entries[0].insertions).toBe(0);
|
||||||
|
expect(entries[0].deletions).toBe(527);
|
||||||
|
});
|
||||||
|
|
||||||
|
it("handles pure-insertion lines", () => {
|
||||||
|
const stat = " packages/engine/src/new.ts | 100 +++";
|
||||||
|
const entries = parseDiffStat(stat);
|
||||||
|
expect(entries).toHaveLength(1);
|
||||||
|
expect(entries[0].insertions).toBe(100);
|
||||||
|
expect(entries[0].deletions).toBe(0);
|
||||||
|
});
|
||||||
|
|
||||||
|
it("returns empty for unreadable stat", () => {
|
||||||
|
expect(parseDiffStat("(unable to read diff)")).toEqual([]);
|
||||||
|
expect(parseDiffStat("")).toEqual([]);
|
||||||
|
});
|
||||||
|
|
||||||
|
it("skips summary line", () => {
|
||||||
|
const stat = " 1 file changed, 5 insertions(+)";
|
||||||
|
expect(parseDiffStat(stat)).toEqual([]);
|
||||||
|
});
|
||||||
|
});
|
||||||
|
|
||||||
|
describe("extractFileScope", () => {
|
||||||
|
it("extracts file patterns from PROMPT.md", () => {
|
||||||
|
const prompt = [
|
||||||
|
"# Task: FN-100 - Add feature",
|
||||||
|
"",
|
||||||
|
"## File Scope",
|
||||||
|
"",
|
||||||
|
"- `packages/core/src/types.ts`",
|
||||||
|
"- `packages/engine/src/notifier.ts`",
|
||||||
|
"- `packages/dashboard/app/components/*`",
|
||||||
|
"",
|
||||||
|
"## Steps",
|
||||||
|
"",
|
||||||
|
"### Step 1: Do things",
|
||||||
|
].join("\n");
|
||||||
|
|
||||||
|
const scope = extractFileScope(prompt);
|
||||||
|
expect(scope).toEqual([
|
||||||
|
"packages/core/src/types.ts",
|
||||||
|
"packages/engine/src/notifier.ts",
|
||||||
|
"packages/dashboard/app/components/*",
|
||||||
|
]);
|
||||||
|
});
|
||||||
|
|
||||||
|
it("handles patterns with artifact annotations", () => {
|
||||||
|
const prompt = [
|
||||||
|
"## File Scope",
|
||||||
|
"",
|
||||||
|
"- `src/foo.ts` (new)",
|
||||||
|
"- `src/bar.ts` (modified)",
|
||||||
|
"",
|
||||||
|
"## Steps",
|
||||||
|
].join("\n");
|
||||||
|
|
||||||
|
const scope = extractFileScope(prompt);
|
||||||
|
expect(scope).toEqual(["src/foo.ts", "src/bar.ts"]);
|
||||||
|
});
|
||||||
|
|
||||||
|
it("returns empty for missing File Scope section", () => {
|
||||||
|
const prompt = "# Task\n\n## Steps\n### Step 1\n";
|
||||||
|
expect(extractFileScope(prompt)).toEqual([]);
|
||||||
|
});
|
||||||
|
});
|
||||||
|
|
||||||
|
describe("validateDiffScope", () => {
|
||||||
|
it("returns warnings for large deletions outside scope", async () => {
|
||||||
|
const store = {
|
||||||
|
getTask: vi.fn().mockResolvedValue({
|
||||||
|
prompt: [
|
||||||
|
"## File Scope",
|
||||||
|
"",
|
||||||
|
"- `packages/dashboard/app/components/Header.tsx`",
|
||||||
|
"",
|
||||||
|
"## Steps",
|
||||||
|
].join("\n"),
|
||||||
|
}),
|
||||||
|
logEntry: vi.fn(),
|
||||||
|
} as unknown as TaskStore;
|
||||||
|
|
||||||
|
const diffStat = [
|
||||||
|
" packages/dashboard/app/components/Header.tsx | 20 ++--",
|
||||||
|
" packages/engine/src/usage.ts | 527 ---",
|
||||||
|
" packages/engine/src/usage.test.ts | 524 ---",
|
||||||
|
" 3 files changed, 5 insertions(+), 1066 deletions(-)",
|
||||||
|
].join("\n");
|
||||||
|
|
||||||
|
const result = await validateDiffScope(store, "FN-100", diffStat);
|
||||||
|
expect(result.outOfScopeFiles).toContain("packages/engine/src/usage.ts");
|
||||||
|
expect(result.outOfScopeFiles).toContain("packages/engine/src/usage.test.ts");
|
||||||
|
expect(result.largeOutOfScopeDeletions).toHaveLength(2);
|
||||||
|
expect(result.warnings.length).toBeGreaterThan(0);
|
||||||
|
expect(result.warnings[0]).toContain("SCOPE WARNING");
|
||||||
|
});
|
||||||
|
|
||||||
|
it("allows changeset files outside scope", async () => {
|
||||||
|
const store = {
|
||||||
|
getTask: vi.fn().mockResolvedValue({
|
||||||
|
prompt: "## File Scope\n\n- `src/foo.ts`\n\n## Steps",
|
||||||
|
}),
|
||||||
|
} as unknown as TaskStore;
|
||||||
|
|
||||||
|
const diffStat = [
|
||||||
|
" src/foo.ts | 10 +++",
|
||||||
|
" .changeset/my-change.md | 5 +++",
|
||||||
|
" 2 files changed, 15 insertions(+)",
|
||||||
|
].join("\n");
|
||||||
|
|
||||||
|
const result = await validateDiffScope(store, "FN-100", diffStat);
|
||||||
|
expect(result.outOfScopeFiles).not.toContain(".changeset/my-change.md");
|
||||||
|
expect(result.warnings).toHaveLength(0);
|
||||||
|
});
|
||||||
|
|
||||||
|
it("returns empty result when no scope is declared", async () => {
|
||||||
|
const store = {
|
||||||
|
getTask: vi.fn().mockResolvedValue({
|
||||||
|
prompt: "# Task\n\n## Steps\n",
|
||||||
|
}),
|
||||||
|
} as unknown as TaskStore;
|
||||||
|
|
||||||
|
const result = await validateDiffScope(store, "FN-100", " foo.ts | 500 ---");
|
||||||
|
expect(result.warnings).toHaveLength(0);
|
||||||
|
});
|
||||||
|
|
||||||
|
it("does not warn for in-scope changes", async () => {
|
||||||
|
const store = {
|
||||||
|
getTask: vi.fn().mockResolvedValue({
|
||||||
|
prompt: "## File Scope\n\n- `packages/engine/src/*`\n\n## Steps",
|
||||||
|
}),
|
||||||
|
} as unknown as TaskStore;
|
||||||
|
|
||||||
|
const diffStat = [
|
||||||
|
" packages/engine/src/executor.ts | 50 +++---",
|
||||||
|
" packages/engine/src/triage.ts | 30 +++---",
|
||||||
|
" 2 files changed, 40 insertions(+), 40 deletions(-)",
|
||||||
|
].join("\n");
|
||||||
|
|
||||||
|
const result = await validateDiffScope(store, "FN-100", diffStat);
|
||||||
|
expect(result.outOfScopeFiles).toHaveLength(0);
|
||||||
|
expect(result.warnings).toHaveLength(0);
|
||||||
|
});
|
||||||
|
});
|
||||||
|
|||||||
@@ -92,6 +92,156 @@ function matchGlob(path: string, pattern: string): boolean {
|
|||||||
return regex.test(fileName) || regex.test(path);
|
return regex.test(fileName) || regex.test(path);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// ── Pre-merge diffstat scope validation ──────────────────────────────
|
||||||
|
|
||||||
|
interface DiffFileEntry {
|
||||||
|
file: string;
|
||||||
|
insertions: number;
|
||||||
|
deletions: number;
|
||||||
|
}
|
||||||
|
|
||||||
|
interface DiffScopeResult {
|
||||||
|
warnings: string[];
|
||||||
|
outOfScopeFiles: string[];
|
||||||
|
largeOutOfScopeDeletions: { file: string; deletions: number }[];
|
||||||
|
}
|
||||||
|
|
||||||
|
/**
|
||||||
|
* Parse git `--stat` output into per-file insertion/deletion counts.
|
||||||
|
*
|
||||||
|
* Example line: ` packages/core/src/types.ts | 9 ++--`
|
||||||
|
* Binary line: ` some/image.png | Bin 0 -> 1234 bytes`
|
||||||
|
*/
|
||||||
|
export function parseDiffStat(diffStat: string): DiffFileEntry[] {
|
||||||
|
const entries: DiffFileEntry[] = [];
|
||||||
|
for (const line of diffStat.split("\n")) {
|
||||||
|
// Skip the summary line ("5 files changed, 10 insertions(+), 3 deletions(-)")
|
||||||
|
if (line.includes("files changed") || line.includes("file changed")) continue;
|
||||||
|
// Match: " path/to/file | 42 +++---" or " path/to/file | Bin ..."
|
||||||
|
const match = line.match(/^\s*(.+?)\s+\|\s+(\d+)\s+(\+*)(-*)\s*$/);
|
||||||
|
if (!match) continue;
|
||||||
|
const file = match[1].trim();
|
||||||
|
const plusses = match[3].length;
|
||||||
|
const minuses = match[4].length;
|
||||||
|
// The number is total changes; +/- chars show the ratio
|
||||||
|
const total = parseInt(match[2], 10);
|
||||||
|
if (total === 0) continue;
|
||||||
|
const ratio = plusses + minuses > 0 ? plusses / (plusses + minuses) : 0.5;
|
||||||
|
entries.push({
|
||||||
|
file,
|
||||||
|
insertions: Math.round(total * ratio),
|
||||||
|
deletions: Math.round(total * (1 - ratio)),
|
||||||
|
});
|
||||||
|
}
|
||||||
|
return entries;
|
||||||
|
}
|
||||||
|
|
||||||
|
/**
|
||||||
|
* Extract the `## File Scope` section from a PROMPT.md string.
|
||||||
|
* Returns an array of file/glob patterns (lines starting with `- \``).
|
||||||
|
*/
|
||||||
|
export function extractFileScope(promptContent: string): string[] {
|
||||||
|
const lines = promptContent.split("\n");
|
||||||
|
const patterns: string[] = [];
|
||||||
|
let inScope = false;
|
||||||
|
for (const line of lines) {
|
||||||
|
if (/^##\s+File Scope/.test(line)) {
|
||||||
|
inScope = true;
|
||||||
|
continue;
|
||||||
|
}
|
||||||
|
if (inScope && /^##\s/.test(line)) break; // next section
|
||||||
|
if (inScope) {
|
||||||
|
// Match "- `path/to/file`" or "- path/to/file"
|
||||||
|
const m = line.match(/^-\s+`?([^`\s]+)`?\s*(?:\(.*\))?\s*$/);
|
||||||
|
if (m) patterns.push(m[1]);
|
||||||
|
}
|
||||||
|
}
|
||||||
|
return patterns;
|
||||||
|
}
|
||||||
|
|
||||||
|
/**
|
||||||
|
* Check whether a file path matches any of the declared scope patterns.
|
||||||
|
* Reuses the existing `matchGlob` helper. Also matches if the file is
|
||||||
|
* inside a directory that's in scope (e.g., scope has `src/utils/*` and
|
||||||
|
* file is `src/utils/helpers.ts`).
|
||||||
|
*/
|
||||||
|
function matchesScope(filePath: string, scopePatterns: string[]): boolean {
|
||||||
|
for (const pattern of scopePatterns) {
|
||||||
|
if (matchGlob(filePath, pattern)) return true;
|
||||||
|
// Directory match: if pattern ends with /* or /**, check prefix
|
||||||
|
const dirPattern = pattern.replace(/\/\*+$/, "");
|
||||||
|
if (dirPattern !== pattern && filePath.startsWith(dirPattern + "/")) return true;
|
||||||
|
// Exact directory match: scope says `src/foo/` and file is inside it
|
||||||
|
if (pattern.endsWith("/") && filePath.startsWith(pattern)) return true;
|
||||||
|
// Also match if both share the same directory
|
||||||
|
const patternDir = pattern.lastIndexOf("/") >= 0 ? pattern.slice(0, pattern.lastIndexOf("/")) : "";
|
||||||
|
const fileDir = filePath.lastIndexOf("/") >= 0 ? filePath.slice(0, filePath.lastIndexOf("/")) : "";
|
||||||
|
if (patternDir && fileDir === patternDir) return true;
|
||||||
|
}
|
||||||
|
return false;
|
||||||
|
}
|
||||||
|
|
||||||
|
/**
|
||||||
|
* Validate that the diff stays within the task's declared File Scope.
|
||||||
|
* Returns warnings for out-of-scope changes, especially large deletions.
|
||||||
|
* This is a soft guardrail — warnings are logged but do not block merge.
|
||||||
|
*/
|
||||||
|
export async function validateDiffScope(
|
||||||
|
store: TaskStore,
|
||||||
|
taskId: string,
|
||||||
|
diffStat: string,
|
||||||
|
): Promise<DiffScopeResult> {
|
||||||
|
const result: DiffScopeResult = { warnings: [], outOfScopeFiles: [], largeOutOfScopeDeletions: [] };
|
||||||
|
|
||||||
|
// Parse the diffstat
|
||||||
|
const entries = parseDiffStat(diffStat);
|
||||||
|
if (entries.length === 0) return result;
|
||||||
|
|
||||||
|
// Read the task's PROMPT.md for file scope
|
||||||
|
let promptContent = "";
|
||||||
|
try {
|
||||||
|
const task = await store.getTask(taskId);
|
||||||
|
promptContent = task.prompt || "";
|
||||||
|
} catch {
|
||||||
|
return result; // can't validate without prompt
|
||||||
|
}
|
||||||
|
|
||||||
|
const scopePatterns = extractFileScope(promptContent);
|
||||||
|
if (scopePatterns.length === 0) return result; // no scope declared, skip
|
||||||
|
|
||||||
|
// Check each changed file
|
||||||
|
for (const entry of entries) {
|
||||||
|
// Skip changeset files — always allowed
|
||||||
|
if (entry.file.startsWith(".changeset/")) continue;
|
||||||
|
|
||||||
|
if (!matchesScope(entry.file, scopePatterns)) {
|
||||||
|
result.outOfScopeFiles.push(entry.file);
|
||||||
|
|
||||||
|
// Flag large deletions outside scope (>50 net deletions or 100% deletions)
|
||||||
|
const netDeletions = entry.deletions - entry.insertions;
|
||||||
|
if (netDeletions > 50 || (entry.deletions > 0 && entry.insertions === 0)) {
|
||||||
|
result.largeOutOfScopeDeletions.push({ file: entry.file, deletions: entry.deletions });
|
||||||
|
}
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
// Build warnings
|
||||||
|
if (result.largeOutOfScopeDeletions.length > 0) {
|
||||||
|
const files = result.largeOutOfScopeDeletions
|
||||||
|
.map((d) => `${d.file} (${d.deletions} deletions)`)
|
||||||
|
.join(", ");
|
||||||
|
result.warnings.push(
|
||||||
|
`⚠ SCOPE WARNING: Large deletions outside File Scope: ${files}`,
|
||||||
|
);
|
||||||
|
} else if (result.outOfScopeFiles.length > 3) {
|
||||||
|
result.warnings.push(
|
||||||
|
`⚠ SCOPE WARNING: ${result.outOfScopeFiles.length} files changed outside declared File Scope`,
|
||||||
|
);
|
||||||
|
}
|
||||||
|
|
||||||
|
return result;
|
||||||
|
}
|
||||||
|
|
||||||
/**
|
/**
|
||||||
* Get list of conflicted files from git.
|
* Get list of conflicted files from git.
|
||||||
* Runs `git diff --name-only --diff-filter=U` and returns array of file paths.
|
* Runs `git diff --name-only --diff-filter=U` and returns array of file paths.
|
||||||
@@ -609,6 +759,17 @@ export async function aiMergeTask(
|
|||||||
diffStat = "(unable to read diff)";
|
diffStat = "(unable to read diff)";
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// 4b. Validate diff scope against task's declared File Scope
|
||||||
|
try {
|
||||||
|
const scopeResult = await validateDiffScope(store, taskId, diffStat);
|
||||||
|
for (const warning of scopeResult.warnings) {
|
||||||
|
mergerLog.warn(`${taskId}: ${warning}`);
|
||||||
|
await store.logEntry(taskId, warning);
|
||||||
|
}
|
||||||
|
} catch {
|
||||||
|
// Scope validation is best-effort — never block merge on validation failure
|
||||||
|
}
|
||||||
|
|
||||||
// 5. Execute merge with retry logic
|
// 5. Execute merge with retry logic
|
||||||
await store.updateTask(taskId, { status: "merging" });
|
await store.updateTask(taskId, { status: "merging" });
|
||||||
|
|
||||||
|
|||||||
@@ -42,6 +42,8 @@ access to the codebase and can run commands to inspect code.
|
|||||||
- A bug or regression is introduced
|
- A bug or regression is introduced
|
||||||
- A critical edge case is unhandled and would cause runtime failure
|
- A critical edge case is unhandled and would cause runtime failure
|
||||||
- Backward compatibility is broken without migration
|
- Backward compatibility is broken without migration
|
||||||
|
- Code outside the task's File Scope is deleted, removed, or gutted (out-of-scope removal)
|
||||||
|
- Existing functionality is removed without a corresponding changeset explaining the removal
|
||||||
|
|
||||||
### Do NOT issue REVISE for
|
### Do NOT issue REVISE for
|
||||||
- STATUS/formatting preferences
|
- STATUS/formatting preferences
|
||||||
|
|||||||
@@ -129,6 +129,14 @@ Commits at step boundaries. All commits include the task ID:
|
|||||||
- Skip tests
|
- Skip tests
|
||||||
- Modify files outside the File Scope without good reason
|
- Modify files outside the File Scope without good reason
|
||||||
- Commit without the task ID prefix
|
- Commit without the task ID prefix
|
||||||
|
- Remove, delete, or gut modules, settings, interfaces, exports, or test files outside the File Scope
|
||||||
|
- Remove features as "cleanup" — if something seems unused, create a task via \`task_create\`
|
||||||
|
|
||||||
|
## Changeset Requirements
|
||||||
|
|
||||||
|
If this task REMOVES existing functionality (deleting modules, settings, API endpoints, or exports), a changeset file is REQUIRED:
|
||||||
|
- Create \`.changeset/{task-id}-removal.md\` explaining what was removed and why
|
||||||
|
- This is mandatory for any net-negative change (more deletions than additions to existing files)
|
||||||
\`\`\`
|
\`\`\`
|
||||||
|
|
||||||
## Testing requirements
|
## Testing requirements
|
||||||
|
|||||||
Reference in New Issue
Block a user