From e2ef633b95b08de852db565155dd3770d3d68a93 Mon Sep 17 00:00:00 2001 From: Paul Bakaus Date: Fri, 17 Jul 2026 17:26:55 -0700 Subject: [PATCH] Finish the failed-accept classification my last commit only half did MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit All three new findings are the same root cause, and it is my incomplete fix: operationFailure only covered results built from a *thrown* error. Two paths it missed: - Two catches wrote the failure result as a multi-line literal, so the single-line replace skipped them. The Vue accept catch was still bare, exactly as reported; the Svelte one too, though its failures happened to be caught by completion.mjs's Svelte-only special case. - The accept implementations also *return* `{handled: false, error}` for their own checks (variant missing, template empty, original text ambiguous). Those never throw, so no catch ran and no `mode` was set. Both layers now agree, because each is reachable on its own: - live-accept marks any unhandled preview-path result via markPreviewFailure, keyed on `previewMode` — a clean discriminator, since only the preview branches set it and a plain wrapper never does. This is what the agent reads: reference/live.md routes on `mode`, so without it the agent was told "read file, find markers, edit" for a preview that has no markers in source. - completion.mjs replaces its arbitrary svelte-component special case with the set of preview modes whose variants live outside the user's source. That case existed for precisely this reason; Vue and source-artifact were simply never added, so the identical failure on those paths acknowledged as success. The plain wrapper keeps its manual handoff, which is the one shape with editable markers in source. Both deliberate handoffs (mode: 'fallback' and markers not found) still classify as agent_done, now pinned by a test so the generalization cannot swallow them. Prepared with AI assistance under maintainer direction. Co-Authored-By: Claude --- skill/scripts/live-accept.mjs | 37 +++++++++++++++++++++++-------- skill/scripts/live/completion.mjs | 14 +++++++++++- tests/live-accept.test.mjs | 27 ++++++++++++++++++++++ tests/live-completion.test.mjs | 23 +++++++++++++++++++ 4 files changed, 91 insertions(+), 10 deletions(-) diff --git a/skill/scripts/live-accept.mjs b/skill/scripts/live-accept.mjs index a3d59f253..8e80ff42e 100644 --- a/skill/scripts/live-accept.mjs +++ b/skill/scripts/live-accept.mjs @@ -55,6 +55,28 @@ function operationFailure(err, extra = {}) { return { handled: false, mode: 'error', error: err.message, ...extra }; } +/** + * Mark an unhandled preview-path result as a real failure. + * + * operationFailure only covers results built from a *thrown* error. The accept + * implementations also return `{handled: false, error}` for their own checks + * (variant missing, template empty, original text ambiguous), and those arrived + * without `mode`, so completion.mjs classified them as agent_done and + * reference/live.md routed the agent to "read file, find markers, edit". + * + * That handoff only makes sense for a plain wrapper session, which is the one + * shape with markers in the user's source to edit. Component and isolated + * artifact previews keep the source clean until Accept, so there is nothing to + * hand-edit and an unhandled result is always a failure. `previewMode` is + * exactly that discriminator: only the preview branches set it. + */ +function markPreviewFailure(result) { + if (result?.handled === false && !result.mode && result.previewMode) { + return { ...result, mode: 'error' }; + } + return result; +} + // --------------------------------------------------------------------------- // CLI // --------------------------------------------------------------------------- @@ -127,7 +149,8 @@ Output (JSON): })); return; } - const emitResult = (result) => { + const emitResult = (rawResult) => { + const result = markPreviewFailure(rawResult); if (result?.handled !== false) { writeAcceptReceipt(process.cwd(), id, { operation: requestedOperation, @@ -252,15 +275,13 @@ Output (JSON): { waitMs: ACCEPT_LOCK_WAIT_MS }, ); } catch (err) { - result = { - handled: false, - error: err.message, + result = operationFailure(err, { file: vueComponentManifest.sourceFile, sourceFile: vueComponentManifest.sourceFile, previewMode: 'vue-component', componentDir: vueComponentManifest.componentDir, carbonize: false, - }; + }); } emitResult(result); return; @@ -306,14 +327,12 @@ Output (JSON): { waitMs: ACCEPT_LOCK_WAIT_MS }, ); } catch (err) { - result = { - handled: false, - error: err.message, + result = operationFailure(err, { file: svelteComponentManifest.sourceFile, sourceFile: svelteComponentManifest.sourceFile, previewMode: 'svelte-component', componentDir: svelteComponentManifest.componentDir, - }; + }); } if (result.carbonize) { result.todo = 'REQUIRED before next poll: carbonize cleanup in ' + result.file + '. See reference/live.md "Required after accept".'; diff --git a/skill/scripts/live/completion.mjs b/skill/scripts/live/completion.mjs index 986773066..6779bdb01 100644 --- a/skill/scripts/live/completion.mjs +++ b/skill/scripts/live/completion.mjs @@ -1,9 +1,21 @@ +// A preview whose variants live outside the user's source: component modules or +// an isolated artifact. These keep the real file untouched until Accept, so a +// failed accept leaves nothing in source for the agent to hand-edit and must be +// reported as a failure rather than reference/live.md's manual-cleanup handoff. +// Previously only `svelte-component` was special-cased here, so the same failure +// on a Vue or isolated-artifact preview was acknowledged as a success. +const PREVIEW_MODES_WITHOUT_SOURCE_MARKERS = new Set([ + 'svelte-component', + 'vue-component', + 'source-artifact', +]); + export function completionTypeForAcceptResult(eventType, acceptResult) { if (eventType === 'discard') return acceptResult?.handled === true ? 'discarded' : 'error'; if (acceptResult?.handled === true && acceptResult?.carbonize === true) return 'agent_done'; if (acceptResult?.handled === true) return 'complete'; if (acceptResult?.mode === 'error') return 'error'; - if (eventType === 'accept' && acceptResult?.previewMode === 'svelte-component') return 'error'; + if (eventType === 'accept' && PREVIEW_MODES_WITHOUT_SOURCE_MARKERS.has(acceptResult?.previewMode)) return 'error'; return 'agent_done'; } diff --git a/tests/live-accept.test.mjs b/tests/live-accept.test.mjs index 85eb7d40f..ef8f20249 100644 --- a/tests/live-accept.test.mjs +++ b/tests/live-accept.test.mjs @@ -133,6 +133,33 @@ describe('live-accept — isolated source artifacts', () => { assert.equal(existsSync(join(tmp, session.sessionDir)), false); }); + // reference/live.md routes on `mode`: without it the agent is told "manual + // cleanup: read file, find markers, edit". There are no markers in source for + // an isolated preview, so every failure here must self-describe as mode:error. + it('marks a failed artifact accept as mode:error, not a manual handoff', () => { + scaffold('isolatedmissing'); + const result = runAccept(tmp, ['--id', 'isolatedmissing', '--variant', '9']); + assert.equal(result.handled, false, JSON.stringify(result)); + assert.equal(result.mode, 'error', 'the agent must not be told to hand-edit a source file with no markers'); + assert.equal(result.previewMode, 'source-artifact'); + }); + + it('marks an artifact accept blocked by the source lock as mode:error', () => { + const { original } = scaffold('isolatedlockacc'); + const realTmp = realpathSync(tmp); + const lockPath = sourceLockPath(join(realTmp, 'page.html'), realTmp); + mkdirSync(dirname(lockPath), { recursive: true }); + writeFileSync(lockPath, JSON.stringify({ + owner: 'generation:isolatedlockacc:1', token: 'other', pid: process.pid, at: Date.now(), + }) + '\n'); + + const result = runAccept(tmp, ['--id', 'isolatedlockacc', '--variant', '1']); + assert.equal(result.handled, false, JSON.stringify(result)); + assert.equal(result.mode, 'error'); + assert.equal(result.error, 'source_locked'); + assert.equal(readFileSync(join(tmp, 'page.html'), 'utf-8'), original, 'source must be untouched'); + }); + // Every other discard path (Vue, Svelte, plain wrapper) takes the source lock. // This one deleted the preview bare, so it could pull the artifact out from // under an in-flight publisher instead of serializing behind it. diff --git a/tests/live-completion.test.mjs b/tests/live-completion.test.mjs index 7fc51a742..3292a894e 100644 --- a/tests/live-completion.test.mjs +++ b/tests/live-completion.test.mjs @@ -53,6 +53,29 @@ describe('live completion type classification', () => { ); }); + // Previews whose variants live outside the user's source leave nothing in the + // file to hand-edit, so a failed accept there is a failure, not live.md's + // "read file, find markers, edit" handoff. Only svelte-component was special + // cased, so the identical failure on a Vue or isolated-artifact preview was + // acknowledged as a success and Live continued past it. + for (const previewMode of ['svelte-component', 'vue-component', 'source-artifact']) { + it(`treats a failed ${previewMode} accept as an error, not a manual handoff`, () => { + assert.equal( + completionTypeForAcceptResult('accept', { handled: false, error: 'source_locked', previewMode }), + 'error', + ); + }); + } + + it('still treats a failed plain-wrapper accept as a manual handoff', () => { + // The one shape with editable markers in source. This must not regress into + // an error, or every hand-editable session starts failing the poll loop. + assert.equal( + completionTypeForAcceptResult('accept', { handled: false, error: 'Markers not found' }), + 'agent_done', + ); + }); + it('classifies handled accept/discard and real failures explicitly', () => { assert.equal(completionTypeForAcceptResult('accept', { handled: true }), 'complete'); assert.equal(completionTypeForAcceptResult('discard', { handled: true }), 'discarded');