mirror of
https://github.com/paperclipai/paperclip.git
synced 2026-10-26 10:26:26 +01:00
Address built-in agent review feedback
This commit is contained in:
1 parent
d6f73eb1cf
commit
bce8dacb21
8 files changed
+86
-28
No files matched your search
@@ -91,6 +91,7 @@ export type CreateAgent = z.infer<typeof createAgentSchema>;
|
||||
export const builtInAgentProvisionSchema = z.object({
|
||||
adapterType: agentAdapterTypeSchema.optional(),
|
||||
adapterConfig: adapterConfigSchema.optional(),
|
||||
budgetMonthlyCents: z.number().int().nonnegative().optional(),
|
||||
}).strict();
|
||||
|
||||
export type BuiltInAgentProvision = z.infer<typeof builtInAgentProvisionSchema>;
|
||||
|
||||
@@ -115,6 +115,7 @@ describe("built-in agent routes", () => {
|
||||
expect(res.status, JSON.stringify(res.body)).toBe(200);
|
||||
expect(mockBuiltInAgentService.list).toHaveBeenCalledWith(companyId);
|
||||
expect(res.body).toEqual([expect.objectContaining({ status: "ready", agentId })]);
|
||||
expect(res.body[0].agent.adapterConfig).toEqual({});
|
||||
});
|
||||
|
||||
it("denies list requests outside the actor company boundary", async () => {
|
||||
@@ -143,7 +144,7 @@ describe("built-in agent routes", () => {
|
||||
|
||||
const res = await request(app)
|
||||
.post(`/api/companies/${companyId}/built-in-agents/briefs/provision`)
|
||||
.send({ adapterType: "codex_local", adapterConfig: { model: "gpt-5.4" } });
|
||||
.send({ adapterType: "codex_local", adapterConfig: { model: "gpt-5.4" }, budgetMonthlyCents: 5000 });
|
||||
|
||||
expect(res.status, JSON.stringify(res.body)).toBe(200);
|
||||
expect(mockAccessService.decide).toHaveBeenCalledWith({
|
||||
@@ -157,6 +158,7 @@ describe("built-in agent routes", () => {
|
||||
{
|
||||
adapterType: "codex_local",
|
||||
adapterConfig: { model: "gpt-5.4" },
|
||||
budgetMonthlyCents: 5000,
|
||||
},
|
||||
{ requestedByAgentId: null, requestedByUserId: "board-user" },
|
||||
);
|
||||
|
||||
@@ -6,6 +6,7 @@ import {
|
||||
agentConfigRevisions,
|
||||
agents,
|
||||
approvals,
|
||||
budgetPolicies,
|
||||
companies,
|
||||
createDb,
|
||||
} from "@paperclipai/db";
|
||||
@@ -52,6 +53,7 @@ describeEmbeddedPostgres("built-in agents", () => {
|
||||
await db.delete(activityLog);
|
||||
await db.delete(approvals);
|
||||
await db.delete(agents);
|
||||
await db.delete(budgetPolicies);
|
||||
await db.delete(companies);
|
||||
});
|
||||
|
||||
@@ -142,6 +144,7 @@ describeEmbeddedPostgres("built-in agents", () => {
|
||||
const result = await builtIns.provision(companyId, "briefs", {
|
||||
adapterType: "process",
|
||||
adapterConfig: { command: "echo safe" },
|
||||
budgetMonthlyCents: 5000,
|
||||
}, { requestedByUserId: "board-user" });
|
||||
|
||||
expect(result.state).toMatchObject({
|
||||
@@ -152,6 +155,7 @@ describeEmbeddedPostgres("built-in agents", () => {
|
||||
status: "pending_approval",
|
||||
adapterType: "process",
|
||||
adapterConfig: { command: "echo safe" },
|
||||
budgetMonthlyCents: 5000,
|
||||
},
|
||||
});
|
||||
expect(result.approval).toMatchObject({
|
||||
@@ -165,6 +169,7 @@ describeEmbeddedPostgres("built-in agents", () => {
|
||||
role: "general",
|
||||
adapterType: "process",
|
||||
adapterConfig: { command: "echo safe" },
|
||||
budgetMonthlyCents: 5000,
|
||||
agentId: result.state.agentId,
|
||||
sourceBuiltInAgentKey: "briefs",
|
||||
featureKeys: ["briefs"],
|
||||
@@ -205,7 +210,7 @@ describeEmbeddedPostgres("built-in agents", () => {
|
||||
await expect(builtIns.get(companyId, "briefs")).resolves.toMatchObject({
|
||||
status: "ready",
|
||||
agentId: result.state.agentId,
|
||||
agent: { status: "idle", adapterType: "process", adapterConfig: { command: "echo safe" } },
|
||||
agent: { status: "idle", adapterType: "process", adapterConfig: { command: "echo safe" }, budgetMonthlyCents: 5000 },
|
||||
});
|
||||
});
|
||||
|
||||
|
||||
@@ -7,6 +7,19 @@ import { accessService, logActivity } from "../services/index.js";
|
||||
import { builtInAgentService } from "../services/built-in-agents.js";
|
||||
import { authorizationDeniedDetails } from "../services/authorization.js";
|
||||
import { assertCompanyAccess, getActorInfo } from "./authz.js";
|
||||
import type { BuiltInAgentState } from "../services/built-in-agents.js";
|
||||
|
||||
function redactBuiltInAgentListState(state: BuiltInAgentState): BuiltInAgentState {
|
||||
if (!state.agent) return state;
|
||||
return {
|
||||
...state,
|
||||
agent: {
|
||||
...state.agent,
|
||||
adapterConfig: {},
|
||||
runtimeConfig: {},
|
||||
},
|
||||
};
|
||||
}
|
||||
|
||||
export function builtInAgentRoutes(db: Db) {
|
||||
const router = Router();
|
||||
@@ -56,7 +69,8 @@ export function builtInAgentRoutes(db: Db) {
|
||||
router.get("/companies/:companyId/built-in-agents", async (req, res) => {
|
||||
const companyId = req.params.companyId as string;
|
||||
assertCompanyAccess(req, companyId);
|
||||
res.json(await svc.list(companyId));
|
||||
const states = await svc.list(companyId);
|
||||
res.json(states.map(redactBuiltInAgentListState));
|
||||
});
|
||||
|
||||
router.post(
|
||||
|
||||
@@ -35,6 +35,7 @@ export interface BuiltInAgentState {
|
||||
export interface BuiltInAgentProvisionInput {
|
||||
adapterType?: string;
|
||||
adapterConfig?: Record<string, unknown>;
|
||||
budgetMonthlyCents?: number;
|
||||
}
|
||||
|
||||
export interface BuiltInAgentProvisionActor {
|
||||
@@ -224,7 +225,7 @@ function definitionPatch(definition: BuiltInAgentDefinition, input: BuiltInAgent
|
||||
capabilities: definition.shortPurpose,
|
||||
adapterType,
|
||||
adapterConfig: input.adapterConfig ?? {},
|
||||
budgetMonthlyCents: definition.defaultBudgetMonthlyCents ?? 0,
|
||||
budgetMonthlyCents: input.budgetMonthlyCents ?? definition.defaultBudgetMonthlyCents ?? 0,
|
||||
};
|
||||
}
|
||||
|
||||
@@ -314,6 +315,9 @@ export function builtInAgentService(db: Db) {
|
||||
patch.adapterType = adapterType;
|
||||
patch.adapterConfig = input.adapterConfig ?? existing.adapterConfig;
|
||||
}
|
||||
if (input.budgetMonthlyCents !== undefined) {
|
||||
patch.budgetMonthlyCents = input.budgetMonthlyCents;
|
||||
}
|
||||
const updated = await agentSvc.update(existing.id, patch, {
|
||||
allowBuiltInAgentMetadata: true,
|
||||
recordRevision: { source: "built-in-agent:ensure" },
|
||||
|
||||
@@ -41,6 +41,7 @@ export interface BuiltInAgentState {
|
||||
export interface BuiltInAgentProvisionInput {
|
||||
adapterType?: string;
|
||||
adapterConfig?: Record<string, unknown>;
|
||||
budgetMonthlyCents?: number;
|
||||
}
|
||||
|
||||
/**
|
||||
|
||||
@@ -52,7 +52,7 @@ vi.mock("@/components/agent-config-primitives", () => ({
|
||||
),
|
||||
}));
|
||||
|
||||
function makeState(): BuiltInAgentState {
|
||||
function makeState(overrides: Partial<BuiltInAgentState> = {}): BuiltInAgentState {
|
||||
return {
|
||||
definition: {
|
||||
key: "briefs",
|
||||
@@ -68,6 +68,7 @@ function makeState(): BuiltInAgentState {
|
||||
agentId: null,
|
||||
agent: null,
|
||||
pauseReason: null,
|
||||
...overrides,
|
||||
};
|
||||
}
|
||||
|
||||
@@ -91,7 +92,7 @@ describe("ConfigureBuiltInAgentModal (PAP-12978)", () => {
|
||||
const onOpenChange = vi.fn();
|
||||
const onConfigured = vi.fn();
|
||||
|
||||
async function renderModal() {
|
||||
async function renderModal(state: BuiltInAgentState = makeState()) {
|
||||
root = createRoot(container);
|
||||
const queryClient = new QueryClient({ defaultOptions: { queries: { retry: false } } });
|
||||
flushSync(() => {
|
||||
@@ -99,7 +100,7 @@ describe("ConfigureBuiltInAgentModal (PAP-12978)", () => {
|
||||
<QueryClientProvider client={queryClient}>
|
||||
<ConfigureBuiltInAgentModal
|
||||
companyId="c1"
|
||||
state={makeState()}
|
||||
state={state}
|
||||
open
|
||||
onOpenChange={onOpenChange}
|
||||
onConfigured={onConfigured}
|
||||
@@ -160,15 +161,13 @@ describe("ConfigureBuiltInAgentModal (PAP-12978)", () => {
|
||||
expect(onOpenChange).toHaveBeenCalledWith(false);
|
||||
});
|
||||
|
||||
it("patches the budget after provisioning when set", async () => {
|
||||
it("sends the budget with provisioning so approval-gated setup preserves it", async () => {
|
||||
provisionMock.mockResolvedValue({
|
||||
...makeState(),
|
||||
status: "ready",
|
||||
status: "pending_approval",
|
||||
agentId: "a1",
|
||||
// eslint-disable-next-line @typescript-eslint/no-explicit-any
|
||||
agent: { id: "a1", budgetMonthlyCents: 0 } as any,
|
||||
approval: { id: "approval-1", status: "pending" },
|
||||
});
|
||||
updateMock.mockResolvedValue({});
|
||||
await renderModal();
|
||||
|
||||
const modelInput = document.body.querySelector('[data-testid="model-input"]') as HTMLInputElement;
|
||||
@@ -192,7 +191,32 @@ describe("ConfigureBuiltInAgentModal (PAP-12978)", () => {
|
||||
await flushReact();
|
||||
|
||||
expect(provisionMock).toHaveBeenCalled();
|
||||
expect(updateMock).toHaveBeenCalledWith("a1", { budgetMonthlyCents: 5000 }, "c1");
|
||||
expect(provisionMock).toHaveBeenCalledWith("c1", "briefs", {
|
||||
adapterType: "codex_local",
|
||||
adapterConfig: { model: "gpt-5" },
|
||||
budgetMonthlyCents: 5000,
|
||||
});
|
||||
expect(updateMock).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it("does not submit non-model adapters that need command or endpoint fields", async () => {
|
||||
await renderModal(makeState({
|
||||
definition: {
|
||||
...makeState().definition,
|
||||
allowedAdapterTypes: ["process"],
|
||||
},
|
||||
}));
|
||||
|
||||
expect(document.body.textContent).toContain("can only complete model-based adapters");
|
||||
const submit = findButton("Configure");
|
||||
expect(submit).toBeTruthy();
|
||||
expect(submit!.disabled).toBe(true);
|
||||
flushSync(() => {
|
||||
submit!.dispatchEvent(new MouseEvent("click", { bubbles: true }));
|
||||
});
|
||||
await flushReact();
|
||||
|
||||
expect(provisionMock).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it("surfaces provision errors inline instead of closing", async () => {
|
||||
|
||||
@@ -32,6 +32,13 @@ function defaultAdapterType(state: BuiltInAgentState): string {
|
||||
return state.definition.allowedAdapterTypes?.[0] ?? "codex_local";
|
||||
}
|
||||
|
||||
function parseBudgetMonthlyCents(value: string): number | undefined {
|
||||
const trimmed = value.trim();
|
||||
if (!trimmed) return undefined;
|
||||
const cents = Math.round(Number(trimmed) * 100);
|
||||
return Number.isFinite(cents) && cents >= 0 ? cents : undefined;
|
||||
}
|
||||
|
||||
export interface ConfigureBuiltInAgentModalProps {
|
||||
companyId: string;
|
||||
state: BuiltInAgentState;
|
||||
@@ -75,11 +82,10 @@ export function ConfigureBuiltInAgentModal({
|
||||
// Restrict adapter choices to the registry's allow-list.
|
||||
const disabledTypes = useMemo(() => {
|
||||
const allowed = new Set(definition.allowedAdapterTypes ?? []);
|
||||
if (allowed.size === 0) return new Set<string>();
|
||||
return new Set(
|
||||
listAdapterOptions()
|
||||
.map((option) => option.value)
|
||||
.filter((value) => !allowed.has(value)),
|
||||
.filter((value) => (allowed.size > 0 && !allowed.has(value)) || !isModelBasedAdapter(value)),
|
||||
);
|
||||
}, [definition.allowedAdapterTypes]);
|
||||
|
||||
@@ -90,8 +96,11 @@ export function ConfigureBuiltInAgentModal({
|
||||
});
|
||||
const models = fetchedModels ?? [];
|
||||
|
||||
const modelRequired = isModelBasedAdapter(adapterType);
|
||||
const canSubmit = !modelRequired || model.trim().length > 0;
|
||||
const setupSupportedInModal = isModelBasedAdapter(adapterType);
|
||||
const modelRequired = setupSupportedInModal;
|
||||
const budgetMonthlyCents = parseBudgetMonthlyCents(budgetDollars);
|
||||
const budgetValid = !budgetDollars.trim() || budgetMonthlyCents !== undefined;
|
||||
const canSubmit = setupSupportedInModal && (!modelRequired || model.trim().length > 0) && budgetValid;
|
||||
|
||||
const provision = useMutation({
|
||||
mutationFn: async () => {
|
||||
@@ -100,18 +109,8 @@ export function ConfigureBuiltInAgentModal({
|
||||
const result = await builtInAgentsApi.provision(companyId, definition.key, {
|
||||
adapterType,
|
||||
adapterConfig,
|
||||
...(budgetMonthlyCents !== undefined ? { budgetMonthlyCents } : {}),
|
||||
});
|
||||
// Budget lives on the agent row, not the provision payload — patch it
|
||||
// separately when the operator sets a non-default value.
|
||||
const trimmed = budgetDollars.trim();
|
||||
// Skip the budget patch while approval is pending — the agent isn't active
|
||||
// yet and the board approval already captured the requested config.
|
||||
if (result.status !== "pending_approval" && result.agentId && trimmed) {
|
||||
const cents = Math.round(Number(trimmed) * 100);
|
||||
if (Number.isFinite(cents) && cents >= 0 && cents !== (result.agent?.budgetMonthlyCents ?? 0)) {
|
||||
await agentsApi.update(result.agentId, { budgetMonthlyCents: cents }, companyId);
|
||||
}
|
||||
}
|
||||
return result;
|
||||
},
|
||||
onSuccess: (result) => {
|
||||
@@ -169,6 +168,14 @@ export function ConfigureBuiltInAgentModal({
|
||||
/>
|
||||
)}
|
||||
|
||||
{!setupSupportedInModal && (
|
||||
<InlineBanner tone="warning" compact>
|
||||
This setup dialog can only complete model-based adapters. Choose a model-based
|
||||
adapter here, or configure command and endpoint adapters from the full agent
|
||||
configuration after provisioning.
|
||||
</InlineBanner>
|
||||
)}
|
||||
|
||||
<Field label="Monthly budget (optional)" hint="Leave blank for no cap.">
|
||||
<div className="flex items-center gap-2">
|
||||
<span className="text-sm text-muted-foreground">$</span>
|
||||
|
||||
Reference in new issue
Block a user