diff --git a/packages/cli/src/repl/model.ts b/packages/cli/src/repl/model.ts index 304e8ec83..fb42234a1 100644 --- a/packages/cli/src/repl/model.ts +++ b/packages/cli/src/repl/model.ts @@ -740,8 +740,8 @@ function buildEntry(segment: EntrySegment, markers: readonly string[]): Result(); + /** Every admission by the source path it was admitted from, for owner lookup. */ + const byPath = new Map(); /** 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, scope: ScopeDraft): void { - const held = index.get(scope.path); +function register(index: Map, 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; } /** @@ -1344,18 +1358,13 @@ function register(index: Map, scope: ScopeDraft): void { * * 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, + index: Map, fragments: readonly GeneratedOwner[], announced: ReadonlySet, position: ReplPosition | undefined, @@ -1450,7 +1462,22 @@ 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( @@ -1458,7 +1485,16 @@ function ownerOf( ), ); } - 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(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( diff --git a/packages/cli/tests/fixtures/repl/Advisor.md b/packages/cli/tests/fixtures/repl/Advisor.md new file mode 100644 index 000000000..458d5b145 --- /dev/null +++ b/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 +--- + + diff --git a/packages/cli/tests/repl-agent-execution.test.ts b/packages/cli/tests/repl-agent-execution.test.ts index 409078981..03957916b 100644 --- a/packages/cli/tests/repl-agent-execution.test.ts +++ b/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 = [ + "", + '', + '', + "", +].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 ``, 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('\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); + }); + }); +}); diff --git a/packages/cli/tests/repl-model.test.ts b/packages/cli/tests/repl-model.test.ts index 7d49ebf13..6e38a5dbc 100644 --- a/packages/cli/tests/repl-model.test.ts +++ b/packages/cli/tests/repl-model.test.ts @@ -1299,3 +1299,110 @@ describe("REPL model: which generated fragment owns pathless work", () => { yield* useTempFileCompiler(); }); }); + +/** `` called on one coroutine, where the record of that call lands. */ +function calledPlan(coroutine: string): DurableEvent { + return yielded( + { type: "import_component", name: "Plan" }, + declared(), + { path: "", offset: 0, line: 1, column: 1 }, + coroutine, + ); +} + +/** One question asked inside the packaged Plan's source, on one coroutine. */ +function askedInPlan(coroutine: string): DurableEvent { + return yielded( + { type: "elicit", name: `elicit:${PLAN_ORIGIN}:4:1`, [SCHEMA_FIELD]: { type: "object" } }, + { decision: "Approve" }, + insidePlan(), + coroutine, + ); +} + +/** The entry's own admission, before it has called anything. */ +function entryAdmitted(): DurableEvent[] { + return planJournal().slice(0, 1); +} + +describe("REPL model: which call of a repeated source owns its work", () => { + it("RS1: a component called once in each of two branches keeps each call's own work", function* () { + // Two spawned branches each call ``, and the second branch's + // question settles before the first's. The third question is asked on work + // the second branch spawned beneath itself. + const events = [ + ...entryAdmitted(), + calledPlan("root.0"), + calledPlan("root.1"), + askedInPlan("root.1"), + askedInPlan("root.0"), + askedInPlan("root.1.0"), + ]; + const model = projected(events); + const entry = only(model); + if (entry === undefined) { + throw new Error("the journal admits one entry"); + } + + expect(entry.scopes.map((scope) => scope.key)).toEqual(["Plan-1", "Plan-2"]); + expect(child(entry, "Plan-1").elicitations).toHaveLength(1); + expect(child(entry, "Plan-2").elicitations).toHaveLength(2); + expect(entry.elicitations).toEqual([]); + expect(model.transcript.flatMap((row) => (row.kind === "elicit" ? [row.scope] : []))).toEqual([ + "Plan-2", + "Plan-1", + "Plan-2", + ]); + + // The same attribution from the protocol's own text form, so a cold reading + // of the saved work reaches it without anything live. + const cold = only(projected(copied(events))); + expect(cold?.scopes.map((scope) => [scope.key, scope.elicitations.length])).toEqual([ + ["Plan-1", 1], + ["Plan-2", 2], + ]); + yield* useTempFileCompiler(); + }); + + it("RS2: a call this work is not part of owns none of it, and two it could be refuse", function* () { + // A sibling branch's work: the one call is not above it. + expect(refusal([...entryAdmitted(), calledPlan("root.0"), askedInPlan("root.1")])).toContain( + "is not part of", + ); + // The parent's own work, after both branches called: neither call is above it. + expect( + refusal([ + ...entryAdmitted(), + calledPlan("root.0"), + calledPlan("root.1"), + askedInPlan("root"), + ]), + ).toContain("is not part of"); + // Work recorded before the call that would own it: when it ran, nothing had + // admitted that source. + expect(refusal([...entryAdmitted(), askedInPlan("root.0"), calledPlan("root.0")])).toContain( + "never admitted", + ); + // The same call repeated on one branch: nothing recorded tells them apart, + // and the later one is not chosen for being later. + expect( + refusal([ + ...entryAdmitted(), + calledPlan("root.0"), + calledPlan("root.0"), + askedInPlan("root.0"), + ]), + ).toContain("more than once"); + // A call above another call: both are above the work, and the inner one is + // not chosen for being nearer. + expect( + refusal([ + ...entryAdmitted(), + calledPlan("root"), + calledPlan("root.0"), + askedInPlan("root.0"), + ]), + ).toContain("more than once"); + yield* useTempFileCompiler(); + }); +});