diff --git a/README.md b/README.md index 62ce48e..5a5394f 100644 --- a/README.md +++ b/README.md @@ -14,14 +14,22 @@ Blockers are for real damage or clear wrong turns: destructive commands, secret leaks, pushing broken code, ignoring explicit instructions, going in circles. Concerns cover likely bugs, wrong assumptions, unverified claims, or a much better approach. -## Picking the advisor in the composer +## Per session, off by default + +The advisor is chosen **per session**. Every session, new or existing, starts with the advisor **off**. Turning it on, or picking a model, in one session doesn't affect any other. The plugin adds an **Advisor** control to the composer bar, next to the model picker: -- **Off**: no reviews. -- **Same model as the agent**: the default. +- **Off**: no reviews. This is the default. +- **Same model as the agent**: turns it on with the session's own model. - **Any model from DSH's model picker**: the same provider-grouped list. **Hover** a model that has reasoning efforts to open a side menu and pick its effort; clicking the model uses its default effort. The keyboard works too (→ opens the effort menu, ← goes back, Esc closes). -The trigger shows the current choice, e.g. `Claude Haiku · Low` or `Advisor off`. The choice is saved on the host (`/omp-advisor.json`, through `GET`/`POST /omp-advisor/settings`). It applies from the advisor's next review, with no restart, and it overrides `enabled`, `provider`, `model` and `reasoningEffort` from the config below. Picking an advisor never changes the agent's own model. +The trigger shows this session's choice, e.g. `Claude Haiku · Low` or `Advisor off`. +- **Where it's saved:** on the host in `/omp-advisor/sessions.json`, keyed by session id, through `GET`/`POST /omp-advisor/settings?session=`. The newest 2,000 sessions are kept. +- **When it applies:** from that session's next review, with no restart. +- **What it overrides:** `enabled`, `provider`, `model`, `reasoningEffort` and `minLevel` from the config below, for that session only. +- **What it never touches:** the agent's own model. + +Upgrading from 1.1: the old global `/omp-advisor.json` is ignored, so no session stays on without being turned on again. ## Installation @@ -29,14 +37,14 @@ The trigger shows the current choice, e.g. `Claude Haiku · Low` or `Advisor off dsh plugin --profile web add git+https://git.rizzy.dev/DSH-Plugins/omp-advisor.git ``` -Or in the Web UI, go to **Settings → Plugins → Install**. For development, use `link:` to a clone. Restart DSH afterwards; new sessions are advised automatically. +Or in the Web UI, go to **Settings → Plugins → Install**. For development, use `link:` to a clone. Restart DSH afterwards, then turn the advisor on in the sessions where you want it. ## Configuration ```yaml - id: omp-advisor config: - enabled: true + enabled: false # default for sessions that haven't been switched on in the composer provider: '' # advisor model route; empty = same as the agent model: '' # e.g. provider: omni, model: claude-haiku-4-5 for a cheap fast reviewer reasoningEffort: '' # optional effort for the advisor model diff --git a/client.js b/client.js index 2d6f29e..7e52a7b 100644 --- a/client.js +++ b/client.js @@ -85,14 +85,15 @@ window.__ModuleLoader__.load({ const fill = (s, v) => String(s).replace(/\{(\w+)\}/g, (_, k) => (v && v[k] !== undefined ? v[k] : '')); const effortName = (e) => (e?.id === 'xhigh' ? 'Extra' : e?.name ?? e?.id); - // ---- host settings store (shared by every session's control) ---- - const settings = (() => { + // ---- host settings, one store per session (the advisor is chosen per session) ---- + function makeStore(sessionId) { let value = null; let inflight = null; const subs = new Set(); const emit = () => subs.forEach((f) => f()); + const url = `omp-advisor/settings?session=${encodeURIComponent(sessionId)}`; async function call(init) { - const res = await fetch('omp-advisor/settings', { ...init, headers: { accept: 'application/json', ...(init?.body ? { 'content-type': 'application/json' } : {}) } }); + const res = await fetch(url, { ...init, headers: { accept: 'application/json', ...(init?.body ? { 'content-type': 'application/json' } : {}) } }); let json = {}; try { json = await res.json(); } catch { /* empty */ } if (!res.ok) throw new Error(json.message || res.statusText || String(res.status)); @@ -112,11 +113,17 @@ window.__ModuleLoader__.load({ try { value = await call({ method: 'POST', body: JSON.stringify(patch) }); emit(); } catch (e) { value = prev; emit(); throw e; } }, }; - })(); + } + const stores = new Map(); + const storeFor = (sessionId) => { + let s = stores.get(sessionId); + if (!s) { s = makeStore(sessionId); stores.set(sessionId, s); } + return s; + }; - function useSettings() { - const v = React.useSyncExternalStore(settings.subscribe, settings.get); - React.useEffect(() => { if (v === null) settings.load(); }, [v]); + function useSettings(store) { + const v = React.useSyncExternalStore(store.subscribe, store.get); + React.useEffect(() => { if (v === null) store.load(); }, [store, v]); return v; } @@ -136,9 +143,15 @@ window.__ModuleLoader__.load({ return { left: Math.max(8, left), top: Math.max(8, Math.min(top, vh - m.height - 8)) }; } - function AdvisorPicker({ directory, load, t }) { - React.useEffect(installCss, []); - const st = useSettings(); + function AdvisorPicker(props) { + // Session-scoped slot; without an id (draft composer) there is nothing to configure yet. + if (typeof props.sessionId !== 'string' || !props.sessionId) return null; + return h(AdvisorPickerFor, props); + } + + function AdvisorPickerFor({ sessionId, directory, load, t }) { + const store = storeFor(sessionId); + const st = useSettings(store); const dir = React.useSyncExternalStore((f) => directory.subscribe(f), () => directory.getSnapshot()); const [open, setOpen] = React.useState(false); const [hover, setHover] = React.useState(null); // { key, el, model, group } @@ -151,7 +164,7 @@ window.__ModuleLoader__.load({ const subRef = React.useRef(null); const hoverTimer = React.useRef(null); - const enabled = st?.enabled !== false; + const enabled = st?.enabled === true; // off until this session turns it on (also while loading) const own = !!(st?.provider && st?.model); const groups = dir?.groups ?? []; const current = own ? groups.find((g) => g.id === st.provider)?.models.find((m) => m.id === st.model) : null; @@ -187,7 +200,7 @@ window.__ModuleLoader__.load({ const choose = async (patch) => { setErr(null); setOpen(false); - try { await settings.save(patch); } catch (e) { setErr(String(e.message || e)); setOpen(true); } + try { await store.save(patch); } catch (e) { setErr(String(e.message || e)); setOpen(true); } }; const pickModel = (group, model, effort) => choose({ enabled: true, provider: group.id, model: model.id, @@ -292,7 +305,7 @@ window.__ModuleLoader__.load({ const directory = ctx.modelDirectories.directoryFor(sessionId); // Read-only use of the directory: the advisor never calls directory.select (that would // switch the agent's own model). - return { directory: directory.store, load: () => { directory.load().catch(() => {}); } }; + return { sessionId, directory: directory.store, load: () => { directory.load().catch(() => {}); } }; }, }, AdvisorPicker)); }, diff --git a/cordis.patch.yml b/cordis.patch.yml index ee268b5..440f3df 100644 --- a/cordis.patch.yml +++ b/cordis.patch.yml @@ -2,7 +2,8 @@ - id: omp-advisor name: '@local/omp-advisor' config: - enabled: true + # default for sessions not yet switched on in the composer (each session chooses its own) + enabled: false # empty = same provider/model as the agent; set a cheaper/faster reviewer here provider: '' model: '' diff --git a/index.js b/index.js index 059b4d6..5805a34 100644 --- a/index.js +++ b/index.js @@ -18,17 +18,36 @@ import { createUserMessage } from '@deepseek-ai/dsh-llm' export const name = 'omp-advisor' export const inject = ['agents', 'llm'] -/** Live settings chosen in the UI (enabled + advisor route), persisted next to the profile. */ +/** + * Per-session advisor choices made in the UI: { [sessionId]: { enabled, provider, model, + * reasoningEffort, minLevel, at } }. A session without an entry uses the config defaults, which + * are OFF. The pre-1.2 global file (`omp-advisor.json`) is intentionally ignored so that no + * existing session silently stays on. + */ function settingsFile() { const dir = process.env.DSH_PROFILE_DIR || path.join(process.env.DSH_HOME || path.join(os.homedir(), '.dsh'), 'profiles', 'web') - return path.join(dir, 'omp-advisor.json') + return path.join(dir, 'omp-advisor', 'sessions.json') } -function loadSettings() { try { return JSON.parse(fs.readFileSync(settingsFile(), 'utf8')) } catch { return {} } } -function saveSettings(v) { +const MAX_SESSIONS = 2000 +function loadSessions() { + try { + const v = JSON.parse(fs.readFileSync(settingsFile(), 'utf8')) + return v && typeof v === 'object' && !Array.isArray(v) ? v : {} + } catch { return {} } +} +function saveSessions(all) { const f = settingsFile() fs.mkdirSync(path.dirname(f), { recursive: true }) - fs.writeFileSync(f, JSON.stringify(v, null, 2)) + // Keep the most recently changed sessions only. + const entries = Object.entries(all) + const kept = entries.length > MAX_SESSIONS + ? Object.fromEntries(entries.sort((a, b) => (b[1]?.at ?? 0) - (a[1]?.at ?? 0)).slice(0, MAX_SESSIONS)) + : all + const tmp = `${f}.${process.pid}.tmp` + fs.writeFileSync(tmp, JSON.stringify(kept, null, 1)) + fs.renameSync(tmp, f) // atomic replace: a crash never leaves half a file } +const validSessionId = (s) => typeof s === 'string' && s.length > 0 && s.length <= 200 && /^[\w.:-]+$/.test(s) const SOURCE = { kind: 'omp-advisor' } @@ -41,7 +60,7 @@ Be specific and short (max 3 sentences), reference files/commands concretely, an export function apply(ctx, config = {}) { const cfg = { - enabled: true, + enabled: false, // default for every session until it is turned on in the composer provider: '', // empty = same provider as the agent model: '', // empty = same model as the agent reasoningEffort: undefined, @@ -55,14 +74,20 @@ export function apply(ctx, config = {}) { ...config, } const LEVEL = { ok: 0, note: 1, concern: 2, blocker: 3 } - let live = loadSettings() // { enabled?, provider?, model?, reasoningEffort? } - const eff = () => ({ - enabled: live.enabled ?? cfg.enabled, - provider: live.provider ?? cfg.provider, - model: live.model ?? cfg.model, - reasoningEffort: live.reasoningEffort === null ? undefined : (live.reasoningEffort ?? cfg.reasoningEffort), - }) - const minLevel = () => LEVEL[live.minLevel ?? cfg.minLevel] ?? 1 + let sessions = loadSessions() + const sessionIdOf = (agent) => agent?.id ?? agent?.session?.header?.id + /** Effective settings for one session: its own UI choice over the config defaults. */ + const eff = (sessionId) => { + const live = (sessionId && sessions[sessionId]) || {} + return { + enabled: live.enabled ?? cfg.enabled, + provider: live.provider ?? cfg.provider, + model: live.model ?? cfg.model, + reasoningEffort: live.reasoningEffort === null ? undefined : (live.reasoningEffort ?? cfg.reasoningEffort), + minLevel: live.minLevel ?? cfg.minLevel, + } + } + const minLevel = (sessionId) => LEVEL[eff(sessionId).minLevel] ?? 1 const installed = new Map() function install(agent) { @@ -75,7 +100,7 @@ export function apply(ctx, config = {}) { disposers.push(agent.ctx.on('agent/pre-step', async (payload, next) => { const decision = await next() if (decision.kind !== 'enter') return decision - if (!eff().enabled) { st.queue.length = 0; return decision } + if (!eff(sessionIdOf(agent)).enabled) { st.queue.length = 0; return decision } st.step++ if (payload.step > 1 && st.step % cfg.everyNSteps === 0 && !st.pending) { st.pending = review(agent, st, payload.signal).finally(() => { st.pending = null }) @@ -89,7 +114,7 @@ export function apply(ctx, config = {}) { // At the end of a turn: deliver late advice; a blocker forces one more step. disposers.push(agent.ctx.on('agent/turn-stopping', async ({ signal }) => { - if (!eff().enabled) return + if (!eff(sessionIdOf(agent)).enabled) return if (st.pending) { let t await Promise.race([st.pending, new Promise((r) => { t = setTimeout(r, cfg.blocking ? cfg.timeoutMs : 4000) })]).finally(() => clearTimeout(t)) @@ -114,7 +139,7 @@ export function apply(ctx, config = {}) { async function review(agent, st, signal) { try { const header = agent.session.requestHeader?.() - const e = eff() + const e = eff(sessionIdOf(agent)) // A half-set route (provider without model) falls back to the agent's model as a whole. const own = e.provider && e.model const provider = own ? e.provider : header?.config?.provider @@ -143,7 +168,7 @@ export function apply(ctx, config = {}) { const v = parseVerdict(text) if (!v) return st.history.push(v) - if ((LEVEL[v.verdict] ?? 0) >= minLevel() && LEVEL[v.verdict] > 0) { + if ((LEVEL[v.verdict] ?? 0) >= minLevel(sessionIdOf(agent)) && LEVEL[v.verdict] > 0) { if (st.history.slice(0, -1).some((h) => h.message === v.message)) return st.queue.push(v) } @@ -197,7 +222,7 @@ export function apply(ctx, config = {}) { const connection = wctx.connection const send = (res, code, body) => { res.statusCode = code; res.setHeader('content-type', 'application/json; charset=utf-8'); res.setHeader('cache-control', 'no-store'); res.end(JSON.stringify(body)) } const readBody = async (req) => { let b = ''; for await (const c of req) { b += c; if (b.length > 16384) throw new Error('body too large') } return b ? JSON.parse(b) : {} } - const state = () => ({ ...eff(), configured: { provider: cfg.provider || null, model: cfg.model || null }, minLevel: live.minLevel ?? cfg.minLevel }) + const state = (sessionId) => ({ sessionId, ...eff(sessionId), configured: { provider: cfg.provider || null, model: cfg.model || null, enabled: !!cfg.enabled } }) wctx.effect(() => webServer.register({ kind: 'exact', path: '/omp-advisor/settings', @@ -205,18 +230,22 @@ export function apply(ctx, config = {}) { const rej = connection.requestRejection(req) if (rej !== undefined) return send(res, rej, { code: 'rejected' }) try { - if (req.method === 'GET') return send(res, 200, state()) + // Settings are per session: ?session= on every call. + const sessionId = new URL(req.url ?? '/', 'http://x').searchParams.get('session') + if (!validSessionId(sessionId)) return send(res, 400, { code: 'error', message: 'session query parameter is required' }) + if (req.method === 'GET') return send(res, 200, state(sessionId)) if (req.method !== 'POST') { res.statusCode = 405; return res.end() } const b = await readBody(req) - const next = { ...live } + const next = { ...(sessions[sessionId] ?? {}) } if (typeof b.enabled === 'boolean') next.enabled = b.enabled if ('provider' in b) next.provider = typeof b.provider === 'string' && b.provider ? b.provider : '' if ('model' in b) next.model = typeof b.model === 'string' && b.model ? b.model : '' if ('reasoningEffort' in b) next.reasoningEffort = typeof b.reasoningEffort === 'string' && b.reasoningEffort ? b.reasoningEffort : null if (typeof b.minLevel === 'string' && b.minLevel in LEVEL && b.minLevel !== 'ok') next.minLevel = b.minLevel - live = next - saveSettings(live) - return send(res, 200, state()) + next.at = Date.now() + sessions = { ...sessions, [sessionId]: next } + saveSessions(sessions) + return send(res, 200, state(sessionId)) } catch (e) { return send(res, 400, { code: 'error', message: String(e?.message ?? e) }) } }, }), 'omp-advisor: settings route') diff --git a/package.json b/package.json index dc066d8..d039bf3 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "@local/omp-advisor", - "version": "1.1.1", + "version": "1.2.0", "description": "oh-my-pi style advisor for DSH: a second model reviews every agent step and injects notes, concerns or blockers.", "private": true, "type": "module", diff --git a/test/advisor.test.mjs b/test/advisor.test.mjs index ff5134d..db6f802 100644 --- a/test/advisor.test.mjs +++ b/test/advisor.test.mjs @@ -6,14 +6,23 @@ import path from 'node:path' import { Readable } from 'node:stream' import * as plugin from '../index.js' -// Settings persist to /omp-advisor.json; keep tests away from the real profile. +// Settings persist to /omp-advisor/sessions.json; keep tests away from the real profile. const PROFILE = fs.mkdtempSync(path.join(os.tmpdir(), 'omp-advisor-test-')) process.env.DSH_PROFILE_DIR = PROFILE -const SETTINGS = path.join(PROFILE, 'omp-advisor.json') +const SETTINGS = path.join(PROFILE, 'omp-advisor', 'sessions.json') test.after(() => fs.rmSync(PROFILE, { recursive: true, force: true })) -function harness(replies, config = {}) { +/** + * @param config plugin config overrides + * @param opts { id: session id (default 'session-a'), on: pre-enable this session (default true) } + */ +function harness(replies, config = {}, opts = {}) { + const sessionId = opts.id ?? 'session-a' fs.rmSync(SETTINGS, { force: true }) // live settings load in apply(): never leak between tests + if (opts.on !== false) { + fs.mkdirSync(path.dirname(SETTINGS), { recursive: true }) + fs.writeFileSync(SETTINGS, JSON.stringify({ [sessionId]: { enabled: true, at: 1 } })) + } const listeners = new Map() const on = (n, f) => { listeners.set(n, [...(listeners.get(n) ?? []), f]); return () => listeners.set(n, listeners.get(n).filter((x) => x !== f)) } const prompts = [] @@ -34,7 +43,7 @@ function harness(replies, config = {}) { deriveEventMessage: (e) => e.msg, } const steered = [] - const agent = { session, ctx: { on }, steer: (m) => steered.push(m) } + const agent = { id: sessionId, session: { ...session, header: { ...session.header, id: sessionId } }, ctx: { on }, steer: (m) => steered.push(m) } const routes = new Map() const webServer = { register: (r) => { routes.set(r.path, r.handler); return () => routes.delete(r.path) } } const connection = { requestRejection: (req) => (req.headers?.['x-reject'] ? 401 : undefined) } @@ -52,11 +61,12 @@ function harness(replies, config = {}) { const stopTurn = async () => { for (const f of listeners.get('agent/turn-stopping') ?? []) await f({ agent, turn: 1 }) } const add = (role, content) => events.push({ msg: { role, content } }) /** Call the settings route like the web client does. */ - const http = async (method, body, headers = {}) => { + const http = async (method, body, headers = {}, sid = sessionId) => { const handler = routes.get('/omp-advisor/settings') const req = Readable.from(body === undefined ? [] : [JSON.stringify(body)]) req.method = method req.headers = headers + req.url = sid === null ? '/omp-advisor/settings' : `/omp-advisor/settings?session=${encodeURIComponent(sid)}` let status = 200; let out = '' const res = { statusCode: 200, setHeader() {}, end: (s) => { status = res.statusCode; out = s ?? '' } } await handler(req, res) @@ -111,20 +121,56 @@ test('minLevel filters notes and duplicates are suppressed', async () => { assert.equal((await h.preStep(4)).messages.length, 0) // identical concern suppressed }) -test('settings route: GET/POST, rejection, persistence', async () => { - fs.rmSync(SETTINGS, { force: true }) - const h = harness([]) +test('settings route: per-session GET/POST, rejection, missing session, persistence', async () => { + const h = harness([], {}, { on: false }) assert.equal((await h.http('GET', undefined, { 'x-reject': '1' })).status, 401) + assert.equal((await h.http('GET', undefined, {}, null)).status, 400) // no ?session= + assert.equal((await h.http('GET', undefined, {}, '../etc')).status, 400) // invalid id const g = await h.http('GET') assert.equal(g.status, 200) - assert.equal(g.json.enabled, true) + assert.equal(g.json.enabled, false) // off by default + assert.equal(g.json.sessionId, 'session-a') const p = await h.http('POST', { enabled: true, provider: 'omni', model: 'cheap-model', reasoningEffort: 'low' }) assert.equal(p.json.model, 'cheap-model') assert.equal(p.json.reasoningEffort, 'low') - assert.deepEqual(JSON.parse(fs.readFileSync(SETTINGS, 'utf8')), { enabled: true, provider: 'omni', model: 'cheap-model', reasoningEffort: 'low' }) + const stored = JSON.parse(fs.readFileSync(SETTINGS, 'utf8')) + assert.deepEqual(Object.keys(stored), ['session-a']) + assert.equal(stored['session-a'].model, 'cheap-model') + assert.equal(typeof stored['session-a'].at, 'number') assert.equal((await h.http('PUT', {})).status, 405) }) +test('default is off: no review for a session that never turned the advisor on', async () => { + const h = harness(['{"verdict":"concern","message":"should never be asked"}'], {}, { on: false }) + h.add('user', [{ type: 'text', text: 'task' }]) + assert.equal((await h.preStep(2)).messages.length, 0) + assert.equal(h.prompts.length, 0) +}) + +test('the old global omp-advisor.json is ignored (existing sessions stay off)', async () => { + fs.writeFileSync(path.join(PROFILE, 'omp-advisor.json'), JSON.stringify({ enabled: true, model: 'x' })) + const h = harness(['{"verdict":"ok","message":""}'], {}, { on: false }) + h.add('user', [{ type: 'text', text: 'task' }]) + await h.preStep(2) + assert.equal(h.prompts.length, 0) + assert.equal((await h.http('GET')).json.enabled, false) + fs.rmSync(path.join(PROFILE, 'omp-advisor.json'), { force: true }) +}) + +test('sessions are independent: turning one on does not affect another', async () => { + const h = harness(['{"verdict":"ok","message":""}', '{"verdict":"ok","message":""}'], {}, { on: false }) + h.add('user', [{ type: 'text', text: 'task' }]) + await h.http('POST', { enabled: true, model: 'other-model', provider: 'omni' }, {}, 'session-b') + await h.preStep(2) // this agent is session-a + assert.equal(h.prompts.length, 0) + assert.equal((await h.http('GET', undefined, {}, 'session-b')).json.model, 'other-model') + assert.equal((await h.http('GET')).json.enabled, false) + await h.http('POST', { enabled: true }) + await h.preStep(3) + assert.equal(h.prompts.length, 1) + assert.equal(h.prompts[0].model, 'main-model') // session-a uses the agent model, not session-b's +}) + test('UI choice applies on the next review without restart; off disables reviews', async () => { fs.rmSync(SETTINGS, { force: true }) const h = harness(['{"verdict":"ok","message":""}', '{"verdict":"ok","message":""}'])