From e4315b0325b124bce47aaa6ebf9aef1f0ab37203 Mon Sep 17 00:00:00 2001 From: rejifald Date: Fri, 7 Aug 2026 18:53:12 +0300 Subject: [PATCH] fix(openapi): the ejector refuses to clobber a file it does not own (#694) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `stitch-openapi --out ` wrote every emitted file unconditionally — `cli.ts:146-147` was a bare loop over `result.files`, then `wrote N file(s)` and exit 0. No existence check, no prompt, no `--force`: grepping the whole package for `force`, `existsSync` or `overwrit` returned nothing, so there was no guard to have missed. The measured rule was "a file it emits it replaces whole, a file it does not emit it ignores", which leaves `--out ` one flag away from the destructive thing. The reported loss is the point rather than the ergonomics. An owner had added a `drift(Order)` output to the generated tree; a re-run erased it, and afterwards a response missing the `required` `currency` field went from failing to passing silently. The tool's whole claim is that the artefact is source you own, and replacing it at exit 0 with no warning contradicts that. A manifest was already being written and never read back. It is now. The `.stitch-gen.json` gains a `files` list — every path the run wrote, relative to `--out`, including the manifest itself — and the CLI reads the previous run's copy before writing anything. A target that already exists and is NOT named there belongs to the author: the run names every such file, writes nothing at all, and exits 1. A target the manifest does claim is replaced exactly as before, which keeps the ordinary regen path intact. `--force` opts back into the old behaviour, spelled the way `stitch init --force` already is. The existence probe is a new `exists` member on the CLI's IO seam, mirroring core's `CliIO`; `--dry-run` writes nothing and is untouched. The ownership graph the manifest already carried is not quite an ownership LIST: it names operation and schema files but not `client.ts`, `index.ts` or itself, and a flat-layout private schema records `.ts (inlined)`, a marker rather than a path. Hence the explicit list. A manifest written before that list exists is still honoured — derived from the graph plus those three constants — so upgrading does not cost a blanket `--force` to re-adopt a tree this generator really did produce. Also §3 of the same issue, in `planGen`: the manifest recorded the REQUESTED validator tier, so `--validator zod` durably wrote `"validator": "zod"` over a tree containing zero validators. The fallback warning is a build-time stderr line nobody rereads; the manifest is the durable artefact. It records the tier actually emitted now. Refs #694 — §2 (files orphaned by an operation dropped from the spec), §4 (dropped schema keywords) and §5 (the declared error surface) are untouched and stay open, so the issue should not close. Co-Authored-By: Claude Opus 5 --- CHANGELOG.md | 7 + packages/openapi/README.md | 5 +- packages/openapi/src/cli.ts | 107 +++++++++++- packages/openapi/src/gen-openapi.ts | 22 ++- packages/openapi/test/cli.spec.ts | 192 ++++++++++++++++++++++ packages/openapi/test/gen-openapi.spec.ts | 22 +++ 6 files changed, 343 insertions(+), 12 deletions(-) create mode 100644 packages/openapi/test/cli.spec.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index cbc0b623..fa9ca740 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -566,6 +566,13 @@ npm release are grouped under the in-development version that introduced them. ### Fixed +- **`@stitchapi/openapi` refuses to overwrite a file it did not generate.** + ([#694](https://github.com/rejifald/StitchAPI/issues/694)) A re-run wrote every emitted file + unconditionally at exit `0`, so `--out ` silently discarded + hand-written edits. It now reads back the previous run's `.stitch-gen.json` and refuses to touch + anything that manifest does not claim — naming the files, exiting non-zero — with `--force` to opt + in. That manifest also records the validator tier actually **emitted**, not the one requested. + - **Adding `cache: { ttl }` no longer turns a handled vendor failure into a process exit.** ([#670](https://github.com/rejifald/StitchAPI/issues/670)) A cached stitch whose vendor returned `503` emitted an **unhandled promise rejection**, which under Node's default diff --git a/packages/openapi/README.md b/packages/openapi/README.md index c7092bfd..b262d79f 100644 --- a/packages/openapi/README.md +++ b/packages/openapi/README.md @@ -33,6 +33,7 @@ npx @stitchapi/openapi ./openapi.json --all --dry-run | `--grep ` | only operations whose path contains this substring | | `--layout dir\|flat` | `dir` = one folder per operation (default); `flat` = one file | | `--validator ` | `types-only` (default). valibot/zod tiers are not implemented yet | +| `--force` | overwrite files it did not generate (it refuses by default) | | `--dry-run` | print the files to stdout instead of writing them | ## What it emits @@ -42,7 +43,9 @@ one stitch per operation typed via `stitch()`, and **atomic** component types placed by fan-in — a schema used by ≥2 operations goes to `_shared/`, one used by a single operation lives **inside that operation's directory** so deleting the operation deletes its private types too. A `.stitch-gen.json` manifest records the -ownership graph. +ownership graph and every path the run wrote — so a re-run replaces what it +generated last time and **refuses to overwrite anything else**, naming the files +and exiting non-zero unless you pass `--force`. ``` src/pet-client/ diff --git a/packages/openapi/src/cli.ts b/packages/openapi/src/cli.ts index 90920dce..028f6195 100644 --- a/packages/openapi/src/cli.ts +++ b/packages/openapi/src/cli.ts @@ -1,17 +1,23 @@ // `stitch-openapi` — eject a SELECTED set of operations from an OpenAPI document into ready-to-own // stitch source (ADR 0013). Thin wrapper over the pure `planGen`: parse argv, read the spec (JSON // natively; YAML via the `yaml` dependency, imported lazily), then write the files (or print them -// on --dry-run). -import { type GenOptions, type OpenApiDoc, planGen } from './gen-openapi'; - -import { mkdir, readFile, writeFile } from 'node:fs/promises'; +// on --dry-run). Writing is guarded: a file the previous run's manifest does not claim belongs to +// the author, and is never overwritten without --force. +import { + type GenOptions, + MANIFEST_FILE, + type OpenApiDoc, + planGen, +} from './gen-openapi'; + +import { access, mkdir, readFile, writeFile } from 'node:fs/promises'; import { dirname, resolve } from 'node:path'; const USAGE = `stitch-openapi — eject selected operations from an OpenAPI document into stitch source usage: stitch-openapi --out [--all | --tag | --only | --grep ] - [--layout dir|flat] [--validator types-only] [--dry-run] + [--layout dir|flat] [--validator types-only] [--force] [--dry-run] an OpenAPI 3.x document (JSON or YAML) --out, -o output directory (required unless --dry-run) @@ -21,6 +27,7 @@ usage: --grep only operations whose path contains this substring --layout dir|flat dir = one folder per operation (default); flat = one file per operation --validator types-only (default; v1). valibot/zod tiers are not implemented yet + --force overwrite files this ejector did not generate (it refuses by default) --dry-run print the files to stdout instead of writing them Ejects ready-to-own source you edit afterward: a client.ts seam, one stitch per operation typed @@ -36,6 +43,7 @@ interface Io { writeErr: (s: string) => void; readFileText: (path: string) => Promise; writeFile: (path: string, contents: string) => Promise; + exists: (path: string) => Promise; } function defaultIo(): Io { @@ -48,9 +56,68 @@ function defaultIo(): Io { await mkdir(dirname(path), { recursive: true }); await writeFile(path, contents, 'utf8'); }, + exists: async (path) => { + try { + await access(path); + return true; + } catch { + return false; + } + }, }; } +function asArray(v: unknown): unknown[] { + return Array.isArray(v) ? (v as unknown[]) : []; +} + +/** + * Paths (relative to --out) that a PREVIOUS run of this ejector wrote there, per the manifest it + * left behind. Everything else on disk belongs to the author — an edited `client.ts`, a hand-written + * helper — and is not ours to replace (#694 §1). No manifest means no previous run: own nothing. + */ +async function ownedPaths(io: Io, outDir: string): Promise> { + const owned = new Set(); + let raw: string; + try { + raw = await io.readFileText(resolve(outDir, MANIFEST_FILE)); + } catch { + return owned; + } + let parsed: unknown; + try { + parsed = JSON.parse(raw) as unknown; + } catch { + io.writeErr( + `warning: ${MANIFEST_FILE} is not valid JSON; treating every existing file as yours\n`, + ); + return owned; + } + const m = ( + typeof parsed === 'object' && parsed !== null ? parsed : {} + ) as Record; + + // The exact list, written by every run since the guard shipped. + if (Array.isArray(m['files'])) { + for (const p of asArray(m['files'])) + if (typeof p === 'string') owned.add(p); + return owned; + } + // Older manifests predate that list but still record the ownership graph, so derive from it — + // adopting a tree this ejector really did generate should not cost a blanket --force. The three + // constants are emitted unconditionally by every run (client.ts, index.ts, the manifest itself). + owned.add(MANIFEST_FILE); + owned.add('client.ts'); + owned.add('index.ts'); + for (const e of [...asArray(m['operations']), ...asArray(m['schemas'])]) { + const f = (e as { file?: unknown }).file; + // A flat-layout private schema records `.ts (inlined)` — a marker, not a path. + if (typeof f === 'string' && f !== '' && !f.endsWith('(inlined)')) + owned.add(f); + } + return owned; +} + export async function main( argv: string[], overrides: Partial = {}, @@ -64,6 +131,7 @@ export async function main( let spec: string | undefined; let out: string | undefined; let dryRun = false; + let force = false; const opts: GenOptions = {}; const tags: string[] = []; const only: string[] = []; @@ -87,7 +155,8 @@ export async function main( } else if (a === '--layout') { const v = argv[++i]; if (v) opts.layout = v as NonNullable; - } else if (a === '--dry-run') dryRun = true; + } else if (a === '--force') force = true; + else if (a === '--dry-run') dryRun = true; else if (!a.startsWith('-') && spec === undefined) spec = a; else io.writeErr(`warning: ignored unknown arg: ${a}\n`); } @@ -143,8 +212,32 @@ export async function main( } const base = out as string; + const outDir = resolve(io.cwd, base); + + // Eject, not managed regeneration (ADR 0013): the tree is the author's once written, so a re-run + // may replace only what a previous run put there. Anything else that already exists is theirs. + if (!force) { + const owned = await ownedPaths(io, outDir); + const clobbered: string[] = []; + for (const f of result.files) { + if (owned.has(f.path)) continue; // ours from last time — the normal regen path + if (await io.exists(resolve(outDir, f.path))) + clobbered.push(f.path); + } + if (clobbered.length > 0) { + io.writeErr( + `refusing to overwrite ${clobbered.length} file(s) in ${base} that this generator did not write (no record of them in ${MANIFEST_FILE}):\n`, + ); + for (const p of clobbered) io.writeErr(` ${p}\n`); + io.writeErr( + 'Nothing was written. Eject into a scratch directory and diff (ADR 0013 Decision 1), or pass --force to overwrite them.\n', + ); + return 1; + } + } + for (const f of result.files) - await io.writeFile(resolve(io.cwd, base, f.path), f.contents); + await io.writeFile(resolve(outDir, f.path), f.contents); io.writeErr(`wrote ${result.files.length} file(s) to ${base}\n`); return 0; } diff --git a/packages/openapi/src/gen-openapi.ts b/packages/openapi/src/gen-openapi.ts index 513eb983..9b01892e 100644 --- a/packages/openapi/src/gen-openapi.ts +++ b/packages/openapi/src/gen-openapi.ts @@ -93,6 +93,9 @@ const HTTP_METHODS = [ 'trace', ] as const; +/** The ownership manifest, written into --out (ADR 0013 Decision 9). Read back by the CLI. */ +export const MANIFEST_FILE = '.stitch-gen.json'; + // ---- options & result ----------------------------------------------------- export interface GenOptions { @@ -408,12 +411,17 @@ function directRefs(schema: SchemaNode | undefined, acc: Set): void { export function planGen(doc: OpenApiDoc, opts: GenOptions = {}): GenResult { const warnings: string[] = []; const notices: string[] = []; - const validator = opts.validator ?? 'types-only'; + const requested = opts.validator ?? 'types-only'; + // The tier actually EMITTED. v1 has no valibot/zod emitter, so it is always types-only — and + // the manifest, which is the durable artefact, records THIS rather than what was asked for. + // Recording the request left `"validator": "zod"` sitting over a tree with no validators in it, + // with only a build-time stderr line to contradict it (#694 §3). + const validator = 'types-only'; const layout = opts.layout ?? 'dir'; - if (validator !== 'types-only') { + if (requested !== validator) { warnings.push( - `validator "${validator}" is not implemented in v1; emitting types-only`, + `validator "${requested}" is not implemented in v1; emitting types-only`, ); } notices.push( @@ -629,6 +637,11 @@ export function planGen(doc: OpenApiDoc, opts: GenOptions = {}): GenResult { generator: 'stitch gen openapi', validator, layout, + // Every path this run writes, relative to --out, INCLUDING this manifest. It is the durable + // record of what the ejector OWNS: the CLI reads it back on the next run and refuses to + // overwrite anything absent from it (#694 §1). The ownership graph below records file names + // too, but only for operations and non-inlined schemas — this list is exact by construction. + files: [...files.map((f) => f.path), MANIFEST_FILE].sort(), operations: selected.map((o) => ({ name: o.name, method: o.method, @@ -643,7 +656,7 @@ export function planGen(doc: OpenApiDoc, opts: GenOptions = {}): GenResult { schemas: schemaManifest, }; files.push({ - path: '.stitch-gen.json', + path: MANIFEST_FILE, contents: `${JSON.stringify(manifest, null, 2)}\n`, }); @@ -966,6 +979,7 @@ function emptyManifest(validator: string, layout: string): unknown { generator: 'stitch gen openapi', validator, layout, + files: [], operations: [], schemas: [], }; diff --git a/packages/openapi/test/cli.spec.ts b/packages/openapi/test/cli.spec.ts new file mode 100644 index 00000000..c276c0be --- /dev/null +++ b/packages/openapi/test/cli.spec.ts @@ -0,0 +1,192 @@ +// The ejector's write guard (#694 §1). Eject is not managed regeneration: once written, the tree +// belongs to the author, so a re-run may replace only what a PREVIOUS run put there — which +// `.stitch-gen.json` records. Anything else that already exists is the author's, and replacing it +// silently is data loss: the reported case lost a hand-added `drift(Order)` output, after which a +// response missing a `required` field started passing. +// +// Driven through `main(argv, io)` with an in-memory IO, in the style of core's `stitch init` specs. +// One test at the bottom runs against a REAL temp directory, so the default IO's existence probe is +// covered too rather than only the fake that stands in for it. +import { main } from '../src/cli'; + +import { + mkdir, + mkdtemp, + readFile, + readdir, + rm, + writeFile, +} from 'node:fs/promises'; +import { tmpdir } from 'node:os'; +import { join, resolve } from 'node:path'; + +const SPEC = JSON.stringify({ + openapi: '3.0.0', + info: { title: 'T', version: '1' }, + servers: [{ url: 'https://api.example.com' }], + paths: { + '/orders/{id}': { + get: { operationId: 'getOrder', responses: { '200': {} } }, + }, + }, +}); + +// The same document plus a second operation, so a later run wants a file that is already on disk. +const GROWN = JSON.stringify({ + ...(JSON.parse(SPEC) as Record), + paths: { + '/orders/{id}': { + get: { operationId: 'getOrder', responses: { '200': {} } }, + }, + '/orders': { + get: { operationId: 'listOrders', responses: { '200': {} } }, + }, + }, +}); + +// What an author's own client.ts looks like: edited, and nothing like the emitted one. +const MINE = "// mine\nexport const client = 'hand-written';\n"; + +const SPEC_PATH = 'openapi.json'; +/** An emitted path as the fake filesystem keys it — `main` resolves against cwd + --out. */ +const at = (p: string): string => resolve('/repo', 'out', p); + +// A fake filesystem backing the CLI's IO: writeFile mutates the map, exists/readFileText read it. +// Returns the map (so a tree can be carried into a second run) plus captured stderr. +const runGen = ( + args: string[] = [], + seed: Record = { [SPEC_PATH]: SPEC }, +) => { + const files = new Map(Object.entries(seed)); + const err: string[] = []; + return main([SPEC_PATH, '--all', '--out', 'out', ...args], { + cwd: '/repo', + write: () => undefined, + writeErr: (s) => err.push(s), + readFileText: async (path) => { + const v = files.get(path); + if (v === undefined) throw new Error(`ENOENT: ${path}`); + return v; + }, + writeFile: async (path, contents) => { + files.set(path, contents); + }, + exists: async (path) => files.has(path), + }).then((code) => ({ code, err: err.join(''), files })); +}; + +describe('cli — refusing to clobber a file it does not own (#694 §1)', () => { + test('an existing file no manifest claims blocks the whole write, non-zero', async () => { + const { code, err, files } = await runGen([], { + [SPEC_PATH]: SPEC, + [at('client.ts')]: MINE, + }); + + expect(code).not.toBe(0); + expect(err).toMatch(/refusing to overwrite/); + expect(err).toMatch(/client\.ts/); // names the file it would have destroyed + expect(err).toMatch(/--force/); // and the way out + // Nothing was written at all — not the manifest, not the operation files. + expect(files.get(at('client.ts'))).toBe(MINE); + expect([...files.keys()]).toEqual([SPEC_PATH, at('client.ts')]); + }); + + test('--force overwrites it', async () => { + const { code, files } = await runGen(['--force'], { + [SPEC_PATH]: SPEC, + [at('client.ts')]: MINE, + }); + + expect(code).toBe(0); + expect(files.get(at('client.ts'))).not.toBe(MINE); + expect(files.get(at('client.ts'))).toMatch(/seam\(/); + expect(files.has(at('.stitch-gen.json'))).toBe(true); + }); + + test('a file the manifest owns regenerates without --force', async () => { + const first = await runGen(); + expect(first.code).toBe(0); + const emitted = first.files.get(at('client.ts')) as string; + + // Carry that tree into a second run, with an owner's edit on top. It is still OURS by the + // manifest, so the re-run replaces it — that is eject-and-diff (ADR 0013 Decision 1). + const tree = Object.fromEntries(first.files); + tree[at('client.ts')] = `${emitted}// edited by the owner\n`; + const second = await runGen([], tree); + + expect(second.code).toBe(0); + expect(second.err).not.toMatch(/refusing/); + expect(second.files.get(at('client.ts'))).toBe(emitted); + expect(second.files.get(at('get-order/index.ts'))).toMatch( + /client\.stitch/, + ); + }); + + test('one unowned file blocks the write even when the rest of the tree is owned', async () => { + const first = await runGen(); + // The spec grows an operation, and the author had already hand-written that file. + const tree = Object.fromEntries(first.files); + tree[SPEC_PATH] = GROWN; + tree[at('list-orders/index.ts')] = MINE; + + const { code, err, files } = await runGen([], tree); + + expect(code).not.toBe(0); + expect(err).toMatch(/list-orders\/index\.ts/); + expect(err).not.toMatch(/get-order\/index\.ts/); // that one is ours; only the new file is not + expect(files.get(at('list-orders/index.ts'))).toBe(MINE); + }); + + test('a manifest from before the `files` list still adopts the tree it generated', async () => { + const first = await runGen(); + const tree = Object.fromEntries(first.files); + // Strip the exact list, leaving the pre-guard shape (ownership graph only). + const legacy = JSON.parse( + tree[at('.stitch-gen.json')] as string, + ) as Record; + delete legacy['files']; + tree[at('.stitch-gen.json')] = JSON.stringify(legacy, null, 2); + + const { code, err } = await runGen([], tree); + + expect(code).toBe(0); + expect(err).not.toMatch(/refusing/); + }); + + test('--dry-run writes nothing and the guard never fires', async () => { + const { code, err, files } = await runGen(['--dry-run'], { + [SPEC_PATH]: SPEC, + [at('client.ts')]: MINE, + }); + + expect(code).toBe(0); + expect(err).not.toMatch(/refusing/); + expect([...files.keys()]).toEqual([SPEC_PATH, at('client.ts')]); + }); + + // End-to-end on a real directory: proves the default IO's existence probe sees a real file, so + // the guard is not an artefact of the fake filesystem above. + test('on a real directory, an author file survives the re-run untouched', async () => { + const dir = await mkdtemp(join(tmpdir(), 'stitch-openapi-')); + try { + await mkdir(join(dir, 'out'), { recursive: true }); + await writeFile(join(dir, 'openapi.json'), SPEC, 'utf8'); + await writeFile(join(dir, 'out', 'client.ts'), MINE, 'utf8'); + const err: string[] = []; + + const code = await main( + [join(dir, 'openapi.json'), '--all', '--out', join(dir, 'out')], + { write: () => undefined, writeErr: (s) => err.push(s) }, + ); + + expect(code).not.toBe(0); + expect(err.join('')).toMatch(/refusing to overwrite/); + expect(await readFile(join(dir, 'out', 'client.ts'), 'utf8')).toBe( + MINE, + ); + expect(await readdir(join(dir, 'out'))).toEqual(['client.ts']); + } finally { + await rm(dir, { recursive: true, force: true }); + } + }); +}); diff --git a/packages/openapi/test/gen-openapi.spec.ts b/packages/openapi/test/gen-openapi.spec.ts index 8dea0c8f..c648b8db 100644 --- a/packages/openapi/test/gen-openapi.spec.ts +++ b/packages/openapi/test/gen-openapi.spec.ts @@ -290,6 +290,28 @@ describe('planGen — naming, typing, auth, notice', () => { /runtime validation \+ drift are OFF/, ); }); + + // #694 §3: the warning is a build-time stderr line, the manifest is the DURABLE artefact. It + // recorded the requested tier, so a tree with zero validators claimed `"validator": "zod"`. + test('a requested tier that falls back records the EMITTED tier in the manifest', () => { + const r = planGen(doc, { all: true, validator: 'zod' }); + expect(r.warnings.join('\n')).toMatch( + /validator "zod" is not implemented in v1/, + ); + expect((r.manifest as { validator: string }).validator).toBe( + 'types-only', + ); + }); + + // The manifest's `files` list is what the CLI reads back to decide what it owns (#694 §1), so + // it has to be exact — every emitted path, including the manifest itself. + test('the manifest lists every path the run writes, itself included', () => { + const r = planGen(doc, { all: true }); + const listed = (r.manifest as { files: string[] }).files; + expect(listed).toEqual(r.files.map((f) => f.path).sort()); + expect(listed).toContain('.stitch-gen.json'); + expect(listed).toContain('client.ts'); + }); }); // The codegen turns an UNTRUSTED OpenAPI document into TS source the developer compiles. Spec text