From 0e1ba80245190daa7a21325201ad0e34b6ff1880 Mon Sep 17 00:00:00 2001 From: ManuelStaggl <17688572+ManuelStaggl@users.noreply.github.com> Date: Thu, 12 Nov 2026 15:35:29 +0100 Subject: [PATCH] fix(desktop): a check that runs beside a change no longer hides it from the view A trial (print check, joint sweep, dimension probe) and the autosave end with a quiet revision check. When one ended between a change and the change's own check, it recorded the change's revision, the loud check saw no movement and the view kept the old design. The headless engine now keeps the two apart where the revision is recorded: a change PartBuddy made itself is always told (changed()), and a quiet check that records a moved revision while a tool call of the assistant is under way leaves it for that call's check. A trial alone still tells nobody. Co-Authored-By: Claude Fable 5.2 --- apps/desktop/src/main/solidcraft/headless.ts | 62 ++++++++-- .../desktop/src/main/solidcraft/supervisor.ts | 24 ++-- apps/desktop/test/headless.test.ts | 107 ++++++++++++++++++ 3 files changed, 171 insertions(+), 22 deletions(-) create mode 100644 apps/desktop/test/headless.test.ts diff --git a/apps/desktop/src/main/solidcraft/headless.ts b/apps/desktop/src/main/solidcraft/headless.ts index ea54e36f..0dadbdff 100644 --- a/apps/desktop/src/main/solidcraft/headless.ts +++ b/apps/desktop/src/main/solidcraft/headless.ts @@ -32,6 +32,7 @@ export interface HeadlessOptions { /** Design file to open, or none for a new design. */ design?: string | undefined; env: NodeJS.ProcessEnv; + log?: (message: string) => void; } interface BatchBody { @@ -61,9 +62,16 @@ export class HeadlessEngine { private readonly changeListeners = new Set<(revision: Revision) => void>(); private readonly closeListeners = new Set<(reason: string) => void>(); private last: Revision | null = null; + /** Tool calls of the assistant that have not been checked yet. */ + private calls = 0; + /** A change the listeners have not been told of, although its revision is recorded already. */ + private untold = false; private closed = false; - private constructor(private readonly client: Client) {} + private constructor( + private readonly client: Client, + private readonly log: (message: string) => void = () => undefined, + ) {} /** Starts the engine and waits for the MCP handshake; rejects when the process exits first. */ static async start(options: HeadlessOptions): Promise { @@ -82,7 +90,7 @@ export class HeadlessEngine { }); const client = new Client({ name: 'partbuddy', version: '0.1.0' }); await client.connect(transport); - const engine = new HeadlessEngine(client); + const engine = new HeadlessEngine(client, options.log); client.onclose = () => { engine.shutdown('SolidCraft (headless) ended'); }; @@ -117,15 +125,48 @@ export class HeadlessEngine { } /** - * Reads the revision after a call that may have changed the design, and tells the listeners - * when it moved. `quiet` records it without telling (PartBuddy's own autosave). + * After a change PartBuddy made itself: the listeners are told, also when a quiet check that ran + * beside the change recorded its revision first (a trial of the print check ending, an autosave). */ - async checkForChange(quiet = false): Promise { - if (this.closed) return; - const next = await this.revision(); + async changed(): Promise { + this.untold = true; + await this.check(false); + } + + /** + * After work that moves the revision and leaves the design as it was (a trial, PartBuddy's own + * save): the revision is recorded and nobody is told, so the next read of the assistant mirrors + * nothing. + */ + async unchanged(): Promise { + await this.check(true); + } + + /** + * Reads the revision and tells the listeners when it moved or a change is still untold. A quiet + * check tells nobody; what it records while a tool call of the assistant is under way may be + * that call's change, so it leaves it for the call's own check to tell. + */ + private async check(quiet: boolean, endsCall = false): Promise { + let next: Revision | null = null; + try { + if (!this.closed) next = await this.revision(); + } finally { + // In one go with the check itself: a quiet check that comes right after it finds no call left. + if (endsCall) this.calls -= 1; + } + if (!next) return; const moved = this.last?.documentId !== next.documentId || this.last.revision !== next.revision; this.last = next; - if (moved && !quiet) for (const listener of this.changeListeners) listener(next); + if (quiet) { + if (moved && this.calls > 0) this.untold = true; + return; + } + if (!moved && !this.untold) return; + // The race this guards against, or a change that left the revision where it was. + if (!moved) this.log(`revision ${String(next.revision)} was recorded before its change was told; telling it now`); + this.untold = false; + for (const listener of this.changeListeners) listener(next); } /** The mirror's view of the engine: the same two requests the control channel answers. */ @@ -150,10 +191,11 @@ export class HeadlessEngine { instructions: () => this.client.getInstructions(), listTools: async (): Promise => (await this.client.listTools()).tools, callTool: async (name, args) => { + this.calls += 1; try { return await this.client.callTool({ name, arguments: args }, { timeout: LONG_TIMEOUT_MS }); } finally { - await this.checkForChange().catch(() => undefined); + await this.check(false, true).catch(() => undefined); } }, }; @@ -164,7 +206,7 @@ export class HeadlessEngine { const result = (await this.client.callTool({ name: 'save', arguments: { path } }, { timeout: LONG_TIMEOUT_MS })) as ToolResult; if (result.isError) throw new Error(textOf(result).slice(0, 500) || 'save failed'); // Saving moves the revision; nothing in the design changed, so nobody needs to mirror. - await this.checkForChange(true); + await this.unchanged(); } async close(): Promise { diff --git a/apps/desktop/src/main/solidcraft/supervisor.ts b/apps/desktop/src/main/solidcraft/supervisor.ts index bc71797d..71d63969 100644 --- a/apps/desktop/src/main/solidcraft/supervisor.ts +++ b/apps/desktop/src/main/solidcraft/supervisor.ts @@ -259,7 +259,7 @@ export class SolidCraftSupervisor { 10 * 60 * 1000, ); if (batch.failed) throw new Error(batch.failed.error); - await this.engine?.checkForChange(); + await this.engine?.changed(); const created = (batch.results[0] as { created_bodies?: unknown } | undefined)?.created_bodies; return Array.isArray(created) ? created.filter((name): name is string => typeof name === 'string') : []; } finally { @@ -531,7 +531,7 @@ export class SolidCraftSupervisor { const naming = await requester.request('engine.batch', { commands: steps, rollback: false }, 60_000); if (naming.failed) this.options.log(`the standard part was not named: ${naming.failed.error}`); } - await this.engine?.checkForChange(); + await this.engine?.changed(); return body; } @@ -550,7 +550,7 @@ export class SolidCraftSupervisor { try { return await this.dimensions.probe(target, current, value); } finally { - await this.engine?.checkForChange(true).catch(() => undefined); + await this.engine?.unchanged().catch(() => undefined); } }); this.probing = run.catch(() => undefined); @@ -561,7 +561,7 @@ export class SolidCraftSupervisor { async applyDimension(parameter: string, value: number, factor: 1 | 2, keepFitMark = false): Promise { await this.probing; await this.dimensions.apply(parameter, value, factor, keepFitMark); - await this.engine?.checkForChange(); + await this.engine?.changed(); } /** The user parameters of the design for the parameter panel; reading changes nothing. */ @@ -581,7 +581,7 @@ export class SolidCraftSupervisor { try { return await work(); } finally { - await this.engine?.checkForChange(true).catch(() => undefined); + await this.engine?.unchanged().catch(() => undefined); clearTimeout(slow); const took = performance.now() - started; if (took > 20_000) this.options.log(`a trial (${what}) took ${String(Math.round(took / 1000))} s`); @@ -598,7 +598,7 @@ export class SolidCraftSupervisor { async applyAction(action: DesignAction): Promise { await this.probing; await this.actions.apply(action); - await this.engine?.checkForChange(); + await this.engine?.changed(); } /** @@ -676,7 +676,7 @@ export class SolidCraftSupervisor { const coloured = await this.actions.colourSplitPieces(layers); if (coloured > 0) this.options.log(`coloured the pieces of ${String(coloured)} split logo bodies`); } - await this.engine?.checkForChange(); + await this.engine?.changed(); } /** The placed logo at a picked point, for the logo controls of the selection dock; reading changes nothing. */ @@ -720,7 +720,7 @@ export class SolidCraftSupervisor { // Already where it should be (centred twice): nothing to do. if (planned.steps.length === 0) return; await this.actions.applySteps(planned.steps); - await this.engine?.checkForChange(); + await this.engine?.changed(); } measure(items: MeasureItem[]): Promise { @@ -737,7 +737,7 @@ export class SolidCraftSupervisor { () => undefined, ); await run; - await this.engine?.checkForChange(); + await this.engine?.changed(); } /** Where parts stop each joint value on its way to a limit (ADR 0027, `joint.sweep`); the design stays as it is. */ @@ -1041,7 +1041,7 @@ export class SolidCraftSupervisor { const commands = Array.from({ length: count }, () => ({ command, params: {} })); const batch = await this.requester().request<{ failed?: { error: string } }>('engine.batch', { commands, rollback: false }, 10 * 60 * 1000); if (batch.failed) throw new Error(`${command} failed: ${batch.failed.error}`); - await this.engine?.checkForChange(); + await this.engine?.changed(); } /** One engine command of the user, after any running trial; the change is mirrored as usual. */ @@ -1050,7 +1050,7 @@ export class SolidCraftSupervisor { const batch = await this.requester().request<{ failed?: { error: string } }>('engine.batch', { commands: [step], rollback: false }, 10 * 60 * 1000); if (batch.failed) throw new Error(`${step.command} failed: ${batch.failed.error}`); // The window reports the change with design.changed; the headless engine is asked. - await this.engine?.checkForChange(); + await this.engine?.changed(); } private publish(): void { @@ -1091,7 +1091,7 @@ export class SolidCraftSupervisor { if (!this.launchedBefore && this.options.openLastOnStart === false) this.designs?.forget(); this.launchedBefore = true; const design = this.designs?.last(); - const start = (file: string | undefined) => HeadlessEngine.start({ cli, allowPaths, design: file, env: this.options.environment.env }); + const start = (file: string | undefined) => HeadlessEngine.start({ cli, allowPaths, design: file, env: this.options.environment.env, log: (message) => this.options.log(message) }); let engine: HeadlessEngine; try { engine = await start(design); diff --git a/apps/desktop/test/headless.test.ts b/apps/desktop/test/headless.test.ts new file mode 100644 index 00000000..d609d199 --- /dev/null +++ b/apps/desktop/test/headless.test.ts @@ -0,0 +1,107 @@ +import { describe, expect, it } from 'vitest'; + +import { HeadlessEngine, type Revision } from '../src/main/solidcraft/headless'; + +/** + * The engine's change checks against a stand-in for SolidCraft whose revision the test moves by + * hand. Tool calls other than `batch` stay open until the test ends them, so a check can be put + * between a call and its own check. + */ +function setup() { + let revision = 1; + const open: (() => void)[] = []; + const logged: string[] = []; + const client = { + getInstructions: () => undefined, + listTools: () => Promise.resolve({ tools: [] }), + callTool: ({ name }: { name: string }) => { + if (name === 'batch') { + return Promise.resolve({ content: [{ type: 'text', text: JSON.stringify({ results: [{ document_id: 'doc', revision }] }) }] }); + } + return new Promise((resolve) => { + open.push(() => { + resolve({ content: [{ type: 'text', text: '{}' }] }); + }); + }); + }, + }; + const engine = Reflect.construct(HeadlessEngine, [client, (message: string) => logged.push(message)]) as HeadlessEngine; + const told: Revision[] = []; + engine.onChange((next) => told.push(next)); + return { + engine, + told, + logged, + /** The design changes: SolidCraft counts the revision up. */ + move: () => { + revision += 1; + }, + /** Ends the oldest open tool call. */ + answer: () => { + open.shift()?.(); + }, + }; +} + +describe('headless engine: telling a change', () => { + it('tells a change of PartBuddy whose revision a trial recorded first', async () => { + const { engine, told, logged, move } = setup(); + await engine.unchanged(); + // The change is built; a trial that ran beside it ends before the change's own check. + move(); + await engine.unchanged(); + await engine.changed(); + expect(told).toEqual([{ documentId: 'doc', revision: 2 }]); + expect(logged).toHaveLength(1); + }); + + it('tells a change of the assistant whose revision a trial recorded while the call ran', async () => { + const { engine, told, move, answer } = setup(); + await engine.unchanged(); + const call = engine.tools.callTool('extrude', {}); + move(); + await engine.unchanged(); + answer(); + await call; + expect(told).toEqual([{ documentId: 'doc', revision: 2 }]); + }); + + it('tells nothing after a trial alone, nor after the assistant reads when the trial is over', async () => { + const { engine, told, move, answer } = setup(); + await engine.unchanged(); + move(); + await engine.unchanged(); + expect(told).toEqual([]); + const read = engine.tools.callTool('inspect', {}); + answer(); + await read; + expect(told).toEqual([]); + }); + + it('tells each of two changes that follow one another', async () => { + const { engine, told, move, answer } = setup(); + await engine.unchanged(); + move(); + await engine.changed(); + const call = engine.tools.callTool('extrude', {}); + move(); + answer(); + await call; + expect(told.map((next) => next.revision)).toEqual([2, 3]); + }); + + it('counts a call as over with its check, so a later trial is not taken for its change', async () => { + const { engine, told, move, answer } = setup(); + await engine.unchanged(); + const first = engine.tools.callTool('inspect', {}); + answer(); + await first; + // A trial after the call moves the revision; the next read must not mirror it. + move(); + await engine.unchanged(); + const second = engine.tools.callTool('inspect', {}); + answer(); + await second; + expect(told).toEqual([]); + }); +});