Fix: the tool substituted the deployment sandbox default for the session's
dsh-bash-sandbox is the composed ctx.shell, and its resolve() stamps `request.sandboxPolicy ?? ctx.sandboxPolicy.resolve()`. Omitting the policy therefore does NOT mean "unconfined" -- it silently substitutes the DEPLOYMENT default (`DSH_PERMISSION_MODE ?? 'workspace-write'`) for the session's preset. On a host with no sandbox runner that confines a danger-full-access session and fails with SandboxUnavailableError before slopo is ever reached, which is exactly what happened: the tool was unusable while bash worked. Resolve the calling session's policy the way dsh-tool-bash does, and pass it on every call. A confining executor with no ctx.sandboxPolicy is now refused outright rather than guessed at. That fix exposes a second problem worth naming. Slopo's state lives under $DSH_HOME, which is outside the workspace root a workspace-write sandbox permits writing to, so under that preset nothing would work. The tool now refuses before running and names both ways out -- run under danger-full-access, or set stateDir to a directory inside the workspace -- instead of letting a write fail halfway through an embed. Five tests pin this: the session's policy is resolved exactly once and travels with the session, a missing policy service is refused, state outside the sandbox is refused before any execution, state inside it proceeds, and status/configure never touch the shell at all.
This commit is contained in:
1 parent
a8e577ae38
commit
e758a723e2
3 files changed
+177
-4
No files matched your search
@@ -190,6 +190,21 @@ embedding failure — onto a message and a next step.
|
|||||||
**`--config-version 1` is an alarm, not boilerplate.** It makes slopo fail if its
|
**`--config-version 1` is an alarm, not boilerplate.** It makes slopo fail if its
|
||||||
agent-facing contract has moved on, which is the signal that the skill text in
|
agent-facing contract has moved on, which is the signal that the skill text in
|
||||||
`skills/` needs re-porting from upstream.
|
`skills/` needs re-porting from upstream.
|
||||||
|
**The sandbox can block slopo's state.** Slopo's index, config and log live
|
||||||
|
under `$DSH_HOME`, which is outside the workspace that a `workspace-write`
|
||||||
|
sandbox permits writing to. The tool detects that and refuses *before* running,
|
||||||
|
naming both ways out, rather than letting a write fail halfway through an embed:
|
||||||
|
run the session under the `danger-full-access` preset, or set `stateDir` to a
|
||||||
|
directory inside the workspace.
|
||||||
|
|
||||||
|
**What the tool does with the sandbox policy, and why it matters.** The mounted
|
||||||
|
`ctx.shell` on a confined deployment is `dsh-bash-sandbox`, whose `resolve()`
|
||||||
|
stamps `request.sandboxPolicy ?? ctx.sandboxPolicy.resolve()`. Omitting the
|
||||||
|
policy therefore does *not* mean "unconfined" — it silently substitutes the
|
||||||
|
**deployment** default (`DSH_PERMISSION_MODE ?? 'workspace-write'`), which is not
|
||||||
|
the session's preset. That is what makes a `danger-full-access` session fail with
|
||||||
|
`SandboxUnavailableError` on a host with no sandbox runner. The tool resolves the
|
||||||
|
session's policy the way `dsh-tool-bash` does and passes it per call.
|
||||||
|
|
||||||
## Slopo is an external process, not a library
|
## Slopo is an external process, not a library
|
||||||
|
|
||||||
|
|||||||
@@ -26,7 +26,7 @@
|
|||||||
|
|
||||||
import { existsSync } from 'node:fs'
|
import { existsSync } from 'node:fs'
|
||||||
import { mkdir, readFile, writeFile } from 'node:fs/promises'
|
import { mkdir, readFile, writeFile } from 'node:fs/promises'
|
||||||
import { join } from 'node:path'
|
import { join, sep } from 'node:path'
|
||||||
import { fileURLToPath } from 'node:url'
|
import { fileURLToPath } from 'node:url'
|
||||||
|
|
||||||
import { defineTool } from '@deepseek-ai/dsh-tools'
|
import { defineTool } from '@deepseek-ai/dsh-tools'
|
||||||
@@ -497,6 +497,50 @@ export function apply(ctx, config) {
|
|||||||
})
|
})
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// Resolve the CALLING SESSION's sandbox policy, the way the bash tool
|
||||||
|
// does. `dsh-bash-sandbox.resolve()` stamps
|
||||||
|
// `request.sandboxPolicy ?? ctx.sandboxPolicy.resolve()`, so omitting it
|
||||||
|
// does not mean "unconfined" — it silently substitutes the DEPLOYMENT
|
||||||
|
// default (`DSH_PERMISSION_MODE ?? 'workspace-write'`) for the session's
|
||||||
|
// preset. That confines a call the session asked to run unconfined, and
|
||||||
|
// on a host with no sandbox runner it fails with SandboxUnavailableError
|
||||||
|
// before slopo is ever reached.
|
||||||
|
const sandboxDefault = shell.sandboxMode
|
||||||
|
const policyService = sandboxDefault === undefined ? undefined : ctx.get('sandboxPolicy')
|
||||||
|
if (sandboxDefault !== undefined && policyService === undefined) {
|
||||||
|
return foreground({
|
||||||
|
ok: false,
|
||||||
|
exitCode: null,
|
||||||
|
errorCode: 'NO_SANDBOX_POLICY',
|
||||||
|
message: 'The mounted shell executor confines commands, but ctx.sandboxPolicy is absent.',
|
||||||
|
remedy: 'Compose @deepseek-ai/dsh-sandbox-policy, or mount an unconfining shell executor.',
|
||||||
|
})
|
||||||
|
}
|
||||||
|
const sandboxPolicy = policyService?.resolve(
|
||||||
|
exec.agent === undefined ? {} : { session: exec.agent.session },
|
||||||
|
)
|
||||||
|
|
||||||
|
// Slopo's state is deliberately outside the workspace, which a
|
||||||
|
// workspace-write sandbox forbids writing to. Say so before a run and
|
||||||
|
// name both ways out, rather than letting a write fail mid-embed.
|
||||||
|
if (
|
||||||
|
sandboxPolicy !== undefined &&
|
||||||
|
sandboxPolicy.mode !== 'danger-full-access' &&
|
||||||
|
!isInside(sandboxPolicy.workspaceRoot, paths.stateDir)
|
||||||
|
) {
|
||||||
|
return foreground({
|
||||||
|
ok: false,
|
||||||
|
exitCode: null,
|
||||||
|
errorCode: 'STATE_OUTSIDE_SANDBOX',
|
||||||
|
message:
|
||||||
|
`This session runs under the "${sandboxPolicy.mode}" sandbox, which permits writes only under ` +
|
||||||
|
`${sandboxPolicy.workspaceRoot} — but slopo's index, config and log live in ${paths.stateDir}.`,
|
||||||
|
remedy:
|
||||||
|
'Either run this session under the danger-full-access preset, or set `stateDir` to a directory ' +
|
||||||
|
'inside the workspace using action "configure".',
|
||||||
|
})
|
||||||
|
}
|
||||||
|
|
||||||
/** Run one command through the shell seam. */
|
/** Run one command through the shell seam. */
|
||||||
const run = async (command, { env, timeoutMs, signal } = {}) => {
|
const run = async (command, { env, timeoutMs, signal } = {}) => {
|
||||||
const result = await shell.run(
|
const result = await shell.run(
|
||||||
@@ -507,6 +551,8 @@ export function apply(ctx, config) {
|
|||||||
...(signal === undefined ? {} : { signal }),
|
...(signal === undefined ? {} : { signal }),
|
||||||
// Merges over a parent-derived base, so PATH and HOME survive.
|
// Merges over a parent-derived base, so PATH and HOME survive.
|
||||||
...(env === undefined ? {} : { env }),
|
...(env === undefined ? {} : { env }),
|
||||||
|
// Omitting this substitutes the deployment default; see above.
|
||||||
|
...(sandboxPolicy === undefined ? {} : { sandboxPolicy }),
|
||||||
}),
|
}),
|
||||||
)
|
)
|
||||||
return {
|
return {
|
||||||
@@ -626,3 +672,9 @@ async function readLogTail(path) {
|
|||||||
return ''
|
return ''
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/** True when `path` is `root` itself or sits underneath it. */
|
||||||
|
function isInside(root, path) {
|
||||||
|
const prefix = root.endsWith(sep) ? root : root + sep
|
||||||
|
return path === root || path.startsWith(prefix)
|
||||||
|
}
|
||||||
+109
-3
@@ -76,15 +76,34 @@ const shell = {
|
|||||||
* `jobs` is deliberately absent, which forces the tool down its foreground path
|
* `jobs` is deliberately absent, which forces the tool down its foreground path
|
||||||
* so the assertions can see a result rather than a job handle.
|
* so the assertions can see a result rather than a job handle.
|
||||||
*/
|
*/
|
||||||
function harness(overrides = {}, { withSettings = true, withJobs = false } = {}) {
|
function harness(overrides = {}, { withSettings = true, withJobs = false, sandboxMode, sandboxPolicyResolver } = {}) {
|
||||||
const config = { ...FULL_CONFIG, ...overrides }
|
const config = { ...FULL_CONFIG, ...overrides }
|
||||||
const state = { tools: new Map(), updates: [], provider: undefined, namespace: undefined, shell, jobs: undefined }
|
// A per-harness shell, so a test can vary sandboxMode and every resolved
|
||||||
|
// request is recorded for assertion.
|
||||||
|
const scopedShell = {
|
||||||
|
...shell,
|
||||||
|
sandboxMode,
|
||||||
|
resolve(request) {
|
||||||
|
state.shellRequests.push(request)
|
||||||
|
return { workdir: process.cwd(), timeoutMs: 600_000, stdoutMaxBytes: 10_000_000, ...request }
|
||||||
|
},
|
||||||
|
}
|
||||||
|
const state = {
|
||||||
|
tools: new Map(),
|
||||||
|
updates: [],
|
||||||
|
provider: undefined,
|
||||||
|
namespace: undefined,
|
||||||
|
shell: scopedShell,
|
||||||
|
shellRequests: [],
|
||||||
|
jobs: undefined,
|
||||||
|
}
|
||||||
|
|
||||||
const ctx = {
|
const ctx = {
|
||||||
logger: () => ({ info() {}, warn() {}, debug() {}, error() {} }),
|
logger: () => ({ info() {}, warn() {}, debug() {}, error() {} }),
|
||||||
get(name) {
|
get(name) {
|
||||||
if (name === 'shell') return shell
|
if (name === 'shell') return scopedShell
|
||||||
if (name === 'jobs') return withJobs ? state.jobs : undefined
|
if (name === 'jobs') return withJobs ? state.jobs : undefined
|
||||||
|
if (name === 'sandboxPolicy') return sandboxPolicyResolver
|
||||||
return undefined
|
return undefined
|
||||||
},
|
},
|
||||||
inject(deps, callback) {
|
inject(deps, callback) {
|
||||||
@@ -323,3 +342,90 @@ test(
|
|||||||
assert.match(reviewed.report, /\* /)
|
assert.match(reviewed.report, /\* /)
|
||||||
},
|
},
|
||||||
)
|
)
|
||||||
|
|
||||||
|
test('the sandbox policy is resolved for the CALLING SESSION, not the deployment default', async () => {
|
||||||
|
// dsh-bash-sandbox stamps `request.sandboxPolicy ?? ctx.sandboxPolicy.resolve()`.
|
||||||
|
// Omitting it substitutes the DEPLOYMENT default, which is what made a
|
||||||
|
// danger-full-access session fail with SandboxUnavailableError before slopo
|
||||||
|
// was ever reached.
|
||||||
|
const seen = []
|
||||||
|
const { state } = harness(
|
||||||
|
{ embeddingModel: 'ollama/example:latest', embeddingDimensions: 768 },
|
||||||
|
{
|
||||||
|
sandboxMode: 'workspace-write',
|
||||||
|
sandboxPolicyResolver: {
|
||||||
|
resolve(arg) {
|
||||||
|
seen.push(arg)
|
||||||
|
return { mode: 'danger-full-access', workspaceRoot: '/some/workspace' }
|
||||||
|
},
|
||||||
|
},
|
||||||
|
},
|
||||||
|
)
|
||||||
|
const tool = state.tools.get('slopo')
|
||||||
|
await tool.execute({ action: 'review', foreground: true }, 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)
|
||||||
|
assert.deepEqual(state.shellRequests[0].sandboxPolicy, {
|
||||||
|
mode: 'danger-full-access',
|
||||||
|
workspaceRoot: '/some/workspace',
|
||||||
|
})
|
||||||
|
})
|
||||||
|
|
||||||
|
test('a confining executor with no policy service is refused rather than guessed at', async () => {
|
||||||
|
const { state } = harness(
|
||||||
|
{ embeddingModel: 'ollama/example:latest', embeddingDimensions: 768 },
|
||||||
|
{ sandboxMode: 'workspace-write' },
|
||||||
|
)
|
||||||
|
const tool = state.tools.get('slopo')
|
||||||
|
const value = await tool.execute({ action: 'review' }, execContext('/tmp'))
|
||||||
|
assert.equal(value.ok, false)
|
||||||
|
assert.equal(value.errorCode, 'NO_SANDBOX_POLICY')
|
||||||
|
assert.deepEqual(state.shellRequests, [], 'nothing may be executed')
|
||||||
|
})
|
||||||
|
|
||||||
|
test('a workspace-write session is told its state lives outside the sandbox', async () => {
|
||||||
|
const { state } = harness(
|
||||||
|
{ embeddingModel: 'ollama/example:latest', embeddingDimensions: 768 },
|
||||||
|
{
|
||||||
|
sandboxMode: 'workspace-write',
|
||||||
|
sandboxPolicyResolver: { resolve: () => ({ mode: 'workspace-write', workspaceRoot: '/some/workspace' }) },
|
||||||
|
},
|
||||||
|
)
|
||||||
|
const tool = state.tools.get('slopo')
|
||||||
|
const value = await tool.execute({ action: 'review' }, execContext('/some/workspace'))
|
||||||
|
assert.equal(value.ok, false)
|
||||||
|
assert.equal(value.errorCode, 'STATE_OUTSIDE_SANDBOX')
|
||||||
|
assert.match(value.message, /workspace-write/)
|
||||||
|
assert.match(value.remedy, /danger-full-access/)
|
||||||
|
assert.match(value.remedy, /stateDir/)
|
||||||
|
assert.deepEqual(state.shellRequests, [], 'it must not run where it cannot write')
|
||||||
|
})
|
||||||
|
|
||||||
|
test('a workspace-write session with stateDir inside the workspace proceeds', async () => {
|
||||||
|
const workspace = mkdtempSync(join(tmpdir(), 'dsh-slopo-ws-'))
|
||||||
|
const { state } = harness(
|
||||||
|
{
|
||||||
|
embeddingModel: 'ollama/example:latest',
|
||||||
|
embeddingDimensions: 768,
|
||||||
|
stateDir: join(workspace, '.slopo'),
|
||||||
|
},
|
||||||
|
{
|
||||||
|
sandboxMode: 'workspace-write',
|
||||||
|
sandboxPolicyResolver: { resolve: () => ({ mode: 'workspace-write', workspaceRoot: workspace }) },
|
||||||
|
},
|
||||||
|
)
|
||||||
|
const tool = state.tools.get('slopo')
|
||||||
|
const value = await tool.execute({ action: 'review', foreground: true }, 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 })
|
||||||
|
})
|
||||||
|
|
||||||
|
test('configure and status never touch the shell, so they work under any sandbox', async () => {
|
||||||
|
const { state } = harness({}, { sandboxMode: 'workspace-write' })
|
||||||
|
const tool = state.tools.get('slopo')
|
||||||
|
await tool.execute({ action: 'status' }, execContext('/some/workspace'))
|
||||||
|
await tool.execute({ action: 'configure', config: { embeddingModel: 'x' } }, execContext('/some/workspace'))
|
||||||
|
assert.deepEqual(state.shellRequests, [])
|
||||||
|
})
|
||||||
Reference in new issue
Block a user