From 6caa2722588ad4e42269351724acdb85dcdbfd87 Mon Sep 17 00:00:00 2001 From: Josh Joseph Date: Tue, 20 Oct 2026 23:59:44 +0000 Subject: [PATCH 01/14] Handle asynchronous Isaac feedback from the routing form --- docs/claude-mod.md | 11 ++++- .../agents/claude_mods/isaac-feedback.ts | 47 ++++++++++++++----- 2 files changed, 44 insertions(+), 14 deletions(-) diff --git a/docs/claude-mod.md b/docs/claude-mod.md index fa6bc43e6..9c3df40e7 100644 --- a/docs/claude-mod.md +++ b/docs/claude-mod.md @@ -65,8 +65,15 @@ with surrounding whitespace ignored); other values disable it. It controls Isaac availability, so UG's routing presets and on/off toggles leave it unchanged. The native form keeps routing on until Enter accepts a valid reason, then disables routing and uploads the exact text in the background with `isaac feedback`. The -transcript shows the feedback ID the mod passes with `--feedback-id`; a toast reports -the result. Cancel or Escape leaves routing on and sends nothing. A missing +transcript shows the feedback ID the mod passes with `--feedback-id`. When Isaac +supports `--background`, it saves a bounded local snapshot and detaches the upload. +The form passes its submission timestamp with `--target-before` so a later task +cannot become the feedback target. A queued acknowledgement does not trigger a +success toast: the mod checks local `--status` until delivery or for about a minute. +Longer uploads keep running independently; `/isaac-feedback status ` checks +them and `/isaac-feedback retry ` retries the original report. Older Isaac +versions keep their synchronous CLI call in the mod's background task. +Cancel or Escape leaves routing on and sends nothing. A missing `isaac` CLI or an unavailable form leaves routing on. A feedback submission failure after a valid reason leaves routing off and reports the failure in the transcript. The form makes no model requests. diff --git a/src/ucode/agents/claude_mods/isaac-feedback.ts b/src/ucode/agents/claude_mods/isaac-feedback.ts index fc4ecaa73..d2c0fc414 100644 --- a/src/ucode/agents/claude_mods/isaac-feedback.ts +++ b/src/ucode/agents/claude_mods/isaac-feedback.ts @@ -6,6 +6,7 @@ const FEEDBACK_PANE = 'smart-router-feedback'; type FeedbackState = { submitted: boolean; reason: string; + backgroundSupported?: boolean; readRoutingEnabled?: () => Promise; disableRouting?: (sessionControlArgs: readonly string[]) => Promise; }; @@ -25,9 +26,11 @@ export function registerIsaacFeedback(on: On, state: FeedbackState): void { } if (!await state.readRoutingEnabled()) return {text: 'Smart Router is off'}; // The form uploads through Isaac's CLI; without it, keep routing on. `isaac --version` launches a harness. - if ((await $.process.run(['isaac', 'feedback', '--help'], { timeoutMs: 10000 })).exitCode !== 0) { + const help = await $.process.run(['isaac', 'feedback', '--help'], { timeoutMs: 10000 }); + if (help.exitCode !== 0) { throw new Error('Isaac CLI is unavailable.'); } + state.backgroundSupported = ['--background', '--status', '--target-before'].every((flag) => help.stdout.includes(flag)); state.submitted = false; state.reason = ''; const pane = await $.ui.open({ @@ -65,6 +68,7 @@ export function registerIsaacFeedback(on: On, state: FeedbackState): void { return; } state.submitted = true; + const targetBefore = new Date().toISOString(); try { await state.disableRouting(['--smart-router-feedback', value]); } catch { @@ -76,17 +80,36 @@ export function registerIsaacFeedback(on: On, state: FeedbackState): void { // Upload in the background through Isaac's CLI, without a model turn. const feedbackId = crypto.randomUUID(); $.ui.log(`Sending feedback in the background. You can keep working. Feedback ID: ${feedbackId}`); - void $.session.id() - .then((sessionId) => $.process.run( - ['isaac', 'feedback', '--category', 'smart-routing', '--harness', 'claude', - '--session-id', sessionId, '--feedback-id', feedbackId], - { stdin: value, timeoutMs: 30000 }, - )) - .then((result) => result.exitCode === 0, () => false) - // A failure goes to the transcript, where it outlasts a toast. - .then((sent) => sent - ? $.ui.toast('Feedback submitted successfully ✅') - : $.ui.log('Smart Router is off, but feedback could not be sent. Retry with /isaac-feedback --category smart-routing.')); + const background = state.backgroundSupported; + void (async () => { + const sessionId = await $.session.id(); + const args = ['isaac', 'feedback', '--category', 'smart-routing', '--harness', 'claude', + '--session-id', sessionId, '--feedback-id', feedbackId]; + if (background) args.push('--background', '--target-before', targetBefore); + let result = await $.process.run(args, { stdin: value, timeoutMs: 120000 }); + if (result.exitCode !== 0) throw new Error('Feedback command failed.'); + // Older Isaac releases finish delivery in the command itself. + if (!background) { + $.ui.toast('Feedback submitted successfully ✅'); + return; + } + for (let attempt = 0; attempt <= 12; attempt += 1) { + const status = JSON.parse(result.stdout); + if (status.delivery_status === 'submitted') { + $.ui.toast('Feedback submitted successfully ✅'); + return; + } + if (status.delivery_status !== 'queued') throw new Error('Feedback needs a retry.'); + if (attempt === 12) break; + await new Promise((resolve) => setTimeout(resolve, 5000)); + result = await $.process.run(['isaac', 'feedback', '--status', feedbackId], { timeoutMs: 10000 }); + if (result.exitCode !== 0) throw new Error('Feedback status unavailable.'); + } + $.ui.log(`Feedback is still queued. Check it with /isaac-feedback status ${feedbackId}.`); + })().catch(() => { + // Keep the original ID even when the enqueue acknowledgement was lost. + $.ui.log(`Smart Router is off. Feedback delivery is unconfirmed. Retry with /isaac-feedback retry ${feedbackId}.`); + }); }, }), Button({ key: 'cancel', label: 'Cancel (keep routing on)', onPress: () => $.ui.close({ id: FEEDBACK_PANE }) }), From 8e5fee4bf14870f0206e1a485cf0ccfcaae1a489 Mon Sep 17 00:00:00 2001 From: Josh Joseph Date: Wed, 21 Oct 2026 00:57:36 +0000 Subject: [PATCH 02/14] Share native background feedback delivery notifications --- docs/claude-mod.md | 15 +- .../agents/claude_mods/isaac-feedback.ts | 137 +++++++++++++++--- tests/README.md | 5 + tests/integration/README.md | 3 + .../mod_tests/isaac-feedback.test.ts | 101 +++++++++++++ 5 files changed, 243 insertions(+), 18 deletions(-) create mode 100644 tests/native_claude/mod_tests/isaac-feedback.test.ts diff --git a/docs/claude-mod.md b/docs/claude-mod.md index 9c3df40e7..7c8ce60bb 100644 --- a/docs/claude-mod.md +++ b/docs/claude-mod.md @@ -70,6 +70,7 @@ supports `--background`, it saves a bounded local snapshot and detaches the uplo The form passes its submission timestamp with `--target-before` so a later task cannot become the feedback target. A queued acknowledgement does not trigger a success toast: the mod checks local `--status` until delivery or for about a minute. +Delivery produces both a persistent transcript message and a success toast. Longer uploads keep running independently; `/isaac-feedback status ` checks them and `/isaac-feedback retry ` retries the original report. Older Isaac versions keep their synchronous CLI call in the mod's background task. @@ -77,6 +78,16 @@ Cancel or Escape leaves routing on and sends nothing. A missing `isaac` CLI or an unavailable form leaves routing on. A feedback submission failure after a valid reason leaves routing off and reports the failure in the transcript. The form makes no model requests. +With the same feedback flag enabled, `/isaac-feedback` uses these native notifications +too. After a Bash call returns a background-submission receipt, the mod displays +"Sending feedback in the background. You can keep working. Feedback ID: ..." and +attaches the shared delivery monitor. Native clocks keep the monitor running after +the skill's turn ends. It only confirms delivery when `--status` returns `submitted` +for that ID; failures retain the original retry ID. The tool result carries +`delivery_notifications: "native"` so the skill avoids a duplicate acknowledgement. +Category selection, evidence collection, and Bash permission checks stay with the +skill and CLI. No extra model turn is needed for a completion notification. + `isaac-feedback.ts` owns this Databricks-specific policy, form, and submission. `register.ts` binds the routing callbacks before Isaac's command guard, then handles toggles the guard passes through. Claude's mod compiler requires engine @@ -100,7 +111,9 @@ and launch recipe after on. Each phase retains completed native parent/child answers and requires successful real gateway responses, with no recipe header. The `Claude mod · 2.1.290` CI job validates the packaged module and checks event -forwarding and launch-local options. To run those component checks locally: +forwarding, launch-local options, and feedback delivery notifications. The notification +component tests supply CLI results and clock ticks through native events; they do not +upload feedback. To run those component checks locally: ```bash UCODE_TEST_CLAUDE_BINARY=/absolute/path/to/claude \ diff --git a/src/ucode/agents/claude_mods/isaac-feedback.ts b/src/ucode/agents/claude_mods/isaac-feedback.ts index d2c0fc414..32000269f 100644 --- a/src/ucode/agents/claude_mods/isaac-feedback.ts +++ b/src/ucode/agents/claude_mods/isaac-feedback.ts @@ -1,8 +1,91 @@ -import type { On } from 'claude-code'; +import type { EngineInterface, On } from 'claude-code'; import { envBool } from './constants.ts'; const FEEDBACK_PANE = 'smart-router-feedback'; +type DeliveryReceipt = { + feedback_id: string; + delivery_status: string; + [key: string]: unknown; +}; + +function readDeliveryReceipt(output: string): { receipt: DeliveryReceipt; line: string } | undefined { + for (const line of output.trim().split('\n').reverse()) { + try { + const receipt = JSON.parse(line); + if (typeof receipt?.feedback_id === 'string' + && /^[0-9a-f]{8}(-[0-9a-f]{4}){3}-[0-9a-f]{12}$/i.test(receipt.feedback_id) + && typeof receipt.delivery_status === 'string') { + return { receipt, line }; + } + } catch {} + } + return undefined; +} + +function showSending($: EngineInterface, feedbackId: string): void { + $.ui.log(`Sending feedback in the background. You can keep working. Feedback ID: ${feedbackId}`); +} + +function showSubmitted($: EngineInterface, feedbackId: string): void { + $.ui.log(`Feedback submitted successfully. Feedback ID: ${feedbackId}`); + $.ui.toast('Feedback submitted successfully ✅'); +} + +function showUnconfirmed($: EngineInterface, feedbackId: string): void { + $.ui.log(`Feedback delivery is unconfirmed. Retry with /isaac-feedback retry ${feedbackId}.`); +} + +function watchDelivery( + $: EngineInterface, + feedbackId: string, + active: Set, + announce = true, +): void { + if (active.has(feedbackId)) return; + active.add(feedbackId); + if (announce) showSending($, feedbackId); + let attempts = 0; + let checking = false; + let timer: { cancel: () => void } | undefined; + const stop = () => { + timer?.cancel(); + active.delete(feedbackId); + }; + const check = async () => { + if (checking) return; + checking = true; + attempts += 1; + try { + const result = await $.process.run(['isaac', 'feedback', '--status', feedbackId], { timeoutMs: 10000 }); + const status = result.exitCode === 0 ? readDeliveryReceipt(result.stdout)?.receipt : undefined; + if (status?.feedback_id !== feedbackId) throw new Error('Feedback status unavailable.'); + if (status.delivery_status === 'submitted') { + stop(); + showSubmitted($, feedbackId); + } else if (status.delivery_status !== 'queued') { + throw new Error('Feedback needs a retry.'); + } else if (attempts >= 13) { + stop(); + $.ui.log(`Feedback is still queued. Check it with /isaac-feedback status ${feedbackId}.`); + } + } catch { + stop(); + showUnconfirmed($, feedbackId); + } finally { + checking = false; + } + }; + try { + // Native clocks keep checking after the skill's turn ends and are disposed with the mod. + timer = $.clock.every(5000, () => { void check(); }); + void check(); + } catch { + stop(); + $.ui.log(`Check feedback delivery with /isaac-feedback status ${feedbackId}.`); + } +} + type FeedbackState = { submitted: boolean; reason: string; @@ -16,6 +99,33 @@ export function createIsaacFeedbackState(): FeedbackState { } export function registerIsaacFeedback(on: On, state: FeedbackState): void { + const active = new Set(); + on('tool.call', { tool: 'Bash' }, async ($, e, next) => { + const answer = await next(e); + // The skill still owns category/target selection and the original permission-checked call. + if (typeof e.command !== 'string' || !e.command.includes('--background') + || !/\bfeedback\b/.test(e.command)) return answer; + try { + const payload = answer.result; + if (!payload || typeof payload !== 'object' || !('stdout' in payload) + || typeof payload.stdout !== 'string') return answer; + const parsed = readDeliveryReceipt(payload.stdout); + if (!parsed || !['queued', 'submitted'].includes(parsed.receipt.delivery_status) + || !envBool(await $.env.get('ISAAC_SMART_ROUTER_FEEDBACK'))) return answer; + watchDelivery($, parsed.receipt.feedback_id, active); + // Tell the skill that native UI owns the acknowledgement, avoiding a duplicate model reply. + const rendered = JSON.stringify({ ...parsed.receipt, delivery_notifications: 'native' }); + return { + ...answer, + result: { ...payload, stdout: payload.stdout.replace(parsed.line, rendered) }, + ...(typeof answer.text === 'string' ? { text: answer.text.replace(parsed.line, rendered) } : {}), + }; + } catch { + // A notification failure must not hide the submission result from the skill. + return answer; + } + }); + on('command.run', {command: 'smart-router'}, async ($, e, next) => { if (e.args.trim() !== 'off') return next(e); const feedbackRequired = envBool(await $.env.get('ISAAC_SMART_ROUTER_FEEDBACK')); @@ -79,36 +189,29 @@ export function registerIsaacFeedback(on: On, state: FeedbackState): void { await $.ui.close({ id: FEEDBACK_PANE }).catch(() => {}); // Upload in the background through Isaac's CLI, without a model turn. const feedbackId = crypto.randomUUID(); - $.ui.log(`Sending feedback in the background. You can keep working. Feedback ID: ${feedbackId}`); + showSending($, feedbackId); const background = state.backgroundSupported; void (async () => { const sessionId = await $.session.id(); const args = ['isaac', 'feedback', '--category', 'smart-routing', '--harness', 'claude', '--session-id', sessionId, '--feedback-id', feedbackId]; if (background) args.push('--background', '--target-before', targetBefore); - let result = await $.process.run(args, { stdin: value, timeoutMs: 120000 }); + const result = await $.process.run(args, { stdin: value, timeoutMs: 120000 }); if (result.exitCode !== 0) throw new Error('Feedback command failed.'); // Older Isaac releases finish delivery in the command itself. if (!background) { - $.ui.toast('Feedback submitted successfully ✅'); + showSubmitted($, feedbackId); return; } - for (let attempt = 0; attempt <= 12; attempt += 1) { - const status = JSON.parse(result.stdout); - if (status.delivery_status === 'submitted') { - $.ui.toast('Feedback submitted successfully ✅'); - return; - } - if (status.delivery_status !== 'queued') throw new Error('Feedback needs a retry.'); - if (attempt === 12) break; - await new Promise((resolve) => setTimeout(resolve, 5000)); - result = await $.process.run(['isaac', 'feedback', '--status', feedbackId], { timeoutMs: 10000 }); - if (result.exitCode !== 0) throw new Error('Feedback status unavailable.'); + const status = readDeliveryReceipt(result.stdout)?.receipt; + if (status?.feedback_id !== feedbackId + || !['queued', 'submitted'].includes(status.delivery_status)) { + throw new Error('Feedback needs a retry.'); } - $.ui.log(`Feedback is still queued. Check it with /isaac-feedback status ${feedbackId}.`); + watchDelivery($, feedbackId, active, false); })().catch(() => { // Keep the original ID even when the enqueue acknowledgement was lost. - $.ui.log(`Smart Router is off. Feedback delivery is unconfirmed. Retry with /isaac-feedback retry ${feedbackId}.`); + showUnconfirmed($, feedbackId); }); }, }), diff --git a/tests/README.md b/tests/README.md index c14c2e96f..7aafd69ab 100644 --- a/tests/README.md +++ b/tests/README.md @@ -34,6 +34,11 @@ TypeScript event-forwarding and launch-option tests in CI. The existing Claude skill-toggle integration journey checks the live recipe payload field across on/off/on. See [the mod guide](../docs/claude-mod.md). +The native feedback tests cover returning before delivery, later completion messages, +duplicate monitoring, failed uploads, mismatched IDs, disabled notifications, and Bash +permission denials. They supply CLI results and clock ticks through native events; +they do not establish live upload or interactive form coverage. + The required Isaac feedback form needs manual validation of the 16-character minimum, cancellation, repeated toggles, and submission failures. The existing routing-toggle journeys do not cover this form or a live Isaac evidence upload. diff --git a/tests/integration/README.md b/tests/integration/README.md index 4e188c967..12636efd7 100644 --- a/tests/integration/README.md +++ b/tests/integration/README.md @@ -93,6 +93,9 @@ The Claude mod entry point has separate native component checks in `../native_claude/test_mod.py`. Claude 2.1.290 validates the module and runs its TypeScript event-forwarding and launch-option tests in a dedicated CI job. The existing Claude skill-toggle journey checks the live recipe payload field across on/off/on. +The native feedback component tests cover background delivery notifications and +failure handling using supplied CLI results and clock ticks. They do not upload +feedback or exercise the interactive form. The required Isaac feedback form and evidence submission need a separate manual Isaac check. The gateway toggle journey does not cover the form. diff --git a/tests/native_claude/mod_tests/isaac-feedback.test.ts b/tests/native_claude/mod_tests/isaac-feedback.test.ts new file mode 100644 index 000000000..2be772341 --- /dev/null +++ b/tests/native_claude/mod_tests/isaac-feedback.test.ts @@ -0,0 +1,101 @@ +import type { On } from 'claude-code'; +import { expect, test } from 'claude-code/testing'; + +const id = 'e6ef90cf-25b3-4fee-a356-5f8fbc652e7c'; +const command = '/path/to/isaac feedback --category other --background -- "test feedback"'; +const receipt = { feedback_id: id, delivery_status: 'queued', evidence_status: 'not_requested' }; + +// Model the CLI and UI at the native event boundary; the packaged mod handles the real events. +function setup(on: On, enabled = '1') { + const stdout = `CLI diagnostic\n${JSON.stringify(receipt)}`; + const answer = { result: { stdout, stderr: '', interrupted: false }, text: stdout }; + const logs: string[] = []; + const toasts: string[] = []; + let complete: () => void = () => {}; + const completed = new Promise((resolve) => { complete = resolve; }); + let advance: () => void = () => {}; + on('clock.every', async () => { + await new Promise((resolve) => { advance = resolve; }); + return { value: undefined }; + }); + on('env.get', ($, e) => ({ value: e.name === 'ISAAC_SMART_ROUTER_FEEDBACK' ? enabled : undefined })); + on('tool.call', () => answer); + on('ui.log', ($, e) => { + logs.push(e.text); + if (e.text.includes('unconfirmed')) complete(); + return { value: undefined }; + }); + on('ui.toast', ($, e) => { toasts.push(e.text); complete(); return { value: undefined }; }); + return { answer, logs, toasts, completed, tick: () => advance() }; +} + +test('background feedback returns immediately and notifies when a later status confirms delivery', async ($, on) => { + const { logs, toasts, completed, tick } = setup(on); + let checks = 0; + on('process.run', ($, e) => { + expect(e.argv).toEqual(['isaac', 'feedback', '--status', id]); + checks += 1; + return { value: { exitCode: 0, stderr: '', stdout: JSON.stringify({ + ...receipt, delivery_status: checks === 1 ? 'queued' : 'submitted', + }) } }; + }); + + const result = await $.tool.call({ tool: 'Bash', tool_use_id: 'feedback', command }); + expect(result.result.stderr).toBe(''); + expect(result.result.stdout).toContain('CLI diagnostic\n'); + expect(JSON.parse(result.result.stdout.split('\n')[1])).toEqual({ ...receipt, delivery_notifications: 'native' }); + expect(result.text).toBe(result.result.stdout); + expect(logs).toEqual([`Sending feedback in the background. You can keep working. Feedback ID: ${id}`]); + expect(toasts).toEqual([]); + + // A second call for the same report must not attach another monitor. + await $.tool.call({ tool: 'Bash', tool_use_id: 'same-feedback', command }); + const next = await $.tool.call({ tool: 'Bash', tool_use_id: 'next-task', command: 'pwd' }); + expect(next.result.stdout).not.toContain('delivery_notifications'); + tick(); + await completed; + expect(checks).toBe(2); + expect(logs).toEqual([ + `Sending feedback in the background. You can keep working. Feedback ID: ${id}`, + `Feedback submitted successfully. Feedback ID: ${id}`, + ]); + expect(toasts).toEqual(['Feedback submitted successfully ✅']); +}); + +for (const [name, status] of [ + ['failed upload', { ...receipt, delivery_status: 'failed' }], + ['mismatched report', { ...receipt, feedback_id: '00000000-0000-0000-0000-000000000000', delivery_status: 'submitted' }], +] as const) { + test(`${name} retains the original retry ID without claiming success`, async ($, on) => { + const { logs, toasts, completed } = setup(on); + on('process.run', () => ({ value: { exitCode: 0, stderr: '', stdout: JSON.stringify(status) } })); + await $.tool.call({ tool: 'Bash', tool_use_id: 'feedback', command }); + await completed; + expect(logs).toContain(`Feedback delivery is unconfirmed. Retry with /isaac-feedback retry ${id}.`); + expect(toasts).toEqual([]); + }); +} + +for (const [name, enabled, input] of [ + ['feedback disabled', '0', command], + ['synchronous submission', '1', command.replace(' --background', '')], + ['unrelated command', '1', 'pwd'], +] as const) { + test(`${name} preserves the tool result without notifications or status calls`, async ($, on) => { + const { answer, logs, toasts } = setup(on, enabled); + let processes = 0; + on('process.run', () => { processes += 1; return { deny: 'unexpected status query' }; }); + expect(await $.tool.call({ tool: 'Bash', tool_use_id: 'other', command: input })).toEqual(answer); + expect(logs).toEqual([]); + expect(toasts).toEqual([]); + expect(processes).toBe(0); + }); +} + +test('a denied Bash call remains denied and cannot send feedback', async ($, on) => { + const logs: string[] = []; + on('ui.log', ($, e) => { logs.push(e.text); return { value: undefined }; }); + on('tool.call', () => ({ deny: 'user policy' })); + expect(await $.tool.call({ tool: 'Bash', tool_use_id: 'denied', command })).toEqual({ deny: 'user policy' }); + expect(logs).toEqual([]); +}); From 336f14e7c18513ffaf231d88efa6cb161d075287 Mon Sep 17 00:00:00 2001 From: Josh Joseph Date: Wed, 21 Oct 2026 01:52:13 +0000 Subject: [PATCH 03/14] Remove mocked feedback notification tests --- tests/README.md | 12 +-- tests/integration/README.md | 8 +- .../mod_tests/isaac-feedback.test.ts | 101 ------------------ 3 files changed, 7 insertions(+), 114 deletions(-) delete mode 100644 tests/native_claude/mod_tests/isaac-feedback.test.ts diff --git a/tests/README.md b/tests/README.md index 7aafd69ab..33285c734 100644 --- a/tests/README.md +++ b/tests/README.md @@ -34,14 +34,10 @@ TypeScript event-forwarding and launch-option tests in CI. The existing Claude skill-toggle integration journey checks the live recipe payload field across on/off/on. See [the mod guide](../docs/claude-mod.md). -The native feedback tests cover returning before delivery, later completion messages, -duplicate monitoring, failed uploads, mismatched IDs, disabled notifications, and Bash -permission denials. They supply CLI results and clock ticks through native events; -they do not establish live upload or interactive form coverage. - -The required Isaac feedback form needs manual validation of the 16-character minimum, -cancellation, repeated toggles, and submission failures. The existing routing-toggle -journeys do not cover this form or a live Isaac evidence upload. +The required Isaac feedback form and background delivery notifications need manual +validation of the 16-character minimum, cancellation, repeated toggles, and +submission success/failure. The routing-toggle journeys do not cover the form, +notification timing, or live evidence uploads. `test_claude_answer_delivery.py` covers Claude answer selection, composition, ownership, failed tasks and resumed revisions using transcript inputs, plus real subprocess I/O, diff --git a/tests/integration/README.md b/tests/integration/README.md index 12636efd7..726576ed2 100644 --- a/tests/integration/README.md +++ b/tests/integration/README.md @@ -93,11 +93,9 @@ The Claude mod entry point has separate native component checks in `../native_claude/test_mod.py`. Claude 2.1.290 validates the module and runs its TypeScript event-forwarding and launch-option tests in a dedicated CI job. The existing Claude skill-toggle journey checks the live recipe payload field across on/off/on. -The native feedback component tests cover background delivery notifications and -failure handling using supplied CLI results and clock ticks. They do not upload -feedback or exercise the interactive form. -The required Isaac feedback form and evidence submission need a separate manual -Isaac check. The gateway toggle journey does not cover the form. +The required Isaac feedback form, evidence submission, and background delivery +notifications need a separate manual Isaac check. The gateway toggle journey does +not cover the form or notification timing. The existing unit tests keep their fixtures. Integration has an independent pytest configuration and uses `--confcutdir` so those fixtures cannot leak in. diff --git a/tests/native_claude/mod_tests/isaac-feedback.test.ts b/tests/native_claude/mod_tests/isaac-feedback.test.ts deleted file mode 100644 index 2be772341..000000000 --- a/tests/native_claude/mod_tests/isaac-feedback.test.ts +++ /dev/null @@ -1,101 +0,0 @@ -import type { On } from 'claude-code'; -import { expect, test } from 'claude-code/testing'; - -const id = 'e6ef90cf-25b3-4fee-a356-5f8fbc652e7c'; -const command = '/path/to/isaac feedback --category other --background -- "test feedback"'; -const receipt = { feedback_id: id, delivery_status: 'queued', evidence_status: 'not_requested' }; - -// Model the CLI and UI at the native event boundary; the packaged mod handles the real events. -function setup(on: On, enabled = '1') { - const stdout = `CLI diagnostic\n${JSON.stringify(receipt)}`; - const answer = { result: { stdout, stderr: '', interrupted: false }, text: stdout }; - const logs: string[] = []; - const toasts: string[] = []; - let complete: () => void = () => {}; - const completed = new Promise((resolve) => { complete = resolve; }); - let advance: () => void = () => {}; - on('clock.every', async () => { - await new Promise((resolve) => { advance = resolve; }); - return { value: undefined }; - }); - on('env.get', ($, e) => ({ value: e.name === 'ISAAC_SMART_ROUTER_FEEDBACK' ? enabled : undefined })); - on('tool.call', () => answer); - on('ui.log', ($, e) => { - logs.push(e.text); - if (e.text.includes('unconfirmed')) complete(); - return { value: undefined }; - }); - on('ui.toast', ($, e) => { toasts.push(e.text); complete(); return { value: undefined }; }); - return { answer, logs, toasts, completed, tick: () => advance() }; -} - -test('background feedback returns immediately and notifies when a later status confirms delivery', async ($, on) => { - const { logs, toasts, completed, tick } = setup(on); - let checks = 0; - on('process.run', ($, e) => { - expect(e.argv).toEqual(['isaac', 'feedback', '--status', id]); - checks += 1; - return { value: { exitCode: 0, stderr: '', stdout: JSON.stringify({ - ...receipt, delivery_status: checks === 1 ? 'queued' : 'submitted', - }) } }; - }); - - const result = await $.tool.call({ tool: 'Bash', tool_use_id: 'feedback', command }); - expect(result.result.stderr).toBe(''); - expect(result.result.stdout).toContain('CLI diagnostic\n'); - expect(JSON.parse(result.result.stdout.split('\n')[1])).toEqual({ ...receipt, delivery_notifications: 'native' }); - expect(result.text).toBe(result.result.stdout); - expect(logs).toEqual([`Sending feedback in the background. You can keep working. Feedback ID: ${id}`]); - expect(toasts).toEqual([]); - - // A second call for the same report must not attach another monitor. - await $.tool.call({ tool: 'Bash', tool_use_id: 'same-feedback', command }); - const next = await $.tool.call({ tool: 'Bash', tool_use_id: 'next-task', command: 'pwd' }); - expect(next.result.stdout).not.toContain('delivery_notifications'); - tick(); - await completed; - expect(checks).toBe(2); - expect(logs).toEqual([ - `Sending feedback in the background. You can keep working. Feedback ID: ${id}`, - `Feedback submitted successfully. Feedback ID: ${id}`, - ]); - expect(toasts).toEqual(['Feedback submitted successfully ✅']); -}); - -for (const [name, status] of [ - ['failed upload', { ...receipt, delivery_status: 'failed' }], - ['mismatched report', { ...receipt, feedback_id: '00000000-0000-0000-0000-000000000000', delivery_status: 'submitted' }], -] as const) { - test(`${name} retains the original retry ID without claiming success`, async ($, on) => { - const { logs, toasts, completed } = setup(on); - on('process.run', () => ({ value: { exitCode: 0, stderr: '', stdout: JSON.stringify(status) } })); - await $.tool.call({ tool: 'Bash', tool_use_id: 'feedback', command }); - await completed; - expect(logs).toContain(`Feedback delivery is unconfirmed. Retry with /isaac-feedback retry ${id}.`); - expect(toasts).toEqual([]); - }); -} - -for (const [name, enabled, input] of [ - ['feedback disabled', '0', command], - ['synchronous submission', '1', command.replace(' --background', '')], - ['unrelated command', '1', 'pwd'], -] as const) { - test(`${name} preserves the tool result without notifications or status calls`, async ($, on) => { - const { answer, logs, toasts } = setup(on, enabled); - let processes = 0; - on('process.run', () => { processes += 1; return { deny: 'unexpected status query' }; }); - expect(await $.tool.call({ tool: 'Bash', tool_use_id: 'other', command: input })).toEqual(answer); - expect(logs).toEqual([]); - expect(toasts).toEqual([]); - expect(processes).toBe(0); - }); -} - -test('a denied Bash call remains denied and cannot send feedback', async ($, on) => { - const logs: string[] = []; - on('ui.log', ($, e) => { logs.push(e.text); return { value: undefined }; }); - on('tool.call', () => ({ deny: 'user policy' })); - expect(await $.tool.call({ tool: 'Bash', tool_use_id: 'denied', command })).toEqual({ deny: 'user policy' }); - expect(logs).toEqual([]); -}); From 4075ea93285756e731cfdfafc951f25002af0ea1 Mon Sep 17 00:00:00 2001 From: Josh Joseph Date: Wed, 21 Oct 2026 01:54:13 +0000 Subject: [PATCH 04/14] Correct documented feedback test coverage --- docs/claude-mod.md | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/docs/claude-mod.md b/docs/claude-mod.md index 7c8ce60bb..679bddecb 100644 --- a/docs/claude-mod.md +++ b/docs/claude-mod.md @@ -111,9 +111,7 @@ and launch recipe after on. Each phase retains completed native parent/child answers and requires successful real gateway responses, with no recipe header. The `Claude mod · 2.1.290` CI job validates the packaged module and checks event -forwarding, launch-local options, and feedback delivery notifications. The notification -component tests supply CLI results and clock ticks through native events; they do not -upload feedback. To run those component checks locally: +forwarding and launch-local options. To run those component checks locally: ```bash UCODE_TEST_CLAUDE_BINARY=/absolute/path/to/claude \ From 3ce9018ee63f851475c123a18aecbc063f12de04 Mon Sep 17 00:00:00 2001 From: Josh Joseph Date: Wed, 21 Oct 2026 06:28:03 +0000 Subject: [PATCH 05/14] Use native Claude background tasks for feedback --- docs/claude-mod.md | 44 ++-- .../agents/claude_mods/isaac-feedback.ts | 231 ++++++++---------- src/ucode/agents/claude_mods/register.ts | 2 +- src/ucode/smart_routing/v2.py | 9 + tests/README.md | 6 +- tests/integration/README.md | 4 +- tests/test_claude_mod.py | 5 + 7 files changed, 149 insertions(+), 152 deletions(-) diff --git a/docs/claude-mod.md b/docs/claude-mod.md index 679bddecb..32a079361 100644 --- a/docs/claude-mod.md +++ b/docs/claude-mod.md @@ -63,30 +63,32 @@ rollout setting or the user's explicit `1`/`0` or `true`/`false` override. As wi boolean environment flags, `1`, `true`, `yes`, and `on` enable it (case-insensitive, with surrounding whitespace ignored); other values disable it. It controls Isaac feedback availability, so UG's routing presets and on/off toggles leave it unchanged. -The native form keeps routing on until Enter accepts a valid reason, then disables -routing and uploads the exact text in the background with `isaac feedback`. The -transcript shows the feedback ID the mod passes with `--feedback-id`. When Isaac -supports `--background`, it saves a bounded local snapshot and detaches the upload. -The form passes its submission timestamp with `--target-before` so a later task -cannot become the feedback target. A queued acknowledgement does not trigger a -success toast: the mod checks local `--status` until delivery or for about a minute. -Delivery produces both a persistent transcript message and a success toast. -Longer uploads keep running independently; `/isaac-feedback status ` checks -them and `/isaac-feedback retry ` retries the original report. Older Isaac -versions keep their synchronous CLI call in the mod's background task. -Cancel or Escape leaves routing on and sends nothing. A missing +The native form keeps routing on until Enter accepts a valid reason and Isaac saves +the bounded local snapshot. It uses the existing `--with-evidence --capture-local` +options, with the Isaac executable resolved at launch. Then it disables routing, +closes the form, and uploads the saved report with `isaac feedback --retry ` in +the existing native background callback. The transcript shows the sending message +and feedback ID. Delivery produces both a persistent message and a success toast; +a failed upload offers `/isaac-feedback retry ` for the original snapshot. +Before submitting, Cancel or Escape leaves routing on and sends nothing. A missing `isaac` CLI or an unavailable form leaves routing on. A feedback submission failure after a valid reason leaves routing off and reports the failure in the transcript. The form makes no model requests. -With the same feedback flag enabled, `/isaac-feedback` uses these native notifications -too. After a Bash call returns a background-submission receipt, the mod displays -"Sending feedback in the background. You can keep working. Feedback ID: ..." and -attaches the shared delivery monitor. Native clocks keep the monitor running after -the skill's turn ends. It only confirms delivery when `--status` returns `submitted` -for that ID; failures retain the original retry ID. The tool result carries -`delivery_notifications: "native"` so the skill avoids a duplicate acknowledgement. -Category selection, evidence collection, and Bash permission checks stay with the -skill and CLI. No extra model turn is needed for a completion notification. +With the same flag enabled, `/isaac-feedback` keeps the existing skill's category, +target, exact text, and agent note. The Bash hook recognizes a single literal Isaac +feedback command, saves evidence locally in the foreground, and submits that +snapshot. Retries and text-only Other reports use Claude's native +background Bash task. Both calls retain normal Bash permission checks; local +previews, target inspection, and compound shell commands pass through unchanged. +The native task completion event reports success or failure without a model turn +or status polling. `delivery_notifications: "native"` tells the skill to end without +duplicating the sending message. Other reports retain their existing text-only +transport and do not receive an evidence feedback ID. + +These uploads belong to the running Claude session. Keep it open until completion; +if interrupted, retry the saved evidence ID on the same machine. No Isaac background +worker or new CLI options are required. Saving evidence before returning control +keeps a later task or file edit out of that snapshot. `isaac-feedback.ts` owns this Databricks-specific policy, form, and submission. `register.ts` binds the routing callbacks before Isaac's command guard, then diff --git a/src/ucode/agents/claude_mods/isaac-feedback.ts b/src/ucode/agents/claude_mods/isaac-feedback.ts index 32000269f..c30d35225 100644 --- a/src/ucode/agents/claude_mods/isaac-feedback.ts +++ b/src/ucode/agents/claude_mods/isaac-feedback.ts @@ -2,94 +2,40 @@ import type { EngineInterface, On } from 'claude-code'; import { envBool } from './constants.ts'; const FEEDBACK_PANE = 'smart-router-feedback'; +const FEEDBACK_ID = /^[0-9a-f]{8}(?:-[0-9a-f]{4}){3}-[0-9a-f]{12}$/i; -type DeliveryReceipt = { - feedback_id: string; - delivery_status: string; - [key: string]: unknown; -}; - -function readDeliveryReceipt(output: string): { receipt: DeliveryReceipt; line: string } | undefined { - for (const line of output.trim().split('\n').reverse()) { - try { - const receipt = JSON.parse(line); - if (typeof receipt?.feedback_id === 'string' - && /^[0-9a-f]{8}(-[0-9a-f]{4}){3}-[0-9a-f]{12}$/i.test(receipt.feedback_id) - && typeof receipt.delivery_status === 'string') { - return { receipt, line }; - } - } catch {} - } - return undefined; -} - -function showSending($: EngineInterface, feedbackId: string): void { - $.ui.log(`Sending feedback in the background. You can keep working. Feedback ID: ${feedbackId}`); +function literalCommand(command: string): { raw: string[]; args: string[] } | undefined { + // The skill emits one literal command. Leave shell expansion and compound commands alone. + const word = /(?:[^\s'"\\$`;&|<>()*?[\]{}#~]+|'[^']*'|"[^"\\$`]*"|\\[^\r\n])+/g; + const raw = command.match(word); + if (!raw || !/^[ \t]*$/.test(command.replace(word, ''))) return; + const args = raw.map(value => value.replace(/'([^']*)'|"([^"]*)"|\\(.)/g, + (_, single, double, escaped) => single ?? double ?? escaped)); + if (!/(?:^|[/\\])isaac(?:\.exe)?$/.test(args[0]) || args[1] !== 'feedback') return; + return { raw, args }; } -function showSubmitted($: EngineInterface, feedbackId: string): void { - $.ui.log(`Feedback submitted successfully. Feedback ID: ${feedbackId}`); - $.ui.toast('Feedback submitted successfully ✅'); +function feedbackSuffix(feedbackId: string): string { + return feedbackId ? ` Feedback ID: ${feedbackId}` : ''; } -function showUnconfirmed($: EngineInterface, feedbackId: string): void { - $.ui.log(`Feedback delivery is unconfirmed. Retry with /isaac-feedback retry ${feedbackId}.`); +function showSending($: EngineInterface, feedbackId: string): void { + $.ui.log(`Sending feedback in the background. You can keep working.${feedbackSuffix(feedbackId)}`); } -function watchDelivery( - $: EngineInterface, - feedbackId: string, - active: Set, - announce = true, -): void { - if (active.has(feedbackId)) return; - active.add(feedbackId); - if (announce) showSending($, feedbackId); - let attempts = 0; - let checking = false; - let timer: { cancel: () => void } | undefined; - const stop = () => { - timer?.cancel(); - active.delete(feedbackId); - }; - const check = async () => { - if (checking) return; - checking = true; - attempts += 1; - try { - const result = await $.process.run(['isaac', 'feedback', '--status', feedbackId], { timeoutMs: 10000 }); - const status = result.exitCode === 0 ? readDeliveryReceipt(result.stdout)?.receipt : undefined; - if (status?.feedback_id !== feedbackId) throw new Error('Feedback status unavailable.'); - if (status.delivery_status === 'submitted') { - stop(); - showSubmitted($, feedbackId); - } else if (status.delivery_status !== 'queued') { - throw new Error('Feedback needs a retry.'); - } else if (attempts >= 13) { - stop(); - $.ui.log(`Feedback is still queued. Check it with /isaac-feedback status ${feedbackId}.`); - } - } catch { - stop(); - showUnconfirmed($, feedbackId); - } finally { - checking = false; - } - }; - try { - // Native clocks keep checking after the skill's turn ends and are disposed with the mod. - timer = $.clock.every(5000, () => { void check(); }); - void check(); - } catch { - stop(); - $.ui.log(`Check feedback delivery with /isaac-feedback status ${feedbackId}.`); +function showResult($: EngineInterface, feedbackId: string, sent: boolean): void { + if (sent) { + $.ui.log(`Feedback submitted successfully.${feedbackSuffix(feedbackId)}`); + $.ui.toast('Feedback submitted successfully ✅'); + } else { + const retry = feedbackId ? `/isaac-feedback retry ${feedbackId}` : '/isaac-feedback'; + $.ui.log(`Feedback delivery is unconfirmed. Retry with ${retry}.`); } } type FeedbackState = { submitted: boolean; reason: string; - backgroundSupported?: boolean; readRoutingEnabled?: () => Promise; disableRouting?: (sessionControlArgs: readonly string[]) => Promise; }; @@ -98,32 +44,72 @@ export function createIsaacFeedbackState(): FeedbackState { return { submitted: false, reason: '' }; } -export function registerIsaacFeedback(on: On, state: FeedbackState): void { - const active = new Set(); +export function registerIsaacFeedback(on: On, state: FeedbackState, isaacExecutable: string): void { + const uploads = new Map(); on('tool.call', { tool: 'Bash' }, async ($, e, next) => { - const answer = await next(e); - // The skill still owns category/target selection and the original permission-checked call. - if (typeof e.command !== 'string' || !e.command.includes('--background') - || !/\bfeedback\b/.test(e.command)) return answer; - try { - const payload = answer.result; - if (!payload || typeof payload !== 'object' || !('stdout' in payload) - || typeof payload.stdout !== 'string') return answer; - const parsed = readDeliveryReceipt(payload.stdout); - if (!parsed || !['queued', 'submitted'].includes(parsed.receipt.delivery_status) - || !envBool(await $.env.get('ISAAC_SMART_ROUTER_FEEDBACK'))) return answer; - watchDelivery($, parsed.receipt.feedback_id, active); - // Tell the skill that native UI owns the acknowledgement, avoiding a duplicate model reply. - const rendered = JSON.stringify({ ...parsed.receipt, delivery_notifications: 'native' }); - return { - ...answer, - result: { ...payload, stdout: payload.stdout.replace(parsed.line, rendered) }, - ...(typeof answer.text === 'string' ? { text: answer.text.replace(parsed.line, rendered) } : {}), - }; - } catch { - // A notification failure must not hide the submission result from the skill. - return answer; + if (typeof e.command !== 'string' || !e.command.includes('feedback')) return next(e); + const command = literalCommand(e.command); + if (!command || !envBool(await $.env.get('ISAAC_SMART_ROUTER_FEEDBACK'))) return next(e); + const { raw, args } = command; + const delimiter = args.indexOf('--'); + const options = delimiter < 0 ? args.slice(2) : args.slice(2, delimiter); + if (options.some(value => ['--help', '-h', '--inspect-target', '--capture-local', '--retry-pending'].includes(value.split('=')[0]))) { + return next(e); } + const option = (name: string): string | undefined => { + const index = options.findIndex(value => value === name || value.startsWith(`${name}=`)); + if (index < 0) return; + return options[index] === name ? options[index + 1] : options[index].slice(name.length + 1); + }; + let feedbackId = option('--retry') || ''; + if (options.some(value => value.split('=')[0] === '--retry') && !FEEDBACK_ID.test(feedbackId)) return next(e); + const category = option('--category') || option('-c') || 'other'; + const evidence = options.includes('--with-evidence') || category === 'smart-routing'; + let captureNotice = ''; + let answer; + if (!feedbackId && evidence) { + const captureOptions = ['--capture-local', "'~/.isaac/feedback'"]; + const captureCommand = delimiter < 0 ? [...raw, ...captureOptions] + : [...raw.slice(0, delimiter), ...captureOptions, ...raw.slice(delimiter)]; + const capture = await next({ ...e, command: captureCommand.join(' '), run_in_background: false }); + const payload = capture.result; + if (capture.isError || capture.deny || !payload || typeof payload !== 'object' + || !('stdout' in payload) || typeof payload.stdout !== 'string') return capture; + const id = /[/\\]([0-9a-f-]{36})[/\\]receipt\.json/i.exec(payload.stdout)?.[1]; + if (!id || !FEEDBACK_ID.test(id)) return capture; + feedbackId = id; + captureNotice = payload.stdout.split('Local feedback receipt:')[0].trim(); + answer = await $.tool.call({ + tool: 'Bash', tool_use_id: `${e.tool_use_id}-upload`, + command: `${raw[0]} feedback --retry ${id}`, + description: 'Upload saved Isaac feedback', run_in_background: true, + }); + } else { + if (!feedbackId && category !== 'other') return next(e); + answer = await next({ ...e, run_in_background: true }); + } + // Native Bash owns permission checks, process lifetime, and the completion event. + const payload = answer.result; + if (answer.isError || answer.deny || !payload || typeof payload !== 'object' + || !('backgroundTaskId' in payload) || typeof payload.backgroundTaskId !== 'string') return answer; + uploads.set(payload.backgroundTaskId, feedbackId); + showSending($, feedbackId); + const text = JSON.stringify({ + feedback_id: feedbackId || null, + delivery_notifications: 'native', + capture_notice: captureNotice || undefined, + instruction: 'The native UI reports sending and completion. Relay capture_notice if present, then end without repeating these messages, polling, or calling TaskOutput.', + }); + return { ...answer, text, result: { ...payload, stdout: text } }; + }); + + on('prompt.submit', { origin: { kind: 'task-notification' } }, ($, e, next) => { + const taskId = /([^<]+)<\/task-id>/.exec(e.text)?.[1]; + if (!taskId || !uploads.has(taskId)) return next(e); + const feedbackId = uploads.get(taskId)!; + uploads.delete(taskId); + showResult($, feedbackId, e.text.includes('completed')); + return { drop: 'Feedback completion reported by the native UI.' }; }); on('command.run', {command: 'smart-router'}, async ($, e, next) => { @@ -135,12 +121,12 @@ export function registerIsaacFeedback(on: On, state: FeedbackState): void { throw new Error('Smart Router controls are unavailable.'); } if (!await state.readRoutingEnabled()) return {text: 'Smart Router is off'}; + if (!isaacExecutable) throw new Error('Isaac CLI is unavailable.'); // The form uploads through Isaac's CLI; without it, keep routing on. `isaac --version` launches a harness. - const help = await $.process.run(['isaac', 'feedback', '--help'], { timeoutMs: 10000 }); + const help = await $.process.run([isaacExecutable, 'feedback', '--help'], { timeoutMs: 10000 }); if (help.exitCode !== 0) { throw new Error('Isaac CLI is unavailable.'); } - state.backgroundSupported = ['--background', '--status', '--target-before'].every((flag) => help.stdout.includes(flag)); state.submitted = false; state.reason = ''; const pane = await $.ui.open({ @@ -159,6 +145,7 @@ export function registerIsaacFeedback(on: On, state: FeedbackState): void { on('ui.render', { component: 'Pane' }, ($, e, next) => { if (e.requestId !== FEEDBACK_PANE || e.surface === 'mobile') return next(e); const { Box, Text, Input, Button } = $.ui.resolve(e); + if (state.submitted) return Text({ children: ['Saving feedback...'] }); return Box({ flexDirection: 'column', children: [ @@ -178,41 +165,31 @@ export function registerIsaacFeedback(on: On, state: FeedbackState): void { return; } state.submitted = true; - const targetBefore = new Date().toISOString(); + $.ui.invalidate('ui.render'); + const feedbackId = crypto.randomUUID(); try { + const sessionId = await $.session.id(); + const capture = await $.process.run( + [isaacExecutable, 'feedback', '--category', 'smart-routing', '--harness', 'claude', + '--session-id', sessionId, '--feedback-id', feedbackId, '--with-evidence', + '--capture-local', '~/.isaac/feedback'], + { stdin: value, timeoutMs: 120000 }, + ); + if (capture.exitCode !== 0) throw new Error('Could not save feedback.'); + if (capture.stderr.trim()) $.ui.log(capture.stderr.trim()); await state.disableRouting(['--smart-router-feedback', value]); } catch { state.submitted = false; - $.ui.toast('Could not update Smart Router. Smart Router is still on; retry or restart through UG.'); + $.ui.invalidate('ui.render'); + $.ui.toast('Could not save feedback or update Smart Router. Smart Router is still on; retry or restart through UG.'); return; } await $.ui.close({ id: FEEDBACK_PANE }).catch(() => {}); // Upload in the background through Isaac's CLI, without a model turn. - const feedbackId = crypto.randomUUID(); showSending($, feedbackId); - const background = state.backgroundSupported; - void (async () => { - const sessionId = await $.session.id(); - const args = ['isaac', 'feedback', '--category', 'smart-routing', '--harness', 'claude', - '--session-id', sessionId, '--feedback-id', feedbackId]; - if (background) args.push('--background', '--target-before', targetBefore); - const result = await $.process.run(args, { stdin: value, timeoutMs: 120000 }); - if (result.exitCode !== 0) throw new Error('Feedback command failed.'); - // Older Isaac releases finish delivery in the command itself. - if (!background) { - showSubmitted($, feedbackId); - return; - } - const status = readDeliveryReceipt(result.stdout)?.receipt; - if (status?.feedback_id !== feedbackId - || !['queued', 'submitted'].includes(status.delivery_status)) { - throw new Error('Feedback needs a retry.'); - } - watchDelivery($, feedbackId, active, false); - })().catch(() => { - // Keep the original ID even when the enqueue acknowledgement was lost. - showUnconfirmed($, feedbackId); - }); + void $.process.run([isaacExecutable, 'feedback', '--retry', feedbackId], { timeoutMs: 120000 }) + .then(result => showResult($, feedbackId, result.exitCode === 0), + () => showResult($, feedbackId, false)); }, }), Button({ key: 'cancel', label: 'Cancel (keep routing on)', onPress: () => $.ui.close({ id: FEEDBACK_PANE }) }), diff --git a/src/ucode/agents/claude_mods/register.ts b/src/ucode/agents/claude_mods/register.ts index c9452106a..c07222ff8 100644 --- a/src/ucode/agents/claude_mods/register.ts +++ b/src/ucode/agents/claude_mods/register.ts @@ -117,7 +117,7 @@ export function register(on: On, options: PluginOptions): void { } return next(e); }); - registerIsaacFeedback(on, feedback); + registerIsaacFeedback(on, feedback, options.isaacExecutable as string); on('command.run', {command: 'smart-router'}, async ($, e) => { const action = e.args.trim(); try { diff --git a/src/ucode/smart_routing/v2.py b/src/ucode/smart_routing/v2.py index 7a31eed3c..aba9a9b48 100644 --- a/src/ucode/smart_routing/v2.py +++ b/src/ucode/smart_routing/v2.py @@ -3,6 +3,7 @@ import hashlib import json import os +import shutil import signal import socket import subprocess @@ -340,12 +341,14 @@ def _routed_claude_agent_definitions(model_ids: list[str]) -> dict[str, dict[str def _claude_mod_launch_options(env: Mapping[str, str] | None = None) -> dict: source = resolve_environment(env, agent=AGENT_CLAUDE) + isaac = shutil.which("isaac") return { **{key: source.get(key, "") for key in SMART_ROUTING_ENV_KEYS}, # Native plugin options cannot represent null; preserve absent versus empty values. "unset": [key for key in SMART_ROUTING_ENV_KEYS if key not in source], "recipe": source.get(routing.ROUTER_NAME_ENV_VAR, "").strip() or routing.ROUTER_NAME, "routeCommand": [], + "isaacExecutable": str(Path(isaac).absolute()) if isaac else "", } @@ -359,6 +362,12 @@ def _write_routed_claude_plugin(plugin_dir: Path, model_ids: list[str]) -> None: "description": "Launch-scoped agents for Unity Gateway smart routing.", "author": {"name": "Databricks"}, "userConfig": { + "isaacExecutable": { + "type": "string", + "default": "", + "title": "Isaac executable", + "description": "Isaac CLI resolved at launch for feedback submission", + }, "routeCommand": { "type": "string", "multiple": True, diff --git a/tests/README.md b/tests/README.md index 33285c734..19eb68ded 100644 --- a/tests/README.md +++ b/tests/README.md @@ -36,8 +36,10 @@ See [the mod guide](../docs/claude-mod.md). The required Isaac feedback form and background delivery notifications need manual validation of the 16-character minimum, cancellation, repeated toggles, and -submission success/failure. The routing-toggle journeys do not cover the form, -notification timing, or live evidence uploads. +submission success/failure. Also check that a new task can run during upload, +saved evidence stays unchanged, upload permission denials are honored, local +previews stay local, and Other stays text-only. The routing-toggle journeys do +not cover the form, notification timing, or live evidence uploads. `test_claude_answer_delivery.py` covers Claude answer selection, composition, ownership, failed tasks and resumed revisions using transcript inputs, plus real subprocess I/O, diff --git a/tests/integration/README.md b/tests/integration/README.md index 726576ed2..b77b9bd44 100644 --- a/tests/integration/README.md +++ b/tests/integration/README.md @@ -95,7 +95,9 @@ TypeScript event-forwarding and launch-option tests in a dedicated CI job. The existing Claude skill-toggle journey checks the live recipe payload field across on/off/on. The required Isaac feedback form, evidence submission, and background delivery notifications need a separate manual Isaac check. The gateway toggle journey does -not cover the form or notification timing. +not cover the form or notification timing. Use released Isaac to check background +completion during another task, immutable evidence, upload permission denials, +local previews, and text-only Other reports. The existing unit tests keep their fixtures. Integration has an independent pytest configuration and uses `--confcutdir` so those fixtures cannot leak in. diff --git a/tests/test_claude_mod.py b/tests/test_claude_mod.py index e122fe319..28aee7038 100644 --- a/tests/test_claude_mod.py +++ b/tests/test_claude_mod.py @@ -1,6 +1,7 @@ """Generated plugin contents for the native Claude mod entry point.""" import json +import shutil from pathlib import Path import pytest @@ -50,17 +51,20 @@ def test_launch_options_resolve_versions_without_mutating_input(version): expected = config._VERSIONS.get(version, {}).get( AGENT_CLAUDE, dict.fromkeys(SMART_ROUTING_ENV_KEYS, "0") ) + isaac = shutil.which("isaac") assert v2._claude_mod_launch_options(env) == { **expected, "unset": [], "recipe": "custom_recipe", "routeCommand": [], + "isaacExecutable": str(Path(isaac).absolute()) if isaac else "", } # Taking the baseline must not alter the caller's launch environment. assert env == before def test_launch_options_preserve_empty_and_absent_values(): + isaac = shutil.which("isaac") assert v2._claude_mod_launch_options({"ENABLE_SMART_ROUTING_V2": ""}) == { "ENABLE_SMART_ROUTING_V2": "", "ENABLE_SMART_ROUTING_SUBAGENT_ONLY": "", @@ -68,4 +72,5 @@ def test_launch_options_preserve_empty_and_absent_values(): "unset": ["ENABLE_SMART_ROUTING_SUBAGENT_ONLY", "ENABLE_SMART_ROUTER_ORCHESTRATOR"], "recipe": "task_v3", "routeCommand": [], + "isaacExecutable": str(Path(isaac).absolute()) if isaac else "", } From d63973c6b3c079364f027455050aa5c231f9a6ad Mon Sep 17 00:00:00 2001 From: Josh Joseph Date: Wed, 21 Oct 2026 06:30:22 +0000 Subject: [PATCH 06/14] Preserve diagnostics for automatic evidence capture --- src/ucode/agents/claude_mods/isaac-feedback.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/src/ucode/agents/claude_mods/isaac-feedback.ts b/src/ucode/agents/claude_mods/isaac-feedback.ts index c30d35225..0ceac9587 100644 --- a/src/ucode/agents/claude_mods/isaac-feedback.ts +++ b/src/ucode/agents/claude_mods/isaac-feedback.ts @@ -69,6 +69,7 @@ export function registerIsaacFeedback(on: On, state: FeedbackState, isaacExecuta let answer; if (!feedbackId && evidence) { const captureOptions = ['--capture-local', "'~/.isaac/feedback'"]; + if (!options.includes('--with-evidence')) captureOptions.unshift('--with-evidence'); const captureCommand = delimiter < 0 ? [...raw, ...captureOptions] : [...raw.slice(0, delimiter), ...captureOptions, ...raw.slice(delimiter)]; const capture = await next({ ...e, command: captureCommand.join(' '), run_in_background: false }); From ee2c70e5bab5ededc400d70bc57587c0a7ec185a Mon Sep 17 00:00:00 2001 From: Josh Joseph Date: Wed, 21 Oct 2026 06:34:07 +0000 Subject: [PATCH 07/14] Load Claude feedback hooks when routing is disabled --- docs/claude-mod.md | 9 +++- src/ucode/agents/claude.py | 61 +++++++++++++++++++----- src/ucode/agents/claude_mods/register.ts | 2 +- src/ucode/launcher.py | 6 ++- tests/README.md | 7 ++- tests/integration/README.md | 4 +- 6 files changed, 70 insertions(+), 19 deletions(-) diff --git a/docs/claude-mod.md b/docs/claude-mod.md index 32a079361..30f3a7a51 100644 --- a/docs/claude-mod.md +++ b/docs/claude-mod.md @@ -1,7 +1,8 @@ # Claude mod entry point -UG's smart-routed Claude launcher loads `hooks/register.ts` through the existing -launch-scoped `ug-smart-router` plugin. Claude Code 2.1.290 or newer is required. +UG's Claude launcher loads `hooks/register.ts` through the existing launch-scoped +`ug-smart-router` plugin for smart routing or Isaac feedback. Claude Code 2.1.290 or +newer is required. The existing `_write_routed_claude_plugin()` copies the packaged TypeScript entry point from `src/ucode/agents/claude_mods/` and writes `hooks/hooks.json` into that plugin. @@ -85,6 +86,10 @@ or status polling. `delivery_notifications: "native"` tells the skill to end wit duplicating the sending message. Other reports retain their existing text-only transport and do not receive an evidence feedback ID. +Claude launches without smart routing also load the feedback hooks when this flag +is enabled. Routing stays disabled and the status band stays hidden. UG keeps the +temporary plugin alive until Claude exits, then removes it. + These uploads belong to the running Claude session. Keep it open until completion; if interrupted, retry the saved evidence ID on the same machine. No Isaac background worker or new CLI options are required. Saving evidence before returning control diff --git a/src/ucode/agents/claude.py b/src/ucode/agents/claude.py index e642c5663..1d9c95c1c 100644 --- a/src/ucode/agents/claude.py +++ b/src/ucode/agents/claude.py @@ -13,8 +13,10 @@ import sys import threading import traceback -from collections.abc import Callable, Iterable, Sequence +from collections.abc import Callable, Iterable, Iterator, Sequence +from contextlib import contextmanager from pathlib import Path +from tempfile import TemporaryDirectory from ucode.config_io import ( APP_DIR, @@ -27,6 +29,7 @@ ) from ucode.constants import ( AI_GATEWAY_REQUEST_TAGS_HEADER, + ISAAC_SMART_ROUTER_FEEDBACK_ENV_VAR, LOOPBACK_HOST, MCP_CLEANUP_SCOPES, MCP_USER_SCOPE, @@ -50,6 +53,7 @@ maybe_add_gpt_1m_suffix, ug_binary, ) +from ucode.env import env_bool from ucode.launcher import exec_or_spawn from ucode.managed_config import refresh_managed_config from ucode.managed_files import ( @@ -2259,6 +2263,30 @@ def _rewrite_relayed_port(state: dict, port: int) -> None: write_json_file(CLAUDE_SETTINGS_PATH, settings) +@contextmanager +def _feedback_launch_args( + tool_args: list[str], settings_override: dict | None = None +) -> Iterator[tuple[list[str], dict | None]]: + if not env_bool(ISAAC_SMART_ROUTER_FEEDBACK_ENV_VAR): + yield tool_args, settings_override + return + APP_DIR.mkdir(parents=True, exist_ok=True) + with TemporaryDirectory(prefix="claude-feedback-", dir=APP_DIR) as directory: + plugin_dir = Path(directory) / "plugin" + smart_routing_v2._write_routed_claude_plugin(plugin_dir, []) + feedback_settings = { + "pluginConfigs": { + smart_routing_v2.CLAUDE_ROUTING_PLUGIN_NAME: { + "options": smart_routing_v2._claude_mod_launch_options({}) + } + } + } + yield ( + ["--plugin-dir", str(plugin_dir), *tool_args], + _merge_claude_settings(settings_override or {}, feedback_settings), + ) + + def _launch_relayed(state: dict, binary: str, tool_args: list[str]) -> None: """Relayed launch: sign into the Claude subscription, start the loopback refresh proxy, then run Claude Code alongside it (the proxy must outlive the @@ -2288,16 +2316,21 @@ def token_provider(force_refresh: bool) -> str: server_thread = threading.Thread(target=server.serve_forever, daemon=True) server_thread.start() - proc = subprocess_cross_os.popen(_build_claude_argv(binary, tool_args, relayed=True)) - try: - returncode = proc.wait() - except KeyboardInterrupt: - proc.send_signal(signal.SIGINT) - returncode = proc.wait() - finally: - cache.stop() - server.shutdown() - client.close() + with _feedback_launch_args(tool_args) as (launch_args, settings_override): + proc = subprocess_cross_os.popen( + _build_claude_argv( + binary, launch_args, relayed=True, settings_override=settings_override + ) + ) + try: + returncode = proc.wait() + except KeyboardInterrupt: + proc.send_signal(signal.SIGINT) + returncode = proc.wait() + finally: + cache.stop() + server.shutdown() + client.close() raise SystemExit(returncode) @@ -2383,6 +2416,12 @@ def launch( } settings_override = _merge_claude_settings(settings_override or {}, {"env": fallback_env}) os.environ.update(fallback_env) + if env_bool(ISAAC_SMART_ROUTER_FEEDBACK_ENV_VAR): + with _feedback_launch_args(launch_args, settings_override) as (args, override): + exec_or_spawn( + _build_claude_argv(binary, args, settings_override=override), wait_for_child=True + ) + return exec_or_spawn(_build_claude_argv(binary, launch_args, settings_override=settings_override)) diff --git a/src/ucode/agents/claude_mods/register.ts b/src/ucode/agents/claude_mods/register.ts index c07222ff8..82fffee9f 100644 --- a/src/ucode/agents/claude_mods/register.ts +++ b/src/ucode/agents/claude_mods/register.ts @@ -129,6 +129,6 @@ export function register(on: On, options: PluginOptions): void { } return {text: `Smart Router is ${action}`}; }); - registerStatusBand(on); + if (launchedEnabled) registerStatusBand(on); registerSubagentRouting(on, options); } diff --git a/src/ucode/launcher.py b/src/ucode/launcher.py index 382672a44..5fa0d694a 100644 --- a/src/ucode/launcher.py +++ b/src/ucode/launcher.py @@ -9,12 +9,14 @@ from ucode.os_compatibility import subprocess_cross_os -def exec_or_spawn(argv: list[str]) -> None: +def exec_or_spawn(argv: list[str], *, wait_for_child: bool = False) -> None: """Hand the terminal to ``argv``, then exit with its status. On POSIX we ``os.execvp`` — the agent process *replaces* ucode, inheriting the controlling terminal cleanly. + Set wait_for_child when launch resources need cleanup after the agent exits. + On Windows there is no real ``exec``: ``os.execvp`` spawns a *new* process and immediately terminates the parent, so the launching shell resumes its prompt and fights the agent for the console. That produces the garbled, @@ -22,7 +24,7 @@ def exec_or_spawn(argv: list[str]) -> None: for it, and propagate its exit code — the same pattern the token-refreshing agents (gemini/opencode/copilot/pi) already use. """ - if os.name != "nt": + if os.name != "nt" and not wait_for_child: os.execvp(argv[0], argv) return # unreachable on POSIX; keeps type-checkers happy diff --git a/tests/README.md b/tests/README.md index 19eb68ded..f56ce4091 100644 --- a/tests/README.md +++ b/tests/README.md @@ -38,8 +38,11 @@ The required Isaac feedback form and background delivery notifications need manu validation of the 16-character minimum, cancellation, repeated toggles, and submission success/failure. Also check that a new task can run during upload, saved evidence stays unchanged, upload permission denials are honored, local -previews stay local, and Other stays text-only. The routing-toggle journeys do -not cover the form, notification timing, or live evidence uploads. +previews stay local, and Other stays text-only. Run `/isaac-feedback` in sessions +launched with routing both on and off; check that a routing-disabled launch keeps +the status band hidden and removes its temporary plugin after exit. The +routing-toggle journeys do not cover the form, notification timing, or live +evidence uploads. `test_claude_answer_delivery.py` covers Claude answer selection, composition, ownership, failed tasks and resumed revisions using transcript inputs, plus real subprocess I/O, diff --git a/tests/integration/README.md b/tests/integration/README.md index b77b9bd44..5f6020146 100644 --- a/tests/integration/README.md +++ b/tests/integration/README.md @@ -97,7 +97,9 @@ The required Isaac feedback form, evidence submission, and background delivery notifications need a separate manual Isaac check. The gateway toggle journey does not cover the form or notification timing. Use released Isaac to check background completion during another task, immutable evidence, upload permission denials, -local previews, and text-only Other reports. +local previews, and text-only Other reports. Also launch with routing disabled: +`/isaac-feedback` should still submit in the background, the status band should +stay hidden, and the temporary feedback plugin should be removed after exit. The existing unit tests keep their fixtures. Integration has an independent pytest configuration and uses `--confcutdir` so those fixtures cannot leak in. From a8ca299d1fa72eb35d1c63eacf21405966f808c2 Mon Sep 17 00:00:00 2001 From: Josh Joseph Date: Wed, 21 Oct 2026 18:10:03 +0000 Subject: [PATCH 08/14] Use skill-directed background feedback uploads --- docs/claude-mod.md | 22 ++++--- src/ucode/agents/claude.py | 1 + .../agents/claude_mods/isaac-feedback.ts | 63 +++---------------- tests/README.md | 5 +- tests/integration/README.md | 7 ++- 5 files changed, 28 insertions(+), 70 deletions(-) diff --git a/docs/claude-mod.md b/docs/claude-mod.md index 30f3a7a51..74d788709 100644 --- a/docs/claude-mod.md +++ b/docs/claude-mod.md @@ -75,16 +75,18 @@ Before submitting, Cancel or Escape leaves routing on and sends nothing. A missi `isaac` CLI or an unavailable form leaves routing on. A feedback submission failure after a valid reason leaves routing off and reports the failure in the transcript. The form makes no model requests. -With the same flag enabled, `/isaac-feedback` keeps the existing skill's category, -target, exact text, and agent note. The Bash hook recognizes a single literal Isaac -feedback command, saves evidence locally in the foreground, and submits that -snapshot. Retries and text-only Other reports use Claude's native -background Bash task. Both calls retain normal Bash permission checks; local -previews, target inspection, and compound shell commands pass through unchanged. -The native task completion event reports success or failure without a model turn -or status polling. `delivery_notifications: "native"` tells the skill to end without -duplicating the sending message. Other reports retain their existing text-only -transport and do not receive an evidence feedback ID. +With the same flag enabled, `/isaac-feedback` keeps the skill's category, target, +exact text, and agent note. The skill saves evidence locally in the foreground, +then calls `isaac feedback --retry ` using Claude's native background Bash +task. Retries and text-only Other reports also use background Bash. This requires +Isaac's background-submission skill instructions; the CLI commands already exist. + +The hook observes background tasks with the description `Isaac feedback upload ` +(or `Isaac feedback upload` for Other). It leaves commands and Bash permission +checks unchanged. Native task completion reports success or failure without a +model turn or status polling. `delivery_notifications: "native"` tells the skill +to end without duplicating the sending message. Other reports retain their +text-only transport and do not receive an evidence feedback ID. Claude launches without smart routing also load the feedback hooks when this flag is enabled. Routing stays disabled and the status band stays hidden. UG keeps the diff --git a/src/ucode/agents/claude.py b/src/ucode/agents/claude.py index 1d9c95c1c..2d5f314f8 100644 --- a/src/ucode/agents/claude.py +++ b/src/ucode/agents/claude.py @@ -2418,6 +2418,7 @@ def launch( os.environ.update(fallback_env) if env_bool(ISAAC_SMART_ROUTER_FEEDBACK_ENV_VAR): with _feedback_launch_args(launch_args, settings_override) as (args, override): + # Keep the temporary feedback plugin available until this Claude session exits. exec_or_spawn( _build_claude_argv(binary, args, settings_override=override), wait_for_child=True ) diff --git a/src/ucode/agents/claude_mods/isaac-feedback.ts b/src/ucode/agents/claude_mods/isaac-feedback.ts index 0ceac9587..14d03222b 100644 --- a/src/ucode/agents/claude_mods/isaac-feedback.ts +++ b/src/ucode/agents/claude_mods/isaac-feedback.ts @@ -3,17 +3,7 @@ import { envBool } from './constants.ts'; const FEEDBACK_PANE = 'smart-router-feedback'; const FEEDBACK_ID = /^[0-9a-f]{8}(?:-[0-9a-f]{4}){3}-[0-9a-f]{12}$/i; - -function literalCommand(command: string): { raw: string[]; args: string[] } | undefined { - // The skill emits one literal command. Leave shell expansion and compound commands alone. - const word = /(?:[^\s'"\\$`;&|<>()*?[\]{}#~]+|'[^']*'|"[^"\\$`]*"|\\[^\r\n])+/g; - const raw = command.match(word); - if (!raw || !/^[ \t]*$/.test(command.replace(word, ''))) return; - const args = raw.map(value => value.replace(/'([^']*)'|"([^"]*)"|\\(.)/g, - (_, single, double, escaped) => single ?? double ?? escaped)); - if (!/(?:^|[/\\])isaac(?:\.exe)?$/.test(args[0]) || args[1] !== 'feedback') return; - return { raw, args }; -} +const FEEDBACK_UPLOAD_DESCRIPTION = 'Isaac feedback upload'; function feedbackSuffix(feedbackId: string): string { return feedbackId ? ` Feedback ID: ${feedbackId}` : ''; @@ -47,48 +37,12 @@ export function createIsaacFeedbackState(): FeedbackState { export function registerIsaacFeedback(on: On, state: FeedbackState, isaacExecutable: string): void { const uploads = new Map(); on('tool.call', { tool: 'Bash' }, async ($, e, next) => { - if (typeof e.command !== 'string' || !e.command.includes('feedback')) return next(e); - const command = literalCommand(e.command); - if (!command || !envBool(await $.env.get('ISAAC_SMART_ROUTER_FEEDBACK'))) return next(e); - const { raw, args } = command; - const delimiter = args.indexOf('--'); - const options = delimiter < 0 ? args.slice(2) : args.slice(2, delimiter); - if (options.some(value => ['--help', '-h', '--inspect-target', '--capture-local', '--retry-pending'].includes(value.split('=')[0]))) { - return next(e); - } - const option = (name: string): string | undefined => { - const index = options.findIndex(value => value === name || value.startsWith(`${name}=`)); - if (index < 0) return; - return options[index] === name ? options[index + 1] : options[index].slice(name.length + 1); - }; - let feedbackId = option('--retry') || ''; - if (options.some(value => value.split('=')[0] === '--retry') && !FEEDBACK_ID.test(feedbackId)) return next(e); - const category = option('--category') || option('-c') || 'other'; - const evidence = options.includes('--with-evidence') || category === 'smart-routing'; - let captureNotice = ''; - let answer; - if (!feedbackId && evidence) { - const captureOptions = ['--capture-local', "'~/.isaac/feedback'"]; - if (!options.includes('--with-evidence')) captureOptions.unshift('--with-evidence'); - const captureCommand = delimiter < 0 ? [...raw, ...captureOptions] - : [...raw.slice(0, delimiter), ...captureOptions, ...raw.slice(delimiter)]; - const capture = await next({ ...e, command: captureCommand.join(' '), run_in_background: false }); - const payload = capture.result; - if (capture.isError || capture.deny || !payload || typeof payload !== 'object' - || !('stdout' in payload) || typeof payload.stdout !== 'string') return capture; - const id = /[/\\]([0-9a-f-]{36})[/\\]receipt\.json/i.exec(payload.stdout)?.[1]; - if (!id || !FEEDBACK_ID.test(id)) return capture; - feedbackId = id; - captureNotice = payload.stdout.split('Local feedback receipt:')[0].trim(); - answer = await $.tool.call({ - tool: 'Bash', tool_use_id: `${e.tool_use_id}-upload`, - command: `${raw[0]} feedback --retry ${id}`, - description: 'Upload saved Isaac feedback', run_in_background: true, - }); - } else { - if (!feedbackId && category !== 'other') return next(e); - answer = await next({ ...e, run_in_background: true }); - } + if (e.run_in_background !== true || typeof e.description !== 'string' + || !e.description.startsWith(FEEDBACK_UPLOAD_DESCRIPTION)) return next(e); + const feedbackId = e.description.slice(FEEDBACK_UPLOAD_DESCRIPTION.length).trim(); + if ((feedbackId && !FEEDBACK_ID.test(feedbackId)) + || !envBool(await $.env.get('ISAAC_SMART_ROUTER_FEEDBACK'))) return next(e); + const answer = await next(e); // Native Bash owns permission checks, process lifetime, and the completion event. const payload = answer.result; if (answer.isError || answer.deny || !payload || typeof payload !== 'object' @@ -98,8 +52,7 @@ export function registerIsaacFeedback(on: On, state: FeedbackState, isaacExecuta const text = JSON.stringify({ feedback_id: feedbackId || null, delivery_notifications: 'native', - capture_notice: captureNotice || undefined, - instruction: 'The native UI reports sending and completion. Relay capture_notice if present, then end without repeating these messages, polling, or calling TaskOutput.', + instruction: 'The native UI reports sending and completion. End without repeating these messages, polling, or calling TaskOutput.', }); return { ...answer, text, result: { ...payload, stdout: text } }; }); diff --git a/tests/README.md b/tests/README.md index f56ce4091..b0ec760c4 100644 --- a/tests/README.md +++ b/tests/README.md @@ -35,8 +35,9 @@ skill-toggle integration journey checks the live recipe payload field across on/ See [the mod guide](../docs/claude-mod.md). The required Isaac feedback form and background delivery notifications need manual -validation of the 16-character minimum, cancellation, repeated toggles, and -submission success/failure. Also check that a new task can run during upload, +validation using Isaac's background-submission skill instructions and existing +capture/retry CLI. Check the 16-character minimum, cancellation, repeated toggles, +and submission success/failure. Also check that a new task can run during upload, saved evidence stays unchanged, upload permission denials are honored, local previews stay local, and Other stays text-only. Run `/isaac-feedback` in sessions launched with routing both on and off; check that a routing-disabled launch keeps diff --git a/tests/integration/README.md b/tests/integration/README.md index 5f6020146..ef98ae0aa 100644 --- a/tests/integration/README.md +++ b/tests/integration/README.md @@ -95,9 +95,10 @@ TypeScript event-forwarding and launch-option tests in a dedicated CI job. The existing Claude skill-toggle journey checks the live recipe payload field across on/off/on. The required Isaac feedback form, evidence submission, and background delivery notifications need a separate manual Isaac check. The gateway toggle journey does -not cover the form or notification timing. Use released Isaac to check background -completion during another task, immutable evidence, upload permission denials, -local previews, and text-only Other reports. Also launch with routing disabled: +not cover the form or notification timing. Use Isaac's background-submission skill +instructions with the existing capture/retry CLI to check completion during another +task, immutable evidence, upload permission denials, local previews, and text-only +Other reports. Also launch with routing disabled: `/isaac-feedback` should still submit in the background, the status band should stay hidden, and the temporary feedback plugin should be removed after exit. From eec0658b67a8a8f6223b217e4cefbbb83fc11634 Mon Sep 17 00:00:00 2001 From: Josh Joseph Date: Wed, 21 Oct 2026 18:21:06 +0000 Subject: [PATCH 09/14] Pass an absolute directory to Isaac feedback capture --- docs/claude-mod.md | 4 ++-- src/ucode/agents/claude_mods/isaac-feedback.ts | 8 +++++--- src/ucode/agents/claude_mods/register.ts | 2 +- src/ucode/smart_routing/v2.py | 7 +++++++ tests/test_claude_mod.py | 2 ++ 5 files changed, 17 insertions(+), 6 deletions(-) diff --git a/docs/claude-mod.md b/docs/claude-mod.md index 74d788709..63c827827 100644 --- a/docs/claude-mod.md +++ b/docs/claude-mod.md @@ -66,8 +66,8 @@ with surrounding whitespace ignored); other values disable it. It controls Isaac availability, so UG's routing presets and on/off toggles leave it unchanged. The native form keeps routing on until Enter accepts a valid reason and Isaac saves the bounded local snapshot. It uses the existing `--with-evidence --capture-local` -options, with the Isaac executable resolved at launch. Then it disables routing, -closes the form, and uploads the saved report with `isaac feedback --retry ` in +options, with the Isaac executable and absolute capture directory resolved at launch. +Then it disables routing, closes the form, and uploads the saved report with `isaac feedback --retry ` in the existing native background callback. The transcript shows the sending message and feedback ID. Delivery produces both a persistent message and a success toast; a failed upload offers `/isaac-feedback retry ` for the original snapshot. diff --git a/src/ucode/agents/claude_mods/isaac-feedback.ts b/src/ucode/agents/claude_mods/isaac-feedback.ts index 14d03222b..2bb772d81 100644 --- a/src/ucode/agents/claude_mods/isaac-feedback.ts +++ b/src/ucode/agents/claude_mods/isaac-feedback.ts @@ -34,7 +34,9 @@ export function createIsaacFeedbackState(): FeedbackState { return { submitted: false, reason: '' }; } -export function registerIsaacFeedback(on: On, state: FeedbackState, isaacExecutable: string): void { +export function registerIsaacFeedback( + on: On, state: FeedbackState, isaacExecutable: string, isaacFeedbackDirectory: string, +): void { const uploads = new Map(); on('tool.call', { tool: 'Bash' }, async ($, e, next) => { if (e.run_in_background !== true || typeof e.description !== 'string' @@ -75,7 +77,7 @@ export function registerIsaacFeedback(on: On, state: FeedbackState, isaacExecuta throw new Error('Smart Router controls are unavailable.'); } if (!await state.readRoutingEnabled()) return {text: 'Smart Router is off'}; - if (!isaacExecutable) throw new Error('Isaac CLI is unavailable.'); + if (!isaacExecutable || !isaacFeedbackDirectory) throw new Error('Isaac feedback is unavailable.'); // The form uploads through Isaac's CLI; without it, keep routing on. `isaac --version` launches a harness. const help = await $.process.run([isaacExecutable, 'feedback', '--help'], { timeoutMs: 10000 }); if (help.exitCode !== 0) { @@ -126,7 +128,7 @@ export function registerIsaacFeedback(on: On, state: FeedbackState, isaacExecuta const capture = await $.process.run( [isaacExecutable, 'feedback', '--category', 'smart-routing', '--harness', 'claude', '--session-id', sessionId, '--feedback-id', feedbackId, '--with-evidence', - '--capture-local', '~/.isaac/feedback'], + '--capture-local', isaacFeedbackDirectory], { stdin: value, timeoutMs: 120000 }, ); if (capture.exitCode !== 0) throw new Error('Could not save feedback.'); diff --git a/src/ucode/agents/claude_mods/register.ts b/src/ucode/agents/claude_mods/register.ts index 82fffee9f..f4e34ca1a 100644 --- a/src/ucode/agents/claude_mods/register.ts +++ b/src/ucode/agents/claude_mods/register.ts @@ -117,7 +117,7 @@ export function register(on: On, options: PluginOptions): void { } return next(e); }); - registerIsaacFeedback(on, feedback, options.isaacExecutable as string); + registerIsaacFeedback(on, feedback, options.isaacExecutable as string, options.isaacFeedbackDirectory as string); on('command.run', {command: 'smart-router'}, async ($, e) => { const action = e.args.trim(); try { diff --git a/src/ucode/smart_routing/v2.py b/src/ucode/smart_routing/v2.py index aba9a9b48..69149d210 100644 --- a/src/ucode/smart_routing/v2.py +++ b/src/ucode/smart_routing/v2.py @@ -349,6 +349,7 @@ def _claude_mod_launch_options(env: Mapping[str, str] | None = None) -> dict: "recipe": source.get(routing.ROUTER_NAME_ENV_VAR, "").strip() or routing.ROUTER_NAME, "routeCommand": [], "isaacExecutable": str(Path(isaac).absolute()) if isaac else "", + "isaacFeedbackDirectory": str(Path.home() / ".isaac" / "feedback"), } @@ -368,6 +369,12 @@ def _write_routed_claude_plugin(plugin_dir: Path, model_ids: list[str]) -> None: "title": "Isaac executable", "description": "Isaac CLI resolved at launch for feedback submission", }, + "isaacFeedbackDirectory": { + "type": "string", + "default": "", + "title": "Isaac feedback directory", + "description": "Absolute directory for locally captured feedback", + }, "routeCommand": { "type": "string", "multiple": True, diff --git a/tests/test_claude_mod.py b/tests/test_claude_mod.py index 28aee7038..d8e236d2a 100644 --- a/tests/test_claude_mod.py +++ b/tests/test_claude_mod.py @@ -58,6 +58,7 @@ def test_launch_options_resolve_versions_without_mutating_input(version): "recipe": "custom_recipe", "routeCommand": [], "isaacExecutable": str(Path(isaac).absolute()) if isaac else "", + "isaacFeedbackDirectory": str(Path.home() / ".isaac" / "feedback"), } # Taking the baseline must not alter the caller's launch environment. assert env == before @@ -73,4 +74,5 @@ def test_launch_options_preserve_empty_and_absent_values(): "recipe": "task_v3", "routeCommand": [], "isaacExecutable": str(Path(isaac).absolute()) if isaac else "", + "isaacFeedbackDirectory": str(Path.home() / ".isaac" / "feedback"), } From 29839534771d10c8d1b346d8cd06740774cc9c19 Mon Sep 17 00:00:00 2001 From: Josh Joseph Date: Wed, 21 Oct 2026 18:25:11 +0000 Subject: [PATCH 10/14] Keep Claude launch working when feedback setup fails --- docs/claude-mod.md | 3 ++- src/ucode/agents/claude.py | 35 +++++++++++++++++++++-------------- 2 files changed, 23 insertions(+), 15 deletions(-) diff --git a/docs/claude-mod.md b/docs/claude-mod.md index 63c827827..b6524f74d 100644 --- a/docs/claude-mod.md +++ b/docs/claude-mod.md @@ -90,7 +90,8 @@ text-only transport and do not receive an evidence feedback ID. Claude launches without smart routing also load the feedback hooks when this flag is enabled. Routing stays disabled and the status band stays hidden. UG keeps the -temporary plugin alive until Claude exits, then removes it. +temporary plugin alive until Claude exits, then removes it. If setup fails, UG warns +and launches Claude without the feedback hooks. These uploads belong to the running Claude session. Keep it open until completion; if interrupted, retry the saved evidence ID on the same machine. No Isaac background diff --git a/src/ucode/agents/claude.py b/src/ucode/agents/claude.py index 2d5f314f8..d96808c2f 100644 --- a/src/ucode/agents/claude.py +++ b/src/ucode/agents/claude.py @@ -14,7 +14,7 @@ import threading import traceback from collections.abc import Callable, Iterable, Iterator, Sequence -from contextlib import contextmanager +from contextlib import ExitStack, contextmanager from pathlib import Path from tempfile import TemporaryDirectory @@ -2270,21 +2270,28 @@ def _feedback_launch_args( if not env_bool(ISAAC_SMART_ROUTER_FEEDBACK_ENV_VAR): yield tool_args, settings_override return - APP_DIR.mkdir(parents=True, exist_ok=True) - with TemporaryDirectory(prefix="claude-feedback-", dir=APP_DIR) as directory: - plugin_dir = Path(directory) / "plugin" - smart_routing_v2._write_routed_claude_plugin(plugin_dir, []) - feedback_settings = { - "pluginConfigs": { - smart_routing_v2.CLAUDE_ROUTING_PLUGIN_NAME: { - "options": smart_routing_v2._claude_mod_launch_options({}) + with ExitStack() as stack: + try: + APP_DIR.mkdir(parents=True, exist_ok=True) + directory = stack.enter_context( + TemporaryDirectory(prefix="claude-feedback-", dir=APP_DIR) + ) + plugin_dir = Path(directory) / "plugin" + smart_routing_v2._write_routed_claude_plugin(plugin_dir, []) + feedback_settings = { + "pluginConfigs": { + smart_routing_v2.CLAUDE_ROUTING_PLUGIN_NAME: { + "options": smart_routing_v2._claude_mod_launch_options({}) + } } } - } - yield ( - ["--plugin-dir", str(plugin_dir), *tool_args], - _merge_claude_settings(settings_override or {}, feedback_settings), - ) + tool_args, settings_override = ( + ["--plugin-dir", str(plugin_dir), *tool_args], + _merge_claude_settings(settings_override or {}, feedback_settings), + ) + except Exception as exc: # noqa: BLE001 - optional setup must not block normal launch + print_warning(f"Could not load Isaac feedback notifications: {exc}") + yield tool_args, settings_override def _launch_relayed(state: dict, binary: str, tool_args: list[str]) -> None: From e1d7bb663d9105a21f5ede2a999fc0b014e9f568 Mon Sep 17 00:00:00 2001 From: Josh Joseph Date: Wed, 21 Oct 2026 18:41:56 +0000 Subject: [PATCH 11/14] Submit Isaac feedback through the native Claude flow --- docs/claude-mod.md | 25 +- .../agents/claude_mods/isaac-feedback.ts | 227 ++++++++++++------ tests/README.md | 7 +- tests/integration/README.md | 9 +- 4 files changed, 170 insertions(+), 98 deletions(-) diff --git a/docs/claude-mod.md b/docs/claude-mod.md index b6524f74d..628b5fd40 100644 --- a/docs/claude-mod.md +++ b/docs/claude-mod.md @@ -75,18 +75,19 @@ Before submitting, Cancel or Escape leaves routing on and sends nothing. A missi `isaac` CLI or an unavailable form leaves routing on. A feedback submission failure after a valid reason leaves routing off and reports the failure in the transcript. The form makes no model requests. -With the same flag enabled, `/isaac-feedback` keeps the skill's category, target, -exact text, and agent note. The skill saves evidence locally in the foreground, -then calls `isaac feedback --retry ` using Claude's native background Bash -task. Retries and text-only Other reports also use background Bash. This requires -Isaac's background-submission skill instructions; the CLI commands already exist. - -The hook observes background tasks with the description `Isaac feedback upload ` -(or `Isaac feedback upload` for Other). It leaves commands and Bash permission -checks unchanged. Native task completion reports success or failure without a -model turn or status polling. `delivery_notifications: "native"` tells the skill -to end without duplicating the sending message. Other reports retain their -text-only transport and do not receive an evidence feedback ID. +With the same flag enabled, `/isaac-feedback` uses the same native form, +capture, and background uploader as `/smart-router off`. It asks for a category +when none was supplied, and for feedback text when the invocation is empty. +`/isaac-feedback --category smart-routing ` submits directly. Smart-routing +reports capture the current session's most recent task; the native flow does not +generate an agent note. Other reports retain their text-only transport and do not +receive an evidence feedback ID. `/isaac-feedback retry ` uploads the saved +snapshot in the background without capturing it again. + +The submission and its sending/completion messages make no model requests and +need no updated Isaac skill instructions. Explicit inspection and local-preview +requests still use the existing skill. Earlier targets, agent notes, and additional +files remain available through the existing Isaac CLI options. Claude launches without smart routing also load the feedback hooks when this flag is enabled. Routing stays disabled and the status band stays hidden. UG keeps the diff --git a/src/ucode/agents/claude_mods/isaac-feedback.ts b/src/ucode/agents/claude_mods/isaac-feedback.ts index 2bb772d81..122c8fc59 100644 --- a/src/ucode/agents/claude_mods/isaac-feedback.ts +++ b/src/ucode/agents/claude_mods/isaac-feedback.ts @@ -3,7 +3,7 @@ import { envBool } from './constants.ts'; const FEEDBACK_PANE = 'smart-router-feedback'; const FEEDBACK_ID = /^[0-9a-f]{8}(?:-[0-9a-f]{4}){3}-[0-9a-f]{12}$/i; -const FEEDBACK_UPLOAD_DESCRIPTION = 'Isaac feedback upload'; +type FeedbackCategory = 'smart-routing' | 'other'; function feedbackSuffix(feedbackId: string): string { return feedbackId ? ` Feedback ID: ${feedbackId}` : ''; @@ -26,6 +26,8 @@ function showResult($: EngineInterface, feedbackId: string, sent: boolean): void type FeedbackState = { submitted: boolean; reason: string; + category?: FeedbackCategory; + turnOff?: boolean; readRoutingEnabled?: () => Promise; disableRouting?: (sessionControlArgs: readonly string[]) => Promise; }; @@ -34,64 +36,140 @@ export function createIsaacFeedbackState(): FeedbackState { return { submitted: false, reason: '' }; } -export function registerIsaacFeedback( - on: On, state: FeedbackState, isaacExecutable: string, isaacFeedbackDirectory: string, +function uploadFeedback( + $: EngineInterface, isaacExecutable: string, feedbackId: string, message?: string, ): void { - const uploads = new Map(); - on('tool.call', { tool: 'Bash' }, async ($, e, next) => { - if (e.run_in_background !== true || typeof e.description !== 'string' - || !e.description.startsWith(FEEDBACK_UPLOAD_DESCRIPTION)) return next(e); - const feedbackId = e.description.slice(FEEDBACK_UPLOAD_DESCRIPTION.length).trim(); - if ((feedbackId && !FEEDBACK_ID.test(feedbackId)) - || !envBool(await $.env.get('ISAAC_SMART_ROUTER_FEEDBACK'))) return next(e); - const answer = await next(e); - // Native Bash owns permission checks, process lifetime, and the completion event. - const payload = answer.result; - if (answer.isError || answer.deny || !payload || typeof payload !== 'object' - || !('backgroundTaskId' in payload) || typeof payload.backgroundTaskId !== 'string') return answer; - uploads.set(payload.backgroundTaskId, feedbackId); - showSending($, feedbackId); - const text = JSON.stringify({ - feedback_id: feedbackId || null, - delivery_notifications: 'native', - instruction: 'The native UI reports sending and completion. End without repeating these messages, polling, or calling TaskOutput.', - }); - return { ...answer, text, result: { ...payload, stdout: text } }; + showSending($, feedbackId); + const args = feedbackId ? ['--retry', feedbackId] : ['--category', 'other']; + void $.process.run([isaacExecutable, 'feedback', ...args], { stdin: message, timeoutMs: 120000 }) + .then(result => showResult($, feedbackId, result.exitCode === 0), + () => showResult($, feedbackId, false)); +} + +async function submitFeedback( + $: EngineInterface, state: FeedbackState, isaacExecutable: string, + isaacFeedbackDirectory: string, message: string, +): Promise { + if (state.submitted) return; + const minimum = state.turnOff ? 16 : 1; + if (Array.from(message.trim()).length < minimum) { + return state.turnOff + ? 'Enter at least 16 characters, excluding surrounding whitespace.' + : 'Enter feedback before sending.'; + } + if (!state.category || !isaacExecutable || !isaacFeedbackDirectory) return 'Isaac feedback is unavailable.'; + state.submitted = true; + $.ui.invalidate('ui.render'); + const feedbackId = state.category === 'smart-routing' ? crypto.randomUUID() : ''; + try { + if (feedbackId) { + const capture = await $.process.run( + [isaacExecutable, 'feedback', '--category', 'smart-routing', '--harness', 'claude', + '--session-id', await $.session.id(), '--feedback-id', feedbackId, '--with-evidence', + '--capture-local', isaacFeedbackDirectory], + { stdin: message, timeoutMs: 120000 }, + ); + if (capture.exitCode !== 0) throw new Error('Could not save feedback.'); + if (capture.stderr.trim()) $.ui.log(capture.stderr.trim()); + } + if (state.turnOff) { + if (!state.disableRouting) throw new Error('Smart Router controls are unavailable.'); + await state.disableRouting(['--smart-router-feedback', message]); + } + } catch { + state.submitted = false; + $.ui.invalidate('ui.render'); + return state.turnOff + ? 'Could not save feedback or update Smart Router. Smart Router is still on; retry or restart through UG.' + : 'Could not save feedback. Nothing was uploaded; retry /isaac-feedback.'; + } + await $.ui.close({ id: FEEDBACK_PANE }).catch(() => {}); + state.submitted = false; + uploadFeedback($, isaacExecutable, feedbackId, feedbackId ? undefined : message); +} + +async function openFeedbackForm( + $: EngineInterface, state: FeedbackState, isaacExecutable: string, isaacFeedbackDirectory: string, +): Promise { + if (!isaacExecutable || !isaacFeedbackDirectory) return false; + // `isaac --version` launches a harness; the help command checks the installed CLI. + const help = await $.process.run([isaacExecutable, 'feedback', '--help'], { timeoutMs: 10000 }); + if (help.exitCode !== 0) return false; + $.ui.invalidate('ui.render'); + const pane = await $.ui.open({ + id: FEEDBACK_PANE, title: state.turnOff ? 'Turn Smart Router off' : 'Isaac feedback', + focus: true, closeOnEscape: true, rows: 6, columns: 56, }); + if (pane.isPlaced) return true; + await $.ui.close({ id: FEEDBACK_PANE }); + return false; +} - on('prompt.submit', { origin: { kind: 'task-notification' } }, ($, e, next) => { - const taskId = /([^<]+)<\/task-id>/.exec(e.text)?.[1]; - if (!taskId || !uploads.has(taskId)) return next(e); - const feedbackId = uploads.get(taskId)!; - uploads.delete(taskId); - showResult($, feedbackId, e.text.includes('completed')); - return { drop: 'Feedback completion reported by the native UI.' }; +export function registerIsaacFeedback( + on: On, state: FeedbackState, isaacExecutable: string, isaacFeedbackDirectory: string, +): void { + on('command.run', { command: 'isaac-feedback' }, async ($, e, next) => { + if (!envBool(await $.env.get('ISAAC_SMART_ROUTER_FEEDBACK'))) return next(e); + if (state.submitted) return { text: 'Feedback is still being saved.' }; + let message = e.args; + let category: FeedbackCategory | undefined; + if (message.startsWith('-- ')) { + message = message.slice(3); + } else { + for (const value of ['smart-routing', 'other'] as const) { + const prefix = `--category ${value}`; + if (message === prefix || message.startsWith(prefix + ' ')) { + category = value; + message = message.slice(prefix.length + 1); + break; + } + } + if (!category) { + const [action, feedbackId, ...rest] = message.trim().split(/\s+/); + if (action === 'retry' || action === '--retry') { + if (!isaacExecutable) return { text: 'Isaac feedback is unavailable.' }; + if (!feedbackId || rest.length || !FEEDBACK_ID.test(feedbackId)) { + return { text: 'Usage: /isaac-feedback retry ' }; + } + uploadFeedback($, isaacExecutable, feedbackId); + return {}; + } + // Keep explicit inspection and local-preview requests with the existing skill. + if (message.startsWith('--') + || (action === 'status' && feedbackId && !rest.length && FEEDBACK_ID.test(feedbackId))) return next(e); + } + } + state.turnOff = false; + state.category = category; + state.reason = message; + state.submitted = false; + if (category && message.trim()) { + const error = await submitFeedback($, state, isaacExecutable, isaacFeedbackDirectory, message); + return error ? { text: error } : {}; + } + try { + if (await openFeedbackForm($, state, isaacExecutable, isaacFeedbackDirectory)) return {}; + } catch { + // An unavailable form must not submit feedback or leave a delayed question. + } + return { text: 'Could not open the feedback form. Retry /isaac-feedback in the Claude terminal.' }; }); on('command.run', {command: 'smart-router'}, async ($, e, next) => { if (e.args.trim() !== 'off') return next(e); const feedbackRequired = envBool(await $.env.get('ISAAC_SMART_ROUTER_FEEDBACK')); if (!feedbackRequired) return next(e); + if (state.submitted) return { text: 'Feedback is still being saved.' }; try { if (!state.readRoutingEnabled || !state.disableRouting) { throw new Error('Smart Router controls are unavailable.'); } if (!await state.readRoutingEnabled()) return {text: 'Smart Router is off'}; - if (!isaacExecutable || !isaacFeedbackDirectory) throw new Error('Isaac feedback is unavailable.'); - // The form uploads through Isaac's CLI; without it, keep routing on. `isaac --version` launches a harness. - const help = await $.process.run([isaacExecutable, 'feedback', '--help'], { timeoutMs: 10000 }); - if (help.exitCode !== 0) { - throw new Error('Isaac CLI is unavailable.'); - } state.submitted = false; state.reason = ''; - const pane = await $.ui.open({ - id: FEEDBACK_PANE, title: 'Turn Smart Router off', focus: true, - closeOnEscape: true, rows: 6, columns: 56, - }); - if (pane.isPlaced) return {text: 'Smart Router is off'}; - // Do not leave a delayed question waiting for an unrelated task. - await $.ui.close({ id: FEEDBACK_PANE }); + state.category = 'smart-routing'; + state.turnOff = true; + if (await openFeedbackForm($, state, isaacExecutable, isaacFeedbackDirectory)) return {text: 'Smart Router is off'}; } catch { // No reason was collected, so leave routing unchanged. } @@ -102,53 +180,44 @@ export function registerIsaacFeedback( if (e.requestId !== FEEDBACK_PANE || e.surface === 'mobile') return next(e); const { Box, Text, Input, Button } = $.ui.resolve(e); if (state.submitted) return Text({ children: ['Saving feedback...'] }); + if (!state.category) { + const choose = async (category: FeedbackCategory) => { + state.category = category; + if (state.reason.trim()) { + const error = await submitFeedback($, state, isaacExecutable, isaacFeedbackDirectory, state.reason); + if (error) $.ui.toast(error); + } + $.ui.invalidate('ui.render'); + }; + return Box({ flexDirection: 'column', children: [ + Text({ children: ['What is your feedback about?'] }), + Button({ key: 'routing', label: 'Smart routing (with session details)', autoFocus: true, onPress: () => choose('smart-routing') }), + Button({ key: 'other', label: 'Other (text only)', onPress: () => choose('other') }), + Button({ key: 'cancel', label: 'Cancel', onPress: () => $.ui.close({ id: FEEDBACK_PANE }) }), + ] }); + } return Box({ flexDirection: 'column', children: [ - Text({ children: ['Why are you turning Smart Router off?'] }), - Text({ children: ['Enter at least 16 characters. Your reason is sent to the Isaac team with this session\'s details.'] }), + Text({ children: [state.turnOff ? 'Why are you turning Smart Router off?' : 'What would you like to share?'] }), + Text({ children: [state.turnOff + ? 'Enter at least 16 characters. Your reason is sent to the Isaac team with this session\'s details.' + : state.category === 'smart-routing' + ? 'Your feedback is sent to the Isaac team with this session\'s details.' + : 'Your feedback is sent to the Isaac team without session evidence.'] }), Input({ key: 'reason', - label: 'Reason', + label: state.turnOff ? 'Reason' : 'Feedback', value: state.reason, autoFocus: true, - submitLabel: 'Send feedback and turn off', + submitLabel: state.turnOff ? 'Send feedback and turn off' : 'Send feedback', onInput: (value) => { state.reason = value; }, onSubmit: async (value) => { - if (state.submitted || !state.disableRouting) return; - if (Array.from(value.trim()).length < 16) { - $.ui.toast('Enter at least 16 characters, excluding surrounding whitespace.'); - return; - } - state.submitted = true; - $.ui.invalidate('ui.render'); - const feedbackId = crypto.randomUUID(); - try { - const sessionId = await $.session.id(); - const capture = await $.process.run( - [isaacExecutable, 'feedback', '--category', 'smart-routing', '--harness', 'claude', - '--session-id', sessionId, '--feedback-id', feedbackId, '--with-evidence', - '--capture-local', isaacFeedbackDirectory], - { stdin: value, timeoutMs: 120000 }, - ); - if (capture.exitCode !== 0) throw new Error('Could not save feedback.'); - if (capture.stderr.trim()) $.ui.log(capture.stderr.trim()); - await state.disableRouting(['--smart-router-feedback', value]); - } catch { - state.submitted = false; - $.ui.invalidate('ui.render'); - $.ui.toast('Could not save feedback or update Smart Router. Smart Router is still on; retry or restart through UG.'); - return; - } - await $.ui.close({ id: FEEDBACK_PANE }).catch(() => {}); - // Upload in the background through Isaac's CLI, without a model turn. - showSending($, feedbackId); - void $.process.run([isaacExecutable, 'feedback', '--retry', feedbackId], { timeoutMs: 120000 }) - .then(result => showResult($, feedbackId, result.exitCode === 0), - () => showResult($, feedbackId, false)); + const error = await submitFeedback($, state, isaacExecutable, isaacFeedbackDirectory, value); + if (error) $.ui.toast(error); }, }), - Button({ key: 'cancel', label: 'Cancel (keep routing on)', onPress: () => $.ui.close({ id: FEEDBACK_PANE }) }), + Button({ key: 'cancel', label: state.turnOff ? 'Cancel (keep routing on)' : 'Cancel', onPress: () => $.ui.close({ id: FEEDBACK_PANE }) }), ], }); }); diff --git a/tests/README.md b/tests/README.md index b0ec760c4..05bb10729 100644 --- a/tests/README.md +++ b/tests/README.md @@ -35,9 +35,10 @@ skill-toggle integration journey checks the live recipe payload field across on/ See [the mod guide](../docs/claude-mod.md). The required Isaac feedback form and background delivery notifications need manual -validation using Isaac's background-submission skill instructions and existing -capture/retry CLI. Check the 16-character minimum, cancellation, repeated toggles, -and submission success/failure. Also check that a new task can run during upload, +validation using the released Isaac skill and existing capture/retry CLI. Check +category selection, empty and inline feedback, native retries, the off form's +16-character minimum, cancellation, repeated toggles, and submission success/failure. +Also check that submission makes no model request and a new task can run during upload, saved evidence stays unchanged, upload permission denials are honored, local previews stay local, and Other stays text-only. Run `/isaac-feedback` in sessions launched with routing both on and off; check that a routing-disabled launch keeps diff --git a/tests/integration/README.md b/tests/integration/README.md index ef98ae0aa..b6e81ebd3 100644 --- a/tests/integration/README.md +++ b/tests/integration/README.md @@ -95,10 +95,11 @@ TypeScript event-forwarding and launch-option tests in a dedicated CI job. The existing Claude skill-toggle journey checks the live recipe payload field across on/off/on. The required Isaac feedback form, evidence submission, and background delivery notifications need a separate manual Isaac check. The gateway toggle journey does -not cover the form or notification timing. Use Isaac's background-submission skill -instructions with the existing capture/retry CLI to check completion during another -task, immutable evidence, upload permission denials, local previews, and text-only -Other reports. Also launch with routing disabled: +not cover the form or notification timing. Use the released Isaac skill with the +existing capture/retry CLI to check native category selection and retries, +submission without model requests, completion during another task, immutable +evidence, upload permission denials, local previews, and text-only Other reports. +Also launch with routing disabled: `/isaac-feedback` should still submit in the background, the status band should stay hidden, and the temporary feedback plugin should be removed after exit. From b7f292df9fa706551d806a568777e2d3cd3c992d Mon Sep 17 00:00:00 2001 From: Josh Joseph Date: Wed, 21 Oct 2026 20:52:33 +0000 Subject: [PATCH 12/14] Return feedback forms immediately and preserve routing context --- docs/claude-mod.md | 37 +++-- .../agents/claude_mods/isaac-feedback.ts | 134 +++++++++++++----- tests/README.md | 4 +- tests/integration/README.md | 4 +- 4 files changed, 129 insertions(+), 50 deletions(-) diff --git a/docs/claude-mod.md b/docs/claude-mod.md index 628b5fd40..8485af3eb 100644 --- a/docs/claude-mod.md +++ b/docs/claude-mod.md @@ -64,24 +64,26 @@ rollout setting or the user's explicit `1`/`0` or `true`/`false` override. As wi boolean environment flags, `1`, `true`, `yes`, and `on` enable it (case-insensitive, with surrounding whitespace ignored); other values disable it. It controls Isaac feedback availability, so UG's routing presets and on/off toggles leave it unchanged. -The native form keeps routing on until Enter accepts a valid reason and Isaac saves -the bounded local snapshot. It uses the existing `--with-evidence --capture-local` -options, with the Isaac executable and absolute capture directory resolved at launch. -Then it disables routing, closes the form, and uploads the saved report with `isaac feedback --retry ` in -the existing native background callback. The transcript shows the sending message -and feedback ID. Delivery produces both a persistent message and a success toast; -a failed upload offers `/isaac-feedback retry ` for the original snapshot. +The native form keeps routing on until Enter accepts a valid reason. It closes the +form and shows the sending message and feedback ID before starting Isaac. The existing +native background callback runs `isaac feedback --with-evidence` to capture and upload +the report while the user continues working. Routing is turned off independently of +that process. Delivery produces both a persistent message and a success toast; +a failed upload offers `/isaac-feedback retry ` when a local snapshot was saved. Before submitting, Cancel or Escape leaves routing on and sends nothing. A missing -`isaac` CLI or an unavailable form leaves routing on. A feedback submission failure after a valid +`isaac` executable at launch or an unavailable form leaves routing on. A feedback submission failure after a valid reason leaves routing off and reports the failure in the transcript. The form makes no model requests. With the same flag enabled, `/isaac-feedback` uses the same native form, capture, and background uploader as `/smart-router off`. It asks for a category when none was supplied, and for feedback text when the invocation is empty. `/isaac-feedback --category smart-routing ` submits directly. Smart-routing -reports capture the current session's most recent task; the native flow does not -generate an agent note. Other reports retain their text-only transport and do not -receive an evidence feedback ID. `/isaac-feedback retry ` uploads the saved +reports pin the current session's most recent task by its native user-message ID. +The `session.append` hook remembers that ID; a session with no observed prompt uses +Isaac's existing target inspection before opening the form. Empty sessions use an +unmatched `no-prior-task` selector so subsequent work cannot become their target. +The native flow does not generate an agent note. Other reports retain their text-only +transport and do not receive an evidence feedback ID. `/isaac-feedback retry ` uploads the saved snapshot in the background without capturing it again. The submission and its sending/completion messages make no model requests and @@ -94,10 +96,17 @@ is enabled. Routing stays disabled and the status band stays hidden. UG keeps th temporary plugin alive until Claude exits, then removes it. If setup fails, UG warns and launches Claude without the feedback hooks. +Each submission passes Claude's native session ID and the current routing controls +to Isaac. Valid session-file overrides are copied into that child process's environment, +so turning routing off or back on cannot change the report's recorded settings. +These settings describe submission time; subagent execution still requires routing-log +evidence. An invalid controls file remains unverified. + These uploads belong to the running Claude session. Keep it open until completion; -if interrupted, retry the saved evidence ID on the same machine. No Isaac background -worker or new CLI options are required. Saving evidence before returning control -keeps a later task or file edit out of that snapshot. +if interrupted, retry the saved evidence ID on the same machine. If capture did not +finish, submit the feedback again. No Isaac background worker or new CLI options are +required. Evidence is collected in the background: the selected task stays fixed, +while transcript and file snapshots reflect capture time and available file history. `isaac-feedback.ts` owns this Databricks-specific policy, form, and submission. `register.ts` binds the routing callbacks before Isaac's command guard, then diff --git a/src/ucode/agents/claude_mods/isaac-feedback.ts b/src/ucode/agents/claude_mods/isaac-feedback.ts index 122c8fc59..ba13e9db1 100644 --- a/src/ucode/agents/claude_mods/isaac-feedback.ts +++ b/src/ucode/agents/claude_mods/isaac-feedback.ts @@ -26,6 +26,7 @@ function showResult($: EngineInterface, feedbackId: string, sent: boolean): void type FeedbackState = { submitted: boolean; reason: string; + lastPromptId?: string | null; category?: FeedbackCategory; turnOff?: boolean; readRoutingEnabled?: () => Promise; @@ -36,14 +37,66 @@ export function createIsaacFeedbackState(): FeedbackState { return { submitted: false, reason: '' }; } -function uploadFeedback( - $: EngineInterface, isaacExecutable: string, feedbackId: string, message?: string, -): void { - showSending($, feedbackId); - const args = feedbackId ? ['--retry', feedbackId] : ['--category', 'other']; - void $.process.run([isaacExecutable, 'feedback', ...args], { stdin: message, timeoutMs: 120000 }) - .then(result => showResult($, feedbackId, result.exitCode === 0), - () => showResult($, feedbackId, false)); +async function feedbackEnvironment($: EngineInterface): Promise> { + const env: Record = { + CLAUDE_CODE_SESSION_ID: await $.session.id(), + // A nested Claude launch can inherit another harness's identity markers. + CODEX_THREAD_ID: '', CODEX_CI: '', CODEX_HOME: '', CODEX_SANDBOX: '', OPENCODE: '', CURSOR_AGENT: '', + ENABLE_SMART_ROUTING_V2: (await $.env.get('ENABLE_SMART_ROUTING_V2')) === '1' ? '1' : '0', + ENABLE_SMART_ROUTING_SUBAGENT_ONLY: (await $.env.get('ENABLE_SMART_ROUTING_SUBAGENT_ONLY')) === '1' ? '1' : '0', + ENABLE_SMART_ROUTER_ORCHESTRATOR: (await $.env.get('ENABLE_SMART_ROUTER_ORCHESTRATOR')) ?? '', + SMART_ROUTER_NAME: (await $.env.get('SMART_ROUTER_NAME')) ?? '', + SMART_ROUTER_CONFIG_VERSION: (await $.env.get('SMART_ROUTER_CONFIG_VERSION')) ?? '', + UCODE_SESSION_ENV_FILE: '', + }; + const controls = await $.env.get('UCODE_SESSION_ENV_FILE'); + if (controls) { + try { + const overrides = JSON.parse(await $.fs.read(controls)); + const keys = ['ENABLE_SMART_ROUTING_V2', 'ENABLE_SMART_ROUTING_SUBAGENT_ONLY', 'ENABLE_SMART_ROUTER_ORCHESTRATOR']; + if (!overrides || typeof overrides !== 'object' || Array.isArray(overrides) + || Object.entries(overrides).some(([key, value]) => !keys.includes(key) || (value !== '0' && value !== '1'))) { + throw new Error('Invalid routing controls.'); + } + Object.assign(env, overrides); + } catch { + // Let Isaac report unreadable controls instead of claiming a verified state. + env.UCODE_SESSION_ENV_FILE = controls; + } + } + // Valid controls are frozen here; a later toggle must not change this report. + return env; +} + +async function prepareFeedbackTarget( + $: EngineInterface, state: FeedbackState, isaacExecutable: string, +): Promise { + if (state.lastPromptId !== undefined) return; + // Resumed sessions may not have emitted a prompt during this plugin's lifetime. + const result = await $.process.run( + [isaacExecutable, 'feedback', '--inspect-target', '--harness', 'claude', '--session-id', await $.session.id()], + { timeoutMs: 10000 }, + ); + if (result.exitCode !== 0) throw new Error('Could not identify the feedback target.'); + if (state.lastPromptId === undefined) state.lastPromptId = JSON.parse(result.stdout).message_id ?? null; +} + +async function uploadFeedback( + $: EngineInterface, isaacExecutable: string, isaacFeedbackDirectory: string, feedbackId: string, + args: string[], message?: string, env?: Record, +): Promise { + try { + const result = await $.process.run([isaacExecutable, 'feedback', ...args], { stdin: message, env, timeoutMs: 120000 }); + if (result.exitCode === 0) { + if (result.stderr.trim()) $.ui.log(result.stderr.trim()); + showResult($, feedbackId, true); + return; + } + } catch { + // A timeout can leave a saved report, which should be retried with its original ID. + } + const saved = feedbackId && await $.fs.exists(`${isaacFeedbackDirectory}/${feedbackId}/receipt.json`).catch(() => false); + showResult($, saved ? feedbackId : '', false); } async function submitFeedback( @@ -59,42 +112,47 @@ async function submitFeedback( } if (!state.category || !isaacExecutable || !isaacFeedbackDirectory) return 'Isaac feedback is unavailable.'; state.submitted = true; - $.ui.invalidate('ui.render'); const feedbackId = state.category === 'smart-routing' ? crypto.randomUUID() : ''; + const { turnOff, disableRouting } = state; + let env: Record | undefined; + let promptId: string | null | undefined; + let args = ['--category', 'other']; try { if (feedbackId) { - const capture = await $.process.run( - [isaacExecutable, 'feedback', '--category', 'smart-routing', '--harness', 'claude', - '--session-id', await $.session.id(), '--feedback-id', feedbackId, '--with-evidence', - '--capture-local', isaacFeedbackDirectory], - { stdin: message, timeoutMs: 120000 }, - ); - if (capture.exitCode !== 0) throw new Error('Could not save feedback.'); - if (capture.stderr.trim()) $.ui.log(capture.stderr.trim()); - } - if (state.turnOff) { - if (!state.disableRouting) throw new Error('Smart Router controls are unavailable.'); - await state.disableRouting(['--smart-router-feedback', message]); + await prepareFeedbackTarget($, state, isaacExecutable); + promptId = state.lastPromptId; + env = await feedbackEnvironment($); + args = ['--category', 'smart-routing', '--harness', 'claude', + '--session-id', env.CLAUDE_CODE_SESSION_ID, '--feedback-id', feedbackId, '--with-evidence', + // An unmatched selector keeps an empty session from picking up the user's next task. + '--turn-id', promptId ?? 'no-prior-task']; } } catch { state.submitted = false; - $.ui.invalidate('ui.render'); - return state.turnOff - ? 'Could not save feedback or update Smart Router. Smart Router is still on; retry or restart through UG.' - : 'Could not save feedback. Nothing was uploaded; retry /isaac-feedback.'; + return 'Could not prepare feedback. Nothing was uploaded; retry /isaac-feedback.'; } await $.ui.close({ id: FEEDBACK_PANE }).catch(() => {}); + showSending($, feedbackId); + if (feedbackId && promptId === null) { + $.ui.log('No earlier task was found. This feedback can help diagnose setup or controls, but has no task or model request to analyze.'); + } + void uploadFeedback($, isaacExecutable, isaacFeedbackDirectory, feedbackId, args, message, env); + if (turnOff) { + try { + if (!disableRouting) throw new Error('Smart Router controls are unavailable.'); + await disableRouting(['--smart-router-feedback', message]); + } catch { + $.ui.log('Could not turn Smart Router off. Feedback is still being sent; retry /smart-router off or restart through UG.'); + } + } state.submitted = false; - uploadFeedback($, isaacExecutable, feedbackId, feedbackId ? undefined : message); } async function openFeedbackForm( $: EngineInterface, state: FeedbackState, isaacExecutable: string, isaacFeedbackDirectory: string, ): Promise { if (!isaacExecutable || !isaacFeedbackDirectory) return false; - // `isaac --version` launches a harness; the help command checks the installed CLI. - const help = await $.process.run([isaacExecutable, 'feedback', '--help'], { timeoutMs: 10000 }); - if (help.exitCode !== 0) return false; + if (state.category !== 'other') await prepareFeedbackTarget($, state, isaacExecutable); $.ui.invalidate('ui.render'); const pane = await $.ui.open({ id: FEEDBACK_PANE, title: state.turnOff ? 'Turn Smart Router off' : 'Isaac feedback', @@ -108,9 +166,17 @@ async function openFeedbackForm( export function registerIsaacFeedback( on: On, state: FeedbackState, isaacExecutable: string, isaacFeedbackDirectory: string, ): void { + on('session.append', { door: 'prompt' }, ($, e, next) => { + if (!e.agentId && !e.message.isMeta) state.lastPromptId = e.uuid; + return next(e); + }); + on('session.end', ($, e, next) => { + state.lastPromptId = undefined; + return next(e); + }); on('command.run', { command: 'isaac-feedback' }, async ($, e, next) => { if (!envBool(await $.env.get('ISAAC_SMART_ROUTER_FEEDBACK'))) return next(e); - if (state.submitted) return { text: 'Feedback is still being saved.' }; + if (state.submitted) return { text: 'Feedback is being prepared.' }; let message = e.args; let category: FeedbackCategory | undefined; if (message.startsWith('-- ')) { @@ -131,7 +197,8 @@ export function registerIsaacFeedback( if (!feedbackId || rest.length || !FEEDBACK_ID.test(feedbackId)) { return { text: 'Usage: /isaac-feedback retry ' }; } - uploadFeedback($, isaacExecutable, feedbackId); + showSending($, feedbackId); + void uploadFeedback($, isaacExecutable, isaacFeedbackDirectory, feedbackId, ['--retry', feedbackId]); return {}; } // Keep explicit inspection and local-preview requests with the existing skill. @@ -159,7 +226,7 @@ export function registerIsaacFeedback( if (e.args.trim() !== 'off') return next(e); const feedbackRequired = envBool(await $.env.get('ISAAC_SMART_ROUTER_FEEDBACK')); if (!feedbackRequired) return next(e); - if (state.submitted) return { text: 'Feedback is still being saved.' }; + if (state.submitted) return { text: 'Feedback is being prepared.' }; try { if (!state.readRoutingEnabled || !state.disableRouting) { throw new Error('Smart Router controls are unavailable.'); @@ -169,7 +236,7 @@ export function registerIsaacFeedback( state.reason = ''; state.category = 'smart-routing'; state.turnOff = true; - if (await openFeedbackForm($, state, isaacExecutable, isaacFeedbackDirectory)) return {text: 'Smart Router is off'}; + if (await openFeedbackForm($, state, isaacExecutable, isaacFeedbackDirectory)) return {}; } catch { // No reason was collected, so leave routing unchanged. } @@ -179,7 +246,6 @@ export function registerIsaacFeedback( on('ui.render', { component: 'Pane' }, ($, e, next) => { if (e.requestId !== FEEDBACK_PANE || e.surface === 'mobile') return next(e); const { Box, Text, Input, Button } = $.ui.resolve(e); - if (state.submitted) return Text({ children: ['Saving feedback...'] }); if (!state.category) { const choose = async (category: FeedbackCategory) => { state.category = category; diff --git a/tests/README.md b/tests/README.md index 05bb10729..3d9e0b414 100644 --- a/tests/README.md +++ b/tests/README.md @@ -38,7 +38,9 @@ The required Isaac feedback form and background delivery notifications need manu validation using the released Isaac skill and existing capture/retry CLI. Check category selection, empty and inline feedback, native retries, the off form's 16-character minimum, cancellation, repeated toggles, and submission success/failure. -Also check that submission makes no model request and a new task can run during upload, +Also check that the form closes immediately and submission makes no model request. +Start a new task during capture and confirm the report retains the original task; +toggle routing during upload and confirm its recorded settings stay fixed. Check that saved evidence stays unchanged, upload permission denials are honored, local previews stay local, and Other stays text-only. Run `/isaac-feedback` in sessions launched with routing both on and off; check that a routing-disabled launch keeps diff --git a/tests/integration/README.md b/tests/integration/README.md index b6e81ebd3..1d67d20cd 100644 --- a/tests/integration/README.md +++ b/tests/integration/README.md @@ -97,7 +97,9 @@ The required Isaac feedback form, evidence submission, and background delivery notifications need a separate manual Isaac check. The gateway toggle journey does not cover the form or notification timing. Use the released Isaac skill with the existing capture/retry CLI to check native category selection and retries, -submission without model requests, completion during another task, immutable +immediate form dismissal without model requests, and completion during another task. +Verify the original task stays selected when a new prompt precedes capture, and +routing settings stay fixed across subsequent toggles. Check immutable saved evidence, upload permission denials, local previews, and text-only Other reports. Also launch with routing disabled: `/isaac-feedback` should still submit in the background, the status band should From c0ceb13a13ed2f0ff09339d5f84def387e9c1e48 Mon Sep 17 00:00:00 2001 From: Josh Joseph Date: Wed, 21 Oct 2026 21:15:14 +0000 Subject: [PATCH 13/14] Explain the feedback-only status band guard --- src/ucode/agents/claude_mods/register.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/src/ucode/agents/claude_mods/register.ts b/src/ucode/agents/claude_mods/register.ts index f4e34ca1a..f0ecb00a4 100644 --- a/src/ucode/agents/claude_mods/register.ts +++ b/src/ucode/agents/claude_mods/register.ts @@ -129,6 +129,7 @@ export function register(on: On, options: PluginOptions): void { } return {text: `Smart Router is ${action}`}; }); + // Feedback-only sessions load this plugin but cannot enable Smart Router. if (launchedEnabled) registerStatusBand(on); registerSubagentRouting(on, options); } From 02b508e3da54163e2ea5f47c951fdbca7da163b5 Mon Sep 17 00:00:00 2001 From: Josh Joseph Date: Wed, 21 Oct 2026 21:21:51 +0000 Subject: [PATCH 14/14] Limit native async feedback to Smart Router sessions --- docs/claude-mod.md | 14 +++-- src/ucode/agents/claude.py | 69 ++++-------------------- src/ucode/agents/claude_mods/register.ts | 3 +- src/ucode/launcher.py | 6 +-- tests/README.md | 10 ++-- tests/integration/README.md | 6 +-- 6 files changed, 28 insertions(+), 80 deletions(-) diff --git a/docs/claude-mod.md b/docs/claude-mod.md index 8485af3eb..01147b7a8 100644 --- a/docs/claude-mod.md +++ b/docs/claude-mod.md @@ -1,7 +1,7 @@ # Claude mod entry point UG's Claude launcher loads `hooks/register.ts` through the existing launch-scoped -`ug-smart-router` plugin for smart routing or Isaac feedback. Claude Code 2.1.290 or +`ug-smart-router` plugin in sessions launched with smart routing. Claude Code 2.1.290 or newer is required. The existing `_write_routed_claude_plugin()` copies the packaged TypeScript entry point @@ -74,9 +74,10 @@ Before submitting, Cancel or Escape leaves routing on and sends nothing. A missi `isaac` executable at launch or an unavailable form leaves routing on. A feedback submission failure after a valid reason leaves routing off and reports the failure in the transcript. The form makes no model requests. -With the same flag enabled, `/isaac-feedback` uses the same native form, -capture, and background uploader as `/smart-router off`. It asks for a category -when none was supplied, and for feedback text when the invocation is empty. +In these sessions, with the same flag enabled, `/isaac-feedback` uses the same native +form, capture, and background uploader as `/smart-router off`. It remains available +after routing is turned off so users can report on an earlier routed task. It asks +for a category when none was supplied, and for feedback text when the invocation is empty. `/isaac-feedback --category smart-routing ` submits directly. Smart-routing reports pin the current session's most recent task by its native user-message ID. The `session.append` hook remembers that ID; a session with no observed prompt uses @@ -91,10 +92,7 @@ need no updated Isaac skill instructions. Explicit inspection and local-preview requests still use the existing skill. Earlier targets, agent notes, and additional files remain available through the existing Isaac CLI options. -Claude launches without smart routing also load the feedback hooks when this flag -is enabled. Routing stays disabled and the status band stays hidden. UG keeps the -temporary plugin alive until Claude exits, then removes it. If setup fails, UG warns -and launches Claude without the feedback hooks. +Claude sessions launched without smart routing use the existing feedback skill. Each submission passes Claude's native session ID and the current routing controls to Isaac. Valid session-file overrides are copied into that child process's environment, diff --git a/src/ucode/agents/claude.py b/src/ucode/agents/claude.py index d96808c2f..e642c5663 100644 --- a/src/ucode/agents/claude.py +++ b/src/ucode/agents/claude.py @@ -13,10 +13,8 @@ import sys import threading import traceback -from collections.abc import Callable, Iterable, Iterator, Sequence -from contextlib import ExitStack, contextmanager +from collections.abc import Callable, Iterable, Sequence from pathlib import Path -from tempfile import TemporaryDirectory from ucode.config_io import ( APP_DIR, @@ -29,7 +27,6 @@ ) from ucode.constants import ( AI_GATEWAY_REQUEST_TAGS_HEADER, - ISAAC_SMART_ROUTER_FEEDBACK_ENV_VAR, LOOPBACK_HOST, MCP_CLEANUP_SCOPES, MCP_USER_SCOPE, @@ -53,7 +50,6 @@ maybe_add_gpt_1m_suffix, ug_binary, ) -from ucode.env import env_bool from ucode.launcher import exec_or_spawn from ucode.managed_config import refresh_managed_config from ucode.managed_files import ( @@ -2263,37 +2259,6 @@ def _rewrite_relayed_port(state: dict, port: int) -> None: write_json_file(CLAUDE_SETTINGS_PATH, settings) -@contextmanager -def _feedback_launch_args( - tool_args: list[str], settings_override: dict | None = None -) -> Iterator[tuple[list[str], dict | None]]: - if not env_bool(ISAAC_SMART_ROUTER_FEEDBACK_ENV_VAR): - yield tool_args, settings_override - return - with ExitStack() as stack: - try: - APP_DIR.mkdir(parents=True, exist_ok=True) - directory = stack.enter_context( - TemporaryDirectory(prefix="claude-feedback-", dir=APP_DIR) - ) - plugin_dir = Path(directory) / "plugin" - smart_routing_v2._write_routed_claude_plugin(plugin_dir, []) - feedback_settings = { - "pluginConfigs": { - smart_routing_v2.CLAUDE_ROUTING_PLUGIN_NAME: { - "options": smart_routing_v2._claude_mod_launch_options({}) - } - } - } - tool_args, settings_override = ( - ["--plugin-dir", str(plugin_dir), *tool_args], - _merge_claude_settings(settings_override or {}, feedback_settings), - ) - except Exception as exc: # noqa: BLE001 - optional setup must not block normal launch - print_warning(f"Could not load Isaac feedback notifications: {exc}") - yield tool_args, settings_override - - def _launch_relayed(state: dict, binary: str, tool_args: list[str]) -> None: """Relayed launch: sign into the Claude subscription, start the loopback refresh proxy, then run Claude Code alongside it (the proxy must outlive the @@ -2323,21 +2288,16 @@ def token_provider(force_refresh: bool) -> str: server_thread = threading.Thread(target=server.serve_forever, daemon=True) server_thread.start() - with _feedback_launch_args(tool_args) as (launch_args, settings_override): - proc = subprocess_cross_os.popen( - _build_claude_argv( - binary, launch_args, relayed=True, settings_override=settings_override - ) - ) - try: - returncode = proc.wait() - except KeyboardInterrupt: - proc.send_signal(signal.SIGINT) - returncode = proc.wait() - finally: - cache.stop() - server.shutdown() - client.close() + proc = subprocess_cross_os.popen(_build_claude_argv(binary, tool_args, relayed=True)) + try: + returncode = proc.wait() + except KeyboardInterrupt: + proc.send_signal(signal.SIGINT) + returncode = proc.wait() + finally: + cache.stop() + server.shutdown() + client.close() raise SystemExit(returncode) @@ -2423,13 +2383,6 @@ def launch( } settings_override = _merge_claude_settings(settings_override or {}, {"env": fallback_env}) os.environ.update(fallback_env) - if env_bool(ISAAC_SMART_ROUTER_FEEDBACK_ENV_VAR): - with _feedback_launch_args(launch_args, settings_override) as (args, override): - # Keep the temporary feedback plugin available until this Claude session exits. - exec_or_spawn( - _build_claude_argv(binary, args, settings_override=override), wait_for_child=True - ) - return exec_or_spawn(_build_claude_argv(binary, launch_args, settings_override=settings_override)) diff --git a/src/ucode/agents/claude_mods/register.ts b/src/ucode/agents/claude_mods/register.ts index f0ecb00a4..8cf646767 100644 --- a/src/ucode/agents/claude_mods/register.ts +++ b/src/ucode/agents/claude_mods/register.ts @@ -129,7 +129,6 @@ export function register(on: On, options: PluginOptions): void { } return {text: `Smart Router is ${action}`}; }); - // Feedback-only sessions load this plugin but cannot enable Smart Router. - if (launchedEnabled) registerStatusBand(on); + registerStatusBand(on); registerSubagentRouting(on, options); } diff --git a/src/ucode/launcher.py b/src/ucode/launcher.py index 5fa0d694a..382672a44 100644 --- a/src/ucode/launcher.py +++ b/src/ucode/launcher.py @@ -9,14 +9,12 @@ from ucode.os_compatibility import subprocess_cross_os -def exec_or_spawn(argv: list[str], *, wait_for_child: bool = False) -> None: +def exec_or_spawn(argv: list[str]) -> None: """Hand the terminal to ``argv``, then exit with its status. On POSIX we ``os.execvp`` — the agent process *replaces* ucode, inheriting the controlling terminal cleanly. - Set wait_for_child when launch resources need cleanup after the agent exits. - On Windows there is no real ``exec``: ``os.execvp`` spawns a *new* process and immediately terminates the parent, so the launching shell resumes its prompt and fights the agent for the console. That produces the garbled, @@ -24,7 +22,7 @@ def exec_or_spawn(argv: list[str], *, wait_for_child: bool = False) -> None: for it, and propagate its exit code — the same pattern the token-refreshing agents (gemini/opencode/copilot/pi) already use. """ - if os.name != "nt" and not wait_for_child: + if os.name != "nt": os.execvp(argv[0], argv) return # unreachable on POSIX; keeps type-checkers happy diff --git a/tests/README.md b/tests/README.md index 3d9e0b414..c1a4c54bd 100644 --- a/tests/README.md +++ b/tests/README.md @@ -42,11 +42,11 @@ Also check that the form closes immediately and submission makes no model reques Start a new task during capture and confirm the report retains the original task; toggle routing during upload and confirm its recorded settings stay fixed. Check that saved evidence stays unchanged, upload permission denials are honored, local -previews stay local, and Other stays text-only. Run `/isaac-feedback` in sessions -launched with routing both on and off; check that a routing-disabled launch keeps -the status band hidden and removes its temporary plugin after exit. The -routing-toggle journeys do not cover the form, notification timing, or live -evidence uploads. +previews stay local, and Other stays text-only. In a session launched with routing, +check `/isaac-feedback` before and after turning routing off. A session launched +without routing should use the existing feedback skill without loading the router +plugin. The routing-toggle journeys do not cover the form, notification timing, or +live evidence uploads. `test_claude_answer_delivery.py` covers Claude answer selection, composition, ownership, failed tasks and resumed revisions using transcript inputs, plus real subprocess I/O, diff --git a/tests/integration/README.md b/tests/integration/README.md index 1d67d20cd..83be43d97 100644 --- a/tests/integration/README.md +++ b/tests/integration/README.md @@ -101,9 +101,9 @@ immediate form dismissal without model requests, and completion during another t Verify the original task stays selected when a new prompt precedes capture, and routing settings stay fixed across subsequent toggles. Check immutable saved evidence, upload permission denials, local previews, and text-only Other reports. -Also launch with routing disabled: -`/isaac-feedback` should still submit in the background, the status band should -stay hidden, and the temporary feedback plugin should be removed after exit. +In a session launched with routing, check that `/isaac-feedback` remains asynchronous +after routing is turned off. A session launched without routing should use the +existing feedback skill without loading the router plugin. The existing unit tests keep their fixtures. Integration has an independent pytest configuration and uses `--confcutdir` so those fixtures cannot leak in.