🐛 Keep xmd repl usable after repeated component calls

packages/cli/src/repl/model.ts

@@ -740,8 +740,8 @@ function buildEntry(segment: EntrySegment, markers: readonly string[]): Result<E
   /** Every retained turn, in append order, before the chronology is ordered. */
   const turns: ReplAgentTurn[] = [];
   let terminal: ReplTerminal | undefined;
-  /** Every scope by the source path it was admitted from, for owner lookup. */
-  const byPath = new Map<string, ScopeDraft[]>();
+  /** Every admission by the source path it was admitted from, for owner lookup. */
+  const byPath = new Map<string, Admission[]>();
   /** Every generated fragment this prefix admitted, by the identity it carries. */
   const fragments: GeneratedOwner[] = [];
   /**
@@ -780,7 +780,7 @@ function buildEntry(segment: EntrySegment, markers: readonly string[]): Result<E
     generated: [],
     scopes: [],
   };
-  register(byPath, entry);
+  register(byPath, { ...site(segment.admission, 0), scope: entry });
   transcript.push({
     kind: "entry",
     marker: markers[0],
@@ -888,7 +888,7 @@ function buildEntry(segment: EntrySegment, markers: readonly string[]): Result<E
         scopes: [],
       };
       owner.value.scope.scopes.push(scope);
-      register(byPath, scope);
+      register(byPath, { ...site(event, index), scope });
       transcript.push({
         kind: "scope",
         marker,
@@ -1330,32 +1330,41 @@ function bind(scope: ScopeDraft, name: string, value: Json): void {
   scope.bindings[at] = binding;
 }
 
-function register(index: Map<string, ScopeDraft[]>, scope: ScopeDraft): void {
-  const held = index.get(scope.path);
+function register(index: Map<string, Admission[]>, admission: Admission): void {
+  const held = index.get(admission.scope.path);
   if (held === undefined) {
-    index.set(scope.path, [scope]);
+    index.set(admission.scope.path, [admission]);
     return;
   }
-  held.push(scope);
+  held.push(admission);
+}
+
+/**
+ * One scope this prefix admitted, with the two facts that say whether a
+ * candidate effect could have come from it: which coroutine admitted it, and
+ * how far into the history that was. An admission record is written before the
+ * work inside it runs, so everything that work records sits after the
+ * admission, on its coroutine or on one spawned beneath it.
+ */
+interface Admission {
+  readonly coroutine: string;
+  /** Where the admission sits in this prefix, so only later work can be its. */
+  readonly order: number;
+  readonly scope: ScopeDraft;
 }
 
 /**
  * One generated fragment this prefix admitted, as its own effects name it.
  *
  * Generated source is not a file, so the engine records the work inside it at a
  * position carrying the fragment's identity instead of a path — the id the
- * admission was decided under. This is that identity, with the two facts that
- * say whether a candidate effect could have come from it: which coroutine
- * admitted it, and how far into the history that was.
+ * admission was decided under. This is that identity, with the admission it
+ * was decided by.
  */
-interface GeneratedOwner {
+interface GeneratedOwner extends Admission {
   readonly id: string;
   /** The entry that admitted it. One execution holds one, and this states it. */
   readonly entry: string;
-  readonly coroutine: string;
-  /** Where the admission sits in this prefix, so only earlier work can be its. */
-  readonly order: number;
-  readonly scope: ScopeDraft;
 }
 
 /** Where one recorded effect sits: on which coroutine, and how far in. */
@@ -1421,18 +1430,21 @@ function descendsFrom(ancestor: string, coroutine: string): boolean {
  * The one scope an effect's source position belongs to.
  *
  * Two attributions, and a position states which one it is. A position naming a
- * path belongs to the scope admitted from that path. A position naming a
- * generated fragment belongs to the scope that fragment's admission created —
- * chosen by the identity the effect itself carries, with journal order and
- * coroutine ancestry deciding only whether that candidate could be its owner: an
- * admission that happened afterwards, or on work this effect is not part of, is
- * not an owner however recently it ran. Nothing is chosen by the latest
+ * path belongs to the scope admitted from that path; a position naming a
+ * generated fragment belongs to the scope that fragment's admission created. In
+ * both, the position chooses the candidates — a path, or the identity the effect
+ * itself carries — and journal order and coroutine ancestry decide only whether
+ * a candidate could be its owner: an admission that happened afterwards, or on
+ * work this effect is not part of, is not an owner however recently it ran. A
+ * component called once in each of two spawned branches is therefore two owners
+ * that no effect can confuse, while the same call repeated on one coroutine is
+ * two that nothing recorded tells apart. Nothing is chosen by the latest
  * admission, the current scope, an effect's name or its line and column: a
  * binding attached to a guessed owner is a value shown in the wrong place, and
  * there is no spelling of "probably this one" that a reader could check.
  */
 function ownerOf(
-  index: Map<string, ScopeDraft[]>,
+  index: Map<string, Admission[]>,
   fragments: readonly GeneratedOwner[],
   announced: ReadonlySet<string>,
   position: ReplPosition | undefined,
@@ -1450,15 +1462,39 @@ function ownerOf(
   if (position.path === undefined) {
     return generatedOwnerOf(fragments, announced, position, where, subject);
   }
-  const held = index.get(position.path) ?? [];
+  return pathOwnerOf(index.get(position.path) ?? [], position, where, subject);
+}
+
+/**
+ * The admitted source one path-positioned effect belongs to.
+ *
+ * Each way that fails is its own refusal, because "nothing owns this" and "two
+ * things might" are different damage and a reader acting on either needs to
+ * know which they have.
+ */
+function pathOwnerOf(
+  held: readonly Admission[],
+  position: ReplPosition,
+  where: EffectSite,
+  subject: string,
+): Result<{ scope: ScopeDraft; position: ReplPosition }> {
   if (held.length === 0) {
     return Err(
       new ReplProjectionError(
         `${subject} names a source this entry never admitted, so nothing owns it.`,
       ),
     );
   }
-  if (held.length > 1) {
+  const owning = couldOwn(held, where);
+  if (owning.length === 0) {
+    return Err(
+      new ReplProjectionError(
+        `${subject} names a source this entry admitted on work it is not part of, so nothing ` +
+          "owns it.",
+      ),
+    );
+  }
+  if (owning.length > 1) {
     return Err(
       new ReplProjectionError(
         `${subject} names a source this entry admitted more than once, so which occurrence owns ` +
@@ -1469,7 +1505,18 @@ function ownerOf(
   // The position travels back with the scope: an effect that has an owner has a
   // readable position by construction, and saying so here is what lets a caller
   // retain it without asking again whether it was there.
-  return Ok({ scope: held[0], position });
+  return Ok({ scope: owning[0]!.scope, position });
+}
+
+/**
+ * Which of the named admissions one effect could have come from: only one that
+ * had already happened, on this coroutine or on an ancestor of it.
+ */
+function couldOwn<T extends Admission>(named: readonly T[], where: EffectSite): T[] {
+  return named.filter(
+    (admission) =>
+      admission.order < where.order && descendsFrom(admission.coroutine, where.coroutine),
+  );
 }
 
 /**
@@ -1507,10 +1554,7 @@ function generatedOwnerOf(
       ),
     );
   }
-  // Only an admission this effect could have come from: one that had already
-  // happened, on this coroutine or on an ancestor of it.
-  const earlier = named.filter((fragment) => fragment.order < where.order);
-  const owning = earlier.filter((fragment) => descendsFrom(fragment.coroutine, where.coroutine));
+  const owning = couldOwn(named, where);
   if (owning.length === 0) {
     return Err(
       new ReplProjectionError(

packages/cli/tests/fixtures/repl/Advisor.md

@@ -0,0 +1,11 @@
+---
+title: One advisor, asked one question in a session of its own
+required: [session, question]
+props:
+  session:
+    type: string
+  question:
+    type: string
+---
+
+<Session name={props.session}><Prompt text={props.question} /></Session>

packages/cli/tests/repl-agent-execution.test.ts

@@ -64,7 +64,7 @@ import type { ReplSession } from "../src/repl/session.ts";
 import { useReplAgent } from "../src/repl/agent.ts";
 import type { ReplAgentKernel, ReplAgentReading } from "../src/repl/agent.ts";
 import { projectRepl } from "../src/repl/model.ts";
-import type { ReplAgentPermission } from "../src/repl/model.ts";
+import type { ReplAgentPermission, ReplModel } from "../src/repl/model.ts";
 import type { ReplExecution } from "../src/repl/journal.ts";
 
 /**
@@ -2348,3 +2348,96 @@ describe("EL2 — an entry's live Agent state is retired with its execution", ()
     });
   });
 });
+
+/** One Markdown component, called once in each of two branches (#941). */
+const TWO_ADVISORS = [
+  "<All>",
+  '<Spawn><Advisor session="architect" question="same" /></Spawn>',
+  '<Spawn><Advisor session="planner" question="same" /></Spawn>',
+  "</All>",
+].join("\n");
+
+/** The coroutine a marker was recorded on: `yield:root.1:0` was recorded on `root.1`. */
+function coroutineOf(marker: string): string {
+  return marker.split(":")[1] ?? "";
+}
+
+/**
+ * Two calls of `<Advisor />`, each owning the one turn its own branch asked.
+ *
+ * Which call is `Advisor-1` is the order the two admissions were journaled in,
+ * which two concurrent branches do not fix. What is fixed is that each turn
+ * sits under the call recorded on its own branch, so the check reads the branch
+ * off each record's marker rather than assuming an order.
+ */
+function eachCallOwnsItsTurn(model: ReplModel): void {
+  const entry = model.entries[0]!;
+  const calls = entry.scope.scopes;
+  expect(calls.map((scope) => [scope.name, scope.key])).toEqual([
+    ["Advisor", "Advisor-1"],
+    ["Advisor", "Advisor-2"],
+  ]);
+  expect(new Set(calls.map((scope) => coroutineOf(scope.marker))).size).toBe(2);
+  expect(entry.turns.map((turn) => turn.scope).sort()).toEqual(["Advisor-1", "Advisor-2"]);
+  expect(entry.turns.map((turn) => turn.sessionKey).sort()).toEqual([
+    "stub:architect",
+    "stub:planner",
+  ]);
+  for (const turn of entry.turns) {
+    expect(turn.status).toBe("completed");
+    expect(turn.text).toBe("reply");
+    const call = calls.find((scope) => scope.key === turn.scope)!;
+    const branch = coroutineOf(call.marker);
+    const asked = coroutineOf(turn.marker);
+    expect(asked === branch || asked.startsWith(`${branch}.`)).toBe(true);
+  }
+}
+
+describe("R1 — one component called in two branches", () => {
+  beforeAll(() => useTempFileCompiler());
+
+  it("R1: each call keeps its own turn, the next entry is admitted, and a cold read repeats nothing", function* () {
+    const holder = execution();
+    let written: string[] = [];
+    yield* scoped(function* () {
+      const stub = createStub({ same: { deltas: ["reply"] } });
+      yield* useStub(stub);
+      const session = opened(yield* start(holder, TWO_ADVISORS, "deny-all", [REFERENCE_DIRECTORY]));
+      const outcome = yield* session.join();
+      expect(outcome.ok).toBe(true);
+      expect(stub.asked).toEqual(["same", "same"]);
+
+      eachCallOwnsItsTurn(session.model);
+
+      // The completed entry stays readable, and does not stand in the way of
+      // the next one.
+      const submitted = yield* session.submit('<Json value={{ after: 1 }} as="after" />\n');
+      expect(submitted.ok).toBe(true);
+      const second = yield* session.join();
+      expect(second.ok).toBe(true);
+      expect(session.model.entries.map((one) => one.key)).toEqual(["entry-1", "entry-2"]);
+      expect(stub.asked).toEqual(["same", "same"]);
+      written = serialized(yield* holder.stream.readAll());
+    });
+
+    yield* scoped(function* () {
+      const stub = createStub();
+      yield* useStub(stub);
+      const cold = execution(yield* holder.stream.readAll());
+      const reopened = opened(
+        yield* openReplSession({
+          execution: cold,
+          includes: [REFERENCE_DIRECTORY],
+          installations: installations(),
+          generated: ordinaryGeneratedSelection(),
+        }),
+      );
+      yield* reopened.join();
+      // The saved work is read, and nothing is asked or written again.
+      expect(stub.asked).toEqual([]);
+      expect(reopened.model.entries.map((one) => one.key)).toEqual(["entry-1", "entry-2"]);
+      eachCallOwnsItsTurn(reopened.model);
+      expect(serialized(yield* cold.stream.readAll())).toEqual(written);
+    });
+  });
+});