🐛 Serve the plan review: type its schema, and open it as the host

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<R
     );
   }
 
-  // Taken before the profile's own scope, because putting this build's adapter
-  // on disk is host work and the profile refuses a command to everything inside
-  // it. The refusals are installed on the scope below, so this one still answers
-  // with the capabilities the entrypoint gave this invocation.
+  // Taken before the profile's own scope, because two of the things this command
+  // does are the host's rather than the document's — putting this build's adapter
+  // on disk, and opening the review form — and both run a command, which the
+  // profile refuses to everything inside it. The refusals are installed on the
+  // scope below, so this one still answers with the capabilities the entrypoint
+  // gave this invocation.
   const host = yield* useScope();
 
   return yield* scoped(function* (): Operation<Result<string>> {
@@ -161,7 +164,7 @@ export function* runPlanCommandDocument(profile: AuthorshipProfile): Operation<R
     }
     const workdir = established.value;
 
-    yield* refuseDocumentCapabilities();
+    yield* openFormsThroughHost(host);
     yield* profile.installElicitation();
 
     const acpx = createAcpxProvider(authorshipCeiling(profile, workdir, host));
@@ -184,28 +187,38 @@ export function* runPlanCommandDocument(profile: AuthorshipProfile): Operation<R
 
     const source = yield* readPackagedDocument(PLAN_COMMAND_DOCUMENT);
     try {
-      const approved = 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 `<CheckDraft>`, `<Prompt>` 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<Json> {
+        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 `<CheckDraft>`, `<Prompt>` 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<void> {
+  return FormOpener.around({
+    *open([url], next): Operation<void> {
+      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<T>(scope: Scope, operation: () => Operation<T>): Operation<T> {
   return yield* scoped(function* () {

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

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
+   * `<WebForm> 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");
+  });
 });

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 = ['<File path="drafted.txt">the draft ran</File>', ""].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<void> {
+  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<void> {
+    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);
+    });
+  });
+});

packages/web/mod.ts

@@ -7,9 +7,16 @@
  * elicitations with the same form, which is how `<Elicit>` 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";

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", () => {