fix: address latest Greptile runtime review
This commit is contained in:
parent
55b26ed590
commit
c8f8f6752f
7 changed files with 177 additions and 12 deletions
|
|
@ -73,6 +73,7 @@ export const updateAgentSchema = createAgentSchema
|
|||
.partial()
|
||||
.extend({
|
||||
permissions: z.never().optional(),
|
||||
replaceAdapterConfig: z.boolean().optional(),
|
||||
status: z.enum(AGENT_STATUSES).optional(),
|
||||
spentMonthlyCents: z.number().int().nonnegative().optional(),
|
||||
});
|
||||
|
|
|
|||
|
|
@ -274,4 +274,45 @@ describe("agent instructions bundle routes", () => {
|
|||
expect.any(Object),
|
||||
);
|
||||
});
|
||||
|
||||
it("replaces adapter config when replaceAdapterConfig is true", async () => {
|
||||
mockAgentService.getById.mockResolvedValue({
|
||||
...makeAgent(),
|
||||
adapterType: "codex_local",
|
||||
adapterConfig: {
|
||||
instructionsBundleMode: "managed",
|
||||
instructionsRootPath: "/tmp/agent-1",
|
||||
instructionsEntryFile: "AGENTS.md",
|
||||
instructionsFilePath: "/tmp/agent-1/AGENTS.md",
|
||||
model: "gpt-5.4",
|
||||
},
|
||||
});
|
||||
|
||||
const res = await request(createApp())
|
||||
.patch("/api/agents/11111111-1111-4111-8111-111111111111?companyId=company-1")
|
||||
.send({
|
||||
replaceAdapterConfig: true,
|
||||
adapterConfig: {
|
||||
command: "codex --profile engineer",
|
||||
},
|
||||
});
|
||||
|
||||
expect(res.status, JSON.stringify(res.body)).toBe(200);
|
||||
expect(mockAgentService.update).toHaveBeenCalledWith(
|
||||
"11111111-1111-4111-8111-111111111111",
|
||||
expect.objectContaining({
|
||||
adapterConfig: expect.objectContaining({
|
||||
command: "codex --profile engineer",
|
||||
}),
|
||||
}),
|
||||
expect.any(Object),
|
||||
);
|
||||
expect(res.body.adapterConfig).toMatchObject({
|
||||
command: "codex --profile engineer",
|
||||
});
|
||||
expect(res.body.adapterConfig.instructionsBundleMode).toBeUndefined();
|
||||
expect(res.body.adapterConfig.instructionsRootPath).toBeUndefined();
|
||||
expect(res.body.adapterConfig.instructionsEntryFile).toBeUndefined();
|
||||
expect(res.body.adapterConfig.instructionsFilePath).toBeUndefined();
|
||||
});
|
||||
});
|
||||
|
|
|
|||
|
|
@ -236,6 +236,88 @@ describe("agent instructions service", () => {
|
|||
expect(exported.files).toEqual({ "AGENTS.md": "# Managed Agent\n" });
|
||||
});
|
||||
|
||||
it("heals stale managed metadata when writing bundle files", async () => {
|
||||
const paperclipHome = await makeTempDir("paperclip-agent-instructions-heal-write-");
|
||||
const staleRoot = await makeTempDir("paperclip-agent-instructions-heal-write-stale-");
|
||||
cleanupDirs.add(paperclipHome);
|
||||
cleanupDirs.add(staleRoot);
|
||||
process.env.PAPERCLIP_HOME = paperclipHome;
|
||||
process.env.PAPERCLIP_INSTANCE_ID = "test-instance";
|
||||
|
||||
const managedRoot = path.join(
|
||||
paperclipHome,
|
||||
"instances",
|
||||
"test-instance",
|
||||
"companies",
|
||||
"company-1",
|
||||
"agents",
|
||||
"agent-1",
|
||||
"instructions",
|
||||
);
|
||||
await fs.mkdir(path.join(managedRoot, "docs"), { recursive: true });
|
||||
await fs.writeFile(path.join(managedRoot, "AGENTS.md"), "# Managed Agent\n", "utf8");
|
||||
|
||||
const svc = agentInstructionsService();
|
||||
const agent = makeAgent({
|
||||
instructionsBundleMode: "managed",
|
||||
instructionsRootPath: staleRoot,
|
||||
instructionsEntryFile: "docs/MISSING.md",
|
||||
instructionsFilePath: path.join(staleRoot, "docs", "MISSING.md"),
|
||||
});
|
||||
|
||||
const result = await svc.writeFile(agent, "docs/TOOLS.md", "## Tools\n");
|
||||
|
||||
expect(result.adapterConfig).toMatchObject({
|
||||
instructionsBundleMode: "managed",
|
||||
instructionsRootPath: managedRoot,
|
||||
instructionsEntryFile: "AGENTS.md",
|
||||
instructionsFilePath: path.join(managedRoot, "AGENTS.md"),
|
||||
});
|
||||
await expect(fs.readFile(path.join(managedRoot, "docs", "TOOLS.md"), "utf8")).resolves.toBe("## Tools\n");
|
||||
});
|
||||
|
||||
it("heals stale managed metadata when deleting bundle files", async () => {
|
||||
const paperclipHome = await makeTempDir("paperclip-agent-instructions-heal-delete-");
|
||||
const staleRoot = await makeTempDir("paperclip-agent-instructions-heal-delete-stale-");
|
||||
cleanupDirs.add(paperclipHome);
|
||||
cleanupDirs.add(staleRoot);
|
||||
process.env.PAPERCLIP_HOME = paperclipHome;
|
||||
process.env.PAPERCLIP_INSTANCE_ID = "test-instance";
|
||||
|
||||
const managedRoot = path.join(
|
||||
paperclipHome,
|
||||
"instances",
|
||||
"test-instance",
|
||||
"companies",
|
||||
"company-1",
|
||||
"agents",
|
||||
"agent-1",
|
||||
"instructions",
|
||||
);
|
||||
await fs.mkdir(path.join(managedRoot, "docs"), { recursive: true });
|
||||
await fs.writeFile(path.join(managedRoot, "AGENTS.md"), "# Managed Agent\n", "utf8");
|
||||
await fs.writeFile(path.join(managedRoot, "docs", "TOOLS.md"), "## Tools\n", "utf8");
|
||||
|
||||
const svc = agentInstructionsService();
|
||||
const agent = makeAgent({
|
||||
instructionsBundleMode: "managed",
|
||||
instructionsRootPath: staleRoot,
|
||||
instructionsEntryFile: "docs/MISSING.md",
|
||||
instructionsFilePath: path.join(staleRoot, "docs", "MISSING.md"),
|
||||
});
|
||||
|
||||
const result = await svc.deleteFile(agent, "docs/TOOLS.md");
|
||||
|
||||
expect(result.adapterConfig).toMatchObject({
|
||||
instructionsBundleMode: "managed",
|
||||
instructionsRootPath: managedRoot,
|
||||
instructionsEntryFile: "AGENTS.md",
|
||||
instructionsFilePath: path.join(managedRoot, "AGENTS.md"),
|
||||
});
|
||||
await expect(fs.stat(path.join(managedRoot, "docs", "TOOLS.md"))).rejects.toThrow();
|
||||
expect(result.bundle.files.map((file) => file.path)).toEqual(["AGENTS.md"]);
|
||||
});
|
||||
|
||||
it("recovers the managed bundle when stale root metadata is present but mode is missing", async () => {
|
||||
const paperclipHome = await makeTempDir("paperclip-agent-instructions-partial-managed-");
|
||||
const staleRoot = await makeTempDir("paperclip-agent-instructions-partial-root-");
|
||||
|
|
|
|||
|
|
@ -149,6 +149,8 @@ describe("execution workspace policy helpers", () => {
|
|||
expect(issueExecutionWorkspaceModeForPersistedWorkspace("shared_workspace")).toBe("shared_workspace");
|
||||
expect(issueExecutionWorkspaceModeForPersistedWorkspace("adapter_managed")).toBe("agent_default");
|
||||
expect(issueExecutionWorkspaceModeForPersistedWorkspace("cloud_sandbox")).toBe("agent_default");
|
||||
expect(issueExecutionWorkspaceModeForPersistedWorkspace(null)).toBe("agent_default");
|
||||
expect(issueExecutionWorkspaceModeForPersistedWorkspace(undefined)).toBe("agent_default");
|
||||
});
|
||||
|
||||
it("disables project execution workspace policy when the instance flag is off", () => {
|
||||
|
|
|
|||
|
|
@ -1702,6 +1702,8 @@ export function agentRoutes(db: Db) {
|
|||
}
|
||||
|
||||
const patchData = { ...(req.body as Record<string, unknown>) };
|
||||
const replaceAdapterConfig = patchData.replaceAdapterConfig === true;
|
||||
delete patchData.replaceAdapterConfig;
|
||||
if (Object.prototype.hasOwnProperty.call(patchData, "adapterConfig")) {
|
||||
const adapterConfig = asRecord(patchData.adapterConfig);
|
||||
if (!adapterConfig) {
|
||||
|
|
@ -1729,8 +1731,17 @@ export function agentRoutes(db: Db) {
|
|||
const requestedAdapterConfig = Object.prototype.hasOwnProperty.call(patchData, "adapterConfig")
|
||||
? (asRecord(patchData.adapterConfig) ?? {})
|
||||
: null;
|
||||
if (
|
||||
requestedAdapterConfig
|
||||
&& replaceAdapterConfig
|
||||
&& KNOWN_INSTRUCTIONS_BUNDLE_KEYS.some((key) =>
|
||||
existingAdapterConfig[key] !== undefined && requestedAdapterConfig[key] === undefined,
|
||||
)
|
||||
) {
|
||||
await assertCanManageInstructionsPath(req, existing);
|
||||
}
|
||||
let rawEffectiveAdapterConfig = requestedAdapterConfig ?? existingAdapterConfig;
|
||||
if (requestedAdapterConfig && !changingAdapterType) {
|
||||
if (requestedAdapterConfig && !changingAdapterType && !replaceAdapterConfig) {
|
||||
rawEffectiveAdapterConfig = { ...existingAdapterConfig, ...requestedAdapterConfig };
|
||||
}
|
||||
if (changingAdapterType) {
|
||||
|
|
|
|||
|
|
@ -383,6 +383,36 @@ function applyBundleConfig(
|
|||
return next;
|
||||
}
|
||||
|
||||
function buildPersistedBundleConfig(
|
||||
derived: BundleState,
|
||||
current: BundleState,
|
||||
options?: { clearLegacyPromptTemplate?: boolean },
|
||||
): Record<string, unknown> {
|
||||
const currentRootPath = current.rootPath ? path.resolve(current.rootPath) : null;
|
||||
const derivedRootPath = derived.rootPath ? path.resolve(derived.rootPath) : null;
|
||||
const configMatchesRecoveredState =
|
||||
derived.mode === current.mode
|
||||
&& derivedRootPath !== null
|
||||
&& currentRootPath !== null
|
||||
&& derivedRootPath === currentRootPath
|
||||
&& derived.entryFile === current.entryFile;
|
||||
|
||||
if (configMatchesRecoveredState && !options?.clearLegacyPromptTemplate) {
|
||||
return current.config;
|
||||
}
|
||||
|
||||
if (!current.rootPath || !current.mode) {
|
||||
return current.config;
|
||||
}
|
||||
|
||||
return applyBundleConfig(current.config, {
|
||||
mode: current.mode,
|
||||
rootPath: current.rootPath,
|
||||
entryFile: current.entryFile,
|
||||
clearLegacyPromptTemplate: options?.clearLegacyPromptTemplate,
|
||||
});
|
||||
}
|
||||
|
||||
async function writeBundleFiles(
|
||||
rootPath: string,
|
||||
files: Record<string, string>,
|
||||
|
|
@ -481,14 +511,7 @@ export function agentInstructionsService() {
|
|||
const derived = deriveBundleState(agent);
|
||||
const current = await recoverManagedBundleState(agent, derived);
|
||||
if (current.rootPath && current.mode) {
|
||||
const adapterConfig = derived.rootPath
|
||||
? current.config
|
||||
: applyBundleConfig(current.config, {
|
||||
mode: current.mode,
|
||||
rootPath: current.rootPath,
|
||||
entryFile: current.entryFile,
|
||||
clearLegacyPromptTemplate: options?.clearLegacyPromptTemplate,
|
||||
});
|
||||
const adapterConfig = buildPersistedBundleConfig(derived, current, options);
|
||||
return {
|
||||
adapterConfig,
|
||||
state: deriveBundleState({ ...agent, adapterConfig }),
|
||||
|
|
@ -612,7 +635,8 @@ export function agentInstructionsService() {
|
|||
bundle: AgentInstructionsBundle;
|
||||
adapterConfig: Record<string, unknown>;
|
||||
}> {
|
||||
const state = await recoverManagedBundleState(agent, deriveBundleState(agent));
|
||||
const derived = deriveBundleState(agent);
|
||||
const state = await recoverManagedBundleState(agent, derived);
|
||||
if (relativePath === LEGACY_PROMPT_TEMPLATE_PATH) {
|
||||
throw unprocessable("Cannot delete the legacy promptTemplate pseudo-file");
|
||||
}
|
||||
|
|
@ -623,8 +647,9 @@ export function agentInstructionsService() {
|
|||
}
|
||||
const absolutePath = resolvePathWithinRoot(state.rootPath, normalizedPath);
|
||||
await fs.rm(absolutePath, { force: true });
|
||||
const bundle = await getBundle(agent);
|
||||
return { bundle, adapterConfig: state.config };
|
||||
const adapterConfig = buildPersistedBundleConfig(derived, state);
|
||||
const bundle = await getBundle({ ...agent, adapterConfig });
|
||||
return { bundle, adapterConfig };
|
||||
}
|
||||
|
||||
async function exportFiles(agent: AgentLike): Promise<{
|
||||
|
|
|
|||
|
|
@ -135,6 +135,9 @@ export function defaultIssueExecutionWorkspaceSettingsForProject(
|
|||
export function issueExecutionWorkspaceModeForPersistedWorkspace(
|
||||
mode: string | null | undefined,
|
||||
): IssueExecutionWorkspaceSettings["mode"] {
|
||||
if (mode === null || mode === undefined) {
|
||||
return "agent_default";
|
||||
}
|
||||
if (mode === "isolated_workspace" || mode === "operator_branch" || mode === "shared_workspace") {
|
||||
return mode;
|
||||
}
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue