feat(cli-v2): add fern api merge, split; drop parentpath#13164
feat(cli-v2): add fern api merge, split; drop parentpath#13164patrickthornton merged 18 commits intomainfrom
fern api merge, split; drop parentpath#13164Conversation
🌱 Seed Test SelectorSelect languages to run seed tests for:
How to use: Click the ⋯ menu above → "Edit" → check the boxes you want → click "Update comment". Tests will run automatically and snapshots will be committed to this PR. |
packages/cli/cli-v2/src/commands/api/overrides/merge/command.ts
Outdated
Show resolved
Hide resolved
Co-authored-by: graphite-app[bot] <96075541+graphite-app[bot]@users.noreply.github.com>
81db0be to
0bcc761
Compare
amckinney
left a comment
There was a problem hiding this comment.
Nice, looking good so far -- mostly just style comments on maintaining consistency with the current CLI structure, but some other notes on flag naming / functionality too.
packages/cli/cli-v2/src/commands/api/overrides/split/gitUtils.ts
Outdated
Show resolved
Hide resolved
packages/cli/cli-v2/src/commands/api/overrides/compare/command.ts
Outdated
Show resolved
Hide resolved
packages/cli/cli-v2/src/commands/api/overrides/compare/command.ts
Outdated
Show resolved
Hide resolved
packages/cli/cli-v2/src/commands/api/overrides/merge/command.ts
Outdated
Show resolved
Hide resolved
packages/cli/cli-v2/src/commands/api/overrides/merge/command.ts
Outdated
Show resolved
Hide resolved
| .option("merge-into", { | ||
| type: "string", | ||
| description: "Merge the extracted diff into this existing override file" | ||
| }) |
There was a problem hiding this comment.
Can we just specify --merge as a boolean flag since we already know where the overrides file is? We don't need them to re-specify.
There was a problem hiding this comment.
i'm accounting for the potential of there being multiple overrides files, but this UX could definitely be improved (having both --merge-into and --output is confusing)
packages/cli/cli-v2/src/commands/api/overrides/split/command.ts
Outdated
Show resolved
Hide resolved
packages/cli/cli-v2/src/commands/api/overrides/split/command.ts
Outdated
Show resolved
Hide resolved
fern api overrides merge, split, compare; drop parentpathfern api overrides merge, split; drop parentpath
amckinney
left a comment
There was a problem hiding this comment.
The stderr indentation looks a little off when playing with this myself:
$ fern api merge
/Users/alex/code/openapi/openapi.yml: no overrides or overlays configured, skipping.
No specs had overrides or overlays to merge.The successful output looks great though 👏 :
$ fern api merge
✓ Merged 1 override(s) → /Users/alex/code/demo2/openapi/openapi.ymlWe should also make sure all filepaths reported are relative paths -- not absolute. I actually need to change this in some of the fern sdk generate output, too so we can handle that in a follow-up if not here.
The automatic removal from fern.yml is also slick 🎉 :
$ fernie api merge --remove
✓ Merged 1 override(s) → /Users/alex/code/demo2/openapi/openapi.yml
Deleted /Users/alex/code/demo2/openapi/overrides.yml
fern.yml:6: removed reference to overrides.ymlI'd consider unindenting the dim output on this one (even though we use indentation elsewhere like the --strict flag suggestion).
$ fernie api merge --remove
✓ Merged 1 override(s) → /Users/alex/code/demo2/openapi/openapi.yml
Deleted /Users/alex/code/demo2/openapi/overrides.yml
fern.yml:6: removed reference to overrides.yml| try { | ||
| raw = await readFile(outputPath, "utf8"); | ||
| } catch (error: unknown) { | ||
| if (error instanceof Error && "code" in error && (error as NodeJS.ErrnoException).code === "ENOENT") { |
There was a problem hiding this comment.
Is there a cleaner way to recognize these sort of exceptions? We shouldn't need to type error: unknown above (or at least this is not very common / idiomatic).
There was a problem hiding this comment.
i'm seeing that we're on strict, so useUnknownInCatchVariables is on, which forces some kind of error: unknown somewhere (at least to my understanding). i've extracted this to a helper cause it shows up in a few places but i think the dirtiness is somewhat inevitable:
/** Narrows an unknown caught value to a Node.js system error with an error code. */
function isNodeError(error: unknown): error is NodeJS.ErrnoException {
return error instanceof Error && "code" in error;
}
/**
* Returns true if the given error is a Node.js filesystem error with code ENOENT (file not found).
*/
export function isEnoentError(error: unknown): boolean {
return isNodeError(error) && error.code === "ENOENT";
}| import { deriveOutputPath, type SplitFormat } from "./deriveOutputPath.js"; | ||
| import { generateOverlay, generateOverrides, hasChanges, type OverlayOutputAction } from "./diffSpecs.js"; | ||
|
|
||
| const execFileAsync = promisify(execFile); |
There was a problem hiding this comment.
Can we just use exec here?
There was a problem hiding this comment.
i think this is better for security; it skips the shell, which (1) we don't need and (2) would require me to do lots of escaping and shell-injection-prevention stuff
| if (await this.tryMergeOverlayIntoExistingFile(outputPath, overlay.actions)) { | ||
| context.stderr.info( | ||
| `${Icons.success} Merged ${chalk.bold(String(overlay.actions.length))} action(s) into existing ${chalk.cyan(outputPath)}` | ||
| ); | ||
| } else { |
There was a problem hiding this comment.
Race condition: The method attempts to merge actions into an existing overlay file, but if the file doesn't exist, it returns false and continues to write a new file. However, between checking for file existence (line 180-183) and writing the merged content (line 190), another process could delete or modify the file, leading to data loss or corruption.
// Add file locking or atomic write operation:
try {
raw = await readFile(outputPath, "utf8");
} catch (error: unknown) {
if (error instanceof Error && "code" in error && (error as NodeJS.ErrnoException).code === "ENOENT") {
return false;
}
throw error;
}
const existing = parseSpec(raw, outputPath);
const existingActions = Array.isArray(existing.actions) ? existing.actions : [];
existing.actions = [...existingActions, ...newActions];
// Use atomic write with temp file + rename
const tempPath = `${outputPath}.tmp`;
await writeFile(tempPath, serializeSpec(existing, outputPath));
await rename(tempPath, outputPath);Spotted by Graphite
Is this helpful? React 👍 or 👎 to let us know.
There was a problem hiding this comment.
not sure if we care about this
fern api overrides merge, split; drop parentpathfern api merge, split; drop parentpath
| if (editor != null) { | ||
| // Clean up overrides | ||
| if (overridePaths != null) { | ||
| const edit = await editor.removeOverrides(entry.specFilePath); | ||
| await this.cleanupFiles(context, overridePaths); | ||
| if (edit != null) { | ||
| const relPath = path.relative(context.cwd, await editor.getApiFilePath()); | ||
| const names = overridePaths.map((o) => path.basename(o)).join(", "); | ||
| context.stderr.info(chalk.dim(`${relPath}:${edit.line}: removed reference to ${names}`)); | ||
| } | ||
| } | ||
|
|
||
| // Clean up overlays | ||
| if (overlayPath != null) { | ||
| const edit = await editor.removeOverlay(entry.specFilePath); | ||
| await this.cleanupFiles(context, [overlayPath]); | ||
| if (edit != null) { | ||
| const relPath = path.relative(context.cwd, await editor.getApiFilePath()); | ||
| context.stderr.info( | ||
| chalk.dim(`${relPath}:${edit.line}: removed reference to ${path.basename(overlayPath)}`) | ||
| ); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Critical Bug: File deletion before YAML save creates data loss risk
The override/overlay files are deleted before editor.save() is called. If the save fails (disk full, permissions error, etc.), the override/overlay files are permanently deleted but the YAML still references them, leaving the system in an inconsistent state.
Current order:
- Delete override/overlay files (
cleanupFiles) - Save YAML changes (
editor.save()) ← if this fails, files are already gone
Fix: Move editor.save() before file deletion:
for (const entry of entries) {
// ... merge logic ...
if (editor != null) {
// Only remove references, don't delete files yet
if (overridePaths != null) {
await editor.removeOverrides(entry.specFilePath);
}
if (overlayPath != null) {
await editor.removeOverlay(entry.specFilePath);
}
}
}
if (editor != null) {
await editor.save(); // Save YAML first
}
// Delete files only after successful YAML save
for (const entry of entries) {
if (entry.overrides != null) {
await this.cleanupFiles(context, entry.overrides);
}
if (entry.overlays != null) {
await this.cleanupFiles(context, [entry.overlays]);
}
}Spotted by Graphite
Is this helpful? React 👍 or 👎 to let us know.
| return; | ||
| } | ||
|
|
||
| const format: SplitFormat = args.format ?? "overlays"; |
Description
Adds the
fern apisubcommand, in which now lie four subcommands:compareis the old command for working with overrides, which requires you to duplicate your spec, edit the second one, remember which one is the edited one and the non-edited one, and then you can generate an overrides file from the difference; we've cut it in favor of a new workflow.mergeandsplitare a new workflow, wheremergemerges API files with their overrides files, andsplitsplits off any git-tracked changes to an API file into an overrides file, restoring the source API file to its git HEAD state. The idea is thatmergeandsplitare dual or inverse operations, which we explicitly test for.These functions can work with overrides or overlays; by default,
splitgenerates overlays, which I'd prefer for their indexing and removal syntax. [Note: worth making sure we can stipulate multiple overlays for a single API file as well as multiple overrides.]The full interface is:
splitalways merges into the overrides file if it already exists. If--outputisn't specified, it defaults to[API-FILE-NAME]-overrides.ymlor[API-FILE-NAME]-overlays.yml.There's also a simplification here which could be wrong; the
parentPathparameter passed between functions I think can be cut becauseyargsdeals with that automatically.Testing
New test suite for all commands + the merge/split round-trip.