From c83f4b222b8b2f9d6319c21277c5818785ebec35 Mon Sep 17 00:00:00 2001 From: notgitika Date: Tue, 29 Sep 2026 18:27:00 -0400 Subject: [PATCH 1/2] feat(project): add policy-engine and policy TUI wizards MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A bare `agentcore add policy-engine` on a TTY asks for a name, bounded live by the deployed 48-character cap, and — when the project has Gateways — which to attach the engine to, with the enforcement mode revealed beneath the checklist once one is checked, which is the --attach-mode-requires-gateways rule. A bare `agentcore add policy` picks the engine from the project, names the policy, takes the Cedar statement either pasted into a multi-line editor or from a file whose path opens in place under that row and is recorded as sourceFile, asks active or log-only, and shows the inferred authorization phase on the review so the heuristic's guess is seen before it is written. Both handlers gain a shared builder the flags and the wizard call, so the two paths bound names, attach Gateways, map modes and infer the phase identically. --description, --encryption-key-arn and --tags on the engine and --description, --validation-mode and --authorization-phase on the policy stay flag-only. --- src/components/Root.tsx | 7 + src/handlers/project/add/add.screen.test.tsx | 2 + src/handlers/project/add/index.ts | 2 + .../project/add/policy-engine/index.ts | 92 ++-- .../policy-engine.screen.test.tsx | 312 ++++++++++++++ .../project/add/policy-engine/screen.tsx | 323 +++++++++++++++ src/handlers/project/add/policy/index.ts | 63 ++- .../project/add/policy/policy.screen.test.tsx | 392 ++++++++++++++++++ src/handlers/project/add/policy/screen.tsx | 280 +++++++++++++ 9 files changed, 1428 insertions(+), 45 deletions(-) create mode 100644 src/handlers/project/add/policy-engine/policy-engine.screen.test.tsx create mode 100644 src/handlers/project/add/policy-engine/screen.tsx create mode 100644 src/handlers/project/add/policy/policy.screen.test.tsx create mode 100644 src/handlers/project/add/policy/screen.tsx diff --git a/src/components/Root.tsx b/src/components/Root.tsx index 9859a658bd..edcedb8f4e 100644 --- a/src/components/Root.tsx +++ b/src/components/Root.tsx @@ -137,6 +137,8 @@ import { AddOnlineEvalScreen } from "../handlers/project/add/online-eval/screen. import { AddOnlineInsightScreen } from "../handlers/project/add/online-insight/screen.tsx"; import { AddHarnessScreen } from "../handlers/project/add/harness/screen.tsx"; import { AddConfigBundleScreen } from "../handlers/project/add/config-bundle/screen.tsx"; +import { AddPolicyEngineScreen } from "../handlers/project/add/policy-engine/screen.tsx"; +import { AddPolicyScreen } from "../handlers/project/add/policy/screen.tsx"; import { ProjectStatusScreen } from "../handlers/project/status/screen.tsx"; import { ProjectRemoveScreen } from "../handlers/project/remove/screen.tsx"; import { HelpScreen, RootScreen } from "../handlers/screen.tsx"; @@ -925,6 +927,11 @@ function RouteTable({ ctx, core }: ScreenProps) { path="agentcore/add/config-bundle" element={} /> + } + /> + } /> } /> { diff --git a/src/handlers/project/add/index.ts b/src/handlers/project/add/index.ts index 0cbad35d67..ef63912e76 100644 --- a/src/handlers/project/add/index.ts +++ b/src/handlers/project/add/index.ts @@ -37,6 +37,8 @@ export function createAddProjectResourceHandler( "online-insight", "harness", "config-bundle", + "policy-engine", + "policy", ); projectAdd.default(renderTui(core, config.io)); // withProject first, so it is the outermost wrapper: a resource added outside diff --git a/src/handlers/project/add/policy-engine/index.ts b/src/handlers/project/add/policy-engine/index.ts index eb909cfda5..bf690249cb 100644 --- a/src/handlers/project/add/policy-engine/index.ts +++ b/src/handlers/project/add/policy-engine/index.ts @@ -1,11 +1,66 @@ import z from "zod"; import { InputValidationError } from "../../../../errors"; +import type { AwsDeploymentTarget } from "../../../../projectSchemas/aws-targets"; import type { PolicyEngineSchema } from "../../../../projectSchemas/policy"; import { createHandler, flag, ProjectKey } from "../../../../router"; import { parseTags } from "../../../utils"; +import type { AddResourceInput, Project } from "../../types"; import type { AddProjectResourceConfig } from "../types"; import { addProjectResource, requireDeployedNameFits } from "../shared"; +// The deployed name is __, and the service caps it here. +export const POLICY_ENGINE_DEPLOYED_NAME_MAX = 48; + +export type AttachMode = "enforce" | "log-only"; + +// PolicyEngineInput is the engine as the flags state it: the engine's own +// fields plus, optionally, the project Gateways to attach it to and how. +export type PolicyEngineInput = { + name: string; + description?: string; + encryptionKeyArn?: string; + tags?: z.input["tags"]; + attachToGateways?: string[]; + attachMode?: AttachMode; +}; + +// toAddPolicyEngineInput is the one place a Policy Engine is built from user +// input — the flags, or the wizard's answers — so both paths bound the deployed +// name the same way and attach Gateways under the same rule. +export function toAddPolicyEngineInput( + project: Project, + targets: readonly AwsDeploymentTarget[], + input: PolicyEngineInput, +): AddResourceInput { + if (input.attachMode !== undefined && input.attachToGateways === undefined) { + throw new InputValidationError("--attach-mode requires --attach-to-gateways"); + } + requireDeployedNameFits( + "Policy Engine", + project.name, + input.name, + "_", + POLICY_ENGINE_DEPLOYED_NAME_MAX, + targets, + ); + const engine: z.input = { + name: input.name, + description: input.description, + encryptionKeyArn: input.encryptionKeyArn, + tags: input.tags, + }; + return { + resourceType: "policy-engine", + resourceConfig: engine, + attachGateways: input.attachToGateways + ? { + names: input.attachToGateways, + mode: input.attachMode === "log-only" ? "LOG_ONLY" : "ENFORCE", + } + : undefined, + }; +} + export const createAddPolicyEngineHandler = (config: AddProjectResourceConfig) => createHandler({ name: "policy-engine", @@ -27,40 +82,25 @@ export const createAddPolicyEngineHandler = (config: AddProjectResourceConfig) = ), ], handle: async (ctx, flags) => { - if (flags["attach-mode"] !== undefined && flags["attach-to-gateways"] === undefined) { - throw new InputValidationError("--attach-mode requires --attach-to-gateways"); - } const project = ctx.require(ProjectKey); - requireDeployedNameFits( - "Policy Engine", - project.name, - flags.name, - "_", - 48, + const input = toAddPolicyEngineInput( + project, await config.projectManager.listTargets(project), + { + name: flags.name, + description: flags.description, + encryptionKeyArn: flags["encryption-key-arn"], + tags: parseTags(flags.tags), + attachToGateways: flags["attach-to-gateways"], + attachMode: flags["attach-mode"], + }, ); - const engine: z.input = { - name: flags.name, - description: flags.description, - encryptionKeyArn: flags["encryption-key-arn"], - tags: parseTags(flags.tags), - }; - await addProjectResource( ctx, config, project, - { - resourceType: "policy-engine", - resourceConfig: engine, - attachGateways: flags["attach-to-gateways"] - ? { - names: flags["attach-to-gateways"], - mode: flags["attach-mode"] === "log-only" ? "LOG_ONLY" : "ENFORCE", - } - : undefined, - }, + input, `added Policy Engine '${flags.name}' to '${project.name}'`, { notes: flags["attach-to-gateways"] diff --git a/src/handlers/project/add/policy-engine/policy-engine.screen.test.tsx b/src/handlers/project/add/policy-engine/policy-engine.screen.test.tsx new file mode 100644 index 0000000000..7029e797f9 --- /dev/null +++ b/src/handlers/project/add/policy-engine/policy-engine.screen.test.tsx @@ -0,0 +1,312 @@ +import { afterEach, describe, expect, test } from "bun:test"; +import { writeFile } from "node:fs/promises"; +import { join } from "node:path"; +import { + cleanupScreens, + createSilentLogger, + flatFrame, + renderScreen, + TestCoreClient, + TestGlobalConfigAccessor, + testIO, + ttyTestIO, + waitFor, + waitForFlatText, + waitForText, + type RenderScreenResult, +} from "../../../../testing"; +import { createRootHandler } from "../../../index"; +import { InputValidationError } from "../../../../errors"; +import type { AppIO } from "../../../../io"; +import { createGatewayProjectTestHarness } from "../gateway-test-support"; + +const { addGateway, cleanup, inProject, projectSpec, run } = createGatewayProjectTestHarness( + "add-policy-engine-wizard", +); + +afterEach(cleanup); +afterEach(cleanupScreens); + +async function nameStep(screen: RenderScreenResult, name: string): Promise { + await waitForText(screen.lastFrame, "what should this Policy Engine be called?"); + await screen.write(name); + await screen.press("return"); +} + +describe("project add policy-engine wizard", () => { + test("in a project without Gateways it asks only for a name", async () => { + const projectRoot = await inProject(); + const screen = renderScreen("/agentcore/add/policy-engine"); + + await waitForText(screen.lastFrame, "what should this Policy Engine be called?"); + expect(screen.lastFrame()).not.toContain("gateways"); + await screen.write("Guardrails"); + await screen.press("return"); + + // No Gateways to attach to, so the attachment step is not offered at all. + await waitForText(screen.lastFrame, "this Policy Engine will be added to agentcore.json"); + const review = flatFrame(screen.lastFrame); + expect(review).toContain("policy engine Guardrails"); + expect(review).toContain("gateways (none)"); + expect(review).not.toContain("mode"); + await screen.press("return"); + + await waitForText(screen.lastFrame, "added Policy Engine 'Guardrails' to 'TestProject'"); + expect(screen.lastFrame()).toContain("agentcore add policy --engine Guardrails"); + expect(screen.lastFrame()).toContain("[enter] go back"); + // The same bare engine `--name Guardrails` writes. + expect((await projectSpec(projectRoot)).policyEngines).toEqual([ + { name: "Guardrails", policies: [] }, + ]); + + await screen.press("return"); + await waitForText(screen.lastFrame, "add project resources"); + screen.unmount(); + }, 15000); + + test("attaches to the Gateways checked, in the mode revealed beneath them", async () => { + const projectRoot = await inProject(); + await addGateway("tools"); + await addGateway("search"); + const screen = renderScreen("/agentcore/add/policy-engine"); + await nameStep(screen, "Guardrails"); + + await waitForText(screen.lastFrame, "attach it to any Gateways now?"); + expect(screen.lastFrame()).toContain("❯ [ ] tools"); + expect(screen.lastFrame()).toContain("[ ] search"); + // Nothing checked yet, so the mode question is not on screen. + expect(screen.lastFrame()).not.toContain("Enforcement on those Gateways"); + + await screen.write(" "); + await waitForText(screen.lastFrame, "[✓] tools"); + // Checking a Gateway reveals the mode beneath the list, with enforce + // preselected; the list keeps the pointer. + await waitForText(screen.lastFrame, "Enforcement on those Gateways"); + expect(screen.lastFrame()).toContain("● enforce (default)"); + expect(screen.lastFrame()).toContain("❯ [✓] tools"); + + await screen.press("down"); + await screen.write(" "); + await waitForText(screen.lastFrame, "❯ [✓] search"); + + // Enter moves focus from the list into the mode rows; down picks log-only. + await screen.press("return"); + await waitForText(screen.lastFrame, "❯ ● enforce (default)"); + await screen.press("down"); + await waitForText(screen.lastFrame, "❯ ● log-only"); + await screen.press("return"); + + await waitForText(screen.lastFrame, "this Policy Engine will be added to agentcore.json"); + const review = flatFrame(screen.lastFrame); + expect(review).toContain("gateways tools, search"); + expect(review).toContain("mode log-only"); + await screen.press("return"); + + await waitForText(screen.lastFrame, "added Policy Engine 'Guardrails'"); + expect(screen.lastFrame()).toContain("attached to 2 Gateways in log-only mode"); + const spec = await projectSpec(projectRoot); + for (const gateway of spec.agentCoreGateways) { + expect(gateway.policyEngineConfiguration).toEqual({ + policyEngineName: "Guardrails", + mode: "LOG_ONLY", + }); + } + screen.unmount(); + }, 15000); + + test("enter with nothing checked continues without attaching", async () => { + const projectRoot = await inProject(); + await addGateway("tools"); + const screen = renderScreen("/agentcore/add/policy-engine"); + await nameStep(screen, "Guardrails"); + + await waitForText(screen.lastFrame, "attach it to any Gateways now?"); + await screen.press("return"); + + await waitForText(screen.lastFrame, "this Policy Engine will be added to agentcore.json"); + expect(flatFrame(screen.lastFrame)).toContain("gateways (none)"); + await screen.press("return"); + + await waitForText(screen.lastFrame, "added Policy Engine 'Guardrails'"); + expect(screen.lastFrame()).not.toContain("attached to"); + const spec = await projectSpec(projectRoot); + expect(spec.policyEngines).toEqual([{ name: "Guardrails", policies: [] }]); + expect(spec.agentCoreGateways[0].policyEngineConfiguration).toBeUndefined(); + screen.unmount(); + }, 15000); + + test("up from the first mode row returns to the Gateway list", async () => { + await inProject(); + await addGateway("tools"); + const screen = renderScreen("/agentcore/add/policy-engine"); + await nameStep(screen, "Guardrails"); + await waitForText(screen.lastFrame, "attach it to any Gateways now?"); + await screen.write(" "); + await screen.press("return"); + await waitForText(screen.lastFrame, "❯ ● enforce (default)"); + + await screen.press("up"); + + await waitForText(screen.lastFrame, "❯ [✓] tools"); + expect(screen.lastFrame()).not.toContain("❯ ● enforce (default)"); + screen.unmount(); + }); + + test("a name that breaks the schema's pattern is rejected as it is typed", async () => { + await inProject(); + const screen = renderScreen("/agentcore/add/policy-engine"); + + await waitForText(screen.lastFrame, "what should this Policy Engine be called?"); + await screen.write("9starts"); + + await waitForText(screen.lastFrame, "Must begin with a letter"); + screen.unmount(); + }); + + test("validates the deployed name against the longest project target", async () => { + const projectRoot = await inProject(); + await writeFile( + join(projectRoot, "agentcore", "aws-targets.json"), + JSON.stringify([{ name: "production", account: "111122223333", region: "us-east-1" }]), + ); + // 26 characters: legal on its own, one over once the project and the target + // are prefixed. + const name = `E${"x".repeat(25)}`; + const screen = renderScreen("/agentcore/add/policy-engine"); + + await waitForText(screen.lastFrame, "what should this Policy Engine be called?"); + await screen.write(name); + await screen.press("return"); + + await waitForFlatText(screen.lastFrame, "is 49 characters. The maximum is 48."); + const frame = flatFrame(screen.lastFrame); + expect(frame).toContain("TestProject_production_"); + expect(frame).toContain("what should this Policy Engine be called?"); + screen.unmount(); + }); + + test("a rejected add reports itself and hands the form back", async () => { + const projectRoot = await inProject(); + await run(["add", "policy-engine", "--name", "Guardrails"]); + const screen = renderScreen("/agentcore/add/policy-engine"); + await nameStep(screen, "Guardrails"); + await waitForText(screen.lastFrame, "this Policy Engine will be added to agentcore.json"); + await screen.press("return"); + + await waitForFlatText(screen.lastFrame, "already exists"); + await screen.press("escape"); + await waitForText(screen.lastFrame, "this Policy Engine will be added to agentcore.json"); + expect(flatFrame(screen.lastFrame)).toContain("policy engine Guardrails"); + + expect((await projectSpec(projectRoot)).policyEngines).toHaveLength(1); + screen.unmount(); + }, 15000); + + test("esc on the first step returns to the add menu", async () => { + await inProject(); + const screen = renderScreen("/agentcore/add/policy-engine"); + + await waitForText(screen.lastFrame, "what should this Policy Engine be called?"); + await screen.press("escape"); + + await waitForText(screen.lastFrame, "add project resources"); + screen.unmount(); + }); +}); + +// These drive the real CLI entrypoint rather than mounting the screen, because +// what they cover is the routing in front of it: a bare `agentcore add +// policy-engine` has to reach the wizard, and everything else has to stay +// headless. +describe("project add policy-engine dispatch", () => { + function buildRoot(io: AppIO) { + return createRootHandler(new TestCoreClient(), { + io, + logger: createSilentLogger(), + globalConfigAccessor: new TestGlobalConfigAccessor(), + }); + } + + const MISSING_NAME = "required option '--name' not specified"; + + async function routeError(io: AppIO, args: string[]): Promise { + return buildRoot(io) + .route(["node", "agentcore", "add", "policy-engine", ...args]) + .then( + () => undefined, + (caught: unknown) => caught, + ); + } + + test("bare add policy-engine in a TTY session opens the wizard", async () => { + await inProject(); + const { streams, stdin } = ttyTestIO(); + + const outcome = buildRoot(streams.io) + .route(["node", "agentcore", "add", "policy-engine"]) + .then( + () => ({ ok: true as const }), + (error: unknown) => ({ ok: false as const, error }), + ); + let settled = false; + void outcome.finally(() => { + settled = true; + }); + + await waitFor( + () => { + if (!settled) stdin.write("\x03"); + return settled; + }, + 5000, + 150, + ); + expect(await outcome).toEqual({ ok: true }); + expect(streams.stderr()).not.toContain("required option"); + }, 10000); + + test("bare add policy-engine without a TTY stays headless and reports the missing --name", async () => { + await inProject(); + + const error = await routeError(testIO().io, []); + + expect(error).toBeInstanceOf(InputValidationError); + expect((error as Error).message).toContain(MISSING_NAME); + }); + + test("any user-supplied flag stays headless even in a TTY", async () => { + await inProject(); + + const error = await routeError(ttyTestIO().streams.io, ["--description", "guardrails"]); + + expect(error).toBeInstanceOf(InputValidationError); + expect((error as Error).message).toContain(MISSING_NAME); + }); + + test("--json stays headless even in a TTY", async () => { + await inProject(); + + const error = await routeError(ttyTestIO().streams.io, ["--json"]); + + expect(error).toBeInstanceOf(InputValidationError); + expect((error as Error).message).toContain(MISSING_NAME); + }); + + test("flag-driven add policy-engine still runs headless in a TTY session", async () => { + const projectRoot = await inProject(); + const { streams } = ttyTestIO(); + + await buildRoot(streams.io).route([ + "node", + "agentcore", + "add", + "policy-engine", + "--name", + "FlagEngine", + ]); + + expect((await projectSpec(projectRoot)).policyEngines).toEqual([ + { name: "FlagEngine", policies: [] }, + ]); + }, 10000); +}); diff --git a/src/handlers/project/add/policy-engine/screen.tsx b/src/handlers/project/add/policy-engine/screen.tsx new file mode 100644 index 0000000000..cbf9618aca --- /dev/null +++ b/src/handlers/project/add/policy-engine/screen.tsx @@ -0,0 +1,323 @@ +import { useMemo, useState } from "react"; +import { useQueryClient } from "@tanstack/react-query"; +import { Box, useInput } from "ink"; +import { useNavigate } from "react-router"; +import { FormCheckboxMultiSelect } from "../../../../components/FormCheckboxMultiSelect"; +import { FormRadioGroup } from "../../../../components/FormRadioGroup"; +import { + Step, + Summary, + TextField, + Wizard, + useKeyHints, + useWizard, + type Choice, +} from "../../../../components/wizard"; +import type { AwsDeploymentTarget } from "../../../../projectSchemas/aws-targets"; +import type { AgentCoreGateway } from "../../../../projectSchemas/gateway"; +import { PolicyEngineNameSchema } from "../../../../projectSchemas/policy"; +import { ProjectKey } from "../../../../router"; +import type { ScreenProps } from "../../../types"; +import type { Project } from "../../types"; +import { LoadingFrame, ProjectGate, projectQueryKey, useProjectTargets } from "../../ProjectGate"; +import { requireDeployedNameFits } from "../shared"; +import { + POLICY_ENGINE_DEPLOYED_NAME_MAX, + toAddPolicyEngineInput, + type AttachMode, + type PolicyEngineInput, +} from "./index"; + +const BREADCRUMB = ["agentcore", "add", "policy-engine"]; +const DESCRIPTION = "add a Policy Engine to the current project"; +const ADD_MENU = "/agentcore/add"; + +const MODE_CHOICES: Choice[] = [ + { + value: "enforce", + label: "enforce (default)", + description: "deny the calls the engine's policies forbid", + }, + { + value: "log-only", + label: "log-only", + description: "record what the policies would decide, without blocking", + }, +]; + +type PolicyEngineFormValues = { + name: string; + // The Gateways to attach the engine to, in the order the project lists them. + attached: string[]; + mode: AttachMode; +}; + +// toPolicyEngineInput is the answers as the flag path would state them: no +// attachment at all when no Gateway was picked, exactly as omitting +// --attach-to-gateways does. --description, --encryption-key-arn and --tags +// stay flag-only. +export function toPolicyEngineInput(values: PolicyEngineFormValues): PolicyEngineInput { + const attaches = values.attached.length > 0; + return { + name: values.name, + attachToGateways: attaches ? values.attached : undefined, + attachMode: attaches ? values.mode : undefined, + }; +} + +function summaryOf(values: PolicyEngineFormValues): Record { + const attaches = values.attached.length > 0; + return { + "policy engine": values.name, + // The row is always shown, so the review says what an empty selection + // means: the engine exists on its own and can be attached later. + gateways: attaches ? values.attached.join(", ") : "(none) · attach from a Gateway later", + ...(attaches ? { mode: values.mode } : {}), + }; +} + +export function AddPolicyEngineScreen({ ctx, core }: ScreenProps) { + const navigate = useNavigate(); + return ( + navigate(ADD_MENU)} + > + {(project) => } + + ); +} + +function AddPolicyEngineLoader({ project, core }: { project: Project; core: ScreenProps["core"] }) { + const navigate = useNavigate(); + const targets = useProjectTargets(core, project); + + if (targets.data !== undefined) { + return ; + } + + return ( + navigate(ADD_MENU)} + /> + ); +} + +function AddPolicyEngineWizard({ + project, + targets, + core, +}: { + project: Project; + targets: readonly AwsDeploymentTarget[]; + core: ScreenProps["core"]; +}) { + const navigate = useNavigate(); + const queryClient = useQueryClient(); + const gateways = project.spec.agentCoreGateways ?? []; + const [values, setValues] = useState({ + name: "", + attached: [], + mode: "enforce", + }); + const set = (update: Partial) => + setValues((current) => ({ ...current, ...update })); + + // The deployed name is __ and must fit the service + // cap, so the live check reports the real budget rather than the schema's 48. + const nameSchema = useMemo( + () => + PolicyEngineNameSchema.superRefine((name, ctx) => { + try { + requireDeployedNameFits( + "Policy Engine", + project.name, + name, + "_", + POLICY_ENGINE_DEPLOYED_NAME_MAX, + targets, + ); + } catch (error) { + ctx.addIssue({ + code: "custom", + message: error instanceof Error ? error.message : String(error), + }); + } + }), + [project.name, targets], + ); + + const attaches = values.attached.length > 0; + + return ( + navigate(ADD_MENU)} + onSubmit={async function* () { + const updated = yield* core.projectManager.addResource( + project, + toAddPolicyEngineInput(project, targets, toPolicyEngineInput(values)), + ); + queryClient.setQueryData(projectQueryKey(), updated); + return updated; + }} + runningLabel={`adding Policy Engine ${values.name}…`} + successLabel={`added Policy Engine '${values.name}' to '${project.name}'`} + successHint={ + attaches + ? `attached to ${values.attached.length} ${values.attached.length === 1 ? "Gateway" : "Gateways"} in ${values.mode} mode` + : undefined + } + successNextSteps={[`agentcore add policy --engine ${values.name}`, "agentcore deploy"]} + onDone={() => navigate(ADD_MENU)} + doneLabel="go back" + > + + __ must fit ${POLICY_ENGINE_DEPLOYED_NAME_MAX} characters`} + placeholder="Guardrails" + value={values.name} + onChange={(name) => set({ name })} + required + schema={nameSchema} + live + /> + + + {/* Skipped when the project has no Gateways: there is nothing to attach + to, and the engine can be attached from a Gateway later. */} + {gateways.length > 0 && ( + + set(update)} + /> + + )} + + + + + + ); +} + +// GatewayAttachmentField is a compound field: a checklist of the project's +// Gateways and, once any is checked, the enforcement mode for those +// attachments revealed beneath it — the question exists only once a Gateway is +// picked, which is the `--attach-mode requires --attach-to-gateways` rule. The +// checklist has focus first; enter (or down past the last Gateway) moves into +// the mode rows when there are attachments, and enter there continues. Enter +// with nothing checked continues straight away. +function GatewayAttachmentField({ + gateways, + attached, + mode, + onChange, +}: { + gateways: readonly AgentCoreGateway[]; + attached: string[]; + mode: AttachMode; + onChange: (update: Partial>) => void; +}) { + const { advance, back } = useWizard(); + const [focused, setFocused] = useState<"gateways" | "mode">("gateways"); + const [cursor, setCursor] = useState(0); + const modeIndex = Math.max( + 0, + MODE_CHOICES.findIndex((choice) => choice.value === mode), + ); + const attaches = attached.length > 0; + + useKeyHints([ + { key: "↑↓", label: "navigate" }, + { key: "space", label: "toggle" }, + { key: "enter", label: "continue" }, + ]); + + useInput((input, key) => { + if (key.escape) { + back(); + return; + } + + if (focused === "gateways") { + if (key.upArrow) { + setCursor((current) => Math.max(0, current - 1)); + return; + } + if (key.downArrow) { + if (cursor < gateways.length - 1) setCursor(cursor + 1); + else if (attaches) setFocused("mode"); + return; + } + if (input === " ") { + const name = gateways[cursor]!.name; + const toggled = attached.includes(name) + ? attached.filter((candidate) => candidate !== name) + : [...attached, name]; + // Kept in the project's order, so the review and the spec agree. + onChange({ + attached: gateways + .filter((gateway) => toggled.includes(gateway.name)) + .map((gateway) => gateway.name), + }); + return; + } + if (key.return) { + if (attaches) setFocused("mode"); + else advance(); + } + return; + } + + if (key.upArrow) { + if (modeIndex === 0) setFocused("gateways"); + else onChange({ mode: MODE_CHOICES[modeIndex - 1]!.value }); + return; + } + if (key.downArrow) { + onChange({ mode: MODE_CHOICES[Math.min(MODE_CHOICES.length - 1, modeIndex + 1)]!.value }); + return; + } + if (key.return) advance(); + }); + + return ( + + ({ + label: gateway.name, + description: `${gateway.targets.length} ${gateway.targets.length === 1 ? "Target" : "Targets"}`, + checked: attached.includes(gateway.name), + }))} + cursorIndex={focused === "gateways" ? cursor : -1} + /> + {attaches && ( + ({ + label: choice.label, + description: choice.description ?? "", + }))} + focusedIndex={focused === "mode" ? modeIndex : undefined} + selectedIndex={modeIndex} + /> + )} + + ); +} diff --git a/src/handlers/project/add/policy/index.ts b/src/handlers/project/add/policy/index.ts index 98543a5fde..ba93732ecc 100644 --- a/src/handlers/project/add/policy/index.ts +++ b/src/handlers/project/add/policy/index.ts @@ -2,6 +2,7 @@ import z from "zod"; import { SourceResolver } from "../../../../io"; import type { PolicySchema } from "../../../../projectSchemas/policy"; import { createHandler, flag, ProjectKey } from "../../../../router"; +import type { AddResourceInput } from "../../types"; import type { AddProjectResourceConfig } from "../types"; import { addProjectResource } from "../shared"; @@ -19,6 +20,39 @@ const VALIDATION_MODES = { } as const; const ENFORCEMENT_MODES = { active: "ACTIVE", "log-only": "LOG_ONLY" } as const; +export type PolicyEnforcementMode = keyof typeof ENFORCEMENT_MODES; + +// PolicyInput is the policy as the flags state it, with the statement already +// resolved to text and, when it came from a file, the path it came from. +export type PolicyInput = { + engine: string; + name: string; + statement: string; + sourceFile?: string; + description?: string; + validationMode?: keyof typeof VALIDATION_MODES; + enforcementMode?: PolicyEnforcementMode; + authorizationPhase?: keyof typeof PHASES; +}; + +// toAddPolicyInput is the one place a Policy is built from user input — the +// flags, or the wizard's answers — so both infer the authorization phase from +// the statement the same way and map modes to the same values. +export function toAddPolicyInput(input: PolicyInput): AddResourceInput { + const policy: z.input = { + name: input.name, + description: input.description, + statement: input.statement, + sourceFile: input.sourceFile, + validationMode: input.validationMode && VALIDATION_MODES[input.validationMode], + enforcementMode: input.enforcementMode && ENFORCEMENT_MODES[input.enforcementMode], + authorizationPhase: input.authorizationPhase + ? PHASES[input.authorizationPhase] + : inferAuthorizationPhase(input.statement), + }; + return { resourceType: "policy", engineName: input.engine, resourceConfig: policy }; +} + export const createAddPolicyHandler = (config: AddProjectResourceConfig) => createHandler({ name: "policy", @@ -57,29 +91,20 @@ export const createAddPolicyHandler = (config: AddProjectResourceConfig) => ? flags.statement.slice("file://".length) : undefined; - const authorizationPhase = flags["authorization-phase"] - ? PHASES[flags["authorization-phase"]] - : inferAuthorizationPhase(statement); - - const policy: z.input = { - name: flags.name, - description: flags.description, - statement, - sourceFile, - validationMode: flags["validation-mode"] && VALIDATION_MODES[flags["validation-mode"]], - enforcementMode: flags["enforcement-mode"] && ENFORCEMENT_MODES[flags["enforcement-mode"]], - authorizationPhase, - }; - await addProjectResource( ctx, config, project, - { - resourceType: "policy", - engineName: flags.engine, - resourceConfig: policy, - }, + toAddPolicyInput({ + engine: flags.engine, + name: flags.name, + statement, + sourceFile, + description: flags.description, + validationMode: flags["validation-mode"], + enforcementMode: flags["enforcement-mode"], + authorizationPhase: flags["authorization-phase"], + }), `added Policy '${flags.name}' to Policy Engine '${flags.engine}' in '${project.name}'`, ); }, diff --git a/src/handlers/project/add/policy/policy.screen.test.tsx b/src/handlers/project/add/policy/policy.screen.test.tsx new file mode 100644 index 0000000000..bc907363aa --- /dev/null +++ b/src/handlers/project/add/policy/policy.screen.test.tsx @@ -0,0 +1,392 @@ +import { afterEach, describe, expect, test } from "bun:test"; +import { writeFile } from "node:fs/promises"; +import { join } from "node:path"; +import { + cleanupScreens, + createSilentLogger, + flatFrame, + renderScreen, + TestCoreClient, + TestGlobalConfigAccessor, + testIO, + ttyTestIO, + waitFor, + waitForFlatText, + waitForText, + type RenderScreenResult, +} from "../../../../testing"; +import { createRootHandler } from "../../../index"; +import { InputValidationError } from "../../../../errors"; +import type { AppIO } from "../../../../io"; +import { createGatewayProjectTestHarness } from "../gateway-test-support"; +import { readableFileSchema } from "./screen"; + +const FORBID_ALL = "forbid (principal, action, resource);"; +const SUPPRESS = + "suppressOutput (principal, action, resource is AgentCore::Gateway)\n" + + 'when guardrails { BedrockGuardrails::ContentFilter(["HATE"], [context.output.message])' + + '["HATE"].confidenceScore.greaterThan(decimal("0.2")) };'; +const FILE_HELP = "a .cedar file, relative to the current directory or absolute"; + +const { cleanup, inProject, projectSpec, run } = + createGatewayProjectTestHarness("add-policy-wizard"); + +afterEach(cleanup); +afterEach(cleanupScreens); + +async function withEngine(): Promise { + const projectRoot = await inProject(); + await run(["add", "policy-engine", "--name", "Guardrails"]); + return projectRoot; +} + +async function policiesOf(projectRoot: string, engine = "Guardrails") { + const spec = await projectSpec(projectRoot); + return spec.policyEngines.find((candidate: { name: string }) => candidate.name === engine) + .policies; +} + +// reachSourceStep confirms the only engine and names the policy, leaving the +// wizard on the statement-source step. +async function reachSourceStep(screen: RenderScreenResult, name: string): Promise { + await waitForText(screen.lastFrame, "which Policy Engine should this Policy belong to?"); + await screen.press("return"); + await waitForText(screen.lastFrame, "what should this Policy be called?"); + await screen.write(name); + await screen.press("return"); + await waitForText(screen.lastFrame, "where is the Cedar statement?"); +} + +describe("policy wizard helpers", () => { + test("readableFileSchema accepts an existing file and refuses anything else", async () => { + const projectRoot = await inProject(); + const path = join(projectRoot, "deny.cedar"); + await writeFile(path, FORBID_ALL); + expect(readableFileSchema.safeParse(path).success).toBe(true); + expect(readableFileSchema.safeParse(join(projectRoot, "missing.cedar")).success).toBe(false); + // A directory is not a statement. + expect(readableFileSchema.safeParse(projectRoot).success).toBe(false); + }); +}); + +describe("project add policy wizard", () => { + test("adds the same inline policy as the flags, with the inferred phase on review", async () => { + const projectRoot = await withEngine(); + const screen = renderScreen("/agentcore/add/policy"); + + await waitForText(screen.lastFrame, "which Policy Engine should this Policy belong to?"); + expect(screen.lastFrame()).toContain("❯ ● Guardrails"); + expect(screen.lastFrame()).toContain("0 policies"); + await screen.press("return"); + + await waitForText(screen.lastFrame, "what should this Policy be called?"); + await screen.write("DenyAll"); + await screen.press("return"); + + await waitForText(screen.lastFrame, "where is the Cedar statement?"); + expect(screen.lastFrame()).toContain("❯ ● type or paste the Cedar statement"); + expect(screen.lastFrame()).toContain("○ load it from a file"); + await screen.press("return"); + + await waitForText(screen.lastFrame, "what is the Cedar statement?"); + expect(screen.lastFrame()).toContain("[ctrl+d] continue"); + await screen.write(FORBID_ALL); + await screen.press("ctrl+d"); + + await waitForText(screen.lastFrame, "should it enforce, or only log?"); + expect(screen.lastFrame()).toContain("❯ ● active (default)"); + await screen.press("return"); + + await waitForText(screen.lastFrame, "this Policy will be added to agentcore.json"); + const review = flatFrame(screen.lastFrame); + expect(review).toContain("policy engine Guardrails"); + expect(review).toContain("policy DenyAll"); + expect(review).toContain(`statement ${FORBID_ALL}`); + expect(review).toContain("enforcement active"); + expect(review).toContain("authorization phase INITIATE · inferred from the statement"); + await screen.press("return"); + + await waitForText( + screen.lastFrame, + "added Policy 'DenyAll' to Policy Engine 'Guardrails' in 'TestProject'", + ); + expect(await policiesOf(projectRoot)).toEqual([ + { + name: "DenyAll", + statement: FORBID_ALL, + validationMode: "FAIL_ON_ANY_FINDINGS", + enforcementMode: "ACTIVE", + authorizationPhase: "INITIATE", + }, + ]); + + await screen.press("return"); + await waitForText(screen.lastFrame, "add project resources"); + screen.unmount(); + }, 15000); + + test("loads the statement from a file, recording its path and inferring RETURN_OUTPUT", async () => { + const projectRoot = await withEngine(); + // Relative to the project, which is the working directory: the path is + // recorded as typed, the way `--statement file://suppress.cedar` records it. + const cedarPath = "suppress.cedar"; + await writeFile(join(projectRoot, cedarPath), SUPPRESS); + const screen = renderScreen("/agentcore/add/policy"); + await reachSourceStep(screen, "Suppress"); + + await screen.press("down"); + await waitForText(screen.lastFrame, "❯ ● load it from a file"); + await screen.press("return"); + + // The path input opens under the row. + await waitForText(screen.lastFrame, FILE_HELP); + expect(screen.lastFrame()).toContain("where is the Cedar statement?"); + await screen.write(cedarPath); + await screen.press("return"); + + // No editor step for a file; straight on to enforcement. + await waitForText(screen.lastFrame, "should it enforce, or only log?"); + expect(screen.lastFrame()).not.toContain("what is the Cedar statement?"); + await screen.press("down"); + await waitForText(screen.lastFrame, "❯ ● log-only"); + await screen.press("return"); + + await waitForText(screen.lastFrame, "this Policy will be added to agentcore.json"); + const review = flatFrame(screen.lastFrame); + expect(review).toContain(`statement from ${cedarPath}`); + expect(review).toContain("enforcement log-only"); + expect(review).toContain("authorization phase RETURN_OUTPUT"); + await screen.press("return"); + + await waitForText(screen.lastFrame, "added Policy 'Suppress'"); + expect((await policiesOf(projectRoot))[0]).toMatchObject({ + statement: SUPPRESS, + sourceFile: cedarPath, + enforcementMode: "LOG_ONLY", + authorizationPhase: "RETURN_OUTPUT", + }); + screen.unmount(); + }, 15000); + + test("a file that is not there keeps the step", async () => { + const projectRoot = await withEngine(); + const screen = renderScreen("/agentcore/add/policy"); + await reachSourceStep(screen, "Missing"); + await screen.press("down"); + await screen.press("return"); + await waitForText(screen.lastFrame, FILE_HELP); + + await screen.press("return"); + await waitForText(screen.lastFrame, "Statement file is required"); + + await screen.write(join(projectRoot, "nope.cedar")); + await screen.press("return"); + await waitForText(screen.lastFrame, "no readable file at"); + expect(screen.lastFrame()).toContain("where is the Cedar statement?"); + screen.unmount(); + }); + + test("an empty statement is refused", async () => { + await withEngine(); + const screen = renderScreen("/agentcore/add/policy"); + await reachSourceStep(screen, "Empty"); + await screen.press("return"); + await waitForText(screen.lastFrame, "what is the Cedar statement?"); + + await screen.press("ctrl+d"); + + await waitForText(screen.lastFrame, "Cedar statement is required"); + expect(screen.lastFrame()).not.toContain("should it enforce"); + screen.unmount(); + }); + + test("a multi-line statement is kept as typed and previewed on one line", async () => { + const projectRoot = await withEngine(); + const screen = renderScreen("/agentcore/add/policy"); + await reachSourceStep(screen, "Suppress"); + await screen.press("return"); + await waitForText(screen.lastFrame, "what is the Cedar statement?"); + + const [first, second] = SUPPRESS.split("\n"); + await screen.write(first!); + await screen.press("return"); + await screen.write(second!); + await screen.press("ctrl+d"); + await waitForText(screen.lastFrame, "should it enforce, or only log?"); + await screen.press("return"); + + await waitForFlatText(screen.lastFrame, "(+1 more line)"); + expect(flatFrame(screen.lastFrame)).toContain("authorization phase RETURN_OUTPUT"); + await screen.press("return"); + + await waitForText(screen.lastFrame, "added Policy 'Suppress'"); + expect((await policiesOf(projectRoot))[0].statement).toBe(SUPPRESS); + screen.unmount(); + }, 15000); + + test("without a Policy Engine the first step says what to add and esc returns to the menu", async () => { + await inProject(); + const screen = renderScreen("/agentcore/add/policy"); + + await waitForText(screen.lastFrame, "no Policy Engines in this project"); + expect(screen.lastFrame()).toContain("agentcore add policy-engine"); + + await screen.press("escape"); + await waitForText(screen.lastFrame, "add project resources"); + screen.unmount(); + }); + + test("a rejected add reports itself and hands the form back", async () => { + const projectRoot = await withEngine(); + await run(["add", "policy-engine", "--name", "Second"]); + await run([ + "add", + "policy", + "--engine", + "Guardrails", + "--name", + "DenyAll", + "--statement", + FORBID_ALL, + ]); + const screen = renderScreen("/agentcore/add/policy"); + + // Pick the second engine; the name is taken by the first, and names are + // unique across engines, so addResource refuses it. + await waitForText(screen.lastFrame, "which Policy Engine should this Policy belong to?"); + await screen.press("down"); + await waitForText(screen.lastFrame, "❯ ● Second"); + await screen.press("return"); + await waitForText(screen.lastFrame, "what should this Policy be called?"); + await screen.write("DenyAll"); + await screen.press("return"); + await waitForText(screen.lastFrame, "where is the Cedar statement?"); + await screen.press("return"); + await waitForText(screen.lastFrame, "what is the Cedar statement?"); + await screen.write(FORBID_ALL); + await screen.press("ctrl+d"); + await waitForText(screen.lastFrame, "should it enforce, or only log?"); + await screen.press("return"); + await waitForText(screen.lastFrame, "this Policy will be added to agentcore.json"); + await screen.press("return"); + + await waitForFlatText(screen.lastFrame, "already exists in policy engine 'Guardrails'"); + await screen.press("escape"); + await waitForText(screen.lastFrame, "this Policy will be added to agentcore.json"); + expect(flatFrame(screen.lastFrame)).toContain("policy DenyAll"); + + expect(await policiesOf(projectRoot, "Second")).toHaveLength(0); + screen.unmount(); + }, 15000); + + test("esc on the first step returns to the add menu", async () => { + await withEngine(); + const screen = renderScreen("/agentcore/add/policy"); + + await waitForText(screen.lastFrame, "which Policy Engine should this Policy belong to?"); + await screen.press("escape"); + + await waitForText(screen.lastFrame, "add project resources"); + screen.unmount(); + }); +}); + +// These drive the real CLI entrypoint rather than mounting the screen, because +// what they cover is the routing in front of it: a bare `agentcore add policy` +// has to reach the wizard, and everything else has to stay headless. +describe("project add policy dispatch", () => { + function buildRoot(io: AppIO) { + return createRootHandler(new TestCoreClient(), { + io, + logger: createSilentLogger(), + globalConfigAccessor: new TestGlobalConfigAccessor(), + }); + } + + const MISSING_ENGINE = "required option '--engine' not specified"; + + async function routeError(io: AppIO, args: string[]): Promise { + return buildRoot(io) + .route(["node", "agentcore", "add", "policy", ...args]) + .then( + () => undefined, + (caught: unknown) => caught, + ); + } + + test("bare add policy in a TTY session opens the wizard", async () => { + await withEngine(); + const { streams, stdin } = ttyTestIO(); + + const outcome = buildRoot(streams.io) + .route(["node", "agentcore", "add", "policy"]) + .then( + () => ({ ok: true as const }), + (error: unknown) => ({ ok: false as const, error }), + ); + let settled = false; + void outcome.finally(() => { + settled = true; + }); + + await waitFor( + () => { + if (!settled) stdin.write("\x03"); + return settled; + }, + 5000, + 150, + ); + expect(await outcome).toEqual({ ok: true }); + expect(streams.stderr()).not.toContain("required option"); + }, 10000); + + test("bare add policy without a TTY stays headless and reports the missing --engine", async () => { + await withEngine(); + + const error = await routeError(testIO().io, []); + + expect(error).toBeInstanceOf(InputValidationError); + expect((error as Error).message).toContain(MISSING_ENGINE); + }); + + test("any user-supplied flag stays headless even in a TTY", async () => { + await withEngine(); + + const error = await routeError(ttyTestIO().streams.io, ["--name", "DenyAll"]); + + expect(error).toBeInstanceOf(InputValidationError); + expect((error as Error).message).toContain(MISSING_ENGINE); + }); + + test("--json stays headless even in a TTY", async () => { + await withEngine(); + + const error = await routeError(ttyTestIO().streams.io, ["--json"]); + + expect(error).toBeInstanceOf(InputValidationError); + expect((error as Error).message).toContain(MISSING_ENGINE); + }); + + test("flag-driven add policy still runs headless in a TTY session", async () => { + const projectRoot = await withEngine(); + const { streams } = ttyTestIO(); + + await buildRoot(streams.io).route([ + "node", + "agentcore", + "add", + "policy", + "--engine", + "Guardrails", + "--name", + "Flagged", + "--statement", + FORBID_ALL, + ]); + + expect((await policiesOf(projectRoot)).map((policy: { name: string }) => policy.name)).toEqual([ + "Flagged", + ]); + }, 10000); +}); diff --git a/src/handlers/project/add/policy/screen.tsx b/src/handlers/project/add/policy/screen.tsx new file mode 100644 index 0000000000..258e590779 --- /dev/null +++ b/src/handlers/project/add/policy/screen.tsx @@ -0,0 +1,280 @@ +import { accessSync, constants, readFileSync, statSync } from "node:fs"; +import { readFile } from "node:fs/promises"; +import { useState } from "react"; +import { useQueryClient } from "@tanstack/react-query"; +import { useNavigate } from "react-router"; +import z from "zod"; +import { + ChoiceField, + ResourceChoiceField, + RevealChoiceField, + Step, + Summary, + TextAreaField, + TextField, + Wizard, + type Choice, +} from "../../../../components/wizard"; +import { InputValidationError } from "../../../../errors"; +import { PolicyNameSchema, type PolicyEngine } from "../../../../projectSchemas/policy"; +import { ProjectKey } from "../../../../router"; +import type { ScreenProps } from "../../../types"; +import type { Project } from "../../types"; +import { ProjectGate, projectQueryKey } from "../../ProjectGate"; +import { + inferAuthorizationPhase, + toAddPolicyInput, + type PolicyEnforcementMode, + type PolicyInput, +} from "./index"; + +const BREADCRUMB = ["agentcore", "add", "policy"]; +const DESCRIPTION = "add a Cedar Policy to a project Policy Engine"; +const ADD_MENU = "/agentcore/add"; + +// Where the statement comes from. A file is the way `--statement file://…` +// arrives, and the flag path records the path as the policy's sourceFile, so +// the wizard keeps that provenance rather than pasting the file's text. +type StatementSource = "inline" | "file"; + +const SOURCE_CHOICES: Choice[] = [ + { + value: "inline", + label: "type or paste the Cedar statement", + description: "on the next step, over as many lines as it takes", + }, + { + value: "file", + label: "load it from a file", + description: "the path is recorded as the policy's sourceFile", + }, +]; + +const ENFORCEMENT_CHOICES: Choice[] = [ + { + value: "active", + label: "active (default)", + description: "deny the calls this policy forbids", + }, + { + value: "log-only", + label: "log-only", + description: "record what this policy would decide, without blocking", + }, +]; + +const STATEMENT_EXAMPLE = "forbid (principal, action, resource);"; + +// readableFileSchema accepts a path to an existing, readable file — checked +// before the step advances, so a typo is caught here rather than on submit. +export const readableFileSchema: z.ZodType = z.string().superRefine((path, ctx) => { + try { + if (!statSync(path).isFile()) throw new Error("not a file"); + accessSync(path, constants.R_OK); + } catch { + ctx.addIssue({ code: "custom", message: `no readable file at '${path}'` }); + } +}); + +type PolicyFormValues = { + engine: string; + name: string; + source: StatementSource; + statement: string; + statementFile: string; + enforcement: PolicyEnforcementMode; +}; + +// statementOf reads the statement the review and the submit both work from: +// the pasted text, or the file's contents. The file is read synchronously here +// because the review needs the inferred phase before anything is written; it is +// small, local, and already checked to be readable on the source step. +function statementOf(values: PolicyFormValues): string | undefined { + if (values.source === "inline") return values.statement; + try { + return readFileSync(values.statementFile, "utf8"); + } catch { + return undefined; + } +} + +// toPolicyInput is the answers as the flag path would state them; the phase is +// left for the shared builder to infer, exactly as an omitted +// --authorization-phase is. --description and --validation-mode stay flag-only. +export function toPolicyInput(values: PolicyFormValues, statement: string): PolicyInput { + return { + engine: values.engine, + name: values.name, + statement, + sourceFile: values.source === "file" ? values.statementFile : undefined, + enforcementMode: values.enforcement, + }; +} + +function firstLine(text: string): string { + const lines = text.trim().split("\n"); + const first = lines[0] ?? ""; + const shown = first.length > 60 ? `${first.slice(0, 59)}…` : first; + const rest = lines.length - 1; + return rest > 0 ? `${shown} (+${rest} more ${rest === 1 ? "line" : "lines"})` : shown; +} + +function summaryOf(values: PolicyFormValues): Record { + const statement = statementOf(values); + return { + "policy engine": values.engine, + policy: values.name, + statement: + values.source === "file" ? `from ${values.statementFile}` : firstLine(values.statement), + enforcement: values.enforcement, + // The phase is a substring heuristic over the statement, so the guess is + // shown before it is written; --authorization-phase overrides it. + "authorization phase": + statement === undefined + ? "(file unreadable)" + : `${inferAuthorizationPhase(statement)} · inferred from the statement`, + }; +} + +function engineChoices(engines: readonly PolicyEngine[]): Choice[] { + return engines.map((engine) => ({ + value: engine.name, + label: engine.name, + description: `${engine.policies.length} ${engine.policies.length === 1 ? "policy" : "policies"}`, + })); +} + +export function AddPolicyScreen({ ctx, core }: ScreenProps) { + const navigate = useNavigate(); + return ( + navigate(ADD_MENU)} + > + {(project) => } + + ); +} + +function AddPolicyWizard({ project, core }: { project: Project; core: ScreenProps["core"] }) { + const navigate = useNavigate(); + const queryClient = useQueryClient(); + const engines = project.spec.policyEngines ?? []; + const [values, setValues] = useState({ + engine: engines[0]?.name ?? "", + name: "", + source: "inline", + statement: "", + statementFile: "", + enforcement: "active", + }); + const set = (update: Partial) => + setValues((current) => ({ ...current, ...update })); + + return ( + navigate(ADD_MENU)} + onSubmit={async function* () { + // Read the file at submit, the way `--statement file://…` does, so what + // is written is what the file holds now. + let statement = values.statement; + if (values.source === "file") { + try { + statement = await readFile(values.statementFile, "utf8"); + } catch (error) { + throw new InputValidationError( + `could not read the statement from '${values.statementFile}'`, + { cause: error }, + ); + } + } + const updated = yield* core.projectManager.addResource( + project, + toAddPolicyInput(toPolicyInput(values, statement)), + ); + queryClient.setQueryData(projectQueryKey(), updated); + return updated; + }} + runningLabel={`adding Policy ${values.name}…`} + successLabel={`added Policy '${values.name}' to Policy Engine '${values.engine}' in '${project.name}'`} + successNextSteps={["agentcore deploy"]} + onDone={() => navigate(ADD_MENU)} + doneLabel="go back" + > + + set({ engine })} + emptyMessage="no Policy Engines in this project" + emptyHint="add one with agentcore add policy-engine" + /> + + + + set({ name })} + required + schema={PolicyNameSchema} + live + /> + + + + set({ source })} + input={{ + // Only a file needs a path, so it is asked under that row; pasting + // continues to the editor on the next step. + opensFor: (source) => source === "file", + label: "Statement file", + name: "statement file", + help: "a .cedar file, relative to the current directory or absolute", + placeholder: "policies/deny-all.cedar", + value: values.statementFile, + onChange: (statementFile) => set({ statementFile }), + required: true, + schema: readableFileSchema, + }} + /> + + + {values.source === "inline" && ( + + set({ statement })} + required + /> + + )} + + + set({ enforcement })} + /> + + + + + + + ); +} From 769f36788e105cae48bdba174a472a0b03177782 Mon Sep 17 00:00:00 2001 From: notgitika Date: Tue, 29 Sep 2026 22:03:05 -0400 Subject: [PATCH 2/2] fix(project): read the policy statement file strictly, off the render path MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A file-sourced statement is read through SourceResolver, as `--statement file://…` reads it, so a file that is not valid UTF-8 is refused instead of decoded with replacement characters that would change what the policy matches. The review loads it asynchronously when the step mounts rather than synchronously on every render, so typing a path never blocks the TUI and only a path the source step has already accepted is read. The engine wizard's next step is bare `agentcore add policy`, which opens the policy wizard; naming the engine with --engine would select the headless path. SourceResolverConfig.stdin is optional now, for callers that only ever resolve inline values and files. --- .../policy-engine.screen.test.tsx | 4 +- .../project/add/policy-engine/screen.tsx | 4 +- .../project/add/policy/policy.screen.test.tsx | 37 ++++++- src/handlers/project/add/policy/screen.tsx | 98 ++++++++++++------- src/io/source.ts | 7 +- 5 files changed, 112 insertions(+), 38 deletions(-) diff --git a/src/handlers/project/add/policy-engine/policy-engine.screen.test.tsx b/src/handlers/project/add/policy-engine/policy-engine.screen.test.tsx index 7029e797f9..50234d1c17 100644 --- a/src/handlers/project/add/policy-engine/policy-engine.screen.test.tsx +++ b/src/handlers/project/add/policy-engine/policy-engine.screen.test.tsx @@ -52,7 +52,9 @@ describe("project add policy-engine wizard", () => { await screen.press("return"); await waitForText(screen.lastFrame, "added Policy Engine 'Guardrails' to 'TestProject'"); - expect(screen.lastFrame()).toContain("agentcore add policy --engine Guardrails"); + // Bare, so following it opens the policy wizard; --engine would make it headless. + expect(screen.lastFrame()).toContain("agentcore add policy"); + expect(screen.lastFrame()).not.toContain("--engine"); expect(screen.lastFrame()).toContain("[enter] go back"); // The same bare engine `--name Guardrails` writes. expect((await projectSpec(projectRoot)).policyEngines).toEqual([ diff --git a/src/handlers/project/add/policy-engine/screen.tsx b/src/handlers/project/add/policy-engine/screen.tsx index cbf9618aca..1739232772 100644 --- a/src/handlers/project/add/policy-engine/screen.tsx +++ b/src/handlers/project/add/policy-engine/screen.tsx @@ -176,7 +176,9 @@ function AddPolicyEngineWizard({ ? `attached to ${values.attached.length} ${values.attached.length === 1 ? "Gateway" : "Gateways"} in ${values.mode} mode` : undefined } - successNextSteps={[`agentcore add policy --engine ${values.name}`, "agentcore deploy"]} + // Bare, so it opens the policy wizard, which asks for the engine; naming + // the engine with --engine would select the headless path instead. + successNextSteps={["agentcore add policy", "agentcore deploy"]} onDone={() => navigate(ADD_MENU)} doneLabel="go back" > diff --git a/src/handlers/project/add/policy/policy.screen.test.tsx b/src/handlers/project/add/policy/policy.screen.test.tsx index bc907363aa..373910aae3 100644 --- a/src/handlers/project/add/policy/policy.screen.test.tsx +++ b/src/handlers/project/add/policy/policy.screen.test.tsx @@ -151,11 +151,12 @@ describe("project add policy wizard", () => { await waitForText(screen.lastFrame, "❯ ● log-only"); await screen.press("return"); - await waitForText(screen.lastFrame, "this Policy will be added to agentcore.json"); + // The file is read when the review mounts, so the phase row fills in a + // moment after the rest of the review. + await waitForFlatText(screen.lastFrame, "authorization phase RETURN_OUTPUT"); const review = flatFrame(screen.lastFrame); expect(review).toContain(`statement from ${cedarPath}`); expect(review).toContain("enforcement log-only"); - expect(review).toContain("authorization phase RETURN_OUTPUT"); await screen.press("return"); await waitForText(screen.lastFrame, "added Policy 'Suppress'"); @@ -186,6 +187,38 @@ describe("project add policy wizard", () => { screen.unmount(); }); + test("a statement file that is not valid UTF-8 is refused rather than decoded lossily", async () => { + const projectRoot = await withEngine(); + // Latin-1 bytes: a lossy decode would turn "café" into "caf�" and silently + // change what the policy matches. + await writeFile( + join(projectRoot, "latin1.cedar"), + Buffer.from('permit (principal, action, resource) when { context.tag == "café" };', "latin1"), + ); + const screen = renderScreen("/agentcore/add/policy"); + await reachSourceStep(screen, "Latin"); + await screen.press("down"); + await screen.press("return"); + await waitForText(screen.lastFrame, FILE_HELP); + await screen.write("latin1.cedar"); + await screen.press("return"); + await waitForText(screen.lastFrame, "should it enforce, or only log?"); + await screen.press("return"); + + // The review says so instead of guessing a phase from mangled text. + await waitForFlatText(screen.lastFrame, "must contain valid UTF-8"); + expect(screen.lastFrame()).toContain("this Policy will be added to agentcore.json"); + await screen.press("return"); + + // And the submit refuses it for the same reason, handing the form back. + await waitFor(() => !(screen.lastFrame() ?? "").includes("this Policy will be added")); + await waitForFlatText(screen.lastFrame, "must contain valid UTF-8"); + await screen.press("escape"); + await waitForText(screen.lastFrame, "this Policy will be added to agentcore.json"); + expect(await policiesOf(projectRoot)).toHaveLength(0); + screen.unmount(); + }, 15000); + test("an empty statement is refused", async () => { await withEngine(); const screen = renderScreen("/agentcore/add/policy"); diff --git a/src/handlers/project/add/policy/screen.tsx b/src/handlers/project/add/policy/screen.tsx index 258e590779..772e2abf44 100644 --- a/src/handlers/project/add/policy/screen.tsx +++ b/src/handlers/project/add/policy/screen.tsx @@ -1,6 +1,5 @@ -import { accessSync, constants, readFileSync, statSync } from "node:fs"; -import { readFile } from "node:fs/promises"; -import { useState } from "react"; +import { accessSync, constants, statSync } from "node:fs"; +import { useEffect, useState } from "react"; import { useQueryClient } from "@tanstack/react-query"; import { useNavigate } from "react-router"; import z from "zod"; @@ -15,7 +14,7 @@ import { Wizard, type Choice, } from "../../../../components/wizard"; -import { InputValidationError } from "../../../../errors"; +import { SourceResolver } from "../../../../io"; import { PolicyNameSchema, type PolicyEngine } from "../../../../projectSchemas/policy"; import { ProjectKey } from "../../../../router"; import type { ScreenProps } from "../../../types"; @@ -85,19 +84,21 @@ type PolicyFormValues = { enforcement: PolicyEnforcementMode; }; -// statementOf reads the statement the review and the submit both work from: -// the pasted text, or the file's contents. The file is read synchronously here -// because the review needs the inferred phase before anything is written; it is -// small, local, and already checked to be readable on the source step. -function statementOf(values: PolicyFormValues): string | undefined { - if (values.source === "inline") return values.statement; - try { - return readFileSync(values.statementFile, "utf8"); - } catch { - return undefined; - } +// readStatementFile reads a statement the way `--statement file://…` does: +// through SourceResolver, so a file that is not valid UTF-8 is refused rather +// than decoded with replacement characters that would change what the policy +// matches. Nothing here reads stdin, so the resolver gets none. +function readStatementFile(path: string): Promise { + return new SourceResolver({}).resolveText("statement", `file://${path}`); } +// StatementLoad is what the review knows about the statement: a pasted one is +// ready at once; a file is read when the review mounts. +type StatementLoad = + | { state: "reading" } + | { state: "ready"; statement: string } + | { state: "failed"; message: string }; + // toPolicyInput is the answers as the flag path would state them; the phase is // left for the shared builder to infer, exactly as an omitted // --authorization-phase is. --description and --validation-mode stay flag-only. @@ -119,8 +120,7 @@ function firstLine(text: string): string { return rest > 0 ? `${shown} (+${rest} more ${rest === 1 ? "line" : "lines"})` : shown; } -function summaryOf(values: PolicyFormValues): Record { - const statement = statementOf(values); +function summaryOf(values: PolicyFormValues, load: StatementLoad): Record { return { "policy engine": values.engine, policy: values.name, @@ -130,12 +130,51 @@ function summaryOf(values: PolicyFormValues): Record { // The phase is a substring heuristic over the statement, so the guess is // shown before it is written; --authorization-phase overrides it. "authorization phase": - statement === undefined - ? "(file unreadable)" - : `${inferAuthorizationPhase(statement)} · inferred from the statement`, + load.state === "reading" + ? `reading ${values.statementFile}…` + : load.state === "failed" + ? `(statement unreadable: ${load.message})` + : `${inferAuthorizationPhase(load.statement)} · inferred from the statement`, }; } +// PolicyReview loads a file-sourced statement when the review step mounts — +// asynchronously, so the TUI never blocks on a read, and only after the source +// step has checked that the path is a readable file. A pasted statement needs +// no loading. +function PolicyReview({ values }: { values: PolicyFormValues }) { + const [load, setLoad] = useState(() => + values.source === "inline" + ? { state: "ready", statement: values.statement } + : { state: "reading" }, + ); + + // A pasted statement is ready from the initializer; only a file has anything + // to load, and it resolves through the promise rather than in the effect body. + useEffect(() => { + if (values.source !== "file") return; + let cancelled = false; + readStatementFile(values.statementFile).then( + (statement) => { + if (!cancelled) setLoad({ state: "ready", statement }); + }, + (error: unknown) => { + if (!cancelled) { + setLoad({ + state: "failed", + message: error instanceof Error ? error.message : String(error), + }); + } + }, + ); + return () => { + cancelled = true; + }; + }, [values.source, values.statementFile]); + + return ; +} + function engineChoices(engines: readonly PolicyEngine[]): Choice[] { return engines.map((engine) => ({ value: engine.name, @@ -181,18 +220,11 @@ function AddPolicyWizard({ project, core }: { project: Project; core: ScreenProp onCancel={() => navigate(ADD_MENU)} onSubmit={async function* () { // Read the file at submit, the way `--statement file://…` does, so what - // is written is what the file holds now. - let statement = values.statement; - if (values.source === "file") { - try { - statement = await readFile(values.statementFile, "utf8"); - } catch (error) { - throw new InputValidationError( - `could not read the statement from '${values.statementFile}'`, - { cause: error }, - ); - } - } + // is written is what the file holds now, decoded strictly. + const statement = + values.source === "file" + ? await readStatementFile(values.statementFile) + : values.statement; const updated = yield* core.projectManager.addResource( project, toAddPolicyInput(toPolicyInput(values, statement)), @@ -273,7 +305,7 @@ function AddPolicyWizard({ project, core }: { project: Project; core: ScreenProp - + ); diff --git a/src/io/source.ts b/src/io/source.ts index 8dfa73a64b..5454ca8c5d 100644 --- a/src/io/source.ts +++ b/src/io/source.ts @@ -8,7 +8,9 @@ const FILE_PREFIX = "file://"; const STDIN = "-"; export type SourceResolverConfig = { - stdin: NodeJS.ReadStream; + // Omitted by callers that only ever resolve inline values and files — a + // screen, say — in which case '-' is refused rather than read. + stdin?: NodeJS.ReadStream; signal?: AbortSignal; }; @@ -70,6 +72,9 @@ export class SourceResolver { } private async readStdin(name: string): Promise { + if (this.config.stdin === undefined) { + throw new SourceResolutionError(`'--${name}' cannot be read from stdin here`); + } if (this.stdinClaimedBy !== undefined) { throw new SourceResolutionError( `only one option may read from stdin; '--${name}' conflicts with ` +