diff --git a/README.md b/README.md index 362f1e4..d7a35a6 100644 --- a/README.md +++ b/README.md @@ -148,10 +148,17 @@ Or drive the tool directly: | `analyze-one` | One cluster, optionally by `cluster` hash. | | `update` | Upgrade the slopo CLI. | -The long actions run as **background jobs** by default (pass `foreground: true` -to run inline). Embedding takes minutes, and slopo writes its progress to a log -file rather than to stdout, so there is nothing to stream — the job handle -carries the log path instead. +`prepare` — the warm-up — starts a **background job** by default, because embedding +takes minutes and slopo writes its progress to a log file rather than to stdout, +so there is nothing to stream; the job handle carries the log path instead. The +other actions return their report directly, and any of them can be backgrounded +with `run_in_background: true` — the same parameter name `dsh-tool-bash` uses. + +That asymmetry is deliberate. A completion notice is best-effort: a **killed** job +deliberately produces none, and an agent whose turn has already ended may have the +notice injected rather than be woken for it. So the action that returns a report +the caller can use immediately is worth more than a job id it has to remember to +collect — and only the genuinely long step depends on being watched. ## Where things live diff --git a/index.js b/index.js index f47b95e..c1d5cbf 100644 --- a/index.js +++ b/index.js @@ -70,7 +70,16 @@ const SKILL_NAMES = ['slopo-review', 'slopo-analyze-ignore', 'slopo-analyze-one' const SKILLS_DIR = fileURLToPath(new URL('./skills/', import.meta.url)) /** Long-running actions, in the order the tool description advertises them. */ -const BACKGROUND_ACTIONS = new Set(['prepare', 'review', 'analyze', 'analyze-one']) +/** + * Actions that default to a background job. + * + * Only the warm-up. `index` plus `embed` is the long, watchable step — the one + * the user asked to be able to watch — while a review finds a warm index and + * returns its report directly. Named after dsh-tool-bash's `run_in_background`, + * because a model's prior is that backgrounding is something it ASKS for rather + * than the default. + */ +const BACKGROUND_BY_DEFAULT = new Set(['prepare']) /** Every configuration field this plugin exposes. */ const SETTING_KEYS = [ @@ -386,9 +395,9 @@ export function apply(ctx, config) { 'dsh-slopo settings; "prepare" runs the local index and then embeds (the slow, possibly costly ' + 'step); "review" finds similar code involving Git changes; "analyze" triages the whole codebase; ' + '"analyze-one" reports a single cluster by hash; "update" upgrades the slopo CLI. Slopo is ' + - 'installed and driven as a separate process. Long actions run as background jobs because ' + - 'embedding a repository takes minutes and slopo writes its progress to a log file rather than ' + - 'to stdout.', + 'installed and driven as a separate process. "prepare" starts a background job by default, because ' + + 'indexing and embedding a repository takes minutes and slopo writes its progress to a log file rather ' + + 'than to stdout; everything else returns its report directly.', parameters: { action: { type: 'string', @@ -398,9 +407,11 @@ export function apply(ctx, config) { }, base: { type: 'string', description: 'Git ref to compare against for "review". Defaults to HEAD.' }, cluster: { type: 'string', description: 'Cluster hash for "analyze-one". Omit for the next cluster.' }, - foreground: { + run_in_background: { type: 'boolean', - description: 'Run a long action inline instead of as a background job. Defaults to false.', + description: + 'Return a job id instead of the result, for a run long enough to watch. Defaults to true for ' + + '"prepare" (the warm-up) and false for everything else, which is quick once the index is warm.', }, config: { type: 'json', @@ -417,7 +428,9 @@ export function apply(ctx, config) { return [ { type: 'text', - text: `Started slopo ${value.action} as background job ${value.jobId}.${log} Read the job output when it settles.`, + text: + `Started slopo ${value.action} as background job ${value.jobId}.${log} You will be notified when ` + + `it settles; collect it with job_output for "${value.jobId}" (add wait: true if you are blocked on it).`, }, ] } @@ -740,7 +753,12 @@ export function apply(ctx, config) { details: { counted, excludes, maxIndexUnits: Number(settings.maxIndexUnits) }, }) - const wantsBackground = args.foreground !== true && BACKGROUND_ACTIONS.has(args.action) + // Background is for the long warm-up, not for every action. A completion + // notice is best-effort: a KILLED job deliberately produces none, and an + // agent whose turn has ended may have the message injected rather than be + // woken for it. A review that returns its report directly is worth more + // than a job id the caller has to remember to collect. + const wantsBackground = args.run_in_background ?? BACKGROUND_BY_DEFAULT.has(args.action) const jobs = ctx.get('jobs') if (wantsBackground && jobs !== undefined) { diff --git a/test/plugin.test.js b/test/plugin.test.js index ce95fc0..d3a11de 100644 --- a/test/plugin.test.js +++ b/test/plugin.test.js @@ -271,7 +271,7 @@ test('review writes a config the plugin owns and passes the key only in the envi embeddingApiKey: 'super-secret', }) const tool = state.tools.get('slopo') - const value = await tool.execute({ action: 'review', foreground: true }, execContext('/tmp/nonexistent-repo-xyz')) + const value = await tool.execute({ action: 'review' }, execContext('/tmp/nonexistent-repo-xyz')) // It will fail — there is no git repo there — but the failure must be a // reported one, and the config must have been materialised first. assert.equal(value.kind, 'foreground') @@ -282,27 +282,43 @@ test('review writes a config the plugin owns and passes the key only in the envi assert.doesNotMatch(written, /super-secret/) }) -test('a background-capable profile gets a job handle instead of a blocking call', async () => { +test('only the warm-up backgrounds by default; the query actions return directly', async () => { const started = [] + const ran = [] const { state } = harness( { embeddingModel: 'ollama/example:latest', embeddingDimensions: 768 }, - { withJobs: true }, + { withJobs: true, shellRun: cannedShell(ran, { indexOutput: 'Indexed 1519 code units from 165 files.' }) }, ) // ctx.get('jobs') is consulted at call time, not at apply time, so the fake // registry can be installed after the plugin has loaded. state.jobs = { start(spec) { started.push(spec) - return 'slopo-1' + return `slopo-${started.length}` }, } const tool = state.tools.get('slopo') - const value = await tool.execute({ action: 'review' }, execContext('/tmp')) - assert.equal(value.kind, 'background') - assert.equal(value.jobId, 'slopo-1') + + // A review returns its report. A completion notice is best-effort — a killed + // job produces none, and an ended turn may only get the message injected — so + // the query actions must not depend on one. + const reviewed = await tool.execute({ action: 'review' }, execContext('/tmp')) + assert.equal(reviewed.kind, 'foreground') + assert.match(reviewed.report, /cluster 1/) + assert.equal(started.length, 0, 'a review must not become a job the caller has to collect') + + // prepare is the long, watchable step, so it does. + const prepared = await tool.execute({ action: 'prepare' }, execContext('/tmp')) + assert.equal(prepared.kind, 'background') + assert.equal(prepared.jobId, 'slopo-1') assert.equal(started.length, 1) assert.equal(started[0].kind, 'slopo') assert.equal(typeof started[0].run, 'function') + + // And either can be asked for explicitly, under the framework's own name. + const forced = await tool.execute({ action: 'analyze', run_in_background: true }, execContext('/tmp')) + assert.equal(forced.kind, 'background') + assert.equal(started.length, 2) }) test( @@ -337,10 +353,10 @@ test( }) const tool = state.tools.get('slopo') - const prepared = await tool.execute({ action: 'prepare', foreground: true }, execContext(repo)) + const prepared = await tool.execute({ action: 'prepare', run_in_background: false }, execContext(repo)) assert.equal(prepared.ok, true, JSON.stringify(prepared)) - const reviewed = await tool.execute({ action: 'review', foreground: true }, execContext(repo)) + const reviewed = await tool.execute({ action: 'review' }, execContext(repo)) assert.equal(reviewed.ok, true, JSON.stringify(reviewed)) assert.match(reviewed.report, /cluster 1/) assert.match(reviewed.report, /\* /) @@ -366,7 +382,7 @@ test('the sandbox policy is resolved for the CALLING SESSION, not the deployment }, ) const tool = state.tools.get('slopo') - await tool.execute({ action: 'review', foreground: true }, execContext('/tmp/nonexistent-repo-xyz')) + await tool.execute({ action: 'review' }, execContext('/tmp/nonexistent-repo-xyz')) assert.equal(seen.length, 1, 'the policy must be resolved exactly once') assert.ok(seen[0].session !== undefined, 'the calling session must travel with the policy') assert.equal(state.shellRequests.length, 1) @@ -420,7 +436,7 @@ test('a workspace-write session with stateDir inside the workspace proceeds', as }, ) const tool = state.tools.get('slopo') - const value = await tool.execute({ action: 'review', foreground: true }, execContext(workspace)) + const value = await tool.execute({ action: 'review' }, execContext(workspace)) assert.notEqual(value.errorCode, 'STATE_OUTSIDE_SANDBOX') assert.ok(state.shellRequests.length > 0, 'it should have attempted the run') assert.deepEqual(state.shellRequests[0].sandboxPolicy, { mode: 'workspace-write', workspaceRoot: workspace }) @@ -565,7 +581,7 @@ test('a scope over the budget is refused BEFORE anything is embedded', async () { embeddingModel: 'ollama/example:latest', embeddingDimensions: 768 }, { shellRun: cannedShell(ran, { indexOutput: 'Indexed 113768 code units from 1300 files (0 unchanged, 0 removed).' }) }, ) - const value = await state.tools.get('slopo').execute({ action: 'review', foreground: true }, execContext('/tmp')) + const value = await state.tools.get('slopo').execute({ action: 'review' }, execContext('/tmp')) assert.equal(value.ok, false) assert.equal(value.errorCode, 'SCOPE_TOO_LARGE') @@ -587,7 +603,7 @@ test('a scope under the budget indexes and then runs the action', async () => { { embeddingModel: 'ollama/example:latest', embeddingDimensions: 768 }, { shellRun: cannedShell(ran, { indexOutput: 'Indexed 1519 code units from 165 files (0 unchanged, 0 removed).' }) }, ) - const value = await state.tools.get('slopo').execute({ action: 'review', foreground: true }, execContext('/tmp')) + const value = await state.tools.get('slopo').execute({ action: 'review' }, execContext('/tmp')) assert.equal(value.ok, true) assert.match(value.report, /cluster 1/) assert.equal(ran.length, 2) @@ -601,7 +617,7 @@ test('raising the budget lets the same scope through', async () => { { embeddingModel: 'ollama/example:latest', embeddingDimensions: 768, maxIndexUnits: 0 }, { shellRun: cannedShell(ran, { indexOutput: 'Indexed 113768 code units from 1300 files.' }) }, ) - const value = await state.tools.get('slopo').execute({ action: 'review', foreground: true }, execContext('/tmp')) + const value = await state.tools.get('slopo').execute({ action: 'review' }, execContext('/tmp')) assert.equal(value.ok, true) assert.equal(ran.length, 2) }) @@ -616,7 +632,7 @@ test('the config slopo is handed excludes the defaults, and a user list is added }, { shellRun: cannedShell(ran, { indexOutput: 'Indexed 1519 code units from 165 files.' }) }, ) - const value = await state.tools.get('slopo').execute({ action: 'review', foreground: true }, execContext('/tmp')) + const value = await state.tools.get('slopo').execute({ action: 'review' }, execContext('/tmp')) const { readFileSync } = await import('node:fs') const written = readFileSync(value.configPath, 'utf8') for (const pattern of ['**/node_modules/**', 'ui/**', 'src/controllers/**']) { @@ -632,7 +648,7 @@ test('slopo never sees a directory it should not, even when it is asked twice', { shellRun: cannedShell(ran, { indexOutput: 'Indexed 1519 code units from 165 files.' }) }, ) const tool = state.tools.get('slopo') - await tool.execute({ action: 'prepare', foreground: true }, execContext('/tmp')) + await tool.execute({ action: 'prepare', run_in_background: false }, execContext('/tmp')) const afterPrepare = ran.length assert.match(ran[0], /\bindex\b/) assert.match(ran[1], /\bembed\b/) @@ -680,7 +696,7 @@ test('the gate reads the DATABASE, so a warm index cannot hide a full backlog', { embeddingModel: 'ollama/example:latest', embeddingDimensions: 768, stateDir }, { shellRun: cannedShell(ran, { indexOutput: 'Indexed 0 code units from 0 files (562 unchanged, 0 removed).' }) }, ) - const value = await state.tools.get('slopo').execute({ action: 'prepare', foreground: true }, execContext('/tmp')) + const value = await state.tools.get('slopo').execute({ action: 'prepare', run_in_background: false }, execContext('/tmp')) assert.equal(value.errorCode, 'SCOPE_TOO_LARGE', 'the summary line said zero, the database said 8,607') assert.equal(value.details.counted.units, 8607) @@ -714,7 +730,7 @@ test('a fully embedded index has no backlog and is never refused', async () => { { embeddingModel: 'ollama/example:latest', embeddingDimensions: 768, stateDir }, { shellRun: cannedShell(ran, { indexOutput: 'Indexed 0 code units from 0 files (562 unchanged, 0 removed).' }) }, ) - const value = await state.tools.get('slopo').execute({ action: 'review', foreground: true }, execContext('/tmp')) + const value = await state.tools.get('slopo').execute({ action: 'review' }, execContext('/tmp')) assert.equal(value.ok, true, JSON.stringify(value)) assert.equal(ran.length, 2, 'a done index goes straight to the action') })