From 9e7df8ac2f8490ea398c76c05b50f64d8be609c2 Mon Sep 17 00:00:00 2001 From: Endri Bezati Date: Fri, 4 Sep 2026 09:45:41 +0200 Subject: [PATCH] feat(api): let a profile say what its source files are called MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The core decided whether a URI was a diagram's source by testing the path against one product's file extension, written out as a literal. Six places did it — both open-diagram commands, the rename command's active-file lookup, the editor provider's save handler and its on-disk change handler — plus a file-system watcher glob pinned to the same extension. For any consumer whose files end in something else, all of that is dead weight that silently refuses. The open commands resolve nothing and show a warning telling the user to open a kind of file the product does not have, naming an extension it does not use, in text it has no way to change. Rename says the same thing in different words. The watcher fires for files the product does not own and never for the ones it does, so an edit made outside the editor — the chat agent through the edit backend, git, a formatter — never reaches the open diagram. A profile declares `sourceExtensions` instead and every site asks it. Messages are built from the list rather than written out, because a message that names an extension is a message the core is not in a position to write. Declaring nothing filters nothing. That is the deliberate half. The core cannot know how a product names its files, and the failure mode of a wrong guess is the worst one available: commands that refuse every file, which looks from the outside like a workspace with no sources in it. An unrecognised file reaches `canOpenSource` instead, where the product can refuse it for a reason it actually knows. The cost is a watcher on every file for a profile that declares neither `sourceExtensions` nor `watch.globs`; the artifact-directory filter that already existed is what keeps that affordable, and either declaration replaces it. `watch.globs` had been on the profile since the v2 contract and nothing read it. It does now, and wins over the derived globs — a product may want to watch more than its own sources, a manifest or a generated index, and it is the more specific statement of intent. The toolkit's profile builder already knew the extension and now hands it over, so the shipped consumer keeps the filtering it had. Gate 5 catches the class. The four existing gates were green for as long as this defect existed, because they look for WORDS and an extension is not a word — it reads as punctuation, which is exactly why it survived. The new gate is narrow on purpose: it bans deciding from a literal extension whether a PATH is one of a product's sources. It does not ban naming an extension inside a directory layout the platform itself defines, which is the platform's to name. --- README.md | 6 +- packages/extension-core/src/api.ts | 20 +++ .../diagram/diagram-editor-provider.ts | 50 ++++-- .../src/extension/diagram/glsp-activation.ts | 27 ++- .../extension/diagram/open-diagram-target.ts | 6 +- .../extension/diagram/source-extensions.ts | 110 ++++++++++++ .../test/profile-source-extensions.test.ts | 168 ++++++++++++++++++ .../src/sidecar-diagram-profile.ts | 6 + .../test/sidecar-diagram-profile.test.ts | 5 + scripts/check-neutrality.sh | 39 +++- 10 files changed, 405 insertions(+), 32 deletions(-) create mode 100644 packages/extension-core/src/extension/diagram/source-extensions.ts create mode 100644 packages/extension-core/test/profile-source-extensions.test.ts diff --git a/README.md b/README.md index f4ab03d..1178bb4 100644 --- a/README.md +++ b/README.md @@ -164,7 +164,7 @@ CSP, esbuild options, timing constraints) is in | Group | Fields | | --- | --- | -| Identity | `key`, `displayName`, `settingsNamespace`, `customEditorViewType`, `glspClientId`, `glspClientName` | +| Identity | `key`, `displayName`, `settingsNamespace`, `customEditorViewType`, `glspClientId`, `glspClientName`, `sourceExtensions` (which files this diagram is a view of) | | Commands | `commands` (21 consumer-owned command ids), `operationKinds` (port create/delete kind strings) | | Model & edits | `modelSource` factory, `edits` (`'read-only'` or a strategy with operation modules), `serverModules`, `serverDiagramModule` (library mode only), `storageOptions` | | Client | `clientBehavior` (neutral capability flags injected into the webview), `clientAssets` (custom webview bundle — data only), `onWebviewMessage` (inbound message hook) | @@ -172,7 +172,9 @@ CSP, esbuild options, timing constraints) is in Everything optional degrades gracefully: no `chat` → no chat backend, no `runDriver` → no run/stop commands or live glow, no `clientAssets` → the -stock webview bundle. +stock webview bundle, no `sourceExtensions` → the platform filters by +extension nowhere and words its "please open a source file" messages without +naming one. The returned `DiagramProfileHandle` exposes chat diagnostics plus two webview channels: `dispatchToWebview(uri, action)` (host→client GLSP action diff --git a/packages/extension-core/src/api.ts b/packages/extension-core/src/api.ts index a9b5e37..615769b 100644 --- a/packages/extension-core/src/api.ts +++ b/packages/extension-core/src/api.ts @@ -248,6 +248,26 @@ export interface DiagramProfile { glspClientName: string; /** Consumer-owned command ids. */ commands: DiagramCommandIds; + /** + * File extensions this product's diagram is a view of — lower-case, leading + * dot: `['.foo', '.bar']`. + * + * Everywhere the platform has to decide whether a URI is one of this + * product's sources — the open-diagram commands, the rename command's + * "which file is active" lookup, the editor provider's save and on-disk + * watchers — it asks this list. User-facing messages are worded from it too, + * so a consumer is told to open a `.foo` file rather than whatever the + * platform was written against. + * + * Declaring nothing is legal and deliberately permissive: the platform then + * filters by extension nowhere, and phrases those messages without naming an + * extension at all. The core cannot know how a product names its files, and a + * guess would fail silently — commands refusing every file, indistinguishable + * from a workspace with no sources. An unrecognised file instead reaches + * {@link DiagramProfile.canOpenSource}, which can refuse it for a reason the + * product actually knows. + */ + sourceExtensions?: string[]; /** Port operation-kind strings injected into the diagram client. */ operationKinds?: DiagramOperationKinds; /** Neutral behavior flags forwarded into the diagram webview. */ diff --git a/packages/extension-core/src/extension/diagram/diagram-editor-provider.ts b/packages/extension-core/src/extension/diagram/diagram-editor-provider.ts index 1c0906b..6a76c64 100644 --- a/packages/extension-core/src/extension/diagram/diagram-editor-provider.ts +++ b/packages/extension-core/src/extension/diagram/diagram-editor-provider.ts @@ -12,6 +12,7 @@ import { statSync } from 'node:fs'; import { WORKFLOW_DIAGRAM_TYPE } from '@dialogram/shared'; import { type DiagramProfile } from '../../api'; import { normalizeSourceUriKey } from './uri-keys'; +import { matchesSourceExtension, sourceWatchGlobs } from './source-extensions'; const RUN_ID_ARG = 'wf:runId'; @@ -79,9 +80,9 @@ export class WorkflowEditorProvider extends GlspEditorProvider { */ private changeDebounceTimers = new Map(); - /** Debounce timer for external (on-disk) .py changes picked up by the watcher. */ + /** Debounce timer for external (on-disk) source changes picked up by the watcher. */ private externalRefreshTimer?: NodeJS.Timeout; - /** When each .py was last saved through the editor, to skip the watcher's + /** When each source file was last saved through the editor, to skip the watcher's * duplicate refresh for an in-editor save (the save handler already refreshed). */ private lastSavedAt = new Map(); @@ -98,11 +99,15 @@ export class WorkflowEditorProvider extends GlspEditorProvider { * so without this filter unrelated generated files thrash the diagram. * * Incident this guards against: in a CMake-based workspace, the build tool - * regenerated `build/test/lit.site.cfg.py` on every configure, which the watcher - * treated as a cross-file import edit and force-reloaded the open diagram + * regenerated a source file under `build/test/` on every configure, which the + * watcher treated as a cross-file import edit and force-reloaded the open diagram * 4+ times in seconds — each reload paying 1-2.3s of source-CLI spawn (`acquire`) * plus a full webview re-render. A file whose path contains any of these * segments is never a legitimate reload trigger. + * + * It matters more now that the watcher can be told to watch everything: a + * profile that declares no source extensions gets `**\/*`, and this filter is + * what keeps a build tree from reloading diagrams it has nothing to do with. */ private static readonly ARTIFACT_PATH_SEGMENTS: ReadonlySet = new Set([ 'build', 'dist', 'out', 'wf-out', 'node_modules', '.git', '__pycache__', '.venv', 'venv' @@ -111,8 +116,8 @@ export class WorkflowEditorProvider extends GlspEditorProvider { /** * True when the URI lives under an artifact / irrelevant directory (see * {@link ARTIFACT_PATH_SEGMENTS}). Segment-based, so it matches at any depth - * (`.../build/test/lit.site.cfg.py`) without matching filenames that merely - * contain a segment word (`build_config.py`). + * (`.../build/test/generated-config`) without matching filenames that merely + * contain a segment word (`build_config`). */ private isUnderArtifactDir(uri: vscode.Uri): boolean { const segments = uri.path.split('/'); @@ -157,15 +162,24 @@ export class WorkflowEditorProvider extends GlspEditorProvider { }) ); - // Catch EXTERNAL .py writes — the chat agent editing via the MCP edit backend, - // git, a formatter, etc. Those bypass the editor (no onDidChange/Save), so - // without this the diagram wouldn't reflect agent-added nodes until a manual + // Catch EXTERNAL source writes — the chat agent editing via the MCP edit + // backend, git, a formatter, etc. Those bypass the editor (no onDidChange/Save), + // so without this the diagram wouldn't reflect agent-added nodes until a manual // save/reopen. onDidChangeTextDocument only fires for in-editor edits. - const sourceWatcher = vscode.workspace.createFileSystemWatcher('**/*.py'); + // + // What to watch comes from the profile, in that order of specificity: + // `watch.globs` is the exact answer when a product has one (it can watch + // more than its own sources — a manifest, a generated index); otherwise the + // globs are derived from the declared extensions; otherwise everything. + // Until this was wired the field existed and nothing read it, and the + // watcher was pinned to one product's extension. const onExternal = (uri: vscode.Uri) => this.handleExternalFileChange(uri); - sourceWatcher.onDidChange(onExternal); - sourceWatcher.onDidCreate(onExternal); - extensionContext.subscriptions.push(sourceWatcher); + for (const glob of profile.watch?.globs ?? sourceWatchGlobs(profile.sourceExtensions)) { + const sourceWatcher = vscode.workspace.createFileSystemWatcher(glob); + sourceWatcher.onDidChange(onExternal); + sourceWatcher.onDidCreate(onExternal); + extensionContext.subscriptions.push(sourceWatcher); + } } /** @@ -417,7 +431,7 @@ export class WorkflowEditorProvider extends GlspEditorProvider { */ protected handleDocumentSave(document: vscode.TextDocument): void { // Only react to source-file saves. - if (!document.uri.path.endsWith('.py')) { + if (!matchesSourceExtension(document.uri.path, this.profile.sourceExtensions)) { return; } @@ -445,7 +459,7 @@ export class WorkflowEditorProvider extends GlspEditorProvider { } /** - * React to a .py changing ON DISK from outside the editor — the chat agent + * React to a source file changing ON DISK from outside the editor — the chat agent * writing via the MCP edit backend, git, a formatter, etc. In-editor edits go * through handleDocumentChange/handleDocumentSave instead, so here we: * - skip files open with unsaved edits (the editor owns those), and @@ -454,15 +468,15 @@ export class WorkflowEditorProvider extends GlspEditorProvider { * save — refresh every open diagram. Debounced to coalesce edit bursts. */ protected handleExternalFileChange(uri: vscode.Uri): void { - if (!uri.path.endsWith('.py') || this.uriToClientId.size === 0) { + if (!matchesSourceExtension(uri.path, this.profile.sourceExtensions) || this.uriToClientId.size === 0) { return; } const canonical = this.canonicalizeUriString(uri); // Always reload when the changed file IS an open diagram's own source. For - // any OTHER watched .py, keep the cross-file-import reload but skip build + // any OTHER watched file, keep the cross-file-import reload but skip build // artifacts / irrelevant trees (see isUnderArtifactDir) so generated files - // like CMake's build/test/lit.site.cfg.py don't thrash open diagrams. + // under a build tree don't thrash open diagrams. const isOwnSource = this.uriToClientId.has(canonical); if (!isOwnSource && this.isUnderArtifactDir(uri)) { return; diff --git a/packages/extension-core/src/extension/diagram/glsp-activation.ts b/packages/extension-core/src/extension/diagram/glsp-activation.ts index e7160a2..1f64394 100644 --- a/packages/extension-core/src/extension/diagram/glsp-activation.ts +++ b/packages/extension-core/src/extension/diagram/glsp-activation.ts @@ -31,6 +31,7 @@ import type { WorkflowEditorProvider } from './diagram-editor-provider'; import { ExecutionOverlayRegistry } from './execution-overlay'; import { forwardExecutionOverlayEvents, type ExecutionOverlayWebviewSink } from './execution-overlay-bridge'; import { resolveDiagramOpenTarget, type DiagramOpenTargetArg } from './open-diagram-target'; +import { matchesSourceExtension, sourceFileNoun } from './source-extensions'; import { normalizeSourceUriKey } from './uri-keys'; import { executeViewerCommand, executeViewerOpen, executeViewerReveal } from './viewer-actions'; import { decideDiagramOpen } from './diagram-open-decision'; @@ -998,13 +999,21 @@ function registerCalDiagramCommands( connector.dispatchAction(action); }; + // The noun every "which file did you mean?" message in this function uses. + // Built from the profile's declaration so the core never writes an extension + // of its own into text a user reads; plain "source file" when none is declared. + const sourceNoun = sourceFileNoun(profile.sourceExtensions); + // A type predicate, so the callers that go on to use the URI still narrow it. + const isProfileSource = (uri: vscode.Uri | undefined): uri is vscode.Uri => + !!uri && matchesSourceExtension(uri.fsPath, profile.sourceExtensions); + const getActiveWorkflowUri = (): vscode.Uri | undefined => { const diagramUri = getActiveWorkflowDiagramUri(); - if (diagramUri?.fsPath?.endsWith('.py')) { + if (isProfileSource(diagramUri)) { return diagramUri; } const activeEditorUri = vscode.window.activeTextEditor?.document.uri; - if (activeEditorUri?.fsPath?.endsWith('.py')) { + if (isProfileSource(activeEditorUri)) { return activeEditorUri; } return undefined; @@ -1093,10 +1102,11 @@ function registerCalDiagramCommands( const sourceUri = await resolveDiagramOpenTarget(arg, { getActiveWorkflowUri, openTextDocument: vscode.workspace.openTextDocument, - workspaceRoot: vscode.workspace.workspaceFolders?.[0]?.uri.fsPath + workspaceRoot: vscode.workspace.workspaceFolders?.[0]?.uri.fsPath, + sourceExtensions: profile.sourceExtensions }); if (!sourceUri) { - vscode.window.showWarningMessage('Please open a .py file first, or pass a .py path/URI to the command.'); + vscode.window.showWarningMessage(`Please open a ${sourceNoun} first, or pass its path/URI to the command.`); return; } const canOpen = profile.canOpenSource @@ -1124,10 +1134,11 @@ function registerCalDiagramCommands( const sourceUri = await resolveDiagramOpenTarget(arg, { getActiveWorkflowUri, openTextDocument: vscode.workspace.openTextDocument, - workspaceRoot: vscode.workspace.workspaceFolders?.[0]?.uri.fsPath + workspaceRoot: vscode.workspace.workspaceFolders?.[0]?.uri.fsPath, + sourceExtensions: profile.sourceExtensions }); if (!sourceUri) { - vscode.window.showWarningMessage('Please open a .py file first, or pass a .py path/URI to the command.'); + vscode.window.showWarningMessage(`Please open a ${sourceNoun} first, or pass its path/URI to the command.`); return; } const canOpen = profile.canOpenSource @@ -1176,8 +1187,8 @@ function registerCalDiagramCommands( profile.commands.renameEntityByName, async (args?: { oldName?: string; newName?: string; sourceUri?: string }) => { const sourceUri = args?.sourceUri ? vscode.Uri.parse(args.sourceUri) : getActiveWorkflowUri(); - if (!sourceUri || !sourceUri.fsPath.endsWith('.py')) { - const message = 'No active .py source found. Focus a workflow diagram or .py editor and try again.'; + if (!isProfileSource(sourceUri)) { + const message = `No active ${sourceNoun} found. Focus a diagram or an editor on one and try again.`; void vscode.window.showWarningMessage(message); return { ok: false, message }; } diff --git a/packages/extension-core/src/extension/diagram/open-diagram-target.ts b/packages/extension-core/src/extension/diagram/open-diagram-target.ts index b3ac0a9..1f9170f 100644 --- a/packages/extension-core/src/extension/diagram/open-diagram-target.ts +++ b/packages/extension-core/src/extension/diagram/open-diagram-target.ts @@ -1,5 +1,6 @@ import * as path from 'node:path'; import * as vscode from 'vscode'; +import { matchesSourceExtension } from './source-extensions'; export type DiagramOpenTargetArg = vscode.Uri | string | { sourceUri?: string; @@ -11,6 +12,9 @@ export type ResolveDiagramOpenTargetOptions = { getActiveWorkflowUri: () => vscode.Uri | undefined; openTextDocument: (uri: vscode.Uri) => Thenable<{ uri: vscode.Uri }>; workspaceRoot?: string; + /** The profile's declared source extensions; absent/empty accepts any file + * (see `source-extensions.ts` for why the default is permissive). */ + sourceExtensions?: readonly string[]; }; function isUriString(value: string): boolean { @@ -81,7 +85,7 @@ export async function resolveDiagramOpenTarget( const candidate = parseTargetCandidate(arg, options.workspaceRoot); const resolved = candidate ?? options.getActiveWorkflowUri(); - if (!resolved || resolved.scheme !== 'file' || !resolved.fsPath.toLowerCase().endsWith('.py')) { + if (!resolved || resolved.scheme !== 'file' || !matchesSourceExtension(resolved.fsPath, options.sourceExtensions)) { return undefined; } diff --git a/packages/extension-core/src/extension/diagram/source-extensions.ts b/packages/extension-core/src/extension/diagram/source-extensions.ts new file mode 100644 index 0000000..ba9779d --- /dev/null +++ b/packages/extension-core/src/extension/diagram/source-extensions.ts @@ -0,0 +1,110 @@ +/** + * Which files a profile's diagram is a view of. + * + * The core used to answer that with one product's file extension, written out as + * a literal: six path tests plus a file-system watcher glob. That is a product's + * file naming compiled into the platform. A consumer whose sources end in + * anything else got a diagram command that silently refused every file it was + * given, and a warning telling it to open a file of a kind it does not have — + * text it could not change, naming an extension it does not use. + * + * A profile declares its own extensions instead ({@link DiagramProfile.sourceExtensions}), + * and every one of those sites asks here. The platform still names none. + * + * The permissive default is the deliberate half of this. A profile that declares + * nothing gets no filtering at all, rather than a guess: the core cannot know how + * a product names its files, and a wrong guess fails in the worst way available — + * the command does nothing, says nothing useful, and looks like the file is + * missing. Letting an unrecognised file through instead hands the decision to the + * profile's own `canOpenSource`, which can refuse it with a reason that means + * something. + */ + +/** + * Fold a declared list into the form the matcher compares against: lower-case, + * dot-prefixed, no blanks. + * + * The API asks for exactly that form, so this normally changes nothing. It runs + * anyway because the failure it prevents is invisible: `'foo'` without the dot, + * or `'.FOO'` from a product whose files are upper-case, would match no file at + * all, and "matches nothing" is indistinguishable here from "the workspace has + * no sources". Accepting the near-miss costs one pass over a list of two. + */ +export function normalizeSourceExtensions( + extensions: readonly string[] | undefined +): string[] { + if (!extensions) { + return []; + } + const normalized: string[] = []; + for (const raw of extensions) { + const trimmed = raw.trim().toLowerCase(); + if (trimmed === '' || trimmed === '.') { + continue; + } + const withDot = trimmed.startsWith('.') ? trimmed : `.${trimmed}`; + if (!normalized.includes(withDot)) { + normalized.push(withDot); + } + } + return normalized; +} + +/** + * True when `filePath` is one of the profile's source files — or when the + * profile declared none, in which case every path qualifies (see the module + * note on why the default is permissive rather than restrictive). + */ +export function matchesSourceExtension( + filePath: string | undefined, + extensions: readonly string[] | undefined +): boolean { + const declared = normalizeSourceExtensions(extensions); + if (declared.length === 0) { + return true; + } + if (!filePath) { + return false; + } + const lowered = filePath.toLowerCase(); + return declared.some(extension => lowered.endsWith(extension)); +} + +/** + * The noun a user-facing message uses for the file the command wanted: `.foo + * file`, `.foo or .bar file`, `.foo, .bar or .baz file` — and plain `source + * file` when the profile declared nothing. + * + * Messages are built from this rather than written out, because a message that + * names an extension is a message the core cannot write: it would have to know + * the answer to the question this whole module exists to delegate. + */ +export function sourceFileNoun(extensions: readonly string[] | undefined): string { + const declared = normalizeSourceExtensions(extensions); + if (declared.length === 0) { + return 'source file'; + } + if (declared.length === 1) { + return `${declared[0]} file`; + } + const last = declared[declared.length - 1]; + return `${declared.slice(0, -1).join(', ')} or ${last} file`; +} + +/** + * Watcher globs for the profile's sources. + * + * With nothing declared this is `**\/*` — every file in the workspace, which is + * the honest reading of "the platform does not filter by extension", and the + * same trade the matcher makes. It is not as expensive as it looks: the handler + * ignores any path outside an open diagram's tree of interest, and a profile + * that cares about the watcher cost declares either `sourceExtensions` or its + * own `watch.globs`, both of which win over this fallback. + */ +export function sourceWatchGlobs(extensions: readonly string[] | undefined): string[] { + const declared = normalizeSourceExtensions(extensions); + if (declared.length === 0) { + return ['**/*']; + } + return declared.map(extension => `**/*${extension}`); +} diff --git a/packages/extension-core/test/profile-source-extensions.test.ts b/packages/extension-core/test/profile-source-extensions.test.ts new file mode 100644 index 0000000..936165e --- /dev/null +++ b/packages/extension-core/test/profile-source-extensions.test.ts @@ -0,0 +1,168 @@ +/** + * A product says what its source files are called; the platform stops guessing. + * + * The core answered "is this URI one of the product's sources?" by testing the path + * against one product's extension, written out as a literal — in the open-diagram + * commands, the rename command's active-file lookup, the editor provider's save + * handler and its on-disk watcher. For a consumer whose files end in anything else, + * every one of those refused every file, and the warning it got named an extension + * it does not use. + * + * These tests pin both halves of the contract: a declared list filters and words the + * messages, and NO declaration filters nothing — the permissive default, which is + * what the core owes a product whose naming it cannot know. + */ +import { describe, expect, it, vi } from 'vitest'; +import * as vscode from 'vscode'; +import type { DiagramProfile } from '../src/api'; +import { resolveDiagramOpenTarget } from '../src/extension/diagram/open-diagram-target'; +import { + matchesSourceExtension, + normalizeSourceExtensions, + sourceFileNoun, + sourceWatchGlobs +} from '../src/extension/diagram/source-extensions'; + +vi.mock('@eclipse-glsp/vscode-integration', () => ({ + GlspEditorProvider: class { + onDidChangeCustomDocument: unknown; + constructor(protected readonly glspVscodeConnector: any) { + this.onDidChangeCustomDocument = glspVscodeConnector?.onDidChangeCustomDocument; + } + }, + GlspVscodeConnector: class {} +})); + +const { WorkflowEditorProvider } = await import('../src/extension/diagram/diagram-editor-provider'); + +describe('matchesSourceExtension', () => { + it('accepts only the declared extensions', () => { + expect(matchesSourceExtension('/repo/graph.foo', ['.foo', '.bar'])).toBe(true); + expect(matchesSourceExtension('/repo/graph.bar', ['.foo', '.bar'])).toBe(true); + expect(matchesSourceExtension('/repo/graph.baz', ['.foo', '.bar'])).toBe(false); + }); + + it('accepts anything when a profile declares nothing', () => { + // The deliberate default. A core that refused what it did not recognise + // would fail as silence — the command does nothing and the file looks + // absent — instead of letting the profile refuse it for a real reason. + expect(matchesSourceExtension('/repo/graph.anything', undefined)).toBe(true); + expect(matchesSourceExtension('/repo/graph.anything', [])).toBe(true); + }); + + it('ignores case, so an upper-case file still matches', () => { + expect(matchesSourceExtension('/repo/GRAPH.FOO', ['.foo'])).toBe(true); + }); + + it('tolerates a declaration missing the dot rather than matching nothing', () => { + // A near-miss declaration would otherwise match no file at all, which + // looks exactly like a workspace with no sources in it. + expect(normalizeSourceExtensions(['foo', '.BAR', ' ', '.foo'])).toEqual(['.foo', '.bar']); + expect(matchesSourceExtension('/repo/graph.foo', ['foo'])).toBe(true); + }); +}); + +describe('sourceFileNoun', () => { + it('names the declared extensions, and nothing when none are declared', () => { + expect(sourceFileNoun(['.foo'])).toBe('.foo file'); + expect(sourceFileNoun(['.foo', '.bar'])).toBe('.foo or .bar file'); + expect(sourceFileNoun(['.foo', '.bar', '.baz'])).toBe('.foo, .bar or .baz file'); + expect(sourceFileNoun(undefined)).toBe('source file'); + }); +}); + +describe('sourceWatchGlobs', () => { + it('derives one glob per declared extension, and watches everything otherwise', () => { + expect(sourceWatchGlobs(['.foo', '.bar'])).toEqual(['**/*.foo', '**/*.bar']); + expect(sourceWatchGlobs(undefined)).toEqual(['**/*']); + }); +}); + +describe('resolveDiagramOpenTarget with declared source extensions', () => { + const openTextDocument = vi.fn(async (uri: vscode.Uri) => ({ uri })); + + it('opens a file whose extension the profile declared', async () => { + const target = '/repo/designs/pipeline.foo'; + const resolved = await resolveDiagramOpenTarget(target, { + getActiveWorkflowUri: () => undefined, + openTextDocument, + sourceExtensions: ['.foo'] + }); + + expect(resolved?.toString()).toBe(vscode.Uri.file(target).toString()); + }); + + it('refuses a file the profile did not declare', async () => { + const resolved = await resolveDiagramOpenTarget('/repo/designs/notes.txt', { + getActiveWorkflowUri: () => undefined, + openTextDocument, + sourceExtensions: ['.foo'] + }); + + expect(resolved).toBeUndefined(); + }); + + it('opens anything when the profile declared nothing', async () => { + // The control for the case above: the refusal has to come from the + // declaration, not from the platform having an opinion of its own. + const target = '/repo/designs/notes.txt'; + const resolved = await resolveDiagramOpenTarget(target, { + getActiveWorkflowUri: () => undefined, + openTextDocument + }); + + expect(resolved?.toString()).toBe(vscode.Uri.file(target).toString()); + }); +}); + +describe('WorkflowEditorProvider source watching', () => { + function build(profile: Partial): { globs: string[]; provider: any } { + const globs: string[] = []; + const spy = vi.spyOn(vscode.workspace, 'createFileSystemWatcher').mockImplementation(((glob: string) => { + globs.push(glob); + return { + onDidChange: () => ({ dispose: () => undefined }), + onDidCreate: () => ({ dispose: () => undefined }), + dispose: () => undefined + }; + }) as any); + const connector = { onDidChangeCustomDocument: undefined, dispatchAction: () => undefined } as any; + const context = { subscriptions: [] as Array<{ dispose(): void }> } as unknown as vscode.ExtensionContext; + const provider: any = new WorkflowEditorProvider(context, connector, profile as DiagramProfile); + spy.mockRestore(); + return { globs, provider }; + } + + it('watches one glob per declared extension', () => { + expect(build({ sourceExtensions: ['.foo', '.bar'] }).globs).toEqual(['**/*.foo', '**/*.bar']); + }); + + it("prefers the profile's own watch globs, which may be wider than its sources", () => { + // `watch.globs` was declared on the profile and read by nothing until now. + // It is the more specific statement, so it wins over the derived globs. + expect(build({ sourceExtensions: ['.foo'], watch: { globs: ['**/*.foo', '**/manifest.json'] } }).globs) + .toEqual(['**/*.foo', '**/manifest.json']); + }); + + it('watches everything when a profile declares neither', () => { + expect(build({}).globs).toEqual(['**/*']); + }); + + it('ignores an on-disk change to a file outside the declared extensions', () => { + const dispatched: string[] = []; + const { provider } = build({ sourceExtensions: ['.foo'] }); + provider.dispatchModelRefresh = (_clientId: string, uri: string) => dispatched.push(uri); + const source = vscode.Uri.file('/repo/designs/pipeline.foo'); + provider.uriToClientId.set(provider.canonicalizeUriString(source), 'client-0'); + + vi.useFakeTimers(); + provider.handleExternalFileChange(vscode.Uri.file('/repo/designs/README.md')); + vi.runAllTimers(); + expect(dispatched, 'a file the product does not own triggered a reload').toEqual([]); + + provider.handleExternalFileChange(vscode.Uri.file('/repo/designs/helper.foo')); + vi.runAllTimers(); + vi.useRealTimers(); + expect(dispatched.length, 'a declared sibling source failed to trigger a reload').toBe(1); + }); +}); diff --git a/packages/sidecar-toolkit/src/sidecar-diagram-profile.ts b/packages/sidecar-toolkit/src/sidecar-diagram-profile.ts index c93fa70..3dadce9 100644 --- a/packages/sidecar-toolkit/src/sidecar-diagram-profile.ts +++ b/packages/sidecar-toolkit/src/sidecar-diagram-profile.ts @@ -360,6 +360,12 @@ export function createSidecarDiagramProfile(input: SidecarProfileInput) { glspClientId: input.glspClientId, glspClientName: input.glspClientName, commands: input.commands, + // The one extension this builder already knows, handed to the platform so + // its open/rename commands and file watchers filter on it — and so the + // "please open a ... file" warnings name it. Without this the platform + // falls back to accepting anything, which is the right default for a + // profile that cannot say, and the wrong one for a builder that can. + sourceExtensions: [input.sourceExtension], operationKinds: input.operationKinds, clientBehavior: input.clientBehavior, edits: { operationModules: createSidecarOperationModules(runtimeConfig) }, diff --git a/packages/sidecar-toolkit/test/sidecar-diagram-profile.test.ts b/packages/sidecar-toolkit/test/sidecar-diagram-profile.test.ts index 5c4ca83..146c808 100644 --- a/packages/sidecar-toolkit/test/sidecar-diagram-profile.test.ts +++ b/packages/sidecar-toolkit/test/sidecar-diagram-profile.test.ts @@ -103,6 +103,11 @@ describe('createSidecarDiagramProfile', () => { useAlternateEntityPalette: undefined }); expect(p.watch).toEqual({ globs: ['**/*.py'] }); + // The same extension reaches the platform as a declaration, so the + // open/rename commands filter on it and their warnings name it. Left + // unset, the platform accepts any file — right for a profile that cannot + // say what its sources are called, wrong for this builder, which can. + expect(p.sourceExtensions).toEqual([baseInput().sourceExtension]); }); it('threads the sidecar operation prefix onto the chat carry-over', () => { diff --git a/scripts/check-neutrality.sh b/scripts/check-neutrality.sh index f68328f..66886a9 100755 --- a/scripts/check-neutrality.sh +++ b/scripts/check-neutrality.sh @@ -4,8 +4,10 @@ # dialogram platform packages. Run via `npm run check:neutrality`. # # The four core packages (diagram-server, diagram-client, extension-core, -# shared) plus the sidecar-toolkit must carry no product vocabulary. The one -# sanctioned exception is extension-core's legacy-settings-compat.ts, which is +# shared) plus the sidecar-toolkit must carry no product vocabulary — including a +# product's FILE EXTENSION, which reads as punctuation rather than as vocabulary +# and so slipped past the word-based gates for as long as they have existed. The +# one sanctioned exception is extension-core's legacy-settings-compat.ts, which is # permitted to name legacy settings keys. # # Exits non-zero if any gate fails. @@ -86,9 +88,40 @@ else echo 'PASS' fi +echo +echo '== Gate 5: no product source extension in the core ==' +# The gate the other four missed. One product's source extension was compiled +# into extension-core in seven places — six path tests and a file-system watcher +# glob — and every gate above passed the whole time: gates 1 and 2 look for WORDS, +# and an extension is not a word. A consumer whose files end in anything else got +# commands that refused every file it gave them, and a warning telling it to open +# a kind of file it does not have — naming an extension it does not use, in text +# it could not change. +# +# What is banned is narrow on purpose: deciding, from a literal extension, whether +# a PATH is one of the product's source files. That question belongs to the +# profile's `sourceExtensions`, which the core reads through +# `extension-core/src/extension/diagram/source-extensions.ts` — a comparison +# against a value, which no literal-matching pattern can catch. +# +# Deliberately NOT banned: an extension test on something that is not a path into +# a product's sources — filtering directory entries by name inside a directory the +# PLATFORM itself defines (`.wf/skills/*.sh`, `.claude/agents/*.md`). That layout +# is the platform's own, so the platform is the one entitled to name it. +gate5_hits="$(grep -rnE "(fsPath|[Pp]ath)[^;]*\.endsWith\(\s*['\"]\." "${CORE_SRC[@]}" || true) +$(grep -rnE "createFileSystemWatcher\(\s*['\"][^'\"]*\*\.[A-Za-z0-9]" "${CORE_SRC[@]}" || true)" +if [ -n "$(echo "${gate5_hits}" | tr -d '[:space:]')" ]; then + echo 'FAIL: the core decides what a source file is from a hardcoded extension;' + echo ' ask the profile instead (DiagramProfile.sourceExtensions):' + echo "${gate5_hits}" + failures=$((failures + 1)) +else + echo 'PASS' +fi + echo if [ "${failures}" -ne 0 ]; then echo "NEUTRALITY CHECK FAILED (${failures} gate(s) violated)" exit 1 fi -echo 'NEUTRALITY CHECK PASSED (4/4 gates)' +echo 'NEUTRALITY CHECK PASSED (5/5 gates)'