diff --git a/packages/cli/src/authorship-profile.ts b/packages/cli/src/authorship-profile.ts index c4574c33..dcdc7a0c 100644 --- a/packages/cli/src/authorship-profile.ts +++ b/packages/cli/src/authorship-profile.ts @@ -49,6 +49,7 @@ import { createAcpxProvider } from "@executablemd/acp"; import type { AcpxProviderDependencies } from "@executablemd/acp"; import { InMemoryStream } from "@executablemd/durable-streams"; import { API } from "@executablemd/runtime"; +import { FormOpener } from "@executablemd/web"; import { hostAcpDependencies } from "./agent-stack.ts"; import type { AgentStack } from "./agent-stack.ts"; @@ -143,10 +144,12 @@ export function* runPlanCommandDocument(profile: AuthorshipProfile): Operation> { @@ -161,7 +164,7 @@ export function* runPlanCommandDocument(profile: AuthorshipProfile): Operation`, `` or anything else. - includes: [], - props: { - request: profile.request, - syntax: profile.syntax, - session: profile.session, + // The refusals go on a scope holding the document and nothing else, so + // what the ceiling covers is what it says it covers. It cannot separate + // the host's own acts from the document's on its own — a provider's work + // happens inside this execution, and `API.Process.exec` looks the same + // whichever party reached it — which is why those acts are stated above + // and run in the host's scope (src/host-acts.ts). + const approved = yield* scoped(function* (): Operation { + yield* refuseDocumentCapabilities(); + return yield* collect( + yield* executeInstalled( + { + ...retainedSource(PLAN_COMMAND_IDENTITY, source), + // Invocation-owned and thrown away with the scope. Ordinary + // document and Prompt semantics need a durable stream; nothing + // about writing a Plan needs a durable one, and `--journal` + // belongs to the Plan you approved rather than to the + // conversation that wrote it. + stream: new InMemoryStream(), + // No repository component search. What the document may name is + // what this profile declares, so a file in the caller's tree + // cannot answer for ``, `` or anything else. + includes: [], + props: { + request: profile.request, + syntax: profile.syntax, + session: profile.session, + }, }, - }, - [{ components: [...agentIdentityComponents(), validator(profile)] }], - ), - ); + [{ components: [...agentIdentityComponents(), validator(profile)] }], + ), + ); + }); if (typeof approved !== "string") { return Err(new Error("the plan command document returned something that is not a Plan")); } @@ -269,10 +282,10 @@ function validator(profile: AuthorshipProfile): IdentityComponent { * through ACPX's published pins and could reach neither (#672). * * Putting one on disk runs `npm install`, which is the one thing this profile - * refuses to everything inside it. So preparation runs in `host` — the scope - * this command was called in, which the refusals below were never installed on. - * The distinction is the whole of it: the document decides what to write and may - * run nothing, while the host installs the adapter it was always going to launch. + * refuses to everything inside it, so the host states that act as its own and it + * runs in `host` (src/host-acts.ts). The distinction is the whole of it: the + * document decides what to write and may run nothing, while the host installs + * the adapter it was always going to launch. * * Exported for the suite that pins exactly that: what a provider is built from * is not observable through a provider, and a case that could only watch a turn @@ -302,12 +315,39 @@ export function authorshipCeiling( } /** - * Run one operation in a scope this one is nested inside, and wait for it there. + * Open this host's own review form the way this host opens anything. + * + * A ceiling refuses *ambiently*: the middleware sits on a scope, and a call + * carries no mark saying who made it — `API.Process.exec` looks the same whether + * an `exec` fence reached it or this host did. So showing a person the review, + * which runs `open`, `xdg-open` or `start`, was refused as though the document + * had asked, and `xmd plan` printed its URL and warned that it could not open it. + * + * The act is the host's: its provider asking its question, about a URL it is + * serving, decided by no document, agent or authored element. Only the opening + * moves — a file, a command, the network and a service stay refused — and a + * failed launch is still a warning printed beside a URL that stands on its own. + */ +function openFormsThroughHost(host: Scope): Operation { + return FormOpener.around({ + *open([url], next): Operation { + yield* inScope(host, () => next(url)); + }, + }); +} + +/** + * Run one operation in a scope this one is nested inside, and wait for it here. + * + * The waiting is what makes it this operation's work: a task created in an outer + * scope outlives its creator by construction, so the halt is registered before + * the wait and an ended command takes an unfinished act with it rather than + * leaving one running under a conversation that is over. * - * The wait is what makes it this operation's: a task created in an outer scope - * outlives the caller by construction, so the halt is registered before the wait - * and an ended command takes the work with it rather than leaving an install - * running under a conversation that is over. + * `@effectionx/scope-eval` answers a different question. Its worker decouples the + * call from the work — the operation finishes even when the caller is gone, which + * is what `persist`, `daemon` and `service` want from it and the opposite of what + * a host act wants. */ function* inScope(scope: Scope, operation: () => Operation): Operation { return yield* scoped(function* () { diff --git a/packages/cli/src/documents/plan-command.md b/packages/cli/src/documents/plan-command.md index 2900f58e..c9327fe3 100644 --- a/packages/cli/src/documents/plan-command.md +++ b/packages/cli/src/documents/plan-command.md @@ -155,10 +155,15 @@ you may ask the coding agent to explain what went wrong or stop. required: ["decision"], additionalProperties: false, if: { + type: "object", properties: { decision: { const: "Request changes" } }, required: ["decision"], }, - then: { required: ["feedback"], properties: { feedback: { type: "string", minLength: 1 } } }, + then: { + type: "object", + required: ["feedback"], + properties: { feedback: { type: "string", minLength: 1 } }, + }, }} > ### Original Prompt diff --git a/packages/cli/tests/plan-command-document.test.ts b/packages/cli/tests/plan-command-document.test.ts index 0eeed583..adc0776b 100644 --- a/packages/cli/tests/plan-command-document.test.ts +++ b/packages/cli/tests/plan-command-document.test.ts @@ -189,4 +189,27 @@ describe("the packaged plan command document", () => { expect(run.failure).toBe(undefined); expect(run.value).toBe(CANDIDATE); }); + + /** + * The review this document asks has to be servable as a browser form, which + * is how `xmd plan` asks it: `installWebElicitation` compiles the request's + * schema before a port exists. + * + * Compiling for the browser extracts each conditional branch and compiles it + * as a schema of its own, so a branch that reached `required` through its + * parent's type is refused there while the server accepts it + * (`packages/web/tests/compile.test.ts`, and specs/web-form-spec.md + * §The preflight boundary). Until both branches said `object`, a real + * `xmd plan` completed its turn and then ended at the review with + * ` schema could not be compiled for the browser`. + */ + it("C9: every conditional branch of the review schema declares its own type", function* () { + const run = yield* useWorkingDirectory(function* () { + return yield* runDocument(); + }); + + const schema = Object(run.reviews[0]?.schema); + expect(Reflect.get(Object(Reflect.get(schema, "if")), "type")).toBe("object"); + expect(Reflect.get(Object(Reflect.get(schema, "then")), "type")).toBe("object"); + }); }); diff --git a/packages/cli/tests/plan-host-acts.test.ts b/packages/cli/tests/plan-host-acts.test.ts new file mode 100644 index 00000000..669ae2a3 --- /dev/null +++ b/packages/cli/tests/plan-host-acts.test.ts @@ -0,0 +1,102 @@ +/** + * Tier PH — the acts `xmd plan` performs as the host + * (specs/plan-command-spec.md §The authorship profile, + * specs/acp-client-spec.md §The `xmd plan` authorship profile). + * + * The profile refuses the command document a command, and two of the things the + * command itself does run one: it installs this build's ACP adapter, and it opens + * the review form in a browser. Neither is the document's act, and both were + * refused as though they were — the second one visibly, as + * `could not open a browser automatically (xmd plan asked for a command, …)` + * printed beside the URL a person then had to open by hand. + * + * These drive the real command with the real profile. What stands in for the + * outside world is the command itself: an `API.Process` recorder answers instead + * of spawning, installed at `min` so the profile's own refusal still outranks it + * wherever it applies — a recorder at full strength would answer for a refused + * call too, and these cases would pass against the defect. + */ + +import { describe, it } from "@executablemd/test-support/bdd"; +import { expect } from "@executablemd/test-support/expect"; +import { join } from "node:path"; +import { API } from "@executablemd/runtime"; +import { FormOpener } from "@executablemd/web"; +import type { Operation } from "effection"; + +import { runPlan } from "../src/plan.ts"; +import type { PlanCommand } from "../src/plan.ts"; +import { scanPlanArgs } from "../src/plan-args.ts"; +import type { AgentStack } from "../src/agent-stack.ts"; +import { ADAPTERS, AGENT, createPlanHarness, useWorkingDirectory } from "./support/plan-harness.ts"; +import type { PlanHarness } from "./support/plan-harness.ts"; + +const REQUEST = "write a greeting"; + +/** A Plan the host's validator accepts. */ +const PLAN = ['the draft ran', ""].join("\n"); + +const STACK: AgentStack = { + provider: "acpx", + defaultAgent: AGENT, + permissionMode: "deny-all", + adapters: ADAPTERS, +}; + +function writing(dir: string, output: string): PlanCommand { + const argv = ["plan", REQUEST]; + return { argv, scan: scanPlanArgs(argv), include: [dir], output, run: false, stack: STACK }; +} + +/** Every command this invocation reached, answered rather than spawned. */ +function* recordCommands(commands: string[][]): Operation { + yield* API.Process.around( + { + // deno-lint-ignore require-yield + *exec([options]) { + commands.push([...options.command]); + return { exitCode: 0, stdout: "", stderr: "" }; + }, + }, + { at: "min" }, + ); +} + +/** + * A harness whose review opens a form first, the way the CLI's own does. + * + * `installWebElicitation` announces the URL and asks `FormOpener` to open it + * before it waits for an answer; this is that one act, without a port, a page or + * a browser. + */ +function openingHarness(harness: PlanHarness, url: string): PlanHarness { + const scripted = harness.deps.installElicitation; + harness.deps.installElicitation = function* (): Operation { + yield* FormOpener.operations.open(url); + yield* scripted(); + }; + return harness; +} + +describe("Tier PH — the acts xmd plan performs as the host", () => { + it("PH1: opening the review form reaches a command the document cannot", function* () { + yield* useWorkingDirectory(function* (dir, authorshipRoot) { + const commands: string[][] = []; + yield* recordCommands(commands); + + const url = "http://127.0.0.1:0/f/token/"; + const harness = openingHarness(createPlanHarness({ authorshipRoot }), url); + harness.fake.script({ reply: PLAN }); + harness.script({ decision: "Approve" }); + + const code = yield* runPlan(writing(dir, join(dir, "plan.md")), harness.deps); + + // The command that opens a browser on this platform, whichever it is, with + // the URL the form is being served at. + expect(commands).toHaveLength(1); + expect(commands[0]).toContain(url); + expect(code).toBe(0); + expect(harness.reviews).toHaveLength(1); + }); + }); +}); diff --git a/packages/web/mod.ts b/packages/web/mod.ts index addefa3f..6ef87cd6 100644 --- a/packages/web/mod.ts +++ b/packages/web/mod.ts @@ -7,9 +7,16 @@ * elicitations with the same form, which is how `` reaches a person * under the CLI. `liveForm()` is the browser interaction on its own, for a host * that wants an answer without either component. + * + * `FormOpener` is how any of them asks for the URL to be opened. A host composes + * around it to say where that act belongs — a profile that refuses the document a + * command still opens its own form — and a failed open is a warning: the URL is + * printed first and the form keeps waiting either way. */ export { installWebComponents, WEB_REGISTRATIONS } from "./src/components.ts"; export { installWebElicitation } from "./src/elicitation.ts"; export { liveForm } from "./src/live-form.ts"; export type { LiveFormInput } from "./src/live-form.ts"; +export { FormOpener } from "./src/opener.ts"; +export type { FormOpenerApi } from "./src/opener.ts"; diff --git a/packages/web/tests/compile.test.ts b/packages/web/tests/compile.test.ts index 15c23966..8d5b8697 100644 --- a/packages/web/tests/compile.test.ts +++ b/packages/web/tests/compile.test.ts @@ -232,6 +232,66 @@ describe("compile: the server is built the way the browser is", () => { }); } }); + + /** + * A conditional branch is compiled on its own, so it declares its own type. + * + * The generator does not compile the schema as one piece: RJSF's schema parser + * extracts `if`, `then` and the rest into separate schemas and compiles each as + * a root. A branch that leaned on its parent for `type` is then a schema with + * `required` and no type at all, which strict mode refuses — while the server, + * compiling the whole thing in one piece, reads the type from the parent and + * accepts it. + * + * So the two sides disagree about a schema an author can write, and the + * disagreement surfaces at the moment the form is asked for. This is the + * authoring rule that follows, pinned in both directions. + */ + it("refuses a conditional branch that leans on its parent for a type", function* () { + const branchWithoutType = { + type: "object", + properties: { decision: { type: "string" }, feedback: { type: "string" } }, + required: ["decision"], + if: { properties: { decision: { const: "Request changes" } }, required: ["decision"] }, + then: { required: ["feedback"], properties: { feedback: { type: "string", minLength: 1 } } }, + }; + // The server compiles it, which is the disagreement: only the message the + // browser generator produced says this schema cannot be served. + expect(() => compileForm(parseDeclaration("WebForm", branchWithoutType))).toThrow( + SchemaCompileError, + ); + expect(() => compileForm(parseDeclaration("WebForm", branchWithoutType))).toThrow( + /compiled for the browser[\s\S]*strictTypes/, + ); + + const branchWithType = { + ...branchWithoutType, + if: { + type: "object", + properties: { decision: { const: "Request changes" } }, + required: ["decision"], + }, + then: { + type: "object", + required: ["feedback"], + properties: { feedback: { type: "string", minLength: 1 } }, + }, + }; + const compiled = compileForm(parseDeclaration("WebForm", branchWithType)); + // More than one, which is the mechanism itself: the branches were extracted + // and compiled as schemas of their own, which is why each needs a type. + const registration = yield* runValidatorScript(compiled.validatorScript); + expect(Object.keys(registration.validateFns).length).toBeGreaterThan(1); + + const accepted: JsonObject[] = [ + { decision: "Approve" }, + { decision: "Request changes", feedback: "say more" }, + ]; + for (const data of accepted) { + expect({ data, server: compiled.validate(data) }).toEqual({ data, server: true }); + } + expect(compiled.validate({ decision: "Request changes" })).toBe(false); + }); }); describe("compile: what the browser receives is JSON, not source", () => { diff --git a/specs/acp-client-spec.md b/specs/acp-client-spec.md index 2df50065..18544649 100644 --- a/specs/acp-client-spec.md +++ b/specs/acp-client-spec.md @@ -581,11 +581,18 @@ network capability, and the host decides for that whole execution that a failing `` ends it — so a turn that streamed text and then failed presents nothing. -That refusal covers the document, and installing this build's adapter is not the -document's act: it runs a command, and it runs it in the scope the invocation was -called in rather than inside the profile. Nothing the document, the assistant or -an authored element reaches can get there — the host prepares the adapter it was -always going to launch, and an ended command takes an unfinished install with it. +Those refusals are installed on a scope holding the document execution and +nothing else, so what the ceiling covers is what it says it covers. + +It cannot tell the host's own acts from the document's on its own. A call carries +no mark saying who made it, and a provider's work happens inside the execution +that reached it — core constructs a root provider from within the document — so a +refusal over that scope covers both parties. Two things the command does are the +host's, and both run a command: installing this build's adapter, and opening the +review form in a browser. The command therefore takes its own scope before it +installs the ceiling and states those two acts as its own; they run there, and +everything else stays refused. An ended command takes an unfinished install with +it, and a failed open stays a warning beside the printed URL. The profile's working directory is derived from the logical session name rather than shared or freshly made: `~/.xmd/plan/sessions/`, with the diff --git a/specs/plan-command-spec.md b/specs/plan-command-spec.md index 846ce3ff..f03bffb6 100644 --- a/specs/plan-command-spec.md +++ b/specs/plan-command-spec.md @@ -288,10 +288,12 @@ provider: document itself. The provider may use its own transport to perform the model turn; that does not -grant the Agent a native network tool. Nor does putting this build's own ACP -adapter on disk, which runs a command in the scope that invoked `xmd plan`: the -document is refused a command, and the host still installs the adapter it is -about to launch ([`xmd run` and `xmd plan`](./acp-client-spec.md)). +grant the Agent a native network tool. Nor do the two acts the command performs +as the host — putting this build's own ACP adapter on disk, and opening the +review form in a browser — each of which runs a command in the scope that invoked +`xmd plan`. The document is refused a command; the host still installs the +adapter it is about to launch and opens the form it is already serving +([`xmd run` and `xmd plan`](./acp-client-spec.md)). `--approve-all`, `--approve-reads` and `--deny-all` do not change this ceiling. They apply to the approved Plan later. A provider that cannot establish this diff --git a/specs/web-form-spec.md b/specs/web-form-spec.md index 2c3991b8..7c7eee9e 100644 --- a/specs/web-form-spec.md +++ b/specs/web-form-spec.md @@ -50,6 +50,14 @@ same-document `$ref` that resolves to nothing is valid draft-07 and unusable. Ha compilation happened during the run, that schema would have read the browser assets, begun a durable operation, and recorded its failure. +Compiling for the browser takes the schema apart: `if`, `then`, `else` and the +other subschema keywords are extracted and compiled as schemas of their own. A +branch therefore declares its own `type` — one that reached `required` through +its parent's type is a typeless schema by the time it is compiled, and strict +mode refuses it. The server, compiling the whole schema in one piece, accepts +that same branch, so this is the one shape the two sides read differently and the +refusal names which side produced it. + ## The live form `liveForm()` is the browser interaction without the component around it, so