root/fusion
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

fix(engine): address PR review — remove dead skill scoping, fix memory layer placement, clarify JSDoc, sort builtin allowlist, remove redundant tests

Browse Source
This commit is contained in:
Matthew Greenberg
2026-05-10 12:33:44 -04:00
parent 616e3c7ec4
commit aed62b0dd6
12 changed files with 40 additions and 325 deletions
Download Patch File Download Diff File Expand all files Collapse all files

53
packages/engine/src/__tests__/pi-prompt-layers.test.ts
View File

@@ -1,53 +0,0 @@
import { describe, it, expect } from "vitest";
describe("createFnAgent prompt layer configuration", () => {
it("uses stable layer as systemPromptOverride when layers provided", () => {
const options = {
systemPrompt: "Stable.\n\nDynamic.",
systemPromptLayers: { stable: "Stable.", dynamic: "Dynamic." },
};
const systemPromptOverride =
options.systemPromptLayers?.stable ?? options.systemPrompt;
const appendSystemPromptOverride = options.systemPromptLayers?.dynamic
? [options.systemPromptLayers.dynamic]
: [];
expect(systemPromptOverride).toBe("Stable.");
expect(appendSystemPromptOverride).toEqual(["Dynamic."]);
});
it("falls back to full systemPrompt when layers not provided", () => {
const options = {
systemPrompt: "Full prompt.",
systemPromptLayers: undefined as
| { stable: string; dynamic: string }
| undefined,
};
const systemPromptOverride =
options.systemPromptLayers?.stable ?? options.systemPrompt;
const appendSystemPromptOverride = options.systemPromptLayers?.dynamic
? [options.systemPromptLayers.dynamic]
: [];
expect(systemPromptOverride).toBe("Full prompt.");
expect(appendSystemPromptOverride).toEqual([]);
});
it("handles empty dynamic layer", () => {
const options = {
systemPrompt: "Stable.",
systemPromptLayers: { stable: "Stable.", dynamic: "" },
};
const systemPromptOverride =
options.systemPromptLayers?.stable ?? options.systemPrompt;
const appendSystemPromptOverride = options.systemPromptLayers?.dynamic
? [options.systemPromptLayers.dynamic]
: [];
expect(systemPromptOverride).toBe("Stable.");
expect(appendSystemPromptOverride).toEqual([]);
});
});

3
packages/engine/src/__tests__/prompt-cache-integration.test.ts
View File

@@ -7,8 +7,9 @@ describe("cross-session prompt cache integration", () => {
function simulateReviewerSession(sessionIndex: number): SystemPromptLayers {
return buildPromptLayers({
basePrompt: REVIEWER_SYSTEM_PROMPT + MEMORY_INSTRUCTIONS,
basePrompt: REVIEWER_SYSTEM_PROMPT,
agentInstructions: `Session ${sessionIndex}: custom instructions that vary per agent.`,
memorySection: MEMORY_INSTRUCTIONS,
pluginContributions: sessionIndex % 2 === 0
? "## Plugin: lint\n\nCheck lint rules."
: "",

27
packages/engine/src/__tests__/prompt-layers-backward-compat.test.ts
View File

@@ -83,30 +83,29 @@ describe("prompt layers backward compatibility", () => {
});
describe("reviewer assembly pattern", () => {
it("reproduces the reviewer prompt assembly", () => {
it("produces the expected reviewer prompt with memory in dynamic layer", () => {
const basePrompt = "You are an independent code and plan reviewer.";
const memoryInstructions = "\n## Memory\n\nUse fn_memory_search.";
const agentInstructions = "Focus on security.";
const plugins = "## Plugin: lint\n\nCheck eslint.";
// Old reviewer pattern:
// 1. buildSystemPromptWithInstructions(base + memory, instructions)
// 2. if plugins: concatenate
const oldSystemPrompt = buildSystemPromptWithInstructions(
basePrompt + memoryInstructions,
agentInstructions,
);
const oldResult = `${oldSystemPrompt}\n\n${plugins}`;
// New reviewer pattern:
// New reviewer pattern: memory goes in dynamic layer (not stable) so
// the stable prefix is byte-identical across sessions even when memory
// changes mid-task.
const layers = buildPromptLayers({
basePrompt: basePrompt + memoryInstructions,
basePrompt,
agentInstructions,
memorySection: memoryInstructions,
pluginContributions: plugins,
});
const newResult = collapsePromptLayers(layers);
const result = collapsePromptLayers(layers);
expect(newResult).toBe(oldResult);
// Base prompt is the stable layer
expect(layers.stable).toBe(basePrompt);
// Dynamic layer contains instructions, memory, and plugins
expect(result).toContain(agentInstructions);
expect(result).toContain("Memory");
expect(result).toContain(plugins);
});
});

18
packages/engine/src/__tests__/reviewer-prompt-layers.test.ts
View File

@@ -5,26 +5,30 @@ describe("reviewer prompt layering", () => {
const REVIEWER_BASE = "You are an independent code and plan reviewer.";
const MEMORY_INSTRUCTIONS = "\n## Memory\n\nUse fn_memory_search to look up context.";
it("puts base prompt + memory instructions in stable layer", () => {
it("puts base prompt in stable layer and memory in dynamic layer", () => {
const layers = buildPromptLayers({
basePrompt: REVIEWER_BASE + MEMORY_INSTRUCTIONS,
basePrompt: REVIEWER_BASE,
agentInstructions: "Custom reviewer guidance.",
memorySection: MEMORY_INSTRUCTIONS,
pluginContributions: "## Plugin: lint\n\nCheck lint.",
});
expect(layers.stable).toBe(REVIEWER_BASE + MEMORY_INSTRUCTIONS);
expect(layers.stable).not.toContain("Custom reviewer guidance");
expect(layers.stable).not.toContain("lint");
expect(layers.stable).toBe(REVIEWER_BASE);
expect(layers.stable).not.toContain("Memory");
expect(layers.dynamic).toContain("Memory");
expect(layers.dynamic).toContain("Custom reviewer guidance");
});
it("produces identical stable layer across simulated sessions", () => {
const layers1 = buildPromptLayers({
basePrompt: REVIEWER_BASE + MEMORY_INSTRUCTIONS,
basePrompt: REVIEWER_BASE,
agentInstructions: "Session 1 instructions.",
memorySection: MEMORY_INSTRUCTIONS,
});
const layers2 = buildPromptLayers({
basePrompt: REVIEWER_BASE + MEMORY_INSTRUCTIONS,
basePrompt: REVIEWER_BASE,
agentInstructions: "Session 2 instructions.",
memorySection: MEMORY_INSTRUCTIONS,
});
expect(layers1.stable).toBe(layers2.stable);

20
packages/engine/src/__tests__/session-helpers-layers.test.ts
View File

@@ -1,20 +0,0 @@
import { describe, it, expect } from "vitest";
import type { ResolvedSessionOptions } from "../agent-session-helpers.js";
import type { SystemPromptLayers } from "../prompt-layers.js";
describe("ResolvedSessionOptions layer forwarding", () => {
it("includes systemPromptLayers in the type", () => {
const layers: SystemPromptLayers = {
stable: "Stable prefix.",
dynamic: "Dynamic suffix.",
};
const options: Partial<ResolvedSessionOptions> = {
systemPrompt: "Stable prefix.\n\nDynamic suffix.",
systemPromptLayers: layers,
};
expect(options.systemPromptLayers).toBeDefined();
expect(options.systemPromptLayers!.stable).toBe("Stable prefix.");
});
});

39
packages/engine/src/__tests__/skill-resolver-scoping.test.ts
View File

@@ -1,39 +0,0 @@
import { describe, it, expect } from "vitest";
import { getSkillPurposeFilter } from "../skill-resolver.js";
describe("getSkillPurposeFilter", () => {
it("returns a pass-all filter for executor sessions", () => {
const filter = getSkillPurposeFilter("executor");
expect(filter("any-skill")).toBe(true);
expect(filter("fusion")).toBe(true);
expect(filter("deployment")).toBe(true);
});
it("filters to review-relevant skills for reviewer sessions", () => {
const filter = getSkillPurposeFilter("reviewer");
expect(filter("code-review")).toBe(true);
expect(filter("security-review")).toBe(true);
expect(filter("review")).toBe(true);
expect(filter("deployment")).toBe(false);
expect(filter("fusion")).toBe(false);
});
it("returns a pass-all filter for unknown session purposes", () => {
const filter = getSkillPurposeFilter("unknown-purpose");
expect(filter("any-skill")).toBe(true);
});
it("filters to minimal skills for heartbeat sessions", () => {
const filter = getSkillPurposeFilter("heartbeat");
expect(filter("monitoring")).toBe(true);
expect(filter("heartbeat")).toBe(true);
expect(filter("fusion")).toBe(false);
expect(filter("code-review")).toBe(false);
});
it("returns a pass-all filter for triage sessions", () => {
const filter = getSkillPurposeFilter("triage");
expect(filter("any-skill")).toBe(true);
expect(filter("fusion")).toBe(true);
});
});

104
packages/engine/src/__tests__/skill-scoping-integration.test.ts
View File

@@ -1,104 +0,0 @@
import { describe, it, expect } from "vitest";
import { getSkillPurposeFilter } from "../skill-resolver.js";
/**
* Integration-style tests for getSkillPurposeFilter that verify filtering
* against a realistic set of skill names typical of a Fusion project.
*
* The existing skill-resolver-scoping.test.ts tests individual skill names
* in isolation. These tests verify the aggregate filtering behavior: how
* many skills each purpose loads from a full project skill set, and that
* the filter produces sensible subsets.
*/
describe("getSkillPurposeFilter integration", () => {
// Simulate a realistic set of skill names from a Fusion project
const ALL_SKILLS = [
"fusion",
"code-review",
"security-review",
"review",
"deployment",
"task-management",
"monitoring",
"heartbeat",
"research",
"web-search",
];
it("executor loads all skills", () => {
const filter = getSkillPurposeFilter("executor");
const loaded = ALL_SKILLS.filter(filter);
expect(loaded).toEqual(ALL_SKILLS);
});
it("reviewer loads only review-related skills", () => {
const filter = getSkillPurposeFilter("reviewer");
const loaded = ALL_SKILLS.filter(filter);
expect(loaded).toEqual(["code-review", "security-review", "review"]);
expect(loaded).not.toContain("fusion");
expect(loaded).not.toContain("deployment");
expect(loaded).not.toContain("monitoring");
});
it("heartbeat loads only monitoring-related skills", () => {
const filter = getSkillPurposeFilter("heartbeat");
const loaded = ALL_SKILLS.filter(filter);
expect(loaded).toEqual(["monitoring", "heartbeat"]);
expect(loaded).not.toContain("fusion");
expect(loaded).not.toContain("code-review");
});
it("triage loads all skills (same as executor)", () => {
const filter = getSkillPurposeFilter("triage");
const loaded = ALL_SKILLS.filter(filter);
expect(loaded).toEqual(ALL_SKILLS);
});
it("reviewer filters out the majority of skills", () => {
const filter = getSkillPurposeFilter("reviewer");
const loaded = ALL_SKILLS.filter(filter);
// Reviewer should load significantly fewer skills than total
expect(loaded.length).toBeLessThan(ALL_SKILLS.length / 2);
});
it("heartbeat filters to a minimal subset", () => {
const filter = getSkillPurposeFilter("heartbeat");
const loaded = ALL_SKILLS.filter(filter);
// Heartbeat should have even fewer than reviewer
expect(loaded.length).toBeLessThanOrEqual(2);
});
it("unknown purpose passes all skills through (safe fallback)", () => {
const filter = getSkillPurposeFilter("some-future-purpose");
const loaded = ALL_SKILLS.filter(filter);
expect(loaded).toEqual(ALL_SKILLS);
});
it("reviewer and heartbeat produce disjoint sets from the same input", () => {
const reviewerFilter = getSkillPurposeFilter("reviewer");
const heartbeatFilter = getSkillPurposeFilter("heartbeat");
const reviewerSkills = ALL_SKILLS.filter(reviewerFilter);
const heartbeatSkills = ALL_SKILLS.filter(heartbeatFilter);
// No skill should appear in both reviewer and heartbeat sets
const overlap = reviewerSkills.filter((s) => heartbeatSkills.includes(s));
expect(overlap).toEqual([]);
});
it("executor is a superset of all other purpose filters", () => {
const executorFilter = getSkillPurposeFilter("executor");
const reviewerFilter = getSkillPurposeFilter("reviewer");
const heartbeatFilter = getSkillPurposeFilter("heartbeat");
const executorSkills = new Set(ALL_SKILLS.filter(executorFilter));
const reviewerSkills = ALL_SKILLS.filter(reviewerFilter);
const heartbeatSkills = ALL_SKILLS.filter(heartbeatFilter);
for (const skill of reviewerSkills) {
expect(executorSkills.has(skill)).toBe(true);
}
for (const skill of heartbeatSkills) {
expect(executorSkills.has(skill)).toBe(true);
}
});
});

35
packages/engine/src/__tests__/tool-ordering.test.ts
View File

@@ -1,35 +0,0 @@
import { describe, it, expect } from "vitest";
/**
* Verifies that tools are sorted deterministically by name.
* This is critical for prompt caching — tool schemas are part of the
* API request, and reordering them breaks cache prefix matching.
*/
describe("deterministic tool ordering", () => {
it("sorts tools alphabetically by name", () => {
const tools = [
{ name: "write", execute: async () => {} },
{ name: "bash", execute: async () => {} },
{ name: "read", execute: async () => {} },
{ name: "edit", execute: async () => {} },
];
const sorted = [...tools].sort((a, b) => a.name.localeCompare(b.name));
expect(sorted.map((t) => t.name)).toEqual(["bash", "edit", "read", "write"]);
});
it("is stable across repeated sorts", () => {
const tools = [
{ name: "grep" },
{ name: "bash" },
{ name: "find" },
{ name: "read" },
];
const sorted1 = [...tools].sort((a, b) => a.name.localeCompare(b.name));
const sorted2 = [...tools].sort((a, b) => a.name.localeCompare(b.name));
expect(sorted1.map((t) => t.name)).toEqual(sorted2.map((t) => t.name));
});
});

2
packages/engine/src/pi.ts
View File

@@ -1787,7 +1787,7 @@ export async function createFnAgent(options: AgentOptions): Promise<AgentResult>
...customToolList.map((tool) => tool.name),
...options.builtinToolsAllowlist,
]),
];
].sort();
}
return createAgentSession(createSessionOptions);

8
packages/engine/src/reviewer.ts
View File

@@ -410,8 +410,9 @@ export async function reviewStep(
: "";
// Build structured layers for cross-session prompt caching.
// The stable layer (base prompt + memory instructions) is byte-identical
// across all reviewer sessions in this task, enabling cache hits.
// The stable layer (base prompt only) is byte-identical across all
// reviewer sessions in this task, enabling cache hits. Memory goes
// into the dynamic layer because it can change between sessions.
const reviewerPluginContributions = buildPluginPromptSection(
"reviewer",
options.pluginRunner,
@@ -421,8 +422,9 @@ export async function reviewStep(
}
const layers = buildPromptLayers({
basePrompt: reviewerBasePrompt + memorySection,
basePrompt: reviewerBasePrompt,
agentInstructions: reviewerInstructions,
memorySection,
pluginContributions: reviewerPluginContributions,
});

14
packages/engine/src/session-token-usage.ts
View File

@@ -92,13 +92,15 @@ export async function accumulateSessionTokenUsage(
}
/**
* Compute the cache hit ratio: the fraction of input tokens served from
* cache. Returns a number in [0, 1]. Useful for measuring the effectiveness
* of prompt caching optimizations.
* Compute the cache hit ratio: the fraction of effective input tokens served
* from cache. Returns a number in [0, 1]. Useful for measuring the
* effectiveness of prompt caching optimizations.
*
* @param inputTokens - Non-cached input tokens (includes cache-write tokens)
* @param cachedTokens - Tokens read from cache
* @returns Cache hit ratio in [0, 1], or 0 if no tokens used
* @param inputTokens - Non-cached input tokens (NOT including cache-write tokens;
* use only `tokens.input` here, not `tokens.input + tokens.cacheWrite`)
* @param cachedTokens - Tokens read from cache (`tokens.cacheRead`)
* @returns Cache hit ratio in [0, 1] matching Anthropic console's `cache_read / input` metric,
* or 0 if no tokens used
*/
export function computeCacheHitRatio(
inputTokens: number,

42
packages/engine/src/skill-resolver.ts
View File

@@ -485,45 +485,3 @@ export function createSkillsOverrideFromSelection(
};
}
/**
* Skills that are relevant to each session purpose. Executors get all
* skills (they do the implementation work). Other roles get a scoped
* subset to avoid loading 71KB of reference material for every session.
*
* The filter matches against the skill's directory name (the last path
* segment before SKILL.md).
*/
const REVIEWER_SKILL_ALLOWLIST = new Set([
"code-review",
"security-review",
"review",
]);
const HEARTBEAT_SKILL_ALLOWLIST = new Set([
"monitoring",
"heartbeat",
]);
/**
* Return a filter function that decides whether a skill (by name) should
* be included in a session of the given purpose.
*
* - "executor" and "triage": all skills (pass-through)
* - "reviewer": only review-related skills
* - "heartbeat": only monitoring-related skills
* - unknown: all skills (safe fallback)
*/
export function getSkillPurposeFilter(
sessionPurpose: string,
): (skillName: string) => boolean {
switch (sessionPurpose) {
case "reviewer":
return (name) => REVIEWER_SKILL_ALLOWLIST.has(name);
case "heartbeat":
return (name) => HEARTBEAT_SKILL_ALLOWLIST.has(name);
case "executor":
case "triage":
default:
return () => true;
}
}