diff --git a/README.md b/README.md index 60d4b2a..c452318 100644 --- a/README.md +++ b/README.md @@ -294,6 +294,16 @@ invisible to every settings UI, and the `jobs` registry wants `exec.agent.id` (a session id) as its owner — handing it the agent object registered a job no session could collect. +`volatile()` cuts the other way as well, and this one is easy to miss because the +page still looks right. A volatile field resolves to a live *reference* rather +than to a value — that is what lets a save land without re-applying the plugin — +so `apply` receives an object where the code expects a string, a `typeof` check +falls through to its default, and the plugin runs on autodiscovery while the card +displays the settings that were ignored. This bundle reads its own settings +through `configValue`/`settingsSnapshot`, and the tests that matter resolve a +fixture through `Config['~standard'].validate` first, because a fixture built +from plain literals is the one shape a real profile never produces. + Two habits follow. First, this bundle's tests bind to the *installed* harness's own slot catalog and package exports, so a rename fails a test instead of quietly emptying a page. Second, the browser half keeps its slot and service diff --git a/index.js b/index.js index 26d985d..fd2328e 100644 --- a/index.js +++ b/index.js @@ -119,6 +119,14 @@ const SETTING_KEYS = [ * unmarked field would still be *configurable* through the profile patch — it * would simply be invisible to every settings UI. * + * `volatile()` has a second consequence, and it is the one that bites: the + * resolved config does not carry VALUES for these fields. Each arrives as a + * reference object with a `get()`, because that is what lets a settings write + * land in the running instance without re-applying the plugin. So a plain-value + * read (`typeof config.embeddingModel === 'string'`) is false for every field, + * and the plugin silently behaves as if nothing were configured. Every read + * goes through {@link configValue} or {@link settingsSnapshot}. + * * `embeddingApiKey` additionally carries `role('secret')`, so the settings * wire never carries it: every remote read is redacted (`secrets` reports only * whether a value is set) and the browser sends the key one way, on write. @@ -156,6 +164,40 @@ export const Config = Schema.object({ timeoutMs: Schema.number().default(600_000).volatile(), }) +/** + * Read one configuration field. + * + * A volatile field is a reference in the running config, not a plain value; a + * non-volatile one (or a config object a test handed straight to `apply`) is + * the value itself. Both are accepted so the same path serves a real profile + * and a direct call. Duck-typed on `get` rather than importing cosmokit's + * `isVolatile`, because the only thing this plugin needs to know about a + * reference is how to read it. + * + * @param {unknown} value a field of the resolved config + * @returns {unknown} the current value + */ +export function configValue(value) { + return value !== null && typeof value === 'object' && typeof value.get === 'function' ? value.get() : value +} + +/** + * A plain snapshot of the whole settings section. + * + * Taken per tool call rather than cached, because that is exactly what a + * volatile field buys: the running instance's references are written in place + * when the document changes, so the next snapshot sees the new value without + * the plugin being remounted. + * + * @param {Record} config the resolved composition config + * @returns {Record} every declared field, unwrapped + */ +export function settingsSnapshot(config) { + const snapshot = {} + for (const key of Object.keys(config ?? {})) snapshot[key] = configValue(config[key]) + return snapshot +} + /** Resolve the harness home, preferring the harness's own resolver. */ function dshHomeOf(ctx) { try { @@ -170,8 +212,16 @@ function dshHomeOf(ctx) { return resolveDshHome() } -/** Every path one workspace's slopo state occupies. */ -export function workspacePaths(dshHome, cwd, settings) { +/** + * Every path one workspace's slopo state occupies. + * + * `config` may be the live composition config — whose fields are all volatile + * references — or the plain snapshot a tool call already took of it. Both are + * accepted, because this is exported and the next caller cannot be assumed to + * know the difference; a plain value passes through `settingsSnapshot` intact. + */ +export function workspacePaths(dshHome, cwd, config) { + const settings = settingsSnapshot(config) const stateDir = typeof settings.stateDir === 'string' && settings.stateDir.trim() !== '' ? settings.stateDir.trim() @@ -220,9 +270,13 @@ function embeddingParams(settings, problems) { * The names differ on purpose: dsh settings are camelCase by convention while * slopo's config file is snake_case, and slopo treats an unknown key as a hard * error, so this mapping is the only place a name can go wrong. + * + * `config` is accepted in either shape for the same reason `workspacePaths` + * is: exported, so called with a live volatile config or with a plain one. */ -export function toSlopoConfig(settings, paths, problems = []) { - const config = { +export function toSlopoConfig(config, paths, problems = []) { + const settings = settingsSnapshot(config) + const mapped = { source_dir: paths.sourceDir, source_dir_exclude: effectiveExcludes(settings), db_file: paths.dbFile, @@ -236,7 +290,7 @@ export function toSlopoConfig(settings, paths, problems = []) { embedding_params: embeddingParams(settings, problems), embedding_request_delay: Number(settings.embeddingRequestDelay) || 0, } - const { config: normalized, problems: numericProblems } = normalizeConfig(config) + const { config: normalized, problems: numericProblems } = normalizeConfig(mapped) problems.push(...numericProblems) delete normalized.embedding_api_key return normalized @@ -347,17 +401,23 @@ export const inject = ['tools', 'skills'] * Register the skills, the settings namespace, and the tool. * * @param {import('@deepseek-ai/cordis').Context} ctx - * @param {Record} config validated composition config + * @param {Record} config validated composition config, whose + * fields are volatile references rather than values */ export function apply(ctx, config) { const logger = ctx.logger('dsh-slopo') const dshHome = dshHomeOf(ctx) - // `config` is the live configuration, not a snapshot of the composition - // layer: a settings write rewrites this entry's patch and the Loader - // re-applies the plugin with the new values, so every read below sees the - // deployment's current answer. There is deliberately no cached copy — a cache - // is exactly what a reload invalidates. + // The authoritative settings source: a plain snapshot of the running config, + // taken per call. + // + // `config` holds LIVE REFERENCES, not values. Every field is `volatile()`, so + // a settings write does NOT re-apply this plugin: the Loader writes the new + // value into the reference this instance already holds (`_commitVolatile`), + // and that is the whole point of the marker. It is also why nothing here is + // cached — a snapshot taken once at apply time would be frozen at the + // composition's values and would never see an edit. + const readSettings = () => settingsSnapshot(config) let writeSettings // The settings namespace IS this plugin's Config. The framework projects the @@ -481,7 +541,9 @@ export function apply(ctx, config) { }, async execute(args, exec) { const cwd = exec.agent?.session.header.cwd ?? process.cwd() - const settings = config + // Fresh per call, so a settings edit made while this tool is loaded is + // picked up by the very next call — see `readSettings` above. + const settings = readSettings() const paths = workspacePaths(dshHome, cwd, settings) const shell = ctx.get('shell') // Resolved once, up front, so `status` can probe without a second diff --git a/test/config.test.js b/test/config.test.js index cd58178..ff844f0 100644 --- a/test/config.test.js +++ b/test/config.test.js @@ -1,6 +1,7 @@ import assert from 'node:assert/strict' import { test } from 'node:test' +import { Config, configValue, settingsSnapshot, toSlopoConfig, workspacePaths } from '../index.js' import { SLOPO_CONFIG_KEYS, missingRequired, @@ -105,3 +106,95 @@ test('normalizeConfig keeps thresholds fractional but rejects fractional counts' assert.equal(Object.hasOwn(bad.config, 'embedding_batch_size'), false) assert.match(bad.problems[0], /whole number/) }) + +/** + * The other half of the config contract: the shape a section ARRIVES in. + * + * Every field of `Config` is `volatile()`, so Cordis hands `apply` a reference + * object per field rather than a value — that is what lets a settings edit land + * without re-applying the plugin. A plain-value read of such a field is silently + * false, which is how this plugin came to ignore its own settings and fall back + * to autodiscovery. These tests resolve through the real schema path, because a + * fixture built from plain literals can never catch that. + */ + +/** The section as the Loader builds it: schema-resolved, references intact. */ +function resolved(patch = {}) { + const result = Config['~standard'].validate(patch) + assert.equal(result.issues, undefined, `the schema refused the fixture: ${JSON.stringify(result.issues)}`) + return result.value +} + +test('a volatile field arrives as a reference, and configValue unwraps it', () => { + const config = resolved({ embeddingApiBase: 'http://192.0.2.10:11434' }) + assert.equal(typeof config.embeddingApiBase, 'object', 'a volatile field is a reference, not a value') + assert.equal(configValue(config.embeddingApiBase), 'http://192.0.2.10:11434') + // …and a plain value is handed straight back, so a caller that passes a + // literal object — every other test in this suite, and `apply` in a profile + // whose fields are not volatile — takes the very same code path. + assert.equal(configValue('http://plain'), 'http://plain') + assert.equal(configValue(undefined), undefined) + assert.equal(configValue(0), 0) +}) + +test('a snapshot reads the configured values out of a schema-resolved section', () => { + const settings = settingsSnapshot( + resolved({ + embeddingModel: 'ollama/example:latest', + embeddingDimensions: 768, + embeddingRequestDelay: 25, + embeddingParamsJson: '{"keep_alive":"5m"}', + }), + ) + assert.equal(settings.embeddingModel, 'ollama/example:latest') + assert.equal(settings.embeddingDimensions, 768) + assert.equal(settings.embeddingRequestDelay, 25) + assert.equal(settings.embeddingParamsJson, '{"keep_alive":"5m"}') + // The fields left unset are still the schema's own defaults, not `undefined`. + assert.equal(settings.sourceDir, '') + assert.deepEqual(settings.sourceDirExclude, []) +}) + +test('toSlopoConfig turns a schema-resolved section into slopo config, not into defaults', () => { + // The regression: every read in this path was a plain-value check, so the + // model read as '' and the dimensions as undefined — the tool reported itself + // unconfigured while the settings card was displaying a configured one. + const config = resolved({ + embeddingModel: 'ollama/example:latest', + embeddingDimensions: 768, + embeddingRequestDelay: 25, + embeddingApiBase: 'http://192.0.2.10:11434', + embeddingParamsJson: '{"keep_alive":"5m"}', + sourceDir: '/src', + }) + const paths = workspacePaths('/home/dsh', '/work', config) + assert.equal(paths.sourceDir, '/src') + const problems = [] + const slopoConfig = toSlopoConfig(config, paths, problems) + assert.deepEqual(problems, []) + assert.equal(slopoConfig.embedding_model, 'ollama/example:latest') + assert.equal(slopoConfig.embedding_dimensions, 768) + assert.equal(slopoConfig.embedding_request_delay, 25) + assert.deepEqual(slopoConfig.embedding_params, { keep_alive: '5m', api_base: 'http://192.0.2.10:11434' }) +}) + +test('the same helpers still take a plain object, which is how they are called elsewhere', () => { + const paths = workspacePaths('/home/dsh', '/work', { sourceDir: '/src', stateDir: '/state' }) + assert.equal(paths.stateDir, '/state') + assert.equal(paths.sourceDir, '/src') + const slopoConfig = toSlopoConfig({ embeddingModel: 'ollama/plain:latest', embeddingDimensions: 1024 }, paths) + assert.equal(slopoConfig.embedding_model, 'ollama/plain:latest') + assert.equal(slopoConfig.embedding_dimensions, 1024) +}) + +test('a snapshot taken per call sees a live edit, because the reference is written in place', () => { + const config = resolved({ embeddingDimensions: 768 }) + assert.equal(settingsSnapshot(config).embeddingDimensions, 768) + // This is what the Loader does on a volatile-only settings change: it writes + // the new value into the reference the running instance already holds, without + // re-applying the plugin. A snapshot cached at apply time would never see it, + // which is why the tool takes a fresh one per call. + const [write] = Object.getOwnPropertySymbols(config.embeddingDimensions) + config.embeddingDimensions[write](1024) + assert.equal(settingsSnapshot(config).embeddingDimensions, 1024) +}) diff --git a/test/plugin.test.js b/test/plugin.test.js index deba8fc..0757c11 100644 --- a/test/plugin.test.js +++ b/test/plugin.test.js @@ -101,9 +101,18 @@ writeFileSync(stubSlopo, '#!/bin/sh\nexit 0\n', { mode: 0o755 }) * * `jobs` is deliberately absent, which forces the tool down its foreground path * so the assertions can see a result rather than a job handle. + * + * `configFromSchema` hands `apply` the config the way Cordis actually builds it + * — through `Config['~standard'].validate`, so every volatile field arrives as a + * reference object rather than a value. Without it a test can only prove the + * plugin works on a shape no real profile ever produces. */ -function harness(overrides = {}, { withSettings = true, withJobs = false, withShell = true, sandboxMode, sandboxPolicyResolver, shellRun } = {}) { - const config = { ...FULL_CONFIG, binary: stubSlopo, ...overrides } +function harness( + overrides = {}, + { withSettings = true, withJobs = false, withShell = true, sandboxMode, sandboxPolicyResolver, shellRun, configFromSchema = false } = {}, +) { + const plain = { ...FULL_CONFIG, binary: stubSlopo, ...overrides } + const config = configFromSchema ? resolveConfig(plain) : plain // A per-harness shell, so a test can vary sandboxMode and every resolved // request is recorded for assertion. const scopedShell = { @@ -187,6 +196,13 @@ function harness(overrides = {}, { withSettings = true, withJobs = false, withSh const execContext = (cwd) => ({ signal: new AbortController().signal, agent: { id: 'session-1', session: { header: { cwd } } } }) +/** Resolve a fixture the way Cordis does, volatile references and all. */ +function resolveConfig(patch) { + const result = Config['~standard'].validate(patch) + assert.equal(result.issues, undefined, `the schema refused the fixture: ${JSON.stringify(result.issues)}`) + return result.value +} + test('Config is a schemastery schema, which is what cordis requires', () => { assert.equal(typeof Config, 'function') assert.ok(Config['~standard'] !== undefined, 'expected a Standard Schema') @@ -281,6 +297,42 @@ test('the embedding API key is a declared secret, so the settings wire never car } }) +test('a schema-resolved config reaches the tool as values, not as volatile references', async () => { + // The regression: Cordis hands `apply` a reference object per volatile field, + // so every plain-value read (`typeof settings.embeddingModel === 'string'`) + // was false and the configured settings were silently discarded. It looked + // healthy because `status` fell back to autodiscovery and to whatever stale + // slopo.conf.yaml was already on disk. + const { state } = harness( + { embeddingModel: 'ollama/example:latest', embeddingDimensions: 768 }, + { configFromSchema: true }, + ) + const value = await state.tools.get('slopo').execute({ action: 'status' }, execContext('/tmp')) + assert.deepEqual(value.details.missingSettings, [], 'the settings must not read as unset') + assert.equal(value.ok, true) + assert.equal(value.details.embeddingModel, 'ollama/example:latest') + assert.equal(value.details.embeddingDimensions, 768) +}) + +test('a settings edit lands in the running tool, because the snapshot is taken per call', async () => { + // A volatile-only write does not re-apply the plugin — the Loader writes the + // new value into the reference the instance already holds — so a snapshot + // cached in `apply` would freeze this tool at its composition values forever. + const { config, state } = harness( + { embeddingModel: 'ollama/first:latest', embeddingDimensions: 768 }, + { configFromSchema: true }, + ) + const tool = state.tools.get('slopo') + const before = await tool.execute({ action: 'status' }, execContext('/tmp')) + assert.match(before.message, /ollama\/first:latest/) + + const [write] = Object.getOwnPropertySymbols(config.embeddingModel) + config.embeddingModel[write]('ollama/second:latest') + + const after = await tool.execute({ action: 'status' }, execContext('/tmp')) + assert.match(after.message, /ollama\/second:latest/) +}) + test('status reports the missing settings instead of pretending to be ready', async () => { const { state } = harness() const tool = state.tools.get('slopo')