diff --git a/README.md b/README.md index 0e27ec9..38066f9 100644 --- a/README.md +++ b/README.md @@ -168,7 +168,99 @@ machine. The tool description steers the model on when *not* to use it: prefer `grep` when you already know the symbol, `read` when you need the whole file, and do not -follow up with a `read` of a range you were just handed. +follow up with a `read` of a range you were just handed. It also claims the one +move it exists to replace: **reaching for it before reading files to find out +what a module contains**, because that is a search — and one search that warms a +bounded slice of the index costs less than the first round of reads it avoids. + +## Extra source roots: indexing the code you write *against* + +A workspace index answers questions about the code you are writing. It cannot +answer questions about the library that code is written against — the framework, +the SDK, the installed platform — because none of that is under the workspace. + +Measured on one real plugin-development session, **64 of 80 tool calls** went to +the installed harness checkout under `/usr/local/lib/node_modules` to look up the +contract being implemented: *"Read workspace slot contract"*, *"Locate +DirectoryListing definition"*, *"Read UiWorkspace service type"*. That is the only +authoritative copy of the API, and every one of those reads went into the +conversation the expensive way. + +```yaml +# your profile's cordis.patch.yml +- id: dsh-recall + name: dsh-recall + config: + sourceRoots: + - /usr/local/lib/node_modules/@deepseek-ai/dsh/node_modules/@deepseek-ai + # or, with a chosen label and a workspace-relative path: + # - vendor=../packages/node_modules/@vendor +``` + +Registered through the settings form it is a comma-separated list, as every +list-valued setting is. + +An entry is a path, optionally prefixed with `name=`: + +- an **absolute** path is taken as given — what a deployment's installed tree needs; +- a **relative** path resolves against the workspace, which is what makes a + checked-in configuration portable; +- the **name** defaults to the directory's basename and becomes the first segment + of every result path from that root, so a result says which tree it came from. + +### The root is entered; its dependencies are not + +A root is usually *a directory of packages*, which is the one shape the workspace +walk handles worst: `node_modules` and friends are skipped at every depth, so the +interesting files are exactly the ones it refuses to enter. So the rule is +asymmetric, and the asymmetry is the feature: + +- **the root itself is entered whatever it is called** — a path ending in + `node_modules/...` is the normal way to name a package-manager tree; +- **below the root** the usual skip list applies, so a `node_modules` inside one + of those packages is not indexed. A dependency is somebody else's code, and it + is the same code the root already holds. + +Measured against the real harness packages (`@deepseek-ai`): **2,927 files** into +11,926 chunks, a **113 ms** walk, and **zero** indexed paths containing +`node_modules`. A query — *"how does a plugin register a settings section the +client renders"* — returned its top six hits inside `dsh-client-ui-settings*`, the +correct module, at 11 KB of excerpts against 373 KB across the 7 files they came +from. + +### How a result from a root is addressed + +The paths a search returns are what the caller hands to `read`, which resolves +them against the workspace. A file under an extra root is not under the workspace, +so no prefixing makes `@dsh/dsh-agent/lib/index.js` readable. Each such result +therefore carries **both**: the labelled path that says where it came from, and an +absolute `sourcePath`: + +``` +--- 3. @dsh/dsh-client-ui-settings/lib/types/client/contract/slots.d.ts:128-169 similarity 0.47 + at /usr/local/lib/node_modules/@deepseek-ai/.../slots.d.ts +``` + +Reading that absolute path may still need a scope grant from the caller's own +policy — a facet guard, say. This plugin cannot grant one and does not pretend to. + +An unusable entry — a typo, a path that is a file rather than a directory, two +roots claiming one name, or a name that collides with the active focus — is +**reported in the result** and skipped. One bad entry must not cost the other +roots, and it must not look like the feature doing nothing. + +### Warming it before you need it + +An extra root makes the index bigger, and a cold index is the one call that is +slow. `index` runs as a background job by default for exactly that reason — and +it is what makes the first search of a session cheap: + +``` +recall action=index +``` + +The job is owned by the session that started it, so its completion reaches that +conversation and its cancellation follows the session. ## Focus: letting a conversation see less diff --git a/client/client.js b/client/client.js index 623bba8..7a45baa 100644 --- a/client/client.js +++ b/client/client.js @@ -252,6 +252,16 @@ window.__ModuleLoader__.load({ label: 'Scope marker files', hint: 'Comma separated filenames that make a directory its own scope. Empty uses the built-ins.', }, + { + key: 'sourceRoots', + kind: 'list', + group: 'Focus', + label: 'Extra source roots', + hint: + 'Comma separated trees to index beside the workspace, for code that is not in it. ' + + 'A path, or name=path to choose the label results carry. Relative paths resolve against the ' + + 'workspace; absolute ones are taken as given, e.g. /usr/local/lib/node_modules/@deepseek-ai', + }, { key: 'expansionEnabled', diff --git a/index.js b/index.js index b4e5d7d..2123084 100644 --- a/index.js +++ b/index.js @@ -41,7 +41,7 @@ import { readFile, stat } from 'node:fs/promises' import { createHash } from 'node:crypto' -import { basename, join, resolve } from 'node:path' +import { basename, join, resolve, sep } from 'node:path' import { defineTool } from '@deepseek-ai/dsh-tools' // A real dependency rather than a duck-typed `{ parse }`: the projection service @@ -55,6 +55,7 @@ import { EmbeddingError, createEmbedder } from './lib/embed.js' import { DEFAULT_FOCUS_MARKERS, discoverScopes, focusKey, resolveFocus } from './lib/focus.js' import { indexFile, resolveDshHome, workspaceStateDir } from './lib/paths.js' import { fuseRankings, rankOrder, selectFrom } from './lib/rank.js' +import { listRootFiles, resolveSourceRoots } from './lib/roots.js' import { RecallIndex } from './lib/store.js' import { createExpander } from './lib/expand.js' import { createReranker } from './lib/rerank.js' @@ -100,6 +101,7 @@ export const SETTING_KEYS = [ 'stateDir', 'focus', 'focusMarkers', + 'sourceRoots', 'expansionEnabled', 'expansionModel', 'expansionCount', @@ -209,6 +211,38 @@ export const Config = Schema.object({ focus: Schema.string().default('').volatile(), /** Filenames that mark a directory as its own scope. Empty means the built-ins. */ focusMarkers: Schema.array(String).default([]).volatile(), + /** + * Extra trees to index beside the workspace, for code that is not in it. + * + * The workspace index cannot answer questions about the code you are writing + * *against* — an installed framework, a platform SDK, a sibling checkout. In + * plugin development that is where most of the reading goes: the harness's own + * packages are the only authoritative copy of the API being implemented, and + * they live outside the workspace. + * + * Each entry is a path, optionally prefixed with `name=`: + * + * - an **absolute** path is taken as given — what a deployment's installed + * tree needs; + * - a **relative** path resolves against the workspace, which is what makes a + * checked-in configuration portable; + * - the **name** defaults to the directory's basename and becomes the first + * segment of every result path from that root (`@deepseek-ai/dsh-agent/…`), + * so a result says which tree it came from. Use `name=` when the basename is + * long or ambiguous. + * + * The root's *contents* are indexed, never its dependencies: `node_modules`, + * build output and VCS directories are skipped below the root while the root + * itself is always entered — which is what lets a root be the scoped-packages + * directory of a package manager. An unusable entry is reported in the result + * rather than being fatal, so one typo cannot cost the other roots. + * + * A result from a root carries an absolute `sourcePath` for the follow-up read, + * because the labelled path is relative to the root rather than the workspace. + * Reading it may still need a scope grant from the caller's own policy; this + * plugin cannot grant one. + */ + sourceRoots: Schema.array(String).default([]).volatile(), /** * Ask the local model for alternative phrasings and fuse the rankings. * @@ -575,6 +609,7 @@ export function apply(ctx, config) { 'oversized', 'unreadable', 'truncated', + 'sourceRoots', ] for (const key of fields) if (extra[key] !== undefined) out[key] = extra[key] return out @@ -655,8 +690,18 @@ export function apply(ctx, config) { * knows exactly what is stale, so a partial index is described rather than * guessed at. */ - async function syncWorkspace(input) { - const { root, prefix = '', index, settings, budgetMs, signal, onProgress } = input + + /** + * The candidate file list for one index: the workspace (or focus) tree plus + * every extra root. + * + * Split out from {@link syncWorkspace} because a search reports the absolute + * path of a result that came from an extra root, and the read phase is where + * that mapping is known. Both callers therefore see one list and one set of + * problems. + */ + async function walkIndex(input) { + const { root, prefix = '', settings, signal, sourceRoots = [] } = input const walk = await listCodeFiles(root, { ...(settings.extensions?.length ? { extensions: settings.extensions } : {}), skipDirs: DEFAULT_SKIP_DIRS, @@ -672,8 +717,46 @@ export function apply(ctx, config) { for (const file of walk.files) file.path = `${prefix}/${file.path}` } + // Extra roots are walked as their own trees and merged into the same index, + // so one search ranks workspace code and library code together. Each root + // reports its own truncation: a root too large to index must say so rather + // than answer from its first few files. + const files = [...walk.files] + const problems = [...(input.sourceRootProblems ?? [])] + const summary = [] + let truncated = walk.truncated + for (const sourceRoot of sourceRoots) { + const walkOfRoot = await listRootFiles({ + name: sourceRoot.name, + path: sourceRoot.path, + ...(settings.extensions?.length ? { extensions: settings.extensions } : {}), + exclude: settings.exclude ?? [], + maxFiles: Number(settings.maxFiles), + signal, + }) + // A root labelled with the focus path itself would index two different + // files under one key — the second write silently replacing the first. + if (prefix !== '' && (prefix === sourceRoot.name || prefix.startsWith(`${sourceRoot.name}/`))) { + problems.push( + `the source root "${sourceRoot.name}" collides with the focus path "${prefix}"; ` + + 'give it a different name= or search without that focus', + ) + continue + } + files.push(...walkOfRoot.files) + summary.push({ name: sourceRoot.name, path: sourceRoot.path, files: walkOfRoot.files.length }) + truncated = truncated || walkOfRoot.truncated + } + + return { files, truncated, problems, summary } + } + + async function syncWorkspace(input) { + const { index, settings, budgetMs, signal, onProgress } = input + const { files, truncated, problems: rootProblems, summary: rootSummary } = await walkIndex(input) + const known = index.fileHashes() - const present = new Set(walk.files.map((file) => file.path)) + const present = new Set(files.map((file) => file.path)) const removed = index.prune([...known.keys()].filter((path) => !present.has(path))) const maxBytes = Number(settings.maxFileBytes) @@ -681,7 +764,7 @@ export function apply(ctx, config) { let oversized = 0 let unreadable = 0 - for (const file of walk.files) { + for (const file of files) { if (signal?.aborted) throw signal.reason ?? new Error('aborted') let content try { @@ -746,8 +829,8 @@ export function apply(ctx, config) { const stats = index.stats() return { ...stats, - scanned: walk.files.length, - truncated: walk.truncated, + scanned: files.length, + truncated, removed, embedded, chunksWritten, @@ -755,6 +838,13 @@ export function apply(ctx, config) { stale: [...stale], oversized, unreadable, + sourceRoots: rootSummary, + sourceRootProblems: rootProblems, + // Not part of the model-facing contract — the schema is closed — but a + // search needs it: a result from an extra root is labelled relative to + // that root, so the absolute path is the only thing `read` can use, and + // only the walk knows it. + fileAbsolutes: new Map(files.map((file) => [file.path, file.absolute])), } } @@ -790,6 +880,13 @@ export function apply(ctx, config) { excerpt: { type: 'string', required: true }, truncated: { type: 'boolean', required: true }, stale: { type: 'boolean', required: true }, + /** + * Absolute path, present only for a result that came from an extra + * index root. The labelled `path` of such a result is relative to + * its root rather than the workspace, so it is not what `read` + * resolves — this is. + */ + sourcePath: { type: 'string' }, }, }, }, @@ -808,8 +905,23 @@ export function apply(ctx, config) { truncated: { type: 'boolean' }, database: { type: 'string' }, stateDir: { type: 'string' }, + /** The extra roots this index holds, and how many files each contributed. */ + sourceRoots: { + type: 'array', + items: { + type: 'object', + additionalProperties: false, + properties: { + name: { type: 'string', required: true }, + path: { type: 'string', required: true }, + files: { type: 'integer', required: true }, + }, + }, + }, }, }, + /** Entries of `sourceRoots` that could not be used, one message each. */ + sourceRootProblems: { type: 'array', items: { type: 'string' } }, best: { type: 'number' }, endpoint: { type: 'object', @@ -880,12 +992,15 @@ export function apply(ctx, config) { } if (value.action !== 'search') { const { files = 0, chunks = 0, pending = 0, scanned = 0, embedded = 0 } = value.index ?? {} + const refused = (value.sourceRootProblems ?? []).length === 0 + ? '' + : `\n\nRefused source roots: ${value.sourceRootProblems.join('; ')}.` return [ { type: 'text', text: `Index for ${value.workspace}: ${files} files, ${chunks} chunks. ` + - `Scanned ${scanned}, embedded ${embedded} this call, ${pending} still pending.`, + `Scanned ${scanned}, embedded ${embedded} this call, ${pending} still pending.${refused}`, }, ] } @@ -893,11 +1008,15 @@ export function apply(ctx, config) { const results = value.results ?? [] if (results.length === 0) { const suffix = (value.index?.pending ?? 0) > 0 ? ` ${value.index.pending} file(s) are still unindexed.` : '' + const refused = + (value.sourceRootProblems ?? []).length === 0 + ? '' + : ` Refused source roots: ${value.sourceRootProblems.join('; ')}.` return [ { type: 'text', text: - `No match for ${JSON.stringify(value.query)}.${suffix} ` + + `No match for ${JSON.stringify(value.query)}.${suffix}${refused} ` + 'If you know the exact identifier, grep is the better tool for this query.', }, ] @@ -912,9 +1031,14 @@ export function apply(ctx, config) { const symbol = result.symbol === undefined ? '' : ` (${result.symbol})` const stale = result.stale ? ' [stale — file changed since indexing]' : '' const cut = result.truncated ? '\n… excerpt truncated; read the file for the rest' : '' + // A result from an extra root is labelled relative to that root, so the + // workspace-relative `path` is not something `read` can resolve. The + // absolute path is stated when it is the one that differs — a workspace + // result already has a usable path and does not need the noise. + const source = result.sourcePath === undefined ? '' : `\n at ${result.sourcePath}` return ( `--- ${at + 1}. ${result.path}:${result.startLine}-${result.endLine}${symbol} ` + - `similarity ${result.similarity}${stale}\n${result.excerpt}${cut}` + `similarity ${result.similarity}${stale}${source}\n${result.excerpt}${cut}` ) }) // A scope is a window, not a filter on relevance. When it holds nothing @@ -938,10 +1062,22 @@ export function apply(ctx, config) { 'Search this workspace semantically with a local embedding model and return ranked code excerpts ' + 'with file paths and line ranges — without reading whole files into the conversation. Use it when ' + 'you do not know the exact identifier: "where is X handled", "which module does Y", "how does Z ' + - 'recover from failure". Prefer grep when you already know the exact symbol, and read when you need ' + - 'the whole file. Because excerpts are returned inline, do not follow up with a read of the same ' + - 'range. Actions: "search" (default) answers a query and warms a bounded slice of the index; ' + - '"index" builds or refreshes the whole index, as a background job by default; "status" reports the ' + + 'recover from failure". ' + + // The move this exists to replace: reading a module's files one by one to + // find out what is in it. That is a search, and this is cheaper than the + // reads it avoids — including on the very first call, because a search + // warms a bounded slice of the index as it answers. + 'Reach for it before reading files to find out what a module contains or ' + + 'which of several alike modules does the job: one search returns the relevant lines from wherever ' + + 'they are, where reading to find them costs the whole file for each wrong guess and every one of ' + + 'those lines is re-sent on every later step. ' + + 'Prefer grep when you already know the exact symbol, and read when you need the whole file. ' + + 'Because excerpts are returned inline, do not follow up with a read of the same ' + + 'range. A result labelled with a leading name (for example "@dsh/…") came from an extra source ' + + 'root rather than the workspace; such a result also carries an absolute `sourcePath`, which is the ' + + 'one `read` can resolve. Actions: "search" (default) answers a query and warms a bounded slice of ' + + 'the index; "index" builds or refreshes the whole index, as a background job by default, which is ' + + 'what makes the first search of a session cheap; "status" reports the ' + 'index and endpoint state. The index is incremental and lives outside your repository.', parameters: { query: { @@ -1040,6 +1176,15 @@ export function apply(ctx, config) { }) const focusPath = focusKey(cwd, focus.root) + // Extra roots are resolved once per call against the live workspace, so + // an entry is judged where it is used rather than at load: a path that + // has not been installed yet is reported on the call that wanted it. A + // bad entry never aborts the call — it is reported beside the result, or + // the caller would lose the workspace index to one typo. + const resolvedRoots = resolveSourceRoots(settings.sourceRoots ?? [], { workspace: cwd }) + const sourceRoots = resolvedRoots.roots + const sourceRootProblems = resolvedRoots.problems + let opened try { opened = indexFor(settings, cwd, focusPath) @@ -1094,6 +1239,7 @@ export function apply(ctx, config) { index: indexView(base.index, stats), endpoint, scopes: scopes.map((entry) => ({ path: entry.path, marker: entry.marker })), + ...(sourceRootProblems.length === 0 ? {} : { sourceRootProblems }), note: `Index ${file} holds ${stats.files} files and ${stats.chunks} chunks, scoped to ` + `${focusPath === '.' ? 'the whole workspace' : focusPath} (${focus.source}` + @@ -1108,6 +1254,17 @@ export function apply(ctx, config) { .slice(0, 12) .map((entry) => entry.path) .join(', ')}${scopes.length > 12 ? ` (+${scopes.length - 12} more)` : ''}. `) + + // Extra roots are named even when the index has not been rebuilt + // since they were configured, so their file counts are deliberately + // absent here — `stats` describes the index, not the configuration. + (sourceRoots.length === 0 + ? '' + : `Extra roots indexed beside the workspace: ${sourceRoots + .map((entry) => `${entry.name} (${entry.path})`) + .join(', ')}. `) + + (sourceRootProblems.length === 0 + ? '' + : `Source roots refused: ${sourceRootProblems.join('; ')}. `) + `Embeddings come from "${settings.embeddingModel}" (${settings.embeddingDimensions} dims) at ` + `${settings.embeddingApiBase} — ` + (endpoint.ok === true @@ -1193,6 +1350,8 @@ export function apply(ctx, config) { settings, budgetMs: Number.POSITIVE_INFINITY, signal, + sourceRoots, + sourceRootProblems, }), ) @@ -1220,7 +1379,14 @@ export function apply(ctx, config) { } const result = await runIndex(exec.signal) - return { ...base, ok: true, index: indexView(base.index, result) } + return { + ...base, + ok: true, + index: indexView(base.index, result), + ...(result.sourceRootProblems.length === 0 + ? {} + : { sourceRootProblems: result.sourceRootProblems }), + } } // search @@ -1250,6 +1416,8 @@ export function apply(ctx, config) { settings, budgetMs: Math.max(0, Number(settings.syncBudgetMs)), signal: exec.signal, + sourceRoots, + sourceRootProblems, }), ) const matrix = index.matrix() @@ -1342,6 +1510,18 @@ export function apply(ctx, config) { const stale = new Set(sync.stale ?? []) const best = results.length === 0 ? 0 : Math.max(...results.map((result) => result.similarity)) + // Where the result actually is, for the cases where the label is not a + // workspace-relative path. A workspace result keeps its plain path: the + // caller resolves it against the workspace and has no use for an + // absolute one. A result from an extra root cannot be resolved that + // way, so it carries the absolute path beside its label. + const absolutes = sync.fileAbsolutes ?? new Map() + const workspacePrefix = resolve(cwd).endsWith(sep) ? resolve(cwd) : `${resolve(cwd)}${sep}` + const sourcePathOf = (result) => { + const absolute = absolutes.get(result.path) + if (absolute === undefined) return undefined + return absolute.startsWith(workspacePrefix) ? undefined : absolute + } // The alternative to these excerpts: the whole of each file they came // from. Sizes come from the index, which recorded them at embed time, @@ -1377,11 +1557,15 @@ export function apply(ctx, config) { excerpt: result.excerpt, truncated: result.truncated, stale: stale.has(result.path), + ...(sourcePathOf(result) === undefined ? {} : { sourcePath: sourcePathOf(result) }), })), index: indexView(base.index, sync), best, weak: results.length === 0 || best < Number(settings.minSimilarity), measure, + ...(sync.sourceRootProblems.length === 0 + ? {} + : { sourceRootProblems: sync.sourceRootProblems }), } } catch (error) { const code = error instanceof EmbeddingError ? error.code : 'RECALL_FAILED' diff --git a/lib/roots.js b/lib/roots.js new file mode 100644 index 0000000..6e15604 --- /dev/null +++ b/lib/roots.js @@ -0,0 +1,254 @@ +/** + * Extra index roots: code that is not in the workspace, indexed beside it. + * + * ## Why this exists + * + * A workspace index answers questions about the code you are writing. It does + * not answer questions about the code that code is written *against* — the + * library, the framework, the platform SDK. In plugin development that is not a + * side interest: measured on one real session, 64 of 80 tool calls went to the + * installed harness checkout under `/usr/local/lib/node_modules` to look up the + * contract being implemented, because that is the only authoritative copy of it. + * None of that is under the workspace, so none of it was indexed, and the agent + * read it into context the expensive way. + * + * An extra root is a second tree, indexed into the same index, whose results are + * distinguishable from the workspace's at a glance. + * + * ## The traversal rule is the whole feature + * + * An extra root is usually *a directory of packages* — the scoped-packages + * directory of a package manager, say — which is the one shape the workspace + * walk handles worst: it skips `node_modules` and friends at any depth, so the + * interesting files are exactly the ones it refuses to enter. + * + * So the rule is deliberately asymmetric, and it is worth stating precisely + * because the wrong version is silently wrong: + * + * - **Below the root**, `node_modules`, build output and VCS directories are + * skipped as usual. A dependency of a package is still somebody else's code, + * and it is the same code the root already holds. + * - **The root itself is entered whatever it is called.** A path that ends in + * `node_modules/...` is the normal way to name a package-manager tree, and a + * rule that skipped it would refuse the configuration this exists for. + * + * The cost of not applying the first half would be an index that embeds every + * dependency copy it can reach; the cost of not applying the second is that the + * feature cannot be configured at all. Only the root is exempt, never a nested + * directory that happens to share its name. + * + * ## Names, not just prefixes + * + * A result from an extra root is labelled `/`. The + * name defaults to the root directory's basename — `@deepseek-ai` for a scope + * directory — and a deployment that wants something shorter says so with the + * `name=path` form, because `@deepseek-ai/dsh-agent/lib/index.js` is precise but + * long, and the label is on every result. + * + * ## Results from a root are not workspace-relative, and cannot pretend to be + * + * The paths a search returns are what the caller hands to `read`, which resolves + * against the workspace. A file under an extra root is not under the workspace, + * so no amount of prefixing makes `/lib/index.js` readable. The honest + * answer is the absolute path, reported beside the relative label: the label + * says where the excerpt came from, the absolute path is what a follow-up read + * needs. Reading it may still need a scope grant from the caller's own policy — + * this module cannot grant it and does not pretend to. + */ + +import { existsSync, statSync } from 'node:fs' +import { basename, isAbsolute, resolve } from 'node:path' + +import { DEFAULT_SKIP_DIRS, listCodeFiles } from './walk.js' + +/** + * Directories skipped inside an extra root. + * + * The workspace default minus `node_modules`: a root is normally a + * package-manager tree whose *top* level is the thing being indexed, while a + * `node_modules` further down is a dependency of one of those packages and is + * skipped by the depth rule in {@link listRootFiles}. + * + * `node_modules` stays IN the set: it is skipped below a root like anything else, + * and it is {@link skipsRootDir} — not a shorter list — that lets the root itself + * be one. Dropping it here as well would index every dependency of every package + * in a package-manager tree, which is the cost the depth rule exists to avoid. + */ +export const ROOT_SKIP_DIRS = Object.freeze([...DEFAULT_SKIP_DIRS]) + +/** Whether `path` exists and is a directory. */ +function statIsDirectory(path) { + try { + return statSync(path).isDirectory() + } catch { + return false + } +} + +/** + * Read one `sourceRoots` entry. + * + * Accepted forms, both orderings of the same two facts: + * + * ``` + * /abs/path/to/packages # name defaults to the basename + * @dsh=/abs/path/to/packages # explicit name + * ``` + * + * A relative path is resolved against the workspace, which is what makes a + * checked-in configuration portable; an absolute one is taken as given, which is + * what a deployment's own installed tree needs. + * + * @param {string} entry one configured root + * @param {string} workspace absolute workspace root + * @returns {{name: string, path: string, workspaceRelative: boolean}} + */ +export function parseSourceRoot(entry, workspace) { + const text = String(entry ?? '').trim() + if (text === '') throw new Error('an empty entry') + + // `name=path` only when a name is actually present: a Windows path or a + // relative path with an `=` in it must not be split by accident. + let name = '' + let target = text + const equals = text.indexOf('=') + if (equals > 0) { + name = text.slice(0, equals).trim() + target = text.slice(equals + 1).trim() + } + if (target === '') throw new Error(`"${text}" names no path`) + if (name.includes('/') || name.includes('\\')) { + throw new Error(`"${name}" is not a usable name: it contains a path separator`) + } + + const workspaceRelative = !isAbsolute(target) + const absolute = workspaceRelative ? resolve(workspace, target) : resolve(target) + const label = sanitizeRootName(name === '' ? basename(absolute) : name) + if (label === '') throw new Error(`"${text}" does not yield a usable name`) + return { name: label, path: absolute, workspaceRelative } +} + +/** + * Make a name safe to use as a path prefix. + * + * The name becomes the first segment of every result path, so a name carrying a + * separator would forge a deeper path than the root holds, and a name of `.` or + * `..` would forge one outside it. + */ +export function sanitizeRootName(name) { + const cleaned = String(name ?? '') + .trim() + .replace(/[^A-Za-z0-9._@-]+/g, '-') + .replace(/^[-.]+|[-.]+$/g, '') + return cleaned === '.' || cleaned === '..' ? '' : cleaned +} + +/** + * Resolve every configured root, reporting each unusable one by name. + * + * A bad entry is skipped rather than fatal, and said out loud: a typo in one + * root must not cost the caller the other roots, and it must not look like the + * feature doing nothing. This mirrors how an unusable `focus` is reported. + * + * @param {readonly string[]} entries configured `sourceRoots` + * @param {object} [options] + * @param {string} options.workspace absolute workspace root + * @param {(path: string) => boolean} [options.isDirectory] injection point for tests + * @returns {{roots: {name: string, path: string}[], problems: string[]}} + */ +export function resolveSourceRoots(entries, options = {}) { + const { workspace, isDirectory = statIsDirectory } = options + const roots = [] + const problems = [] + const seen = new Set() + + for (const entry of entries ?? []) { + let parsed + try { + parsed = parseSourceRoot(entry, workspace) + } catch (error) { + problems.push(String(error?.message ?? error)) + continue + } + if (!isDirectory(parsed.path)) { + problems.push(`"${String(entry).trim()}" is not a directory: ${parsed.path}`) + continue + } + if (seen.has(parsed.name)) { + problems.push(`"${String(entry).trim()}" duplicates the name "${parsed.name}", which was already indexed`) + continue + } + seen.add(parsed.name) + roots.push({ name: parsed.name, path: parsed.path }) + } + + return { roots, problems } +} + +/** + * Whether a directory is skipped during a *root* walk. + * + * Exported because it is the rule the feature turns on, and a test that asserts + * a nested `node_modules` is skipped is a test of this function, not of a walk. + * + * @param {number} depth level below the root; 0 is the root's own entries + * @param {string} name a directory entry's name + * @param {ReadonlySet} [skip] directory names skipped below the root + */ +export function skipsRootDir(depth, name, skip = new Set(ROOT_SKIP_DIRS)) { + // The root is entered whatever it is called — that is the whole point of an + // explicit root. Everything below it obeys the normal skip list. + if (depth === 0) return false + return skip.has(name) +} + +/** + * Label a file found under an extra root. + * + * @param {string} rootName the root's label + * @param {string} relPath path within the root, `/`-separated + */ +export function rootRelativePath(rootName, relPath) { + return relPath === '' ? rootName : `${rootName}/${relPath}` +} + +/** + * List the indexable files under one extra root. + * + * Keyed by `ROOT_SKIP_DIRS` membership *below* the root and by nothing at all at + * it, because `listCodeFiles` reports the walk root at depth 0 and a directory it + * descends into at depth + 1. So the root's own name never decides anything, + * which is what lets a root be a `node_modules` tree, and a `node_modules` + * *inside* one of its packages is still skipped. + * + * `maxFiles` bounds each root rather than the whole index, and the result reports + * truncation, so a root too large to index says so instead of quietly answering + * from its first few files. + * + * @param {object} input + * @param {string} input.name label for the root, prefixed onto every path + * @param {string} input.path absolute directory to walk + * @param {readonly string[]} [input.extensions] allowed extensions, without dots + * @param {readonly string[]} [input.exclude] glob patterns over the root-relative path + * @param {number} [input.maxFiles] stop after this many candidates + * @param {AbortSignal} [input.signal] + * @returns {Promise<{files: {path: string, absolute: string}[], truncated: boolean, skippedFiles: number}>} + */ +export async function listRootFiles(input) { + const { name, path, extensions, exclude = [], maxFiles, signal } = input + const skip = new Set(ROOT_SKIP_DIRS) + const walk = await listCodeFiles(path, { + ...(extensions === undefined ? {} : { extensions }), + exclude, + ...(maxFiles === undefined ? {} : { maxFiles }), + isSkipped: (depth, dirName) => skipsRootDir(depth, dirName, skip), + signal, + }) + return { + ...walk, + files: walk.files.map((file) => ({ path: rootRelativePath(name, file.path), absolute: file.absolute })), + } +} + +/** One entry of the `sourceRoots` field, as the plugin's own docs describe it. */ +export const SOURCE_ROOT_EXAMPLE = '/usr/local/lib/node_modules/@deepseek-ai' diff --git a/lib/walk.js b/lib/walk.js index db0cd20..67d2983 100644 --- a/lib/walk.js +++ b/lib/walk.js @@ -142,6 +142,11 @@ export function extensionOf(path) { * @param {readonly string[]} [options.extensions] allowed extensions, without dots * @param {readonly string[]} [options.skipDirs] directory names skipped at any depth * @param {readonly string[]} [options.exclude] glob patterns over the relative path + * @param {(depth: number, name: string, absolute: string) => boolean} [options.isSkipped] + * decides directory traversal, replacing the `skipDirs` membership test. This is + * the seam an extra index root uses: its top level is a package-manager + * `node_modules` tree, which the built-in list exists to skip, so it must opt out + * for the root and only the root. Depth 0 is the root's own entries. * @param {number} [options.maxFiles] stop after this many candidates * @param {number} [options.maxDepth] deepest directory level to enter * @param {AbortSignal} [options.signal] @@ -152,6 +157,7 @@ export async function listCodeFiles(root, options = {}) { extensions = DEFAULT_EXTENSIONS, skipDirs = DEFAULT_SKIP_DIRS, exclude = [], + isSkipped, maxFiles = 20_000, maxDepth = 24, signal, @@ -159,6 +165,12 @@ export async function listCodeFiles(root, options = {}) { const wanted = new Set(extensions.map((value) => String(value).toLowerCase())) const skip = new Set(skipDirs) + // Materialised once per walk rather than tested with `isSkipped ?? default` at + // every directory: the hot loop should not carry a branch that cannot change. + const shouldSkip = + isSkipped === undefined + ? (depth, name) => skip.has(name) + : (depth, name, absolute) => isSkipped(depth, name, absolute) const files = [] let truncated = false let skippedFiles = 0 @@ -185,7 +197,7 @@ export async function listCodeFiles(root, options = {}) { const absolute = join(dir, entry.name) const relPath = relative(root, absolute).split(sep).join('/') if (entry.isDirectory()) { - if (skip.has(entry.name)) continue + if (shouldSkip(depth, entry.name, absolute)) continue if (depth >= maxDepth) continue if (matchesAny(relPath, exclude)) continue stack.push({ dir: absolute, depth: depth + 1 }) diff --git a/test/helpers.js b/test/helpers.js index 5df71ab..1f2faf1 100644 --- a/test/helpers.js +++ b/test/helpers.js @@ -102,8 +102,14 @@ export function makeContext(options = {}) { } /** An `exec` argument for one tool call. */ -export function makeExec(cwd, signal) { - return { signal: signal ?? new AbortController().signal, agent: { session: { header: { cwd } } } } +export function makeExec(cwd, signal, sessionId = 'session-under-test') { + // `agent.id` is the SESSION id — the jobs registry resolves a background job's + // owner through `agents.get(id)` and refuses anything that is not that string. + // A fixture without it cannot catch a call that passes the whole handle. + return { + signal: signal ?? new AbortController().signal, + agent: { id: sessionId, session: { header: { cwd } } }, + } } /** Write a map of relative path to content into a fresh temporary directory. */ diff --git a/test/plugin.test.js b/test/plugin.test.js index 51f9542..14d6310 100644 --- a/test/plugin.test.js +++ b/test/plugin.test.js @@ -1,5 +1,5 @@ import { strict as assert } from 'node:assert' -import { readFile, rm, writeFile } from 'node:fs/promises' +import { mkdir, readFile, rm, writeFile } from 'node:fs/promises' import { join } from 'node:path' import { test } from 'node:test' @@ -159,6 +159,7 @@ function configFor(stateDir, extra = {}) { // the two lists from drifting apart again. focus: '', focusMarkers: [], + sourceRoots: [], expansionEnabled: false, expansionModel: '', expansionCount: 4, @@ -173,7 +174,7 @@ function configFor(stateDir, extra = {}) { } /** Apply the plugin to a fresh workspace with a stubbed embedding endpoint. */ -async function setup(t, files = FILES, extra = {}, stubOptions = {}) { +async function setup(t, files = FILES, extra = {}, stubOptions = {}, services = {}) { const workspace = await makeWorkspace(files) const stateDir = await makeStateDir() const stub = makeStubFetch(stubOptions) @@ -185,7 +186,7 @@ async function setup(t, files = FILES, extra = {}, stubOptions = {}) { await rm(stateDir, { recursive: true, force: true }) }) - const { ctx, tools, dispose } = makeContext() + const { ctx, tools, dispose } = makeContext({ services }) t.after(() => dispose()) apply(ctx, configFor(stateDir, extra)) assert.equal(tools.length, 1, 'exactly one tool should be registered') @@ -814,3 +815,157 @@ test('re-ranking degrades to the fused order when the model has no opinion', asy ) assert.ok(reranked.results.length > 0, 'no opinion must not mean no results') }) + +test('a background index hands the jobs registry a session id, not the agent handle', async (t) => { + // The jobs registry resolves `spec.owner` through `agents.get(owner)` and + // throws `session "[object Object]" has no live agent` for anything that is not + // the session id string. This test is a stand-in for that check: the bug it + // catches was invisible in-process and made the DEFAULT `index` call fail on a + // real deployment, where a throwing starter leaves the caller no job at all. + const started = [] + const services = { + jobs: { + start(spec) { + started.push(spec) + return `recall-index-${started.length}` + }, + }, + } + const { tool, workspace } = await setup(t, FILES, {}, {}, services) + const value = await tool.execute({ action: 'index' }, makeExec(workspace, undefined, 'session-42')) + assert.deepEqual(schemaErrors(tool.output.schema, value), []) + assert.equal(value.jobId, 'recall-index-1') + assert.equal(started.length, 1) + assert.equal( + started[0].owner, + 'session-42', + 'owner must be the session id: the registry refuses an Agent handle outright', + ) + assert.equal(typeof started[0].run, 'function') + // And the job the registry was handed actually indexes the workspace. + const outcome = await started[0].run().done + assert.equal(outcome.status, 'completed') + assert.match(outcome.output, /Indexed/) + const search = await tool.execute({ query: 'session expiry' }, makeExec(workspace)) + assert.ok(search.results.length > 0, 'the background job must leave a searchable index behind') +}) + +/** + * A workspace with a library tree beside it, laid out the way a real one is. + * + * The library is a *scoped packages directory* — its own path runs through + * `node_modules` — because that is the shape the workspace walk exists to skip + * and the shape this feature has to enter. `root` is the parent of both trees, so + * the configured root can be given as a relative path and stays portable. + * + * @returns {Promise<{root: string, cleanup: () => Promise}>} + */ +async function makeRootPair() { + const root = await makeWorkspace({}) + const workspace = join(root, 'My Project') + const library = join(root, 'packages', 'node_modules', '@vendor') + await mkdir(join(library, 'widget', 'lib'), { recursive: true }) + await mkdir(join(library, 'widget', 'node_modules', 'dep'), { recursive: true }) + await mkdir(workspace, { recursive: true }) + await writeFile(join(workspace, 'app.ts'), 'export const app = 1\n') + await writeFile( + join(library, 'widget', 'lib', 'index.js'), + 'export function mountWidget(root, options) { return { root, options } }\n', + ) + await writeFile( + join(library, 'widget', 'node_modules', 'dep', 'index.js'), + 'export const vendored = "should never be indexed"\n', + ) + return { root, workspace, library, cleanup: () => rm(root, { recursive: true, force: true }) } +} + +/** Apply the plugin to a workspace the caller made itself. */ +async function setupAt(t, workspace, extra = {}) { + const stateDir = await makeStateDir() + const stub = makeStubFetch() + const originalFetch = globalThis.fetch + globalThis.fetch = stub + t.after(async () => { + globalThis.fetch = originalFetch + await rm(stateDir, { recursive: true, force: true }) + }) + const { ctx, tools, dispose } = makeContext() + t.after(() => dispose()) + apply(ctx, configFor(stateDir, extra)) + return { tool: tools[0], workspace, stub } +} + +test('an extra root is indexed beside the workspace, and its dependencies are not', async (t) => { + const { root, workspace, library } = await makeRootPair() + t.after(async () => rm(root, { recursive: true, force: true })) + + // Relative to the workspace, and named, because the naming is what makes a + // result say which tree it came from. + const { tool } = await setupAt(t, workspace, { sourceRoots: ['vendor=../packages/node_modules/@vendor'] }) + + const index = await tool.execute({ action: 'index', run_in_background: false }, makeExec(workspace)) + assert.deepEqual(schemaErrors(tool.output.schema, index), []) + assert.deepEqual(index.index.sourceRoots, [{ name: 'vendor', path: library, files: 1 }]) + assert.equal(index.index.files, 2, 'the workspace file and the library file, and nothing vendored') + + const search = await tool.execute({ query: 'mountWidget root options' }, makeExec(workspace)) + assert.deepEqual(schemaErrors(tool.output.schema, search), []) + const hit = search.results.find((result) => result.path.startsWith('vendor/')) + assert.ok(hit, `no library result in ${search.results.map((result) => result.path).join(', ')}`) + // The label names the tree; the absolute path is the only thing `read` can use, + // because the label is relative to the root rather than the workspace. + assert.equal(hit.path, 'vendor/widget/lib/index.js') + assert.equal(hit.sourcePath, join(library, 'widget', 'lib', 'index.js')) + // A workspace result is unchanged: its own path already resolves. + const own = search.results.find((result) => result.path === 'app.ts') + if (own !== undefined) assert.equal(own.sourcePath, undefined) + // The nested dependency is absent from the index entirely, not merely unranked. + assert.equal( + search.results.some((result) => result.path.includes('node_modules')), + false, + ) +}) + +test('an unusable source root is reported and never costs the workspace index', async (t) => { + const { root, workspace } = await makeRootPair() + t.after(async () => rm(root, { recursive: true, force: true })) + const { tool } = await setupAt(t, workspace, { sourceRoots: ['ghost=../nowhere', 'vendor=../packages/node_modules/@vendor'] }) + + const value = await tool.execute({ action: 'index', run_in_background: false }, makeExec(workspace)) + assert.deepEqual(schemaErrors(tool.output.schema, value), []) + // The good root still indexed, and the bad one is named rather than swallowed. + assert.deepEqual(value.index.sourceRoots.map((entry) => entry.name), ['vendor']) + assert.equal(value.sourceRootProblems.length, 1) + assert.match(value.sourceRootProblems[0], /nowhere/) + assert.ok(value.index.files >= 2, 'the workspace and the surviving root must both be indexed') + + const status = await tool.execute({ action: 'status' }, makeExec(workspace)) + assert.deepEqual(schemaErrors(tool.output.schema, status), []) + assert.equal(status.sourceRootProblems.length, 1) + assert.ok(status.results === undefined, 'status returns no results') +}) + +test('a source root whose path is a file, not a directory, is refused', async (t) => { + const { root, workspace } = await makeRootPair() + t.after(async () => rm(root, { recursive: true, force: true })) + const { tool } = await setupAt(t, workspace, { sourceRoots: ['app=./app.ts'] }) + const value = await tool.execute({ action: 'index', run_in_background: false }, makeExec(workspace)) + assert.deepEqual(schemaErrors(tool.output.schema, value), []) + assert.equal(value.index.sourceRoots.length, 0) + assert.match(value.sourceRootProblems[0], /not a directory/) +}) + +test('a source root named after the focus path is refused as a collision', async (t) => { + // Two different files would be indexed under one key, the second write + // replacing the first — so this cannot be left to the walk to get right. + const root = await makeWorkspace({ 'vendor/app.ts': 'export const a = 1\n', 'lib/app.ts': 'export const b = 2\n' }) + t.after(() => rm(root, { recursive: true, force: true })) + const { tool } = await setupAt(t, root, { sourceRoots: ['vendor=vendor'] }) + const value = await tool.execute( + { action: 'index', run_in_background: false, focus: 'vendor' }, + makeExec(root), + ) + assert.deepEqual(schemaErrors(tool.output.schema, value), []) + assert.equal(value.index.sourceRoots.length, 0) + assert.match(value.sourceRootProblems[0], /collides with the focus path/) +}) diff --git a/test/roots.test.js b/test/roots.test.js new file mode 100644 index 0000000..07189de --- /dev/null +++ b/test/roots.test.js @@ -0,0 +1,138 @@ +import { strict as assert } from 'node:assert' +import { mkdir, mkdtemp, rm, symlink, writeFile } from 'node:fs/promises' +import { tmpdir } from 'node:os' +import { join } from 'node:path' +import { test } from 'node:test' + +import { + ROOT_SKIP_DIRS, + listRootFiles, + parseSourceRoot, + resolveSourceRoots, + rootRelativePath, + sanitizeRootName, + skipsRootDir, +} from '../lib/roots.js' + +test('a root defaults its name to the directory basename', () => { + const parsed = parseSourceRoot('/usr/local/lib/node_modules/@deepseek-ai', '/repo') + assert.equal(parsed.name, '@deepseek-ai') + assert.equal(parsed.path, '/usr/local/lib/node_modules/@deepseek-ai') + assert.equal(parsed.workspaceRelative, false) +}) + +test('an explicit name is taken as written', () => { + const parsed = parseSourceRoot('dsh=/usr/local/lib/node_modules/@deepseek-ai', '/repo') + assert.equal(parsed.name, 'dsh') + assert.equal(parsed.path, '/usr/local/lib/node_modules/@deepseek-ai') +}) + +test('a relative root resolves against the workspace and says so', () => { + const parsed = parseSourceRoot('vendor/lib', '/repo') + assert.equal(parsed.path, '/repo/vendor/lib') + assert.equal(parsed.name, 'lib') + assert.equal(parsed.workspaceRelative, true) +}) + +test('a name containing a separator is refused, because it would forge a path', () => { + assert.throws(() => parseSourceRoot('a/b=/tmp/x', '/repo'), /path separator/) +}) + +test('an entry that names no path or yields no name is refused', () => { + assert.throws(() => parseSourceRoot('', '/repo'), /empty entry/) + assert.throws(() => parseSourceRoot('name=', '/repo'), /names no path/) + // A root that climbs to the filesystem root has no basename to be labelled + // with, and `resolve` collapsing the traversal is exactly why the check is on + // the resolved path rather than on the text of the entry. + assert.throws(() => parseSourceRoot('/../..', '/repo'), /does not yield a usable name/) +}) + +test('sanitizeRootName keeps the characters a path segment can hold', () => { + assert.equal(sanitizeRootName('@deepseek-ai'), '@deepseek-ai') + assert.equal(sanitizeRootName('my root'), 'my-root') + assert.equal(sanitizeRootName('..'), '') + assert.equal(sanitizeRootName('.'), '') + assert.equal(sanitizeRootName(' '), '') +}) + +test('rootRelativePath never emits a leading or trailing separator', () => { + assert.equal(rootRelativePath('dsh', 'lib/index.js'), 'dsh/lib/index.js') + assert.equal(rootRelativePath('dsh', ''), 'dsh') +}) + +test('resolveSourceRoots skips a bad entry loudly and keeps the rest', () => { + const result = resolveSourceRoots(['/good', 'missing=/nope', '/good'], { + workspace: '/repo', + isDirectory: (path) => path === '/good', + }) + assert.deepEqual(result.roots, [{ name: 'good', path: '/good' }]) + // A duplicate name is dropped rather than silently overwriting: two roots with + // one label would make every result from the second ambiguous. + assert.equal(result.problems.length, 2) + assert.match(result.problems[0], /not a directory/) + assert.match(result.problems[1], /duplicates the name/) +}) + +test('resolveSourceRoots tolerates no configuration at all', () => { + assert.deepEqual(resolveSourceRoots([], { workspace: '/repo' }), { roots: [], problems: [] }) + assert.deepEqual(resolveSourceRoots(undefined, { workspace: '/repo' }), { roots: [], problems: [] }) +}) + +test('the root itself is entered whatever it is called, and node_modules below it is not', () => { + // This asymmetry is the feature: a root is normally named `.../node_modules/...`, + // while a `node_modules` inside one of its packages is somebody else's code. + assert.equal(skipsRootDir(0, 'node_modules'), false, 'the root must not be skipped') + assert.equal(skipsRootDir(0, '.git'), false) + assert.equal(skipsRootDir(1, 'node_modules'), true, 'a nested dependency store must be skipped') + assert.equal(skipsRootDir(3, 'node_modules'), true) + assert.equal(skipsRootDir(1, 'dist'), true) + assert.equal(skipsRootDir(1, 'lib'), false) + assert.ok(ROOT_SKIP_DIRS.includes('node_modules'), 'the skip list must still name node_modules') +}) + +test('listRootFiles indexes a package tree and not its dependencies', async (t) => { + const root = await mkdtemp(join(tmpdir(), 'recall-root-')) + t.after(() => rm(root, { recursive: true, force: true })) + const packages = join(root, 'node_modules', '@scope') + await mkdir(join(packages, 'alpha', 'lib'), { recursive: true }) + await mkdir(join(packages, 'alpha', 'node_modules', 'dep'), { recursive: true }) + await mkdir(join(packages, 'beta', 'dist'), { recursive: true }) + await writeFile(join(packages, 'alpha', 'lib', 'index.js'), 'export const a = 1\n') + await writeFile(join(packages, 'alpha', 'node_modules', 'dep', 'index.js'), 'module.exports = 0\n') + await writeFile(join(packages, 'beta', 'dist', 'bundle.js'), 'var x = 1\n') + await writeFile(join(packages, 'beta', 'index.js'), 'export const b = 2\n') + await writeFile(join(packages, 'README.md'), '# not code\n') + + const walk = await listRootFiles({ name: 'dsh', path: packages }) + assert.deepEqual( + walk.files.map((file) => file.path), + ['dsh/alpha/lib/index.js', 'dsh/beta/index.js'], + 'the root is entered, a dependency below it is not, and dist is not', + ) + // Every path is labelled but resolvable: `read` needs the absolute one. + for (const file of walk.files) assert.ok(file.absolute.startsWith(packages), file.absolute) +}) + +test('listRootFiles does not follow a symlink out of the root', async (t) => { + const root = await mkdtemp(join(tmpdir(), 'recall-root-')) + const outside = await mkdtemp(join(tmpdir(), 'recall-outside-')) + t.after(async () => { + await rm(root, { recursive: true, force: true }) + await rm(outside, { recursive: true, force: true }) + }) + await mkdir(join(root, 'pkg'), { recursive: true }) + await writeFile(join(root, 'pkg', 'a.js'), 'export const a = 1\n') + await writeFile(join(outside, 'secret.js'), 'const secret = 1\n') + await symlink(outside, join(root, 'link'), 'dir') + + const walk = await listRootFiles({ name: 'r', path: root }) + assert.deepEqual(walk.files.map((file) => file.path), ['r/pkg/a.js']) +}) + +test('a file at the root of a root is labelled with the root name alone', async (t) => { + const root = await mkdtemp(join(tmpdir(), 'recall-root-')) + t.after(() => rm(root, { recursive: true, force: true })) + await writeFile(join(root, 'index.js'), 'export const root = 1\n') + const walk = await listRootFiles({ name: 'solo', path: root }) + assert.deepEqual(walk.files.map((file) => file.path), ['solo/index.js']) +})