Background only the warm-up, and use the framework's own parameter name
The tool backgrounded EVERY long action and exposed an inverted `foreground` flag. dsh-tool-bash does neither: its parameter is `run_in_background`, it defaults to foreground, and the model opts in. That difference is what made a finished job go uncollected. The evidence, from this session's own log: the only slopo job that ever ran was slopo-1, and the agent KILLED it — `kill()` sets `reported = true`, which suppresses the completion notice by design, because a caller who asked to stop something does not want to be told it stopped. The agent then worked around the tool entirely and re-ran slopo through bash, which is where the six notices in that session came from. Nothing was broken; the tool was asking the caller to depend on a best-effort signal. So the query actions return their report directly. Only `prepare` — index plus embed, the genuinely long and watchable step, and the one the user asked to be able to watch — backgrounds by default. Any action can still be backgrounded with `run_in_background: true`. The render now names the collect call explicitly (`job_output` for that id, with `wait: true` when blocked on it) instead of the vaguer "read the job output when it settles", so a caller that is not woken still has an unambiguous next step. I checked the waiter path while I was in there: a timed-out `wait` decrements `waiters` in a `finally`, so it does not leak and does not suppress the notice. That was my first hypothesis and it is wrong — worth recording, since it was the plausible one.
This commit is contained in:
1 parent
2d7a7d1e73
commit
5ff10eaefd
3 files changed
+71
-30
No files matched your search
+34
-18
@@ -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')
|
||||
})
|
||||
Reference in new issue
Block a user