From c0abd0b873b152ed7d41f35937cf1e5227f487ac Mon Sep 17 00:00:00 2001 From: Josh Joseph Date: Thu, 15 Oct 2026 18:55:29 +0000 Subject: [PATCH 1/4] Offer optional feedback after turning Smart Router off --- docs/claude-mod.md | 14 ++ skills/smart-router/SKILL.md | 5 +- src/ucode/agents/claude_mods/feedback.ts | 38 +++++ src/ucode/agents/claude_mods/register.ts | 28 +++- src/ucode/cli.py | 2 + src/ucode/smart_routing/v2.py | 8 +- tests/README.md | 6 + tests/integration/README.md | 3 + .../native_claude/mod_tests/feedback.test.ts | 133 ++++++++++++++++++ tests/test_claude_mod.py | 9 +- tests/test_smart_router.py | 22 +++ 11 files changed, 262 insertions(+), 6 deletions(-) create mode 100644 src/ucode/agents/claude_mods/feedback.ts create mode 100644 tests/native_claude/mod_tests/feedback.test.ts diff --git a/docs/claude-mod.md b/docs/claude-mod.md index bc9fe01f7..d2618157a 100644 --- a/docs/claude-mod.md +++ b/docs/claude-mod.md @@ -44,6 +44,20 @@ that file atomically. Session start restores only the launch baseline, so an ove before `/clear` still survives it. The plugin test host provides no process capability, so this path is covered by the live skill-toggle journey rather than `claude plugin test`. +After a successful on-to-off change, Isaac sessions with +`ISAAC_FEEDBACK_EVIDENCE_ENABLED=1` and an available `/isaac-feedback` command +show an optional reason form once per session. Isaac sets this marker from its +existing evidence rollout flag. Enter sends the reason through +`/isaac-feedback --category smart-routing`; Skip or Escape sends nothing. +The form makes no model requests. Only submitting invokes the feedback command. +Command discovery, UI and submission failures leave routing off. The CLI/skill +path, including Codex, prints an optional command invitation after disabling. + +The native component suite exercises the registered toggle and form handlers +with engine boundaries replaced where Claude 2.1.290's test host lacks APIs. +It checks the off-before-prompt ordering, submission, dismissal, repeated +toggles and failures. These checks do not establish a live evidence upload. + The existing Claude skill-toggle integration journey checks recorded inference payloads during its three calculation phases: launch recipe, `DISABLED` after off, and launch recipe after on. Each phase retains completed native parent/child diff --git a/skills/smart-router/SKILL.md b/skills/smart-router/SKILL.md index 439f8d4a8..a7ac2d25b 100644 --- a/skills/smart-router/SKILL.md +++ b/skills/smart-router/SKILL.md @@ -3,7 +3,7 @@ name: smart-router description: Enable or disable Unity Gateway subagent model routing for the current smart-routed Claude or Codex session, along with Smart Router Orchestrator when opted in. allowed-tools: Bash("$UCODE_SMART_ROUTER_PYTHON" -m ucode.cli claude --enable-smart-routing), Bash("$UCODE_SMART_ROUTER_PYTHON" -m ucode.cli claude --disable-smart-routing), Bash("$UCODE_SMART_ROUTER_PYTHON" -m ucode.cli codex --enable-smart-routing), Bash("$UCODE_SMART_ROUTER_PYTHON" -m ucode.cli codex --disable-smart-routing) metadata: - version: "1.2.0" + version: "1.3.0" --- # Smart Router @@ -31,3 +31,6 @@ native tools and normal harness model selection, without Smart Router Orchestrat Existing children can finish. When turned on, apply the `smart-router-orchestrator` skill to further work only if the session was launched with `ENABLE_SMART_ROUTER_ORCHESTRATOR=1`. Do not change the orchestration feature flag. Return the command's result. +If the result includes the optional `/isaac-feedback` invitation, show it at most +once in this session. Do not wait for feedback or submit it automatically. +Declining feedback or a submission failure must leave routing off. diff --git a/src/ucode/agents/claude_mods/feedback.ts b/src/ucode/agents/claude_mods/feedback.ts new file mode 100644 index 000000000..972f39411 --- /dev/null +++ b/src/ucode/agents/claude_mods/feedback.ts @@ -0,0 +1,38 @@ +import type { On } from 'claude-code'; + +export const FEEDBACK_PANE = 'smart-router-feedback'; + +type FeedbackState = { offered: boolean; submitted: boolean; reason: string }; + +export function registerFeedback(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); + return Box({ + flexDirection: 'column', + children: [ + Text({ children: ['Why are you turning Smart Router off? (optional)'] }), + Text({ children: ['Send your reason and available session evidence with /isaac-feedback.'] }), + Input({ + key: 'reason', + label: 'Reason', + value: state.reason, + autoFocus: true, + submitLabel: 'Send feedback', + onInput: (value) => { state.reason = value; }, + onSubmit: async (value) => { + if (state.submitted || !value.trim()) return; + state.submitted = true; + try { + await $.ui.close({ id: FEEDBACK_PANE }); + await $.command.run({ command: 'isaac-feedback', args: `--category smart-routing ${value}` }); + } catch { + $.ui.toast('Could not open /isaac-feedback. Smart Router is still off.'); + } + }, + }), + Button({ key: 'skip', label: 'Skip', 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 2963849fd..d72ade8ac 100644 --- a/src/ucode/agents/claude_mods/register.ts +++ b/src/ucode/agents/claude_mods/register.ts @@ -9,6 +9,7 @@ import { RECIPE_FIELD, } from './constants.ts'; import { mergeExtraBody } from './extra-body.ts'; +import { FEEDBACK_PANE, registerFeedback } from './feedback.ts'; type Environment = Record; @@ -50,7 +51,7 @@ async function setRouting( options: PluginOptions, enabled: boolean, syncSessionControls: boolean, -): Promise { +): Promise { const original = (key: string): string | undefined => (options.unset as readonly string[]).includes(key) ? undefined : options[key] as string; const previous = await readEnvironment($); @@ -78,10 +79,12 @@ async function setRouting( await writeEnvironment($, previous); throw error; } + return previous.ENABLE_SMART_ROUTING_V2 === '1' || previous.ENABLE_SMART_ROUTING_SUBAGENT_ONLY === '1'; } export function register(on: On, options: PluginOptions): void { + const feedback = { offered: false, submitted: false, reason: '' }; const launchedEnabled = options.ENABLE_SMART_ROUTING_V2 === '1' || options.ENABLE_SMART_ROUTING_SUBAGENT_ONLY === '1'; @@ -99,14 +102,33 @@ export function register(on: On, options: PluginOptions): void { const action = e.args.trim(); if (action !== 'on' && action !== 'off') return {text: 'Usage: /smart-router on|off'}; if (!launchedEnabled) return {text: 'Smart Router was not enabled for this session.'}; + let wasEnabled: boolean; try { - await setRouting($, options, action === 'on', true); + wasEnabled = await setRouting($, options, action === 'on', true); showStatusBand(); $.ui.invalidate('ui.render'); - return {text: `Smart Router is ${action}`}; } catch { return {text: 'Could not update Smart Router. Check the recipe, CLAUDE_CODE_EXTRA_BODY, and environment permissions; retry or restart through UG.'}; } + // Feedback is optional and must never roll back a successful toggle. + if (action === 'off' && wasEnabled && !feedback.offered) { + try { + if (await $.env.get('ISAAC_FEEDBACK_EVIDENCE_ENABLED') === '1' && + (await $.command.list()).some((command) => command.name === 'isaac-feedback')) { + feedback.offered = true; + const pane = await $.ui.open({ + id: FEEDBACK_PANE, title: 'Smart Router is off', focus: true, + closeOnEscape: true, rows: 6, columns: 56, + }); + // Do not leave a delayed question waiting for an unrelated task. + if (!pane.isPlaced) await $.ui.close({ id: FEEDBACK_PANE }); + } + } catch { + // Unavailable commands, permissions or UI must leave routing off. + } + } + return {text: `Smart Router is ${action}`}; }); + registerFeedback(on, feedback); registerStatusBand(on); } diff --git a/src/ucode/cli.py b/src/ucode/cli.py index 0f5f542de..a43cb84c1 100644 --- a/src/ucode/cli.py +++ b/src/ucode/cli.py @@ -2712,6 +2712,8 @@ def _toggle_current_smart_routing_session(enabled: bool | None) -> bool: print_err(str(exc)) raise typer.Exit(1) from None print_success(f"Smart Router is {'on' if enabled else 'off'} for this session") + if not enabled and os.environ.get("ISAAC_FEEDBACK_EVIDENCE_ENABLED") == "1": + print_note("Optional: share why with /isaac-feedback --category smart-routing .") return True diff --git a/src/ucode/smart_routing/v2.py b/src/ucode/smart_routing/v2.py index 4d5e0dee7..b1f8fd388 100644 --- a/src/ucode/smart_routing/v2.py +++ b/src/ucode/smart_routing/v2.py @@ -396,7 +396,13 @@ def _write_routed_claude_plugin(plugin_dir: Path, model_ids: list[str]) -> None: orchestrator.add_claude_agents(plugin_dir) mods = Path(__file__).parents[1] / "agents" / "claude_mods" # These are imported by the entry point, so they ship beside it rather than as modules. - for name in ("register.ts", "constants.ts", "extra-body.ts", "smart-routing-status.ts"): + for name in ( + "register.ts", + "constants.ts", + "extra-body.ts", + "feedback.ts", + "smart-routing-status.ts", + ): write_text_file(plugin_dir / "hooks" / name, (mods / name).read_text(encoding="utf-8")) write_json_file(plugin_dir / "hooks" / "hooks.json", {"modules": ["./register.ts"]}) diff --git a/tests/README.md b/tests/README.md index c1687e11b..fe332adfb 100644 --- a/tests/README.md +++ b/tests/README.md @@ -19,6 +19,12 @@ 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 optional Isaac feedback form has component coverage for disabling before +prompting, exact reason handoff, Skip, empty input, repeated toggles and failures. +Missing APIs in Claude 2.1.290's test host are replaced at the engine boundary; +the production handlers execute. CLI tests check the rollout marker and off-only +invitation. These tests do not perform a live Isaac upload. + The **harness-latest** workflow runs every six hours on `main` and currently covers smart routing only. It resolves npm's `claude-code@latest` diff --git a/tests/integration/README.md b/tests/integration/README.md index 7f0c87955..e796a94c3 100644 --- a/tests/integration/README.md +++ b/tests/integration/README.md @@ -65,6 +65,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 optional Isaac feedback form is checked separately by native component tests +using engine boundaries, including Skip and fail-open behavior. The gateway +toggle journey does not prove Isaac evidence submission. 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/feedback.test.ts b/tests/native_claude/mod_tests/feedback.test.ts new file mode 100644 index 000000000..5a92529bb --- /dev/null +++ b/tests/native_claude/mod_tests/feedback.test.ts @@ -0,0 +1,133 @@ +import type { EngineInterface, On } from 'claude-code'; +import { expect, test } from 'claude-code/testing'; +import { register } from '../hooks/register.ts'; + +const ROUTED = { + ENABLE_SMART_ROUTING_V2: '0', + ENABLE_SMART_ROUTING_SUBAGENT_ONLY: '1', + ENABLE_SMART_ROUTER_ORCHESTRATOR: '1', + unset: [], + recipe: 'task_v3', +}; + +// Claude 2.1.290's test host omits process.run, command.list and ui.open. +// Replace those engine boundaries, then drive the real registered hooks. +function fixture({ evidence = true, available = true, fail = '' } = {}) { + let toggle: any; + let render: any; + const on = ((event: string, filter: any, handler: any) => { + if (event === 'command.run') toggle = handler; + if (event === 'ui.render' && filter.component === 'Pane') render = handler; + }) as On; + register(on, ROUTED); + const env: Record = { + ...ROUTED, + unset: undefined, + CLAUDE_CODE_EXTRA_BODY: '{"caller":"keep"}', + UCODE_SMART_ROUTER_PYTHON: '/launch/python', + UCODE_SESSION_ENV_FILE: '/session/env.json', + ISAAC_FEEDBACK_EVIDENCE_ENABLED: evidence ? '1' : '0', + }; + let controls = 'on'; + const opened: any[] = []; + const sent: any[] = []; + const closed: any[] = []; + const notices: string[] = []; + const invalidated: string[] = []; + const element = (type: string) => (props: any) => ({ type, ...props }); + const host = { + env: { + get: async (key: string) => env[key], + set: async (key: string, value: string | undefined) => { env[key] = value; }, + }, + process: { run: async (argv: string[]) => { + if (fail === 'controls') return { exitCode: 1, stdout: '', stderr: 'denied' }; + controls = argv.at(-1) === '--disable-smart-routing' ? 'off' : 'on'; + return { exitCode: 0, stdout: '', stderr: '' }; + } }, + command: { + list: async () => available ? [{ name: 'isaac-feedback' }] : [], + run: async (input: any) => { + if (fail === 'submission') throw new Error('command unavailable'); + sent.push(input); + return {}; + }, + }, + ui: { + invalidate: (event: string) => { invalidated.push(event); }, + open: async (input: any) => { + expect(controls).toBe('off'); + expect(env.ENABLE_SMART_ROUTING_SUBAGENT_ONLY).toBe('0'); + if (fail === 'prompt') throw new Error('no UI'); + opened.push(input); + return { isPlaced: true }; + }, + close: async (input: any) => { closed.push(input); }, + toast: (message: string) => { notices.push(message); }, + resolve: () => Object.fromEntries(['Box', 'Text', 'Input', 'Button'].map((name) => [name, element(name)])), + }, + } as unknown as EngineInterface; + return { + env, opened, sent, closed, notices, invalidated, + toggle: (args = 'off') => toggle(host, { args }), + pane: () => render(host, { requestId: 'smart-router-feedback', surface: 'terminal' }), + }; +} + +test('off precedes the optional prompt; only submitting sends the exact reason', async () => { + const f = fixture(); + expect(await f.toggle()).toEqual({ text: 'Smart Router is off' }); + expect(f.invalidated).toEqual(['ui.render']); + expect(f.opened).toHaveLength(1); + expect(f.opened[0].closeOnEscape).toBe(true); + expect(f.sent).toHaveLength(0); + expect(f.env.ENABLE_SMART_ROUTER_ORCHESTRATOR).toBe('0'); + expect(JSON.parse(f.env.CLAUDE_CODE_EXTRA_BODY!)).toEqual({ caller: 'keep', smart_router_recipe_name: 'DISABLED' }); + + const input = f.pane().children.find((child: any) => child.key === 'reason'); + const reason = "retry later: it's slow; $(this is text)\nand lost context"; + await input.onSubmit(reason); + await input.onSubmit(reason); + expect(f.sent).toEqual([{ command: 'isaac-feedback', args: `--category smart-routing ${reason}` }]); + expect(f.closed).toEqual([{ id: 'smart-router-feedback' }]); + expect(f.env.ENABLE_SMART_ROUTING_SUBAGENT_ONLY).toBe('0'); +}); + +test('empty feedback and Skip send nothing; repeated toggles do not prompt again', async () => { + const f = fixture(); + await f.toggle(); + const pane = f.pane(); + await pane.children.find((child: any) => child.key === 'reason').onSubmit(' '); + await pane.children.find((child: any) => child.key === 'skip').onPress(); + await f.toggle(); + await f.toggle('on'); + await f.toggle(); + expect(f.opened).toHaveLength(1); + expect(f.sent).toHaveLength(0); + expect(f.closed).toEqual([{ id: 'smart-router-feedback' }]); + expect(f.env.ENABLE_SMART_ROUTING_SUBAGENT_ONLY).toBe('0'); +}); + +test('disabled rollout, missing command and failed prompt all leave routing off', async () => { + for (const options of [{ evidence: false }, { available: false }, { fail: 'prompt' }]) { + const f = fixture(options); + expect(await f.toggle()).toEqual({ text: 'Smart Router is off' }); + expect(f.opened).toHaveLength(0); + expect(f.sent).toHaveLength(0); + expect(f.env.ENABLE_SMART_ROUTING_SUBAGENT_ONLY).toBe('0'); + } +}); + +test('failed submission leaves routing off; a failed toggle never offers feedback', async () => { + const f = fixture({ fail: 'submission' }); + await f.toggle(); + await f.pane().children.find((child: any) => child.key === 'reason').onSubmit('Too slow'); + expect(f.notices[0]).toContain('Smart Router is still off'); + expect(f.env.ENABLE_SMART_ROUTING_SUBAGENT_ONLY).toBe('0'); + expect(f.sent).toHaveLength(0); + + const failedToggle = fixture({ fail: 'controls' }); + expect((await failedToggle.toggle()).text).toContain('Could not update Smart Router'); + expect(failedToggle.opened).toHaveLength(0); + expect(failedToggle.env.ENABLE_SMART_ROUTING_SUBAGENT_ONLY).toBe('1'); +}); diff --git a/tests/test_claude_mod.py b/tests/test_claude_mod.py index a3ef2f719..5b45f8469 100644 --- a/tests/test_claude_mod.py +++ b/tests/test_claude_mod.py @@ -15,11 +15,18 @@ def test_plugin_contains_mod_entry_point(tmp_path): assert json.loads((tmp_path / "hooks/hooks.json").read_text()) == {"modules": ["./register.ts"]} mods = Path(v2.__file__).parents[1] / "agents" / "claude_mods" # The entry point imports these, so they ship beside it unmodified. - for name in ("register.ts", "constants.ts", "extra-body.ts", "smart-routing-status.ts"): + for name in ( + "register.ts", + "constants.ts", + "extra-body.ts", + "feedback.ts", + "smart-routing-status.ts", + ): assert (tmp_path / "hooks" / name).read_bytes() == (mods / name).read_bytes() assert sorted(path.name for path in (tmp_path / "hooks").iterdir()) == [ "constants.ts", "extra-body.ts", + "feedback.ts", "hooks.json", "register.ts", "smart-routing-status.ts", diff --git a/tests/test_smart_router.py b/tests/test_smart_router.py index 504cf7991..8cad3e863 100644 --- a/tests/test_smart_router.py +++ b/tests/test_smart_router.py @@ -18,6 +18,28 @@ runner = CliRunner() +@pytest.mark.parametrize("enabled", [False, True]) +@pytest.mark.parametrize("feedback_enabled", ["0", "1"]) +def test_toggle_offers_optional_feedback_only_after_disabling( + tmp_path, monkeypatch, enabled, feedback_enabled +): + session_file = tmp_path / "env.json" + session_file.write_text("{}") + monkeypatch.setenv(session_env.SESSION_ENV_VAR, str(session_file)) + monkeypatch.setenv("ISAAC_FEEDBACK_EVIDENCE_ENABLED", feedback_enabled) + + action = "--enable-smart-routing" if enabled else "--disable-smart-routing" + result = runner.invoke(cli.app, ["codex", action]) + + assert result.exit_code == 0, result.output + assert ("/isaac-feedback --category smart-routing " in result.output) == ( + not enabled and feedback_enabled == "1" + ) + assert json.loads(session_file.read_text()) == ( + {} if enabled else dict.fromkeys(v2.SMART_ROUTING_ENV_KEYS, "0") + ) + + @pytest.fixture(autouse=True) def _checkout_skill_bundle(monkeypatch): # An editable Python install can still contain an older wheel's copied skill resources. From abb3162abfced511ad273823baa450a7bae0efb0 Mon Sep 17 00:00:00 2001 From: Josh Joseph Date: Fri, 16 Oct 2026 17:06:06 +0000 Subject: [PATCH 2/4] Use Isaac's single smart-router feedback flag --- docs/claude-mod.md | 8 +++++--- src/ucode/agents/claude_mods/register.ts | 2 +- src/ucode/cli.py | 2 +- tests/README.md | 5 +++-- tests/native_claude/mod_tests/feedback.test.ts | 3 ++- tests/test_smart_router.py | 3 ++- 6 files changed, 14 insertions(+), 9 deletions(-) diff --git a/docs/claude-mod.md b/docs/claude-mod.md index d2618157a..4304369b4 100644 --- a/docs/claude-mod.md +++ b/docs/claude-mod.md @@ -45,9 +45,11 @@ before `/clear` still survives it. The plugin test host provides no process capa this path is covered by the live skill-toggle journey rather than `claude plugin test`. After a successful on-to-off change, Isaac sessions with -`ISAAC_FEEDBACK_EVIDENCE_ENABLED=1` and an available `/isaac-feedback` command -show an optional reason form once per session. Isaac sets this marker from its -existing evidence rollout flag. Enter sends the reason through +`ISAAC_SMART_ROUTER_FEEDBACK=1` and an available `/isaac-feedback` command +show an optional reason form once per session. Isaac sets this flag from its +rollout setting or the user's explicit `1`/`0` override. It controls Isaac feedback +availability, so UG's routing presets and on/off toggles leave it unchanged. +Enter sends the reason through `/isaac-feedback --category smart-routing`; Skip or Escape sends nothing. The form makes no model requests. Only submitting invokes the feedback command. Command discovery, UI and submission failures leave routing off. The CLI/skill diff --git a/src/ucode/agents/claude_mods/register.ts b/src/ucode/agents/claude_mods/register.ts index d72ade8ac..071536481 100644 --- a/src/ucode/agents/claude_mods/register.ts +++ b/src/ucode/agents/claude_mods/register.ts @@ -113,7 +113,7 @@ export function register(on: On, options: PluginOptions): void { // Feedback is optional and must never roll back a successful toggle. if (action === 'off' && wasEnabled && !feedback.offered) { try { - if (await $.env.get('ISAAC_FEEDBACK_EVIDENCE_ENABLED') === '1' && + if (await $.env.get('ISAAC_SMART_ROUTER_FEEDBACK') === '1' && (await $.command.list()).some((command) => command.name === 'isaac-feedback')) { feedback.offered = true; const pane = await $.ui.open({ diff --git a/src/ucode/cli.py b/src/ucode/cli.py index a43cb84c1..a0cd6afc2 100644 --- a/src/ucode/cli.py +++ b/src/ucode/cli.py @@ -2712,7 +2712,7 @@ def _toggle_current_smart_routing_session(enabled: bool | None) -> bool: print_err(str(exc)) raise typer.Exit(1) from None print_success(f"Smart Router is {'on' if enabled else 'off'} for this session") - if not enabled and os.environ.get("ISAAC_FEEDBACK_EVIDENCE_ENABLED") == "1": + if not enabled and os.environ.get("ISAAC_SMART_ROUTER_FEEDBACK") == "1": print_note("Optional: share why with /isaac-feedback --category smart-routing .") return True diff --git a/tests/README.md b/tests/README.md index fe332adfb..6664440f0 100644 --- a/tests/README.md +++ b/tests/README.md @@ -22,8 +22,9 @@ See [the mod guide](../docs/claude-mod.md). The optional Isaac feedback form has component coverage for disabling before prompting, exact reason handoff, Skip, empty input, repeated toggles and failures. Missing APIs in Claude 2.1.290's test host are replaced at the engine boundary; -the production handlers execute. CLI tests check the rollout marker and off-only -invitation. These tests do not perform a live Isaac upload. +the production handlers execute. CLI tests check `ISAAC_SMART_ROUTER_FEEDBACK` +and the off-only invitation, including preserving feedback availability after +disabling routing. These tests do not perform a live Isaac upload. The **harness-latest** workflow runs every six hours on `main` and currently covers smart routing only. diff --git a/tests/native_claude/mod_tests/feedback.test.ts b/tests/native_claude/mod_tests/feedback.test.ts index 5a92529bb..32e7c8aa5 100644 --- a/tests/native_claude/mod_tests/feedback.test.ts +++ b/tests/native_claude/mod_tests/feedback.test.ts @@ -26,7 +26,7 @@ function fixture({ evidence = true, available = true, fail = '' } = {}) { CLAUDE_CODE_EXTRA_BODY: '{"caller":"keep"}', UCODE_SMART_ROUTER_PYTHON: '/launch/python', UCODE_SESSION_ENV_FILE: '/session/env.json', - ISAAC_FEEDBACK_EVIDENCE_ENABLED: evidence ? '1' : '0', + ISAAC_SMART_ROUTER_FEEDBACK: evidence ? '1' : '0', }; let controls = 'on'; const opened: any[] = []; @@ -82,6 +82,7 @@ test('off precedes the optional prompt; only submitting sends the exact reason', expect(f.opened[0].closeOnEscape).toBe(true); expect(f.sent).toHaveLength(0); expect(f.env.ENABLE_SMART_ROUTER_ORCHESTRATOR).toBe('0'); + expect(f.env.ISAAC_SMART_ROUTER_FEEDBACK).toBe('1'); expect(JSON.parse(f.env.CLAUDE_CODE_EXTRA_BODY!)).toEqual({ caller: 'keep', smart_router_recipe_name: 'DISABLED' }); const input = f.pane().children.find((child: any) => child.key === 'reason'); diff --git a/tests/test_smart_router.py b/tests/test_smart_router.py index 8cad3e863..1fffcef0f 100644 --- a/tests/test_smart_router.py +++ b/tests/test_smart_router.py @@ -26,7 +26,7 @@ def test_toggle_offers_optional_feedback_only_after_disabling( session_file = tmp_path / "env.json" session_file.write_text("{}") monkeypatch.setenv(session_env.SESSION_ENV_VAR, str(session_file)) - monkeypatch.setenv("ISAAC_FEEDBACK_EVIDENCE_ENABLED", feedback_enabled) + monkeypatch.setenv("ISAAC_SMART_ROUTER_FEEDBACK", feedback_enabled) action = "--enable-smart-routing" if enabled else "--disable-smart-routing" result = runner.invoke(cli.app, ["codex", action]) @@ -38,6 +38,7 @@ def test_toggle_offers_optional_feedback_only_after_disabling( assert json.loads(session_file.read_text()) == ( {} if enabled else dict.fromkeys(v2.SMART_ROUTING_ENV_KEYS, "0") ) + assert os.environ["ISAAC_SMART_ROUTER_FEEDBACK"] == feedback_enabled @pytest.fixture(autouse=True) From 893c521709ec125b119a8dae7abb41287f620e1e Mon Sep 17 00:00:00 2001 From: Josh Joseph Date: Fri, 16 Oct 2026 19:16:38 +0000 Subject: [PATCH 3/4] Require feedback before disabling Smart Router in Isaac --- docs/claude-mod.md | 26 ++++--- skills/smart-router/SKILL.md | 17 +++-- src/ucode/agents/claude_mods/feedback.ts | 29 ++++++-- src/ucode/agents/claude_mods/register.ts | 57 ++++++++------ src/ucode/cli.py | 27 +++++-- tests/README.md | 11 +-- tests/integration/README.md | 5 +- .../native_claude/mod_tests/feedback.test.ts | 74 ++++++++++++------- tests/test_smart_router.py | 37 +++++++--- 9 files changed, 189 insertions(+), 94 deletions(-) diff --git a/docs/claude-mod.md b/docs/claude-mod.md index 4304369b4..f37e8c5af 100644 --- a/docs/claude-mod.md +++ b/docs/claude-mod.md @@ -44,21 +44,27 @@ that file atomically. Session start restores only the launch baseline, so an ove before `/clear` still survives it. The plugin test host provides no process capability, so this path is covered by the live skill-toggle journey rather than `claude plugin test`. -After a successful on-to-off change, Isaac sessions with -`ISAAC_SMART_ROUTER_FEEDBACK=1` and an available `/isaac-feedback` command -show an optional reason form once per session. Isaac sets this flag from its +Before each on-to-off change, Isaac sessions with +`ISAAC_SMART_ROUTER_FEEDBACK=1` require a reason of at least 16 characters after +trimming surrounding whitespace. Isaac sets this flag from its rollout setting or the user's explicit `1`/`0` override. It controls Isaac feedback availability, so UG's routing presets and on/off toggles leave it unchanged. -Enter sends the reason through -`/isaac-feedback --category smart-routing`; Skip or Escape sends nothing. -The form makes no model requests. Only submitting invokes the feedback command. -Command discovery, UI and submission failures leave routing off. The CLI/skill -path, including Codex, prints an optional command invitation after disabling. +The native form keeps routing on until Enter accepts a valid reason, then disables +routing and sends the exact text through `/isaac-feedback --category smart-routing`. +Cancel or Escape leaves routing on and sends nothing. Missing commands or an +unavailable form leave routing on. A feedback submission failure after a valid +reason leaves routing off and reports the failure. Opening or dismissing the form +makes no model requests; only submitting invokes the feedback command. + +The CLI/skill path, including Codex, enforces the same minimum through the session +toggle's `--smart-router-feedback` argument. The skill asks for the reason before +retrying the toggle and submits it through `/isaac-feedback` on success. The CLI +validates the reason; the skill or native form owns submission. The native component suite exercises the registered toggle and form handlers with engine boundaries replaced where Claude 2.1.290's test host lacks APIs. -It checks the off-before-prompt ordering, submission, dismissal, repeated -toggles and failures. These checks do not establish a live evidence upload. +It checks validation before disabling, exact reason submission, cancellation, +repeated toggles and failures. These checks do not establish a live evidence upload. The existing Claude skill-toggle integration journey checks recorded inference payloads during its three calculation phases: launch recipe, `DISABLED` after off, diff --git a/skills/smart-router/SKILL.md b/skills/smart-router/SKILL.md index a7ac2d25b..23974bf24 100644 --- a/skills/smart-router/SKILL.md +++ b/skills/smart-router/SKILL.md @@ -1,9 +1,9 @@ --- name: smart-router description: Enable or disable Unity Gateway subagent model routing for the current smart-routed Claude or Codex session, along with Smart Router Orchestrator when opted in. -allowed-tools: Bash("$UCODE_SMART_ROUTER_PYTHON" -m ucode.cli claude --enable-smart-routing), Bash("$UCODE_SMART_ROUTER_PYTHON" -m ucode.cli claude --disable-smart-routing), Bash("$UCODE_SMART_ROUTER_PYTHON" -m ucode.cli codex --enable-smart-routing), Bash("$UCODE_SMART_ROUTER_PYTHON" -m ucode.cli codex --disable-smart-routing) +allowed-tools: Bash("$UCODE_SMART_ROUTER_PYTHON" -m ucode.cli claude --enable-smart-routing), Bash("$UCODE_SMART_ROUTER_PYTHON" -m ucode.cli claude --disable-smart-routing), Bash("$UCODE_SMART_ROUTER_PYTHON" -m ucode.cli claude --disable-smart-routing --smart-router-feedback *), Bash("$UCODE_SMART_ROUTER_PYTHON" -m ucode.cli codex --enable-smart-routing), Bash("$UCODE_SMART_ROUTER_PYTHON" -m ucode.cli codex --disable-smart-routing), Bash("$UCODE_SMART_ROUTER_PYTHON" -m ucode.cli codex --disable-smart-routing --smart-router-feedback *) metadata: - version: "1.3.0" + version: "1.4.0" --- # Smart Router @@ -21,6 +21,16 @@ In PowerShell, use `& "$env:UCODE_SMART_ROUTER_PYTHON"` in place of If `UCODE_SMART_ROUTER_PYTHON` or `UCODE_SESSION_ENV_FILE` is unset, ask the user to restart through an updated Unity Gateway with smart routing enabled. +If the off command requires feedback, routing is still on. Ask why the user is +turning Smart Router off. Require at least 16 characters after trimming surrounding +whitespace; ask again if the reason is shorter. Never invent or pad their reason. +If they cancel, stop without disabling routing. Once they provide a valid reason, +repeat the off command with `--smart-router-feedback ''` appended, quoting +the exact text as one shell argument. After a successful toggle, submit that text +through `/isaac-feedback --category smart-routing `. Report a submission +failure without re-enabling routing. This requirement applies when Isaac sets +`ISAAC_SMART_ROUTER_FEEDBACK=1`; do not change that flag to bypass it. + With no argument, explain that only `on` and `off` are accepted. Do not edit the state file. This affects subsequent subagent model selection in the current session, not the root model or first prompt. In sessions launched with `ENABLE_SMART_ROUTER_ORCHESTRATOR=1`, it also controls Smart Router @@ -31,6 +41,3 @@ native tools and normal harness model selection, without Smart Router Orchestrat Existing children can finish. When turned on, apply the `smart-router-orchestrator` skill to further work only if the session was launched with `ENABLE_SMART_ROUTER_ORCHESTRATOR=1`. Do not change the orchestration feature flag. Return the command's result. -If the result includes the optional `/isaac-feedback` invitation, show it at most -once in this session. Do not wait for feedback or submit it automatically. -Declining feedback or a submission failure must leave routing off. diff --git a/src/ucode/agents/claude_mods/feedback.ts b/src/ucode/agents/claude_mods/feedback.ts index 972f39411..bf36ea98f 100644 --- a/src/ucode/agents/claude_mods/feedback.ts +++ b/src/ucode/agents/claude_mods/feedback.ts @@ -2,7 +2,11 @@ import type { On } from 'claude-code'; export const FEEDBACK_PANE = 'smart-router-feedback'; -type FeedbackState = { offered: boolean; submitted: boolean; reason: string }; +export type FeedbackState = { + submitted: boolean; + reason: string; + disableRouting?: (reason: string) => Promise; +}; export function registerFeedback(on: On, state: FeedbackState): void { on('ui.render', { component: 'Pane' }, ($, e, next) => { @@ -11,27 +15,38 @@ export function registerFeedback(on: On, state: FeedbackState): void { return Box({ flexDirection: 'column', children: [ - Text({ children: ['Why are you turning Smart Router off? (optional)'] }), - Text({ children: ['Send your reason and available session evidence with /isaac-feedback.'] }), + Text({ children: ['Why are you turning Smart Router off?'] }), + Text({ children: ['Enter at least 16 characters. Your reason and session evidence go to /isaac-feedback.'] }), Input({ key: 'reason', label: 'Reason', value: state.reason, autoFocus: true, - submitLabel: 'Send feedback', + submitLabel: 'Send feedback and turn off', onInput: (value) => { state.reason = value; }, onSubmit: async (value) => { - if (state.submitted || !value.trim()) return; + 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; + try { + await state.disableRouting(value); + } catch { + state.submitted = false; + $.ui.toast('Could not update Smart Router. Smart Router is still on; retry or restart through UG.'); + return; + } try { await $.ui.close({ id: FEEDBACK_PANE }); await $.command.run({ command: 'isaac-feedback', args: `--category smart-routing ${value}` }); } catch { - $.ui.toast('Could not open /isaac-feedback. Smart Router is still off.'); + $.ui.toast('Smart Router is off, but feedback could not open. Retry with /isaac-feedback --category smart-routing.'); } }, }), - Button({ key: 'skip', label: 'Skip', onPress: () => $.ui.close({ id: FEEDBACK_PANE }) }), + 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 071536481..ff4866704 100644 --- a/src/ucode/agents/claude_mods/register.ts +++ b/src/ucode/agents/claude_mods/register.ts @@ -9,7 +9,7 @@ import { RECIPE_FIELD, } from './constants.ts'; import { mergeExtraBody } from './extra-body.ts'; -import { FEEDBACK_PANE, registerFeedback } from './feedback.ts'; +import { FEEDBACK_PANE, registerFeedback, type FeedbackState } from './feedback.ts'; type Environment = Record; @@ -32,7 +32,7 @@ async function writeEnvironment($: EngineInterface, env: Environment): Promise { +async function writeSessionControls($: EngineInterface, enabled: boolean, reason?: string): Promise { // Claude's capability scanner requires literal environment-variable names. const python = await $.env.get('UCODE_SMART_ROUTER_PYTHON'); const sessionFile = await $.env.get('UCODE_SESSION_ENV_FILE'); @@ -40,7 +40,9 @@ async function writeSessionControls($: EngineInterface, enabled: boolean): Promi throw new Error('Smart Router session controls are unavailable for this session.'); } const flag = enabled ? ENABLE_ROUTING_FLAG : DISABLE_ROUTING_FLAG; - const result = await $.process.run([python, '-m', CLI_MODULE, CLI_AGENT, flag]); + const args = [python, '-m', CLI_MODULE, CLI_AGENT, flag]; + if (reason !== undefined) args.push('--smart-router-feedback', reason); + const result = await $.process.run(args); if (result.exitCode !== 0) { throw new Error(`Smart Router controls exited ${result.exitCode}: ${result.stderr.trim()}`); } @@ -51,7 +53,8 @@ async function setRouting( options: PluginOptions, enabled: boolean, syncSessionControls: boolean, -): Promise { + reason?: string, +): Promise { const original = (key: string): string | undefined => (options.unset as readonly string[]).includes(key) ? undefined : options[key] as string; const previous = await readEnvironment($); @@ -72,19 +75,18 @@ async function setRouting( // An explicit toggle must also replace the CLI/skill's saved session overrides, // which the routing hooks apply on top of the environment and would otherwise win. if (syncSessionControls) { - await writeSessionControls($, enabled); + await writeSessionControls($, enabled, reason); } } catch (error) { // Restore earlier successful writes if a later environment write is denied. await writeEnvironment($, previous); throw error; } - return previous.ENABLE_SMART_ROUTING_V2 === '1' || previous.ENABLE_SMART_ROUTING_SUBAGENT_ONLY === '1'; } export function register(on: On, options: PluginOptions): void { - const feedback = { offered: false, submitted: false, reason: '' }; + const feedback: FeedbackState = { submitted: false, reason: '' }; const launchedEnabled = options.ENABLE_SMART_ROUTING_V2 === '1' || options.ENABLE_SMART_ROUTING_SUBAGENT_ONLY === '1'; @@ -102,30 +104,39 @@ export function register(on: On, options: PluginOptions): void { const action = e.args.trim(); if (action !== 'on' && action !== 'off') return {text: 'Usage: /smart-router on|off'}; if (!launchedEnabled) return {text: 'Smart Router was not enabled for this session.'}; - let wasEnabled: boolean; - try { - wasEnabled = await setRouting($, options, action === 'on', true); - showStatusBand(); - $.ui.invalidate('ui.render'); - } catch { - return {text: 'Could not update Smart Router. Check the recipe, CLAUDE_CODE_EXTRA_BODY, and environment permissions; retry or restart through UG.'}; - } - // Feedback is optional and must never roll back a successful toggle. - if (action === 'off' && wasEnabled && !feedback.offered) { + if (action === 'off' && await $.env.get('ISAAC_SMART_ROUTER_FEEDBACK') === '1') { try { - if (await $.env.get('ISAAC_SMART_ROUTER_FEEDBACK') === '1' && - (await $.command.list()).some((command) => command.name === 'isaac-feedback')) { - feedback.offered = true; + const current = await readEnvironment($); + if (current.ENABLE_SMART_ROUTING_V2 !== '1' && current.ENABLE_SMART_ROUTING_SUBAGENT_ONLY !== '1') { + return {text: 'Smart Router is off'}; + } + if ((await $.command.list()).some((command) => command.name === 'isaac-feedback')) { + feedback.submitted = false; + feedback.reason = ''; + feedback.disableRouting = async (reason) => { + await setRouting($, options, false, true, reason); + showStatusBand(); + $.ui.invalidate('ui.render'); + }; const pane = await $.ui.open({ - id: FEEDBACK_PANE, title: 'Smart Router is off', focus: true, + id: FEEDBACK_PANE, title: 'Turn Smart Router off', focus: true, closeOnEscape: true, rows: 6, columns: 56, }); + if (pane.isPlaced) return {text: 'A reason of at least 16 characters is required to turn Smart Router off.'}; // Do not leave a delayed question waiting for an unrelated task. - if (!pane.isPlaced) await $.ui.close({ id: FEEDBACK_PANE }); + await $.ui.close({ id: FEEDBACK_PANE }); } } catch { - // Unavailable commands, permissions or UI must leave routing off. + // No reason was collected, so leave routing unchanged. } + return {text: 'Could not open the feedback form. Smart Router is still on; retry /smart-router off in an Isaac session with /isaac-feedback available.'}; + } + try { + await setRouting($, options, action === 'on', true); + showStatusBand(); + $.ui.invalidate('ui.render'); + } catch { + return {text: 'Could not update Smart Router. Check the recipe, CLAUDE_CODE_EXTRA_BODY, and environment permissions; retry or restart through UG.'}; } return {text: `Smart Router is ${action}`}; }); diff --git a/src/ucode/cli.py b/src/ucode/cli.py index a0cd6afc2..0be309a3c 100644 --- a/src/ucode/cli.py +++ b/src/ucode/cli.py @@ -2699,21 +2699,30 @@ def _smart_routing_v2_flag(enabled: bool | None, *, agent: str) -> Iterator[None smart_routing_v2.restore_smart_routing_env(previous) -def _toggle_current_smart_routing_session(enabled: bool | None) -> bool: +def _toggle_current_smart_routing_session(enabled: bool | None, reason: str | None = None) -> bool: if enabled is None: return False try: session_env_path() except RuntimeError: return False + if ( + not enabled + and os.environ.get("ISAAC_SMART_ROUTER_FEEDBACK") == "1" + and len((reason or "").strip()) < 16 + ): + print_err( + "Turning Smart Router off requires a reason of at least 16 characters, excluding " + "surrounding whitespace. Ask the user why, then pass their reason with " + "--smart-router-feedback and submit it through /isaac-feedback --category smart-routing." + ) + raise typer.Exit(2) try: set_session_environment(dict.fromkeys(SMART_ROUTING_ENV_KEYS, "0") if not enabled else {}) except RuntimeError as exc: print_err(str(exc)) raise typer.Exit(1) from None print_success(f"Smart Router is {'on' if enabled else 'off'} for this session") - if not enabled and os.environ.get("ISAAC_SMART_ROUTER_FEEDBACK") == "1": - print_note("Optional: share why with /isaac-feedback --category smart-routing .") return True @@ -3708,9 +3717,13 @@ def codex_cmd( help="Enable or disable AI Gateway model routing for this Codex launch or session.", ), ] = None, + smart_router_feedback: Annotated[ + str | None, + typer.Option("--smart-router-feedback", hidden=True), + ] = None, ) -> None: """Launch Codex via Databricks.""" - if _toggle_current_smart_routing_session(enable_smart_routing_flag): + if _toggle_current_smart_routing_session(enable_smart_routing_flag, smart_router_feedback): return try: custom_oauth = _custom_oauth_config(client_id, redirect_url, scopes) @@ -3796,9 +3809,13 @@ def claude_cmd( help="Enable or disable AI Gateway model routing for this Claude Code launch or session.", ), ] = None, + smart_router_feedback: Annotated[ + str | None, + typer.Option("--smart-router-feedback", hidden=True), + ] = None, ) -> None: """Launch Claude Code via Databricks.""" - if _toggle_current_smart_routing_session(enable_smart_routing_flag): + if _toggle_current_smart_routing_session(enable_smart_routing_flag, smart_router_feedback): return try: custom_oauth = _custom_oauth_config(client_id, redirect_url, scopes) diff --git a/tests/README.md b/tests/README.md index 6664440f0..5b848d05b 100644 --- a/tests/README.md +++ b/tests/README.md @@ -19,12 +19,13 @@ 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 optional Isaac feedback form has component coverage for disabling before -prompting, exact reason handoff, Skip, empty input, repeated toggles and failures. +The required Isaac feedback form has component coverage for validating before +disabling, exact reason handoff, Cancel, the 15/16-character boundary (including +whitespace and Unicode), repeated toggles and failures. Missing APIs in Claude 2.1.290's test host are replaced at the engine boundary; -the production handlers execute. CLI tests check `ISAAC_SMART_ROUTER_FEEDBACK` -and the off-only invitation, including preserving feedback availability after -disabling routing. These tests do not perform a live Isaac upload. +the production handlers execute. CLI tests check the same minimum when +`ISAAC_SMART_ROUTER_FEEDBACK=1` for both agents, unchanged controls on rejection, +and preservation of the rollout flag. These tests do not perform a live Isaac upload. The **harness-latest** workflow runs every six hours on `main` and currently covers smart routing only. diff --git a/tests/integration/README.md b/tests/integration/README.md index e796a94c3..184db9fee 100644 --- a/tests/integration/README.md +++ b/tests/integration/README.md @@ -65,8 +65,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 optional Isaac feedback form is checked separately by native component tests -using engine boundaries, including Skip and fail-open behavior. The gateway +The required Isaac feedback form is checked separately by native component tests +using engine boundaries, including the 16-character minimum, cancellation before +disabling, and submission failure after a valid reason. The gateway toggle journey does not prove Isaac evidence submission. The existing unit tests keep their fixtures. Integration has an independent diff --git a/tests/native_claude/mod_tests/feedback.test.ts b/tests/native_claude/mod_tests/feedback.test.ts index 32e7c8aa5..4c0d51dbe 100644 --- a/tests/native_claude/mod_tests/feedback.test.ts +++ b/tests/native_claude/mod_tests/feedback.test.ts @@ -42,7 +42,10 @@ function fixture({ evidence = true, available = true, fail = '' } = {}) { }, process: { run: async (argv: string[]) => { if (fail === 'controls') return { exitCode: 1, stdout: '', stderr: 'denied' }; - controls = argv.at(-1) === '--disable-smart-routing' ? 'off' : 'on'; + controls = argv.includes('--disable-smart-routing') ? 'off' : 'on'; + if (controls === 'off' && evidence) { + expect(Array.from(argv[argv.indexOf('--smart-router-feedback') + 1].trim()).length).toBeGreaterThan(15); + } return { exitCode: 0, stdout: '', stderr: '' }; } }, command: { @@ -56,8 +59,8 @@ function fixture({ evidence = true, available = true, fail = '' } = {}) { ui: { invalidate: (event: string) => { invalidated.push(event); }, open: async (input: any) => { - expect(controls).toBe('off'); - expect(env.ENABLE_SMART_ROUTING_SUBAGENT_ONLY).toBe('0'); + expect(controls).toBe('on'); + expect(env.ENABLE_SMART_ROUTING_SUBAGENT_ONLY).toBe('1'); if (fail === 'prompt') throw new Error('no UI'); opened.push(input); return { isPlaced: true }; @@ -74,61 +77,80 @@ function fixture({ evidence = true, available = true, fail = '' } = {}) { }; } -test('off precedes the optional prompt; only submitting sends the exact reason', async () => { +test('a valid reason is required before disabling, and its exact text is submitted once', async () => { const f = fixture(); - expect(await f.toggle()).toEqual({ text: 'Smart Router is off' }); - expect(f.invalidated).toEqual(['ui.render']); + expect((await f.toggle()).text).toContain('at least 16 characters'); + expect(f.invalidated).toHaveLength(0); expect(f.opened).toHaveLength(1); expect(f.opened[0].closeOnEscape).toBe(true); expect(f.sent).toHaveLength(0); - expect(f.env.ENABLE_SMART_ROUTER_ORCHESTRATOR).toBe('0'); - expect(f.env.ISAAC_SMART_ROUTER_FEEDBACK).toBe('1'); - expect(JSON.parse(f.env.CLAUDE_CODE_EXTRA_BODY!)).toEqual({ caller: 'keep', smart_router_recipe_name: 'DISABLED' }); + expect(f.env.ENABLE_SMART_ROUTER_ORCHESTRATOR).toBe('1'); const input = f.pane().children.find((child: any) => child.key === 'reason'); - const reason = "retry later: it's slow; $(this is text)\nand lost context"; + for (const value of [' ', '123456789012345', ' 123456789012345 ', '😀'.repeat(15)]) { + await input.onSubmit(value); + expect(f.sent).toHaveLength(0); + expect(f.env.ENABLE_SMART_ROUTING_SUBAGENT_ONLY).toBe('1'); + expect(f.notices.at(-1)).toContain('at least 16 characters'); + } + const reason = " retry later: it's slow; $(this is text)\nand lost context "; await input.onSubmit(reason); await input.onSubmit(reason); expect(f.sent).toEqual([{ command: 'isaac-feedback', args: `--category smart-routing ${reason}` }]); expect(f.closed).toEqual([{ id: 'smart-router-feedback' }]); expect(f.env.ENABLE_SMART_ROUTING_SUBAGENT_ONLY).toBe('0'); + expect(f.env.ENABLE_SMART_ROUTER_ORCHESTRATOR).toBe('0'); + expect(f.env.ISAAC_SMART_ROUTER_FEEDBACK).toBe('1'); + expect(JSON.parse(f.env.CLAUDE_CODE_EXTRA_BODY!)).toEqual({ caller: 'keep', smart_router_recipe_name: 'DISABLED' }); + expect(f.invalidated).toEqual(['ui.render']); }); -test('empty feedback and Skip send nothing; repeated toggles do not prompt again', async () => { +test('Cancel keeps routing on; every subsequent on-to-off change requires a new reason', async () => { const f = fixture(); await f.toggle(); - const pane = f.pane(); - await pane.children.find((child: any) => child.key === 'reason').onSubmit(' '); - await pane.children.find((child: any) => child.key === 'skip').onPress(); + await f.pane().children.find((child: any) => child.key === 'cancel').onPress(); + expect(f.sent).toHaveLength(0); + expect(f.env.ENABLE_SMART_ROUTING_SUBAGENT_ONLY).toBe('1'); await f.toggle(); + await f.pane().children.find((child: any) => child.key === 'reason').onSubmit(' 1234567890123456 '); + expect(f.env.ENABLE_SMART_ROUTING_SUBAGENT_ONLY).toBe('0'); + expect(await f.toggle()).toEqual({ text: 'Smart Router is off' }); + expect(f.opened).toHaveLength(2); await f.toggle('on'); await f.toggle(); - expect(f.opened).toHaveLength(1); - expect(f.sent).toHaveLength(0); - expect(f.closed).toEqual([{ id: 'smart-router-feedback' }]); + expect(f.opened).toHaveLength(3); + expect(f.env.ENABLE_SMART_ROUTING_SUBAGENT_ONLY).toBe('1'); + expect(f.pane().children.find((child: any) => child.key === 'reason').value).toBe(''); + await f.pane().children.find((child: any) => child.key === 'reason').onSubmit('😀'.repeat(16)); + expect(f.sent).toHaveLength(2); expect(f.env.ENABLE_SMART_ROUTING_SUBAGENT_ONLY).toBe('0'); }); -test('disabled rollout, missing command and failed prompt all leave routing off', async () => { - for (const options of [{ evidence: false }, { available: false }, { fail: 'prompt' }]) { +test('disabled rollout preserves normal toggles; unavailable feedback keeps routing on', async () => { + const unenrolled = fixture({ evidence: false }); + expect(await unenrolled.toggle()).toEqual({ text: 'Smart Router is off' }); + expect(unenrolled.opened).toHaveLength(0); + for (const options of [{ available: false }, { fail: 'prompt' }]) { const f = fixture(options); - expect(await f.toggle()).toEqual({ text: 'Smart Router is off' }); + expect((await f.toggle()).text).toContain('Smart Router is still on'); expect(f.opened).toHaveLength(0); expect(f.sent).toHaveLength(0); - expect(f.env.ENABLE_SMART_ROUTING_SUBAGENT_ONLY).toBe('0'); + expect(f.env.ENABLE_SMART_ROUTING_SUBAGENT_ONLY).toBe('1'); } }); -test('failed submission leaves routing off; a failed toggle never offers feedback', async () => { +test('submission failures leave routing off after a valid reason; failed toggles preserve routing', async () => { const f = fixture({ fail: 'submission' }); await f.toggle(); - await f.pane().children.find((child: any) => child.key === 'reason').onSubmit('Too slow'); - expect(f.notices[0]).toContain('Smart Router is still off'); + await f.pane().children.find((child: any) => child.key === 'reason').onSubmit('The task was too slow'); + expect(f.notices[0]).toContain('Smart Router is off'); expect(f.env.ENABLE_SMART_ROUTING_SUBAGENT_ONLY).toBe('0'); expect(f.sent).toHaveLength(0); const failedToggle = fixture({ fail: 'controls' }); - expect((await failedToggle.toggle()).text).toContain('Could not update Smart Router'); - expect(failedToggle.opened).toHaveLength(0); + await failedToggle.toggle(); + await failedToggle.pane().children.find((child: any) => child.key === 'reason').onSubmit('The task was too slow'); + expect(failedToggle.notices[0]).toContain('Smart Router is still on'); + expect(failedToggle.sent).toHaveLength(0); expect(failedToggle.env.ENABLE_SMART_ROUTING_SUBAGENT_ONLY).toBe('1'); }); diff --git a/tests/test_smart_router.py b/tests/test_smart_router.py index 1fffcef0f..f26798d88 100644 --- a/tests/test_smart_router.py +++ b/tests/test_smart_router.py @@ -18,10 +18,23 @@ runner = CliRunner() -@pytest.mark.parametrize("enabled", [False, True]) -@pytest.mark.parametrize("feedback_enabled", ["0", "1"]) -def test_toggle_offers_optional_feedback_only_after_disabling( - tmp_path, monkeypatch, enabled, feedback_enabled +@pytest.mark.parametrize("agent", ["claude", "codex"]) +@pytest.mark.parametrize( + "enabled,feedback_enabled,reason,exit_code", + [ + (False, "0", None, 0), + (True, "1", None, 0), + (False, "1", None, 2), + (False, "1", " ", 2), + (False, "1", "123456789012345", 2), + (False, "1", " 123456789012345 ", 2), + (False, "1", "😀" * 15, 2), + (False, "1", " 1234567890123456 ", 0), + (False, "1", "😀" * 16, 0), + ], +) +def test_disabling_requires_feedback_when_enrolled( + tmp_path, monkeypatch, agent, enabled, feedback_enabled, reason, exit_code ): session_file = tmp_path / "env.json" session_file.write_text("{}") @@ -29,14 +42,16 @@ def test_toggle_offers_optional_feedback_only_after_disabling( monkeypatch.setenv("ISAAC_SMART_ROUTER_FEEDBACK", feedback_enabled) action = "--enable-smart-routing" if enabled else "--disable-smart-routing" - result = runner.invoke(cli.app, ["codex", action]) - - assert result.exit_code == 0, result.output - assert ("/isaac-feedback --category smart-routing " in result.output) == ( - not enabled and feedback_enabled == "1" - ) + args = [agent, action] + if reason is not None: + args.extend(["--smart-router-feedback", reason]) + result = runner.invoke(cli.app, args) + + assert result.exit_code == exit_code, result.output + if exit_code: + assert "at least 16 characters" in result.output assert json.loads(session_file.read_text()) == ( - {} if enabled else dict.fromkeys(v2.SMART_ROUTING_ENV_KEYS, "0") + {} if enabled or exit_code else dict.fromkeys(v2.SMART_ROUTING_ENV_KEYS, "0") ) assert os.environ["ISAAC_SMART_ROUTER_FEEDBACK"] == feedback_enabled From 01e788e511d70b432b8e99f09d452781c5e3b565 Mon Sep 17 00:00:00 2001 From: Josh Joseph Date: Mon, 19 Oct 2026 04:02:20 +0000 Subject: [PATCH 4/4] Remove mocked feedback form and toggle tests --- docs/claude-mod.md | 7 +- tests/README.md | 10 +- tests/integration/README.md | 6 +- .../native_claude/mod_tests/feedback.test.ts | 156 ------------------ tests/test_smart_router.py | 38 ----- 5 files changed, 8 insertions(+), 209 deletions(-) delete mode 100644 tests/native_claude/mod_tests/feedback.test.ts diff --git a/docs/claude-mod.md b/docs/claude-mod.md index 45ff70121..9f394ec17 100644 --- a/docs/claude-mod.md +++ b/docs/claude-mod.md @@ -73,10 +73,9 @@ toggle's `--smart-router-feedback` argument. The skill asks for the reason befor retrying the toggle and submits it through `/isaac-feedback` on success. The CLI validates the reason; the skill or native form owns submission. -The native component suite exercises the registered toggle and form handlers -with engine boundaries replaced where Claude 2.1.290's test host lacks APIs. -It checks validation before disabling, exact reason submission, cancellation, -repeated toggles and failures. These checks do not establish a live evidence upload. +Validate the form in Isaac, including the 16-character minimum, exact reason +submission, cancellation, repeated toggles and failures. These scenarios and a live +evidence upload are not covered by the automated suite. The existing Claude skill-toggle integration journey checks recorded inference payloads during its three calculation phases: launch recipe, `DISABLED` after off, diff --git a/tests/README.md b/tests/README.md index 4c42082ac..095d3d7ee 100644 --- a/tests/README.md +++ b/tests/README.md @@ -19,13 +19,9 @@ 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 required Isaac feedback form has component coverage for validating before -disabling, exact reason handoff, Cancel, the 15/16-character boundary (including -whitespace and Unicode), repeated toggles and failures. -Missing APIs in Claude 2.1.290's test host are replaced at the engine boundary; -the production handlers execute. CLI tests check the same minimum when -`ISAAC_SMART_ROUTER_FEEDBACK=1` for both agents, unchanged controls on rejection, -and preservation of the rollout flag. These tests do not perform a live Isaac upload. +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 **harness-latest** workflow runs every six hours on `main` and currently covers smart routing only. diff --git a/tests/integration/README.md b/tests/integration/README.md index d40cb567b..8cfcae087 100644 --- a/tests/integration/README.md +++ b/tests/integration/README.md @@ -65,10 +65,8 @@ 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 required Isaac feedback form is checked separately by native component tests -using engine boundaries, including the 16-character minimum, cancellation before -disabling, and submission failure after a valid reason. The gateway -toggle journey does not prove Isaac evidence submission. +The required Isaac feedback form and evidence submission need a separate manual +Isaac check. The gateway toggle journey does not cover the form. 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/feedback.test.ts b/tests/native_claude/mod_tests/feedback.test.ts deleted file mode 100644 index 4c0d51dbe..000000000 --- a/tests/native_claude/mod_tests/feedback.test.ts +++ /dev/null @@ -1,156 +0,0 @@ -import type { EngineInterface, On } from 'claude-code'; -import { expect, test } from 'claude-code/testing'; -import { register } from '../hooks/register.ts'; - -const ROUTED = { - ENABLE_SMART_ROUTING_V2: '0', - ENABLE_SMART_ROUTING_SUBAGENT_ONLY: '1', - ENABLE_SMART_ROUTER_ORCHESTRATOR: '1', - unset: [], - recipe: 'task_v3', -}; - -// Claude 2.1.290's test host omits process.run, command.list and ui.open. -// Replace those engine boundaries, then drive the real registered hooks. -function fixture({ evidence = true, available = true, fail = '' } = {}) { - let toggle: any; - let render: any; - const on = ((event: string, filter: any, handler: any) => { - if (event === 'command.run') toggle = handler; - if (event === 'ui.render' && filter.component === 'Pane') render = handler; - }) as On; - register(on, ROUTED); - const env: Record = { - ...ROUTED, - unset: undefined, - CLAUDE_CODE_EXTRA_BODY: '{"caller":"keep"}', - UCODE_SMART_ROUTER_PYTHON: '/launch/python', - UCODE_SESSION_ENV_FILE: '/session/env.json', - ISAAC_SMART_ROUTER_FEEDBACK: evidence ? '1' : '0', - }; - let controls = 'on'; - const opened: any[] = []; - const sent: any[] = []; - const closed: any[] = []; - const notices: string[] = []; - const invalidated: string[] = []; - const element = (type: string) => (props: any) => ({ type, ...props }); - const host = { - env: { - get: async (key: string) => env[key], - set: async (key: string, value: string | undefined) => { env[key] = value; }, - }, - process: { run: async (argv: string[]) => { - if (fail === 'controls') return { exitCode: 1, stdout: '', stderr: 'denied' }; - controls = argv.includes('--disable-smart-routing') ? 'off' : 'on'; - if (controls === 'off' && evidence) { - expect(Array.from(argv[argv.indexOf('--smart-router-feedback') + 1].trim()).length).toBeGreaterThan(15); - } - return { exitCode: 0, stdout: '', stderr: '' }; - } }, - command: { - list: async () => available ? [{ name: 'isaac-feedback' }] : [], - run: async (input: any) => { - if (fail === 'submission') throw new Error('command unavailable'); - sent.push(input); - return {}; - }, - }, - ui: { - invalidate: (event: string) => { invalidated.push(event); }, - open: async (input: any) => { - expect(controls).toBe('on'); - expect(env.ENABLE_SMART_ROUTING_SUBAGENT_ONLY).toBe('1'); - if (fail === 'prompt') throw new Error('no UI'); - opened.push(input); - return { isPlaced: true }; - }, - close: async (input: any) => { closed.push(input); }, - toast: (message: string) => { notices.push(message); }, - resolve: () => Object.fromEntries(['Box', 'Text', 'Input', 'Button'].map((name) => [name, element(name)])), - }, - } as unknown as EngineInterface; - return { - env, opened, sent, closed, notices, invalidated, - toggle: (args = 'off') => toggle(host, { args }), - pane: () => render(host, { requestId: 'smart-router-feedback', surface: 'terminal' }), - }; -} - -test('a valid reason is required before disabling, and its exact text is submitted once', async () => { - const f = fixture(); - expect((await f.toggle()).text).toContain('at least 16 characters'); - expect(f.invalidated).toHaveLength(0); - expect(f.opened).toHaveLength(1); - expect(f.opened[0].closeOnEscape).toBe(true); - expect(f.sent).toHaveLength(0); - expect(f.env.ENABLE_SMART_ROUTER_ORCHESTRATOR).toBe('1'); - - const input = f.pane().children.find((child: any) => child.key === 'reason'); - for (const value of [' ', '123456789012345', ' 123456789012345 ', '😀'.repeat(15)]) { - await input.onSubmit(value); - expect(f.sent).toHaveLength(0); - expect(f.env.ENABLE_SMART_ROUTING_SUBAGENT_ONLY).toBe('1'); - expect(f.notices.at(-1)).toContain('at least 16 characters'); - } - const reason = " retry later: it's slow; $(this is text)\nand lost context "; - await input.onSubmit(reason); - await input.onSubmit(reason); - expect(f.sent).toEqual([{ command: 'isaac-feedback', args: `--category smart-routing ${reason}` }]); - expect(f.closed).toEqual([{ id: 'smart-router-feedback' }]); - expect(f.env.ENABLE_SMART_ROUTING_SUBAGENT_ONLY).toBe('0'); - expect(f.env.ENABLE_SMART_ROUTER_ORCHESTRATOR).toBe('0'); - expect(f.env.ISAAC_SMART_ROUTER_FEEDBACK).toBe('1'); - expect(JSON.parse(f.env.CLAUDE_CODE_EXTRA_BODY!)).toEqual({ caller: 'keep', smart_router_recipe_name: 'DISABLED' }); - expect(f.invalidated).toEqual(['ui.render']); -}); - -test('Cancel keeps routing on; every subsequent on-to-off change requires a new reason', async () => { - const f = fixture(); - await f.toggle(); - await f.pane().children.find((child: any) => child.key === 'cancel').onPress(); - expect(f.sent).toHaveLength(0); - expect(f.env.ENABLE_SMART_ROUTING_SUBAGENT_ONLY).toBe('1'); - await f.toggle(); - await f.pane().children.find((child: any) => child.key === 'reason').onSubmit(' 1234567890123456 '); - expect(f.env.ENABLE_SMART_ROUTING_SUBAGENT_ONLY).toBe('0'); - expect(await f.toggle()).toEqual({ text: 'Smart Router is off' }); - expect(f.opened).toHaveLength(2); - await f.toggle('on'); - await f.toggle(); - expect(f.opened).toHaveLength(3); - expect(f.env.ENABLE_SMART_ROUTING_SUBAGENT_ONLY).toBe('1'); - expect(f.pane().children.find((child: any) => child.key === 'reason').value).toBe(''); - await f.pane().children.find((child: any) => child.key === 'reason').onSubmit('😀'.repeat(16)); - expect(f.sent).toHaveLength(2); - expect(f.env.ENABLE_SMART_ROUTING_SUBAGENT_ONLY).toBe('0'); -}); - -test('disabled rollout preserves normal toggles; unavailable feedback keeps routing on', async () => { - const unenrolled = fixture({ evidence: false }); - expect(await unenrolled.toggle()).toEqual({ text: 'Smart Router is off' }); - expect(unenrolled.opened).toHaveLength(0); - for (const options of [{ available: false }, { fail: 'prompt' }]) { - const f = fixture(options); - expect((await f.toggle()).text).toContain('Smart Router is still on'); - expect(f.opened).toHaveLength(0); - expect(f.sent).toHaveLength(0); - expect(f.env.ENABLE_SMART_ROUTING_SUBAGENT_ONLY).toBe('1'); - } -}); - -test('submission failures leave routing off after a valid reason; failed toggles preserve routing', async () => { - const f = fixture({ fail: 'submission' }); - await f.toggle(); - await f.pane().children.find((child: any) => child.key === 'reason').onSubmit('The task was too slow'); - expect(f.notices[0]).toContain('Smart Router is off'); - expect(f.env.ENABLE_SMART_ROUTING_SUBAGENT_ONLY).toBe('0'); - expect(f.sent).toHaveLength(0); - - const failedToggle = fixture({ fail: 'controls' }); - await failedToggle.toggle(); - await failedToggle.pane().children.find((child: any) => child.key === 'reason').onSubmit('The task was too slow'); - expect(failedToggle.notices[0]).toContain('Smart Router is still on'); - expect(failedToggle.sent).toHaveLength(0); - expect(failedToggle.env.ENABLE_SMART_ROUTING_SUBAGENT_ONLY).toBe('1'); -}); diff --git a/tests/test_smart_router.py b/tests/test_smart_router.py index f26798d88..504cf7991 100644 --- a/tests/test_smart_router.py +++ b/tests/test_smart_router.py @@ -18,44 +18,6 @@ runner = CliRunner() -@pytest.mark.parametrize("agent", ["claude", "codex"]) -@pytest.mark.parametrize( - "enabled,feedback_enabled,reason,exit_code", - [ - (False, "0", None, 0), - (True, "1", None, 0), - (False, "1", None, 2), - (False, "1", " ", 2), - (False, "1", "123456789012345", 2), - (False, "1", " 123456789012345 ", 2), - (False, "1", "😀" * 15, 2), - (False, "1", " 1234567890123456 ", 0), - (False, "1", "😀" * 16, 0), - ], -) -def test_disabling_requires_feedback_when_enrolled( - tmp_path, monkeypatch, agent, enabled, feedback_enabled, reason, exit_code -): - session_file = tmp_path / "env.json" - session_file.write_text("{}") - monkeypatch.setenv(session_env.SESSION_ENV_VAR, str(session_file)) - monkeypatch.setenv("ISAAC_SMART_ROUTER_FEEDBACK", feedback_enabled) - - action = "--enable-smart-routing" if enabled else "--disable-smart-routing" - args = [agent, action] - if reason is not None: - args.extend(["--smart-router-feedback", reason]) - result = runner.invoke(cli.app, args) - - assert result.exit_code == exit_code, result.output - if exit_code: - assert "at least 16 characters" in result.output - assert json.loads(session_file.read_text()) == ( - {} if enabled or exit_code else dict.fromkeys(v2.SMART_ROUTING_ENV_KEYS, "0") - ) - assert os.environ["ISAAC_SMART_ROUTER_FEEDBACK"] == feedback_enabled - - @pytest.fixture(autouse=True) def _checkout_skill_bundle(monkeypatch): # An editable Python install can still contain an older wheel's copied skill resources.