From d5d7ed7203a8ed1ae5e5870ab6f37c4b280e463f Mon Sep 17 00:00:00 2001 From: kkdev92 <112151103+kkdev92@users.noreply.github.com> Date: Sun, 30 Aug 2026 00:50:54 +0900 Subject: [PATCH 1/2] feat(cli): manifest command, and defineSettings({ contributed: false }) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `vscode-ext-kit manifest ` compares the plan an extension compiles with its package.json — the comparison `assertManifestMatches` makes in a test — and reports every disagreement, as text or JSON, exiting 1 when there is one. `--apply` adds what the manifest is missing and the source can supply: commands and settings, complete in everything mechanical, with placeholder titles and descriptions a person has to replace. What a person has to decide is reported and left alone: a view needs a container the declaration does not name, a drifted default has two candidates, an entry only the manifest has may be there on purpose. Running it against a real extension found a gap in the declarations: `editor.tabSize` belongs to VS Code, and an extension that declares it to read it through the same typed accessor was indistinguishable from one that owns it — so the check asked package.json for it. `defineSettings` now takes `contributed: false` for a section the extension only reads. `describePlan` carries the answer, and `diffManifest`, `assertManifestMatches` and the command line leave such a section out of the comparison. `verify:package` runs the new command from the installed tarball, which is the first place the package's `./testing` entry is resolved through a real install rather than this repository's layout. Co-Authored-By: Claude Fable 5 --- CHANGELOG.md | 20 ++ README.md | 17 +- bin/vscode-ext-kit.mjs | 259 +++++++++++++++--- docs/guide.md | 13 + docs/samples/settings.ts | 9 + scripts/verify-package.mjs | 40 +++ src/foundation/application/describe.ts | 3 + src/foundation/settings/definition.ts | 12 + src/testing/manifest.ts | 17 +- tests/cli/fixtures/manifest-agreeing.json | 21 ++ tests/cli/fixtures/manifest-drifted.json | 15 + tests/cli/fixtures/manifest-plan.mjs | 36 +++ tests/cli/manifest.test.ts | 160 +++++++++++ tests/cli/plan.test.ts | 2 +- tests/foundation/application/describe.test.ts | 1 + tests/node-shims.d.ts | 7 + tests/testing/manifest.test.ts | 15 + 17 files changed, 607 insertions(+), 40 deletions(-) create mode 100644 tests/cli/fixtures/manifest-agreeing.json create mode 100644 tests/cli/fixtures/manifest-drifted.json create mode 100644 tests/cli/fixtures/manifest-plan.mjs create mode 100644 tests/cli/manifest.test.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index a7420c4..1a14402 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -82,6 +82,26 @@ Pre-1.0 releases followed it in spirit; their breaking changes are marked **Brea it concerns, and the JSON that would settle it when the fix is mechanical. The assertion is unchanged and built on top of it. +- **`vscode-ext-kit manifest`: the manifest check, from the command line.** + It compares the plan an extension compiles with its `package.json` — the + comparison `assertManifestMatches` makes in a test — and reports every + disagreement, as text or JSON, with exit code 1 when there is one. `--apply` + adds what the manifest is missing and the source can supply: commands and + settings, complete in everything mechanical and with placeholder titles and + descriptions a person has to replace. What a person has to decide is + reported and left alone: a view needs a container the declaration does not + name, a drifted default has two candidates, and an entry only the manifest + has may be there on purpose. + +- **`defineSettings({ contributed: false })`, for a section the extension only + reads.** `editor.tabSize` belongs to VS Code. An extension that declares it, + to read it through the same typed accessor as its own settings, was + indistinguishable from one that owns it — so the manifest check asked + `package.json` for it. The declaration now says which it is: `describePlan` + carries the answer as `contributed`, and `diffManifest`, + `assertManifestMatches` and the command line leave a section that is not + contributed out of the comparison. + - **An API reference, generated from the JSDoc.** `npm run docs:api` renders every public entry point with TypeDoc, and CI runs it with warnings as errors. Setting that up found what the warnings exist to find: types that diff --git a/README.md b/README.md index ad5c085..f675827 100644 --- a/README.md +++ b/README.md @@ -266,16 +266,27 @@ public signature names but the package does not export cannot ship. ## Command Line -The package ships one command. It reads the plan an extension compiles at -import time and prints it, so what the extension registers can be reviewed, -diffed and drawn without starting VS Code. +The package ships one command with two subcommands. Both read the plan an +extension compiles at import time, so what the extension registers can be +reviewed, diffed, drawn and checked against `package.json` without starting +VS Code. ```bash npx vscode-ext-kit plan ./out/extension.js # the plan as JSON npx vscode-ext-kit plan ./out/extension.js --format mermaid # modules, services and their edges npx vscode-ext-kit plan ./out/extension.js --check # exit 1 with every problem preflight found + +npx vscode-ext-kit manifest ./out/extension.js # every disagreement with package.json +npx vscode-ext-kit manifest ./out/extension.js --apply # add the commands and settings it is missing ``` +`manifest` makes the comparison `assertManifestMatches` makes in a test, from +the command line. `--apply` adds what the manifest is missing and the source +can supply — commands and settings, with placeholder titles and descriptions +marked `TODO` — and reports what a person has to decide: a view without a +container, a default the two sides disagree on, an entry only the manifest +has. + The entry module is evaluated with a stand-in for `vscode`, which only exists inside an extension host. That works because nothing in this package touches VS Code before `activate` — and it means module-scope code in the extension diff --git a/bin/vscode-ext-kit.mjs b/bin/vscode-ext-kit.mjs index 508af1e..2c1f41d 100644 --- a/bin/vscode-ext-kit.mjs +++ b/bin/vscode-ext-kit.mjs @@ -2,12 +2,20 @@ // The command-line tool. // // vscode-ext-kit plan [--export ] [--format json|mermaid|dot] [--check] [--kit ] +// vscode-ext-kit manifest [--manifest ] [--format text|json] [--apply] [--export ] [--kit ] // -// Reads the plan an extension compiles at import time and prints it: as the -// JSON `describePlan` produces, as a Mermaid or Graphviz graph of modules, +// `plan` reads the plan an extension compiles at import time and prints it: as +// the JSON `describePlan` produces, as a Mermaid or Graphviz graph of modules, // services and the edges between them, or — with `--check` — as nothing but // an exit code and the list of problems preflight found. // +// `manifest` compares that plan with the extension's package.json — the same +// comparison `diffManifest` makes — and reports every disagreement. With +// `--apply` it adds what the manifest is missing and the source can supply: +// commands and settings, with placeholder titles and descriptions. Anything a +// person has to decide — a view's container, a drifted default, an entry only +// the manifest has — is reported and left alone. +// // The entry module is evaluated with a stand-in for `vscode` (see // vscode-stub.cjs), because the real module only exists inside an extension // host. Nothing in this package touches VS Code before `activate`, so a @@ -15,31 +23,48 @@ // that reads a VS Code value would get a proxy instead; keep such reads inside // `activate` or a handler, which the framework asks for anyway. -import { existsSync, readFileSync } from 'node:fs'; +import { existsSync, readFileSync, writeFileSync } from 'node:fs'; import { createRequire, register } from 'node:module'; -import { resolve as resolvePath } from 'node:path'; +import { relative, resolve as resolvePath } from 'node:path'; import process from 'node:process'; import { fileURLToPath, pathToFileURL } from 'node:url'; const EXIT_OK = 0; -const EXIT_PREFLIGHT = 1; +// Preflight rejected the plan, or the manifest disagrees with it: one code for +// both, the way a linter exits. +const EXIT_REJECTED = 1; const EXIT_USAGE = 2; -const USAGE = `usage: vscode-ext-kit plan [options] +const USAGE = `usage: vscode-ext-kit [options] - Prints the plan an extension compiles at import time. +commands + plan print the plan an extension compiles at import time + manifest compare that plan with package.json; report what + disagrees, or add what the manifest is missing +options for both the extension's entry module (ESM or a CommonJS bundle) --export the export holding the defineExtension result or the plan (default: tries "app", then the default export) + --kit the @kkdev92/vscode-ext-kit to read the plan with + (default: the copy this tool ships in) + --help this text + +plan --format json (default) | mermaid | dot --check print nothing on success; exit 1 listing every problem preflight found - --kit the @kkdev92/vscode-ext-kit to describe the plan with - (default: the copy this tool ships in) - --help this text -exit codes: 0 ok, 1 preflight rejected the plan, 2 usage or load error +manifest + --manifest the package.json to compare (default: ./package.json) + --format text (default) | json + --apply add the commands and settings the manifest is missing, + with placeholder titles and descriptions marked TODO. + Views, drifted values and entries only the manifest has + are reported, never changed. + +exit codes: 0 ok, 1 preflight rejected the plan or the manifest disagrees with it, + 2 usage or load error `; // --- `vscode` stand-in ---------------------------------------------------- @@ -62,9 +87,11 @@ function parse(argv) { command: undefined, entry: undefined, exportName: undefined, - format: 'json', + format: undefined, check: false, kit: undefined, + manifest: undefined, + apply: false, help: false, }; const positional = []; @@ -95,6 +122,12 @@ function parse(argv) { case '--kit': options.kit = value(); break; + case '--manifest': + options.manifest = value(); + break; + case '--apply': + options.apply = true; + break; default: if (arg.startsWith('--')) { throw new UsageError(`unknown option ${arg}`); @@ -103,12 +136,20 @@ function parse(argv) { } } [options.command, options.entry] = positional; - if (!['json', 'mermaid', 'dot'].includes(options.format)) { - throw new UsageError(`--format must be json, mermaid or dot, not ${options.format}`); - } return options; } +/** `--format`, checked against what the command can print; the first is the default. */ +function formatOf(options, command, allowed) { + const format = options.format ?? allowed[0]; + if (!allowed.includes(format)) { + throw new UsageError( + `--format must be ${allowed.slice(0, -1).join(', ')} or ${allowed.at(-1)} for ${command}, not ${format}` + ); + } + return format; +} + class UsageError extends Error {} // --- loading -------------------------------------------------------------- @@ -149,29 +190,39 @@ async function loadPlan(entryPath, exportName) { ); } +/** Where each entry point lives inside the package, for a copy without an exports map to ask. */ +const KIT_ENTRIES = { '.': 'dist/index.js', './testing': 'dist/testing/index.js' }; + /** - * The package to describe the plan with: the copy this tool ships in, unless - * `--kit` names another. Accepts a package directory or a module file. + * One entry point of the package the plan is read with: the copy this tool + * ships in, unless `--kit` names another. Accepts a package directory or, for + * the root entry, a module file. `expected` names the export that has to be + * there, which is also how a copy too old to have it is recognised. */ -async function loadKit(kitPath) { +async function loadKit(kitPath, subpath, expected) { let target; if (kitPath === undefined) { - target = new URL('../dist/index.js', import.meta.url); + target = new URL(`../${KIT_ENTRIES[subpath]}`, import.meta.url); } else { const absolute = resolvePath(kitPath); - const manifest = resolvePath(absolute, 'package.json'); - if (existsSync(manifest)) { + const manifestPath = resolvePath(absolute, 'package.json'); + if (existsSync(manifestPath)) { const entry = - JSON.parse(readFileSync(manifest, 'utf8')).exports?.['.']?.import ?? './dist/index.js'; + JSON.parse(readFileSync(manifestPath, 'utf8')).exports?.[subpath]?.import ?? + KIT_ENTRIES[subpath]; target = pathToFileURL(resolvePath(absolute, entry)); - } else { + } else if (subpath === '.') { target = pathToFileURL(absolute); + } else { + throw new UsageError( + `--kit must name the package directory, not a file, to reach ${subpath}` + ); } } const kit = await import(target.href); - if (typeof kit.describePlan !== 'function') { + if (typeof kit[expected] !== 'function') { throw new UsageError( - `${fileURLToPath(target)} does not export describePlan; is it @kkdev92/vscode-ext-kit 4.1 or later?` + `${fileURLToPath(target)} does not export ${expected}; is it @kkdev92/vscode-ext-kit 4.1 or later?` ); } return kit; @@ -331,6 +382,143 @@ function formatProblems(problems) { .join('\n'); } +// --- manifest ------------------------------------------------------------- +/** + * What the plan declares, in the shape `diffManifest` compares: command ids, + * setting specs keyed by name, view ids. Built from the description rather + * than the plan, so the only data this reads is the public, printable kind. + */ +function declaredContributions(description) { + return { + commands: description.commands.map((command) => ({ descriptor: { id: command.id } })), + // A section the extension only reads (`contributed: false`) is not its to + // contribute. Compared as "not false" so a description without the field + // is treated the way it always was. + settings: description.settings + .filter((section) => section.contributed !== false) + .map((section) => ({ + section: section.section, + values: Object.fromEntries( + section.values.map((value) => [ + value.key.slice(section.section.length + 1), + { + type: value.type.length === 1 ? value.type[0] : [...value.type], + default: value.default, + scope: value.scope, + ...(value.enum === undefined ? {} : { enum: [...value.enum] }), + }, + ]) + ), + })), + views: [...description.treeViews, ...description.webviewViews].map((view) => view.id), + }; +} + +/** One entry per disagreement, with the JSON to paste under it when there is some. */ +function formatMismatches(mismatches) { + return mismatches + .map((mismatch) => + mismatch.paste === undefined + ? ` - ${mismatch.summary}` + : ` - ${mismatch.summary}\n${mismatch.paste.replace(/^/gm, ' ')}` + ) + .join('\n'); +} + +/** The indentation a JSON file uses, so writing it back does not reformat it. */ +function indentationOf(text) { + return /^(?[ \t]+)"/m.exec(text)?.groups.indent ?? 2; +} + +/** + * Adds what the manifest is missing and the source can supply. Commands and + * settings carry everything mechanical — id, type, default, enum, scope — and + * get placeholder titles and descriptions a person has to replace. Everything + * else is a decision rather than a transcription and stays as it is: a view + * needs a container, a drifted value has two candidates, and an entry only + * the manifest has may be there on purpose. + */ +function applyMismatches(manifest, mismatches) { + const applied = []; + const remaining = []; + manifest.contributes ??= {}; + const contributes = manifest.contributes; + for (const mismatch of mismatches) { + if (mismatch.direction !== 'missing-in-manifest' || mismatch.paste === undefined) { + remaining.push(mismatch); + } else if (mismatch.kind === 'command') { + contributes.commands ??= []; + contributes.commands.push(JSON.parse(mismatch.paste)); + applied.push(mismatch); + } else if (mismatch.kind === 'setting' && !Array.isArray(contributes.configuration)) { + contributes.configuration ??= {}; + contributes.configuration.properties ??= {}; + Object.assign(contributes.configuration.properties, JSON.parse(mismatch.paste)); + applied.push(mismatch); + } else { + // A setting while `contributes.configuration` is a list of sections: + // which section it belongs in is not something the declaration says. + remaining.push(mismatch); + } + } + return { applied, remaining }; +} + +async function runManifest(options, description) { + const format = formatOf(options, 'manifest', ['text', 'json']); + const manifestPath = resolvePath(options.manifest ?? 'package.json'); + if (!existsSync(manifestPath)) { + throw new UsageError( + `${manifestPath} does not exist; point --manifest at the extension's package.json` + ); + } + const text = readFileSync(manifestPath, 'utf8'); + const manifest = JSON.parse(text); + const { diffManifest } = await loadKit(options.kit, './testing', 'diffManifest'); + const declared = declaredContributions(description); + const mismatches = diffManifest(manifest, declared); + const shown = relative(process.cwd(), manifestPath) || manifestPath; + + if (options.apply) { + const { applied, remaining } = applyMismatches(manifest, mismatches); + if (applied.length > 0) { + writeFileSync( + manifestPath, + JSON.stringify(manifest, null, indentationOf(text)) + (text.endsWith('\n') ? '\n' : '') + ); + } + if (format === 'json') { + process.stdout.write(`${JSON.stringify({ applied, remaining }, null, 2)}\n`); + } else { + process.stdout.write( + [ + `applied ${applied.length} change(s) to ${shown}`, + ...applied.map((mismatch) => ` + ${mismatch.kind} "${mismatch.id}"`), + ...(remaining.length === 0 + ? [] + : [`${remaining.length} disagreement(s) need a person:`, formatMismatches(remaining)]), + ].join('\n') + '\n' + ); + } + return remaining.length === 0 ? EXIT_OK : EXIT_REJECTED; + } + + if (format === 'json') { + process.stdout.write(`${JSON.stringify(mismatches, null, 2)}\n`); + } else if (mismatches.length === 0) { + const settings = declared.settings.reduce((n, s) => n + Object.keys(s.values).length, 0); + process.stdout.write( + `manifest ok: ${declared.commands.length} command(s), ${settings} setting(s), ` + + `${declared.views.length} view(s) agree\n` + ); + } else { + process.stdout.write( + `${shown} disagrees with the plan in ${mismatches.length} place(s):\n${formatMismatches(mismatches)}\n` + ); + } + return mismatches.length === 0 ? EXIT_OK : EXIT_REJECTED; +} + // --- main ----------------------------------------------------------------- async function main(argv) { const options = parse(argv); @@ -338,11 +526,13 @@ async function main(argv) { process.stdout.write(USAGE); return options.help ? EXIT_OK : EXIT_USAGE; } - if (options.command !== 'plan') { - throw new UsageError(`unknown command "${options.command}"; only "plan" exists`); + if (options.command !== 'plan' && options.command !== 'manifest') { + throw new UsageError( + `unknown command "${options.command}"; the commands are plan and manifest` + ); } if (options.entry === undefined) { - throw new UsageError('plan needs an module'); + throw new UsageError(`${options.command} needs an module`); } let plan; @@ -360,14 +550,19 @@ async function main(argv) { process.stderr.write( `preflight rejected the plan with ${error.problems.length} problem(s):\n${formatProblems(error.problems)}\n` ); - return EXIT_PREFLIGHT; + return EXIT_REJECTED; } throw error; } - const kit = await loadKit(options.kit); + const kit = await loadKit(options.kit, '.', 'describePlan'); const description = kit.describePlan(plan); + if (options.command === 'manifest') { + return runManifest(options, description); + } + + const format = formatOf(options, 'plan', ['json', 'mermaid', 'dot']); if (options.check) { process.stdout.write( `plan ok: ${description.modules.length} module(s), ${description.services.length} service(s), ` + @@ -377,9 +572,9 @@ async function main(argv) { } const output = - options.format === 'mermaid' + format === 'mermaid' ? toMermaid(description) - : options.format === 'dot' + : format === 'dot' ? toDot(description) : `${JSON.stringify(description, null, 2)}\n`; process.stdout.write(output); diff --git a/docs/guide.md b/docs/guide.md index 15e0167..c57e662 100644 --- a/docs/guide.md +++ b/docs/guide.md @@ -148,6 +148,15 @@ const ProjectSettings = defineSettings({ }, }); +// A section the extension reads but does not own. `contributed: false` says +// so: the manifest check will not ask package.json for it, and `describePlan` +// reports it as read rather than declared. +export const EditorSettings = defineSettings({ + section: 'editor', + values: { tabSize: setting.integer({ default: 4, minimum: 1 }) }, + contributed: false, +}); + export const DescribeSettings = defineCommandContract({ id: 'sample.describeSettings', }); @@ -820,6 +829,10 @@ The same document is available without writing code: `npx vscode-ext-kit plan ./out/extension.js` prints it, `--format mermaid` or `--format dot` draws the modules, services and edges, and `--check` turns a preflight failure into an exit code and a list of problems — the shape a CI step wants. +`npx vscode-ext-kit manifest ./out/extension.js` compares the same plan with +`package.json` — the comparison `assertManifestMatches` makes, from the command +line — and `--apply` adds the commands and settings the manifest is missing, +with placeholders where a person has to write the words. ## Keeping package.json honest diff --git a/docs/samples/settings.ts b/docs/samples/settings.ts index 5209310..8512190 100644 --- a/docs/samples/settings.ts +++ b/docs/samples/settings.ts @@ -25,6 +25,15 @@ const ProjectSettings = defineSettings({ }, }); +// A section the extension reads but does not own. `contributed: false` says +// so: the manifest check will not ask package.json for it, and `describePlan` +// reports it as read rather than declared. +export const EditorSettings = defineSettings({ + section: 'editor', + values: { tabSize: setting.integer({ default: 4, minimum: 1 }) }, + contributed: false, +}); + export const DescribeSettings = defineCommandContract({ id: 'sample.describeSettings', }); diff --git a/scripts/verify-package.mjs b/scripts/verify-package.mjs index 6d86318..7cd9160 100644 --- a/scripts/verify-package.mjs +++ b/scripts/verify-package.mjs @@ -306,6 +306,46 @@ export const app = defineExtension({ name: 'consumer', modules: [greeting] }); } catch (error) { note(false, `the CLI failed against the installed package:\n${error.stderr ?? error.message}`); } + + // 6. `manifest`, from the installed package. The consumer's package.json + // contributes nothing, so the one command its plan declares must come back + // as missing — through the package's `./testing` entry, resolved from a real + // install rather than this repository's layout. + const cli = join( + consumer, + 'node_modules', + ...packageName.split('/'), + 'bin', + 'vscode-ext-kit.mjs' + ); + try { + run( + process.execPath, + [ + cli, + 'manifest', + join(consumer, 'consumer-plan.mjs'), + '--manifest', + join(consumer, 'package.json'), + '--format', + 'json', + ], + consumer + ); + note(false, 'the CLI reported no manifest disagreement where one exists'); + } catch (error) { + const mismatches = error.status === 1 ? JSON.parse(error.stdout) : []; + const [only] = mismatches; + note( + mismatches.length === 1 && + only.kind === 'command' && + only.direction === 'missing-in-manifest' && + only.id === 'consumer.hello', + error.status === 1 + ? 'the CLI diffs a consumer manifest from the installed package' + : `the CLI failed to diff the consumer manifest:\n${error.stderr ?? error.message}` + ); + } } finally { rmSync(work, { recursive: true, force: true }); } diff --git a/src/foundation/application/describe.ts b/src/foundation/application/describe.ts index 6c3ce14..59fb624 100644 --- a/src/foundation/application/describe.ts +++ b/src/foundation/application/describe.ts @@ -93,6 +93,8 @@ export interface SettingsSectionDescription { readonly section: string; /** How an invalid configured value is treated: `'strict'` or `'lenient'`. */ readonly policy: string; + /** Whether the extension contributes the section, or only reads one the host or another extension owns. */ + readonly contributed: boolean; readonly values: readonly SettingDescription[]; readonly moduleId: string; } @@ -332,6 +334,7 @@ export function describePlan(plan: ApplicationPlan): ApplicationPlanDescription settings: plan.settings.map((registration) => ({ section: registration.section, policy: registration.policy, + contributed: registration.contributed, values: Object.entries(registration.values).map(([name, spec]) => ({ key: `${registration.section}.${name}`, type: typeNames(spec), diff --git a/src/foundation/settings/definition.ts b/src/foundation/settings/definition.ts index 0abf105..f51252f 100644 --- a/src/foundation/settings/definition.ts +++ b/src/foundation/settings/definition.ts @@ -295,6 +295,8 @@ export interface SettingsDefinition { readonly values: TSpecs; /** How invalid values are treated. Defaults to lenient. */ readonly policy: SettingsValidationPolicy; + /** Whether this extension contributes the section in its manifest, or only reads it. */ + readonly contributed: boolean; /** Token the accessor is registered under, so handlers can inject it. */ readonly token: ServiceToken>>; } @@ -310,6 +312,7 @@ export interface SettingsRegistration { readonly section: string; readonly values: SettingSpecs; readonly policy: SettingsValidationPolicy; + readonly contributed: boolean; readonly token: ServiceToken; } @@ -321,6 +324,14 @@ export interface DefineSettingsOptions { readonly values: TSpecs; /** Invalid-value behavior. Defaults to {@link SettingsValidationPolicy.Lenient}. */ readonly policy?: SettingsValidationPolicy | undefined; + /** + * Whether this extension contributes the section in `package.json`. + * Defaults to `true`. `false` declares a section the extension only reads — + * `editor`, `files`, another extension's — with the same typed accessor: the + * manifest check then does not ask for it, and `describePlan` reports it as + * read rather than owned. + */ + readonly contributed?: boolean | undefined; } /** @@ -352,6 +363,7 @@ export function defineSettings( // an opaque function and `default` may be any application value). values: frozenCopy(options.values), policy: options.policy ?? SettingsValidationPolicy.Lenient, + contributed: options.contributed ?? true, token: serviceToken>>(`settings:${options.section}`), }); } diff --git a/src/testing/manifest.ts b/src/testing/manifest.ts index 9b4d545..e999b9d 100644 --- a/src/testing/manifest.ts +++ b/src/testing/manifest.ts @@ -18,8 +18,16 @@ import type { SettingSpec, SettingSpecs } from '../foundation/settings/definitio * anything from here. */ export interface DeclaredContributions { - /** Settings groups, from `defineSettings`. */ - readonly settings?: readonly { readonly section: string; readonly values: SettingSpecs }[]; + /** + * Settings groups, from `defineSettings`. A group declared with + * `contributed: false` — a section the extension reads but does not own — is + * not asked of the manifest. + */ + readonly settings?: readonly { + readonly section: string; + readonly values: SettingSpecs; + readonly contributed?: boolean | undefined; + }[]; /** Command contracts, from `defineCommandContract`. */ readonly commands?: readonly { readonly descriptor: CommandDescriptor }[]; /** View ids the application registers, independent of manifest container. */ @@ -160,7 +168,8 @@ function checkSettings(manifest: Manifest, declared: DeclaredContributions): Man summary, }); - for (const group of declared.settings) { + const contributed = declared.settings.filter((group) => group.contributed !== false); + for (const group of contributed) { for (const [name, spec] of Object.entries(group.values)) { const key = `${group.section}.${name}`; expected.add(key); @@ -214,7 +223,7 @@ function checkSettings(manifest: Manifest, declared: DeclaredContributions): Man } } - const sections = declared.settings.map((group) => `${group.section}.`); + const sections = contributed.map((group) => `${group.section}.`); for (const key of Object.keys(properties)) { if (sections.some((prefix) => key.startsWith(prefix)) && !expected.has(key)) { mismatches.push({ diff --git a/tests/cli/fixtures/manifest-agreeing.json b/tests/cli/fixtures/manifest-agreeing.json new file mode 100644 index 0000000..6341ed8 --- /dev/null +++ b/tests/cli/fixtures/manifest-agreeing.json @@ -0,0 +1,21 @@ +{ + "name": "sample", + "contributes": { + "commands": [{ "command": "sample.refresh", "title": "Refresh" }], + "configuration": { + "title": "Sample", + "properties": { + "sample.limit": { "type": "number", "default": 10, "minimum": 1, "description": "How many." }, + "sample.mode": { + "type": "string", + "enum": ["fast", "thorough"], + "default": "fast", + "description": "How hard to look." + } + } + }, + "views": { + "explorer": [{ "id": "sample.projects", "name": "Projects" }] + } + } +} diff --git a/tests/cli/fixtures/manifest-drifted.json b/tests/cli/fixtures/manifest-drifted.json new file mode 100644 index 0000000..337dc78 --- /dev/null +++ b/tests/cli/fixtures/manifest-drifted.json @@ -0,0 +1,15 @@ +{ + "name": "sample", + "contributes": { + "commands": [{ "command": "sample.clear", "title": "Clear" }], + "configuration": { + "title": "Sample", + "properties": { + "sample.limit": { "type": "number", "default": 5, "description": "How many." } + } + }, + "views": { + "explorer": [{ "id": "sample.other", "name": "Other" }] + } + } +} diff --git a/tests/cli/fixtures/manifest-plan.mjs b/tests/cli/fixtures/manifest-plan.mjs new file mode 100644 index 0000000..2c8b854 --- /dev/null +++ b/tests/cli/fixtures/manifest-plan.mjs @@ -0,0 +1,36 @@ +// An extension with one of each thing a manifest must agree with: a command, +// a settings section and a view. Imports the built output like the other +// fixtures; `npm run typecheck` builds it, and the tests skip when it is absent. +import { + defineCommandContract, + defineExtension, + defineModule, + defineSettings, + setting, +} from '../../../dist/index.js'; + +const Refresh = defineCommandContract({ id: 'sample.refresh', title: 'Refresh' }); +const Settings = defineSettings({ + section: 'sample', + values: { + limit: setting.number({ default: 10, minimum: 1 }), + mode: setting.enum({ values: ['fast', 'thorough'], default: 'fast' }), + }, +}); +// A section the extension reads but does not own; the manifest must not be +// asked for it, so none of the manifest fixtures mention `editor`. +const Editor = defineSettings({ + section: 'editor', + values: { tabSize: setting.integer({ default: 4 }) }, + contributed: false, +}); + +const projects = defineModule('projects', (module) => { + module.settings.add(Settings); + module.settings.add(Editor); + module.commands.handle(Refresh, () => undefined); + module.treeViews.add({ id: 'sample.projects', resolveProvider: () => ({}) }); + return undefined; +}); + +export const app = defineExtension({ name: 'sample', modules: [projects] }); diff --git a/tests/cli/manifest.test.ts b/tests/cli/manifest.test.ts new file mode 100644 index 0000000..d835c80 --- /dev/null +++ b/tests/cli/manifest.test.ts @@ -0,0 +1,160 @@ +/** + * `vscode-ext-kit manifest`, run as a consumer runs it: a child process, an + * entry module, a package.json, and whatever comes out. `--apply` is exercised + * against a copy in a temporary directory, so the fixture stays as committed. + */ +import { execFileSync } from 'node:child_process'; +import { existsSync, mkdtempSync, readFileSync, rmSync, writeFileSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import { join, resolve } from 'node:path'; + +import { afterEach, describe, expect, it } from 'vitest'; + +declare const process: { readonly execPath: string }; + +const cli = resolve('bin', 'vscode-ext-kit.mjs'); +const fixture = (name: string): string => resolve('tests', 'cli', 'fixtures', name); +const kit = resolve('dist', 'index.js'); +const kitRoot = resolve('.'); +const built = existsSync(kit); + +interface Run { + readonly code: number; + readonly stdout: string; + readonly stderr: string; +} + +interface Mismatch { + readonly kind: string; + readonly direction: string; + readonly id: string; + readonly paste?: string; +} + +function run(...args: readonly string[]): Run { + try { + const stdout = execFileSync(process.execPath, [cli, ...args], { + encoding: 'utf8', + stdio: ['ignore', 'pipe', 'pipe'], + }); + return { code: 0, stdout, stderr: '' }; + } catch (error) { + const failed = error as { status?: number; stdout?: string; stderr?: string }; + return { code: failed.status ?? -1, stdout: failed.stdout ?? '', stderr: failed.stderr ?? '' }; + } +} + +/** The manifest command against a fixture manifest, with the kit resolved from this checkout. */ +const manifest = (manifestFile: string, ...extra: readonly string[]): Run => + run( + 'manifest', + fixture('manifest-plan.mjs'), + '--manifest', + manifestFile, + '--kit', + kitRoot, + ...extra + ); + +describe.skipIf(!built)('vscode-ext-kit manifest', () => { + const scratch: string[] = []; + afterEach(() => { + for (const dir of scratch.splice(0)) { + rmSync(dir, { recursive: true, force: true }); + } + }); + + it('reports every disagreement at once and exits 1', () => { + const result = manifest(fixture('manifest-drifted.json')); + + expect(result.code).toBe(1); + expect(result.stdout).toContain('disagrees with the plan in 6 place(s)'); + expect(result.stdout).toContain( + 'command "sample.refresh" is declared in src but missing from contributes.commands' + ); + expect(result.stdout).toContain('"command": "sample.refresh"'); + expect(result.stdout).toContain('command "sample.clear" is in contributes.commands'); + expect(result.stdout).toContain( + 'setting "sample.limit" defaults to 5 in the manifest and 10 in src' + ); + expect(result.stdout).toContain('setting "sample.mode" is declared in src but missing'); + expect(result.stdout).toContain('view "sample.projects" is registered in src but missing'); + expect(result.stdout).toContain( + 'view "sample.other" is contributed but nothing in src registers it' + ); + }); + + it('prints the disagreements as JSON with --format json', () => { + const result = manifest(fixture('manifest-drifted.json'), '--format', 'json'); + + expect(result.code).toBe(1); + const mismatches = JSON.parse(result.stdout) as Mismatch[]; + expect(mismatches.map((m) => `${m.kind}:${m.direction}:${m.id}`)).toEqual([ + 'command:missing-in-manifest:sample.refresh', + 'command:missing-in-src:sample.clear', + 'setting:drift:sample.limit', + 'setting:missing-in-manifest:sample.mode', + 'view:missing-in-manifest:sample.projects', + 'view:missing-in-src:sample.other', + ]); + }); + + it('exits 0 with a summary when the manifest agrees with the plan', () => { + const result = manifest(fixture('manifest-agreeing.json')); + + expect(result.code).toBe(0); + expect(result.stdout.trim()).toBe('manifest ok: 1 command(s), 2 setting(s), 1 view(s) agree'); + }); + + it('adds missing commands and settings with --apply, and leaves the rest to a person', () => { + const dir = mkdtempSync(join(tmpdir(), 'vek-manifest-')); + scratch.push(dir); + const copy = join(dir, 'package.json'); + writeFileSync(copy, readFileSync(fixture('manifest-drifted.json'), 'utf8'), 'utf8'); + + const result = manifest(copy, '--apply'); + + expect(result.code).toBe(1); + expect(result.stdout).toContain('applied 2 change(s)'); + expect(result.stdout).toContain('4 disagreement(s) need a person'); + + const written = readFileSync(copy, 'utf8'); + const updated = JSON.parse(written) as { + contributes: { + commands: { command: string; title: string }[]; + configuration: { properties: Record }; + views: Record; + }; + }; + expect(updated.contributes.commands).toEqual([ + { command: 'sample.clear', title: 'Clear' }, + { command: 'sample.refresh', title: 'TODO' }, + ]); + expect(updated.contributes.configuration.properties['sample.mode']).toMatchObject({ + type: 'string', + default: 'fast', + enum: ['fast', 'thorough'], + description: '%TODO: describe this setting%', + }); + // Drift is reported, not resolved: the manifest's default stays until a + // person decides which side is right. + expect(updated.contributes.configuration.properties['sample.limit']?.default).toBe(5); + // A view needs a container, which the declaration does not carry. + expect(updated.contributes.views['explorer']?.map((view) => view.id)).toEqual(['sample.other']); + // The file keeps its two-space indentation and trailing newline. + expect(written.startsWith('{\n "name"')).toBe(true); + expect(written.endsWith('}\n')).toBe(true); + + // A second run has nothing left to apply. + const again = manifest(copy, '--apply'); + expect(again.stdout).toContain('applied 0 change(s)'); + }); + + it('explains a missing manifest as a usage error', () => { + const result = manifest(fixture('does-not-exist.json')); + + expect(result.code).toBe(2); + expect(result.stderr).toContain('does not exist'); + expect(result.stderr).toContain('--manifest'); + }); +}); diff --git a/tests/cli/plan.test.ts b/tests/cli/plan.test.ts index 0ba520e..eaa6ec4 100644 --- a/tests/cli/plan.test.ts +++ b/tests/cli/plan.test.ts @@ -117,6 +117,6 @@ describe.skipIf(!built)('vscode-ext-kit plan', () => { expect(result.code).toBe(2); expect(result.stderr).toContain('unknown option --colour'); - expect(result.stderr).toContain('usage: vscode-ext-kit plan'); + expect(result.stderr).toContain('usage: vscode-ext-kit '); }); }); diff --git a/tests/foundation/application/describe.test.ts b/tests/foundation/application/describe.test.ts index fa49daf..fe339fa 100644 --- a/tests/foundation/application/describe.test.ts +++ b/tests/foundation/application/describe.test.ts @@ -240,6 +240,7 @@ describe('describePlan', () => { { section: 'sample.projects', policy: 'lenient', + contributed: true, values: [ { key: 'sample.projects.enabled', diff --git a/tests/node-shims.d.ts b/tests/node-shims.d.ts index dd8356b..f18daa6 100644 --- a/tests/node-shims.d.ts +++ b/tests/node-shims.d.ts @@ -22,6 +22,13 @@ declare module 'node:fs' { export function readdirSync(path: string, options: { withFileTypes: true }): Dirent[]; export function readFileSync(path: string, encoding: 'utf8'): string; export function existsSync(path: string): boolean; + export function writeFileSync(path: string, data: string, encoding: 'utf8'): void; + export function mkdtempSync(prefix: string): string; + export function rmSync(path: string, options: { recursive: boolean; force: boolean }): void; +} + +declare module 'node:os' { + export function tmpdir(): string; } declare module 'node:path' { diff --git a/tests/testing/manifest.test.ts b/tests/testing/manifest.test.ts index 568a888..1deeb58 100644 --- a/tests/testing/manifest.test.ts +++ b/tests/testing/manifest.test.ts @@ -69,6 +69,21 @@ describe('assertManifestMatches', () => { }).not.toThrow(); }); + it('does not ask the manifest for a section the extension only reads', () => { + // `editor.tabSize` is VS Code's. Declaring it gives the extension the same + // typed accessor; `contributed: false` says the manifest must not carry it. + const Editor = defineSettings({ + section: 'editor', + values: { tabSize: setting.integer({ default: 4 }) }, + contributed: false, + }); + + expect(() => { + assertManifestMatches(agreeingManifest(), { ...declared, settings: [Options, Editor] }); + }).not.toThrow(); + expect(diffManifest(agreeingManifest(), { settings: [Editor] })).toEqual([]); + }); + it('names a command src declares and the manifest is missing, with the JSON to paste', () => { const manifest = agreeingManifest() as { contributes: { commands: unknown[] } }; manifest.contributes.commands = [{ command: 'sample.refresh', title: 'Refresh' }]; From bf97514bce00298832948eb678a6b3c8d7e4e12c Mon Sep 17 00:00:00 2001 From: kkdev92 <112151103+kkdev92@users.noreply.github.com> Date: Sun, 30 Aug 2026 00:55:16 +0900 Subject: [PATCH 2/2] fix(cli): read the manifest and explain a missing file afterwards Checking that the file exists and then reading it leaves a window in which it can change; CodeQL calls it js/file-system-race. The read is the only check that means anything, so a missing file is now recognised from the read's own ENOENT. Co-Authored-By: Claude Fable 5 --- bin/vscode-ext-kit.mjs | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-) diff --git a/bin/vscode-ext-kit.mjs b/bin/vscode-ext-kit.mjs index 2c1f41d..9af3b3c 100644 --- a/bin/vscode-ext-kit.mjs +++ b/bin/vscode-ext-kit.mjs @@ -467,12 +467,20 @@ function applyMismatches(manifest, mismatches) { async function runManifest(options, description) { const format = formatOf(options, 'manifest', ['text', 'json']); const manifestPath = resolvePath(options.manifest ?? 'package.json'); - if (!existsSync(manifestPath)) { - throw new UsageError( - `${manifestPath} does not exist; point --manifest at the extension's package.json` - ); + // Read first and explain a missing file afterwards, rather than checking for + // it and then reading: the file can change between the two, and the read is + // the only check that means anything. + let text; + try { + text = readFileSync(manifestPath, 'utf8'); + } catch (error) { + if (error?.code === 'ENOENT') { + throw new UsageError( + `${manifestPath} does not exist; point --manifest at the extension's package.json` + ); + } + throw error; } - const text = readFileSync(manifestPath, 'utf8'); const manifest = JSON.parse(text); const { diffManifest } = await loadKit(options.kit, './testing', 'diffManifest'); const declared = declaredContributions(description);