fix!: port the settings page and shell seam to dsh 0.1.7-rc.2
Release / release (push) Successful in 6s
Release / release (push) Successful in 6s
0.1.7-rc.2 moved four things this bundle was built on, and every one of them
failed quietly rather than loudly, so the plugin looked installed and did
nothing:
* `ctx.settings.installSection(...)` is gone. The call threw into a
`try/catch` that only logged a warning, so the settings namespace was never
registered and `configure` had nowhere to persist. An entry's exported
Config IS the form now; the namespace is the Loader entry id the bundle's
patch declares, and `ctx.settings.configure({ auto: false })` is what claims
that entry's own page.
* `settingsScope` and the `settings.plugin.item` slot are gone. The card
registered into both, so it never rendered — the tab draws the intersection
of served namespaces and registered cards, and that intersection was empty.
It now registers into `plugins.bundle.config` keyed by the bundle's package
name, over the `configForms` service, and uses the framework's own form
chrome (`SettingsFormModel` / `SettingsForm` / `SettingsValueField` /
`SettingsSecretField`) instead of the hand-rolled equivalent. A save is one
revision-fenced write.
* `ctx.shell.run(spec)` is gone. Every action that runs a command threw
`shell.run is not a function`; the seam is now
`await ctx.shell.execute(ctx.shell.resolve(request)).result()`.
* A job's `owner` is a session id, not the agent object. The wrong owner
registered a job no session could collect or cancel, and the producer's
`done` resolved `output` where the registry reads `result`, so a finished
job handed back nothing.
Two conformance gaps went with them: a Config field that is not marked
`volatile()` is invisible to every settings UI — the form projection keeps only
volatile fields, and an entry with no form is dropped from `settings.describe`
entirely — and `embeddingApiKey` was a plain field, so the key rode every
settings read back to the browser. It is `role('secret')` now: the browser is
told whether a key is stored and never receives the key.
Also declares the `@deepseek-ai/dsh-*` peerDependencies the compatibility gate
reads, so an incompatible runtime refuses this plugin at startup instead of
half-loading it.
Verified against a real 0.1.7-rc.2 boot: `settings.describe` serves the entry
with `autoGenerate: false`, the key redacted and reported only as `set: true`,
and no startup diagnostics. The host mock in the tests now has no `run` and no
`installSection`, so those seams are contract-tested rather than assumed, and
the browser tests read the installed harness's own slot catalog and package
exports — a rename fails a test instead of quietly emptying a page.
This commit is contained in:
1 parent
ffefc42e01
commit
1b2ab3613f
8 files changed
+1549
-689
No files matched your search
+705
-241
File diff suppressed because it is too large.
Load diff
+163
-30
@@ -9,7 +9,7 @@
|
||||
|
||||
import assert from 'node:assert/strict'
|
||||
import { exec } from 'node:child_process'
|
||||
import { existsSync, mkdtempSync, writeFileSync } from 'node:fs'
|
||||
import { existsSync, mkdtempSync, readFileSync, writeFileSync } from 'node:fs'
|
||||
import { tmpdir } from 'node:os'
|
||||
import { join } from 'node:path'
|
||||
import { test } from 'node:test'
|
||||
@@ -39,7 +39,16 @@ const FULL_CONFIG = {
|
||||
maxIndexUnits: 5000,
|
||||
}
|
||||
|
||||
/** A shell executor that really runs the command. */
|
||||
/**
|
||||
* A shell executor shaped like `ShellExecutor` in dsh 0.1.7-rc.2: `resolve`,
|
||||
* then `execute(spec)` returning a process handle whose `result()` is the
|
||||
* foreground projection.
|
||||
*
|
||||
* There is deliberately no `run(spec)`: that single-step method was removed from
|
||||
* the seam, and a plugin that still calls it fails with
|
||||
* `shell.run is not a function` the first time it needs a command. Keeping the
|
||||
* mock honest about the shape is the point — this file is the contract test.
|
||||
*/
|
||||
const shell = {
|
||||
sandboxMode: undefined,
|
||||
resolve: (request) => ({
|
||||
@@ -48,8 +57,8 @@ const shell = {
|
||||
stdoutMaxBytes: 10_000_000,
|
||||
...request,
|
||||
}),
|
||||
run(spec) {
|
||||
return new Promise((resolve) => {
|
||||
async execute(spec) {
|
||||
const result = await new Promise((resolve) => {
|
||||
exec(
|
||||
spec.command,
|
||||
{ cwd: spec.workdir, env: { ...process.env, ...(spec.env ?? {}) }, timeout: spec.timeoutMs, maxBuffer: 64 * 1024 * 1024 },
|
||||
@@ -66,12 +75,27 @@ const shell = {
|
||||
},
|
||||
)
|
||||
})
|
||||
return { ...result, result: async () => result }
|
||||
},
|
||||
start() {
|
||||
throw new Error('background process start is not used by these tests')
|
||||
},
|
||||
}
|
||||
|
||||
/**
|
||||
* A stub slopo executable.
|
||||
*
|
||||
* Discovery itself is `test/slopo.test.js`'s business. Here the tool must never
|
||||
* reach `uv tool install`: that is a network round trip, it makes the suite
|
||||
* slow and flaky, and a canned runner would answer it with success without
|
||||
* installing anything — leaving the plugin to report SLOPO_INSTALL_MISSING.
|
||||
* Pointing `binary` at a file that exists takes the "already installed" branch,
|
||||
* which is the state every one of these tests means to exercise. A test that
|
||||
* wants the missing-executable path passes `binary: ''` and `autoInstall: false`.
|
||||
*/
|
||||
const stubSlopo = join(mkdtempSync(join(tmpdir(), 'dsh-slopo-bin-')), 'slopo')
|
||||
writeFileSync(stubSlopo, '#!/bin/sh\nexit 0\n', { mode: 0o755 })
|
||||
|
||||
/**
|
||||
* Build a context exposing exactly the services the plugin consumes.
|
||||
*
|
||||
@@ -79,7 +103,7 @@ const shell = {
|
||||
* so the assertions can see a result rather than a job handle.
|
||||
*/
|
||||
function harness(overrides = {}, { withSettings = true, withJobs = false, withShell = true, sandboxMode, sandboxPolicyResolver, shellRun } = {}) {
|
||||
const config = { ...FULL_CONFIG, ...overrides }
|
||||
const config = { ...FULL_CONFIG, binary: stubSlopo, ...overrides }
|
||||
// A per-harness shell, so a test can vary sandboxMode and every resolved
|
||||
// request is recorded for assertion.
|
||||
const scopedShell = {
|
||||
@@ -90,19 +114,28 @@ function harness(overrides = {}, { withSettings = true, withJobs = false, withSh
|
||||
return { workdir: process.cwd(), timeoutMs: 600_000, stdoutMaxBytes: 10_000_000, ...request }
|
||||
},
|
||||
// A canned runner lets a test drive the index gate without a real slopo.
|
||||
...(shellRun === undefined ? {} : { run: shellRun }),
|
||||
// It replaces only the low-level run, so the `execute().result()` frame
|
||||
// stays exactly the shape the real seam has.
|
||||
async execute(spec) {
|
||||
state.shellExecutions += 1
|
||||
const result = shellRun === undefined ? await shell.execute(spec).then((handle) => handle.result()) : await shellRun(spec)
|
||||
return { ...result, result: async () => result }
|
||||
},
|
||||
}
|
||||
const state = {
|
||||
tools: new Map(),
|
||||
updates: [],
|
||||
provider: undefined,
|
||||
namespace: undefined,
|
||||
pagePolicy: undefined,
|
||||
settingsEffects: 0,
|
||||
shell: scopedShell,
|
||||
shellRequests: [],
|
||||
shellExecutions: 0,
|
||||
jobs: undefined,
|
||||
}
|
||||
|
||||
const ctx = {
|
||||
fiber: { uid: 'test-fiber' },
|
||||
logger: () => ({ info() {}, warn() {}, debug() {}, error() {} }),
|
||||
get(name) {
|
||||
if (name === 'shell') return withShell ? scopedShell : undefined
|
||||
@@ -112,12 +145,22 @@ function harness(overrides = {}, { withSettings = true, withJobs = false, withSh
|
||||
},
|
||||
inject(deps, callback) {
|
||||
if (!withSettings || !deps.includes('settings')) return
|
||||
// The child context `ctx.inject` hands back: its own effect scope plus
|
||||
// the settings service. 0.1.7-rc.2 has no `installSection` — an entry's
|
||||
// exported Config IS the form — so the only things a plugin does here are
|
||||
// claim its page and write through `update`.
|
||||
callback({
|
||||
effect(effect) {
|
||||
state.settingsEffects += 1
|
||||
return effect()
|
||||
},
|
||||
settings: {
|
||||
installSection(owner, namespace, schema, entry, hooks) {
|
||||
state.namespace = namespace
|
||||
state.schema = schema
|
||||
hooks.setSource(() => config)
|
||||
configure(presentation, owner) {
|
||||
state.pagePolicy = presentation
|
||||
state.pagePolicyOwner = owner
|
||||
return () => {
|
||||
state.pagePolicy = undefined
|
||||
}
|
||||
},
|
||||
async update(namespace, patch) {
|
||||
state.updates.push({ namespace, patch })
|
||||
@@ -142,7 +185,7 @@ function harness(overrides = {}, { withSettings = true, withJobs = false, withSh
|
||||
return { config, state }
|
||||
}
|
||||
|
||||
const execContext = (cwd) => ({ signal: new AbortController().signal, agent: { session: { header: { cwd } } } })
|
||||
const execContext = (cwd) => ({ signal: new AbortController().signal, agent: { id: 'session-1', session: { header: { cwd } } } })
|
||||
|
||||
test('Config is a schemastery schema, which is what cordis requires', () => {
|
||||
assert.equal(typeof Config, 'function')
|
||||
@@ -192,9 +235,50 @@ test('every skill tells the agent to call the tool rather than a shell command',
|
||||
}
|
||||
})
|
||||
|
||||
test('the settings namespace is registered under a grammar-valid name', () => {
|
||||
test('the plugin claims its own settings page instead of a generated one', () => {
|
||||
// `auto: false` says "this entry ships a client page" — client/client.js
|
||||
// registers one onto the Plugins page. The owner must be the plugin's own
|
||||
// fiber, not the settings service's.
|
||||
const { state } = harness()
|
||||
assert.match(state.namespace, /^[a-z][a-z0-9-]*$/)
|
||||
assert.deepEqual(state.pagePolicy, { auto: false })
|
||||
assert.equal(state.pagePolicyOwner?.uid, 'test-fiber', 'the policy belongs to the plugin instance')
|
||||
assert.equal(state.settingsEffects, 1, 'the policy is registered as an effect, so a reload drops it')
|
||||
})
|
||||
|
||||
test('the namespace is the Loader entry id the bundle patch declares', async () => {
|
||||
const { state } = harness()
|
||||
await state.tools.get('slopo').execute({ action: 'configure', config: { embeddingModel: 'x' } }, execContext('/tmp'))
|
||||
assert.equal(state.updates[0].namespace, 'dsh-slopo')
|
||||
})
|
||||
|
||||
test('the host half no longer calls the removed installSection API', () => {
|
||||
// 0.1.7-rc.2 deleted `ctx.settings.installSection`; a plugin that still calls
|
||||
// it loses its whole settings surface, silently, behind a try/catch.
|
||||
const source = readFileSync(new URL('../index.js', import.meta.url), 'utf8')
|
||||
const code = source.replace(/\/\*[\s\S]*?\*\//g, '').replace(/^\s*\/\/.*$/gm, '')
|
||||
assert.doesNotMatch(code, /installSection/)
|
||||
})
|
||||
|
||||
test('every Config field is volatile, which is what puts it on the settings page', () => {
|
||||
// The framework's form projection keeps only fields under a volatile
|
||||
// ancestor, and an entry whose form is empty is dropped from
|
||||
// `settings.describe` — so an unmarked field is invisible to every UI.
|
||||
// The walk is over the live schema: `Config.toJSON()` is a reference
|
||||
// envelope for the wire, not a tree.
|
||||
const fields = Object.entries(Config.dict ?? {})
|
||||
assert.ok(fields.length > 0, 'the schema must declare fields')
|
||||
for (const [key, node] of fields) {
|
||||
assert.equal(node.meta?.volatile, true, `${key} must be marked volatile()`)
|
||||
}
|
||||
})
|
||||
|
||||
test('the embedding API key is a declared secret, so the settings wire never carries it', () => {
|
||||
assert.equal(Config.dict.embeddingApiKey.meta?.role, 'secret')
|
||||
// …and no other field claims to be one, because the browser cannot read a
|
||||
// secret back and every other field is one the card must display.
|
||||
for (const [key, node] of Object.entries(Config.dict)) {
|
||||
if (key !== 'embeddingApiKey') assert.notEqual(node.meta?.role, 'secret', `${key} must not be secret`)
|
||||
}
|
||||
})
|
||||
|
||||
test('status reports the missing settings instead of pretending to be ready', async () => {
|
||||
@@ -264,22 +348,35 @@ test('review refuses to run before the workspace is configured', async () => {
|
||||
})
|
||||
|
||||
test('review writes a config the plugin owns and passes the key only in the environment', async () => {
|
||||
const { state } = harness({
|
||||
embeddingModel: 'ollama/example:latest',
|
||||
embeddingDimensions: 768,
|
||||
embeddingApiBase: 'http://127.0.0.1:11434',
|
||||
embeddingApiKey: 'super-secret',
|
||||
})
|
||||
// Hermetic on purpose: `binary` points at a stub that exists, so the plugin
|
||||
// takes the "already installed" branch instead of running `uv tool install`
|
||||
// over the network, and the workspace is a real directory so the shell seam
|
||||
// has somewhere to run. The canned runner answers both commands.
|
||||
const workspace = mkdtempSync(join(tmpdir(), 'dsh-slopo-ws-'))
|
||||
const binary = join(workspace, 'slopo')
|
||||
writeFileSync(binary, '#!/bin/sh\nexit 0\n', { mode: 0o755 })
|
||||
const ran = []
|
||||
const { state } = harness(
|
||||
{
|
||||
binary,
|
||||
embeddingModel: 'ollama/example:latest',
|
||||
embeddingDimensions: 768,
|
||||
embeddingApiBase: 'http://127.0.0.1:11434',
|
||||
embeddingApiKey: 'super-secret',
|
||||
},
|
||||
{ shellRun: cannedShell(ran, { indexOutput: 'Indexed 12 code units from 3 files.' }) },
|
||||
)
|
||||
const tool = state.tools.get('slopo')
|
||||
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.
|
||||
const value = await tool.execute({ action: 'review' }, execContext(workspace))
|
||||
assert.equal(value.kind, 'foreground')
|
||||
assert.ok(existsSync(value.configPath), `expected config at ${value.configPath}`)
|
||||
const { readFileSync } = await import('node:fs')
|
||||
const written = readFileSync(value.configPath, 'utf8')
|
||||
assert.match(written, /embedding_model: "ollama\/example:latest"/)
|
||||
assert.doesNotMatch(written, /super-secret/)
|
||||
// The key rides the environment of the run, not the file.
|
||||
const executions = state.shellRequests.filter((request) => /agent-review/.test(request.command))
|
||||
assert.equal(executions.length, 1)
|
||||
assert.equal(executions[0].env.SLOPO_EMBEDDING_API_KEY, 'super-secret')
|
||||
})
|
||||
|
||||
test('only the warm-up backgrounds by default; the query actions return directly', async () => {
|
||||
@@ -321,6 +418,37 @@ test('only the warm-up backgrounds by default; the query actions return directly
|
||||
assert.equal(started.length, 2)
|
||||
})
|
||||
|
||||
test('a background job is owned by the SESSION ID and settles a JobOutcome', async () => {
|
||||
// Two things the 0.1.7-rc.2 job contract requires and the old code got wrong:
|
||||
// `owner` is a session id (the registry looks the agent up itself, and a job
|
||||
// owned by the wrong object can be neither collected nor cancelled), and the
|
||||
// producer's `done` resolves a JobOutcome whose payload field is `result` —
|
||||
// `output` is the ring's word, and a job that reports to `output` settles
|
||||
// with nothing for job_output to hand back.
|
||||
const started = []
|
||||
const ran = []
|
||||
const { state } = harness(
|
||||
{ embeddingModel: 'ollama/example:latest', embeddingDimensions: 768 },
|
||||
{ withJobs: true, shellRun: cannedShell(ran, { indexOutput: 'Indexed 5 code units from 1 files.' }) },
|
||||
)
|
||||
state.jobs = {
|
||||
start(spec) {
|
||||
started.push(spec)
|
||||
return 'slopo-1'
|
||||
},
|
||||
}
|
||||
const value = await state.tools.get('slopo').execute({ action: 'prepare' }, execContext('/tmp'))
|
||||
assert.equal(value.kind, 'background')
|
||||
assert.equal(started[0].owner, 'session-1', 'ownership is fenced by session id')
|
||||
|
||||
const hooks = started[0].run()
|
||||
assert.equal(typeof hooks.cancel, 'function')
|
||||
const outcome = await hooks.done
|
||||
assert.equal(outcome.status, 'completed')
|
||||
assert.equal(outcome.detail, 'exit code: 0')
|
||||
assert.equal(typeof outcome.result, 'string', 'the report rides `result`, not `output`')
|
||||
})
|
||||
|
||||
test(
|
||||
'review runs the real slopo CLI end to end against a real repository',
|
||||
{ skip: process.env.DSH_SLOPO_E2E !== '1' || findSlopo(testHome) === undefined },
|
||||
@@ -382,14 +510,19 @@ test('the sandbox policy is resolved for the CALLING SESSION, not the deployment
|
||||
},
|
||||
)
|
||||
const tool = state.tools.get('slopo')
|
||||
await tool.execute({ action: 'review' }, execContext('/tmp/nonexistent-repo-xyz'))
|
||||
await tool.execute({ action: 'review' }, execContext('/tmp'))
|
||||
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',
|
||||
})
|
||||
// Every command the run issued carries it — not just the first. The count is
|
||||
// not the assertion: how many commands a review issues depends on whether the
|
||||
// index gate let it through.
|
||||
assert.ok(state.shellRequests.length >= 1, 'the run must reach the shell')
|
||||
for (const request of state.shellRequests) {
|
||||
assert.deepEqual(request.sandboxPolicy, {
|
||||
mode: 'danger-full-access',
|
||||
workspaceRoot: '/some/workspace',
|
||||
})
|
||||
}
|
||||
})
|
||||
|
||||
test('a confining executor with no policy service is refused rather than guessed at', async () => {
|
||||
|
||||
Reference in new issue
Block a user