Repository navigation
feat!: forge reads native extension keys only, with forge normalize on serde (FORGE-1) - #75
Open
Swimburger wants to merge 15 commits into
Open
Swimburger wants to merge 15 commits into
Swimburger wants to merge 15 commits into
Conversation
…writes the rest `forge` reads a native `x-forge-*` vocabulary and nothing else. The extensions other tools write (`x-fern-*`, `x-stainless-skip`, `x-sensitive`) are ordinary extensions to it, and a new command, `forge normalize`, rewrites them into the native ones, so the spec that is produced is itself what gets transformed. FORGE-1. The native vocabulary: `x-forge-method-name`, `x-forge-group-name` (a dotted string or an array), `x-forge-skip`, `x-forge-availability`, `x-forge-model-name`, `x-forge-property-name`, `x-forge-parameter-name` and `x-forge-sensitive`, kept by `src/metadata/` per kind of object, alongside `x-forge-hidden`, `x-forge-require-confirmation`, `x-forge-aliases` and `x-forge-commands`. The reader asks it for a key and finds one written twice; there is no fallback and no ranking between keys. `forge normalize [--leave-old-extensions] [--check] [-v] <INPUT>...` takes files, folders (every .json file below them, except hidden ones and those in a node_modules), glob patterns, or - for standard input to standard output, and rewrites them in place. - It edits the bytes of the document and does not write it again, so whitespace, the order of keys and the text of every number stay as they were, and a diff is only the keys. It walks the document once with no tree, and checks it to be JSON on a second thread. - A key is rewritten only at the kind of object `forge` reads its native key at, so it never gets a meaning it did not have. The tables of other tools' keys are `src/normalize/vendor/`, per level, the only code that names them; a boundary test enforces it. - A native key is never replaced: when a document says the same thing twice, the native key is kept, and the other is dropped, or reported if it says something else. - A group path that is an array becomes the names joined by dots, unless a name has a dot in it. What no native key says, such as a skip that is a list of targets, is left as it is written. - What it cannot rewrite, a key written twice or a value that cannot be read, is left and warned about, with where a parser would stop. - Files are replaced by writing a new one beside them and renaming it, or in place where that is not possible, as for a hard link. A file with more than one name is rewritten once, and a link stays a link. - `--check` writes nothing and fails if anything would change. RFC 0001 describes the design, and AGENTS.md says that compatibility with other tools' extensions is sectioned off in `src/normalize/vendor/`. Breaking: a document written with other tools' keys builds only what its native keys say until it has been through `forge normalize`.
…or now Until the release that makes `forge normalize` a step to run first, `forge` runs the same rewrite on the bytes it reads, before it parses them, so a document that has not been through `forge normalize` builds what it did: the released aggregate writes the same 3,243 files of `--target ts`, byte for byte, without the step. FORGE-1. It is temporary, and only the binary does it: the engine in `src/` reads native keys and nothing else. - It is the default rewrite, so the keys it rewrites are not kept, as `forge` did not hand them on to plugins before. - It says nothing of what it leaves, as `forge` said nothing of the document before. A key written twice is an error in the words `forge` used when it read those keys itself, down to the line and column: "duplicate field `x-fern-sdk-method-name` at line 7 column 30". A value that cannot be read is left in place, and `forge` says it as it reads it. - An error `forge` finds later in the document gives the column of the document that was rewritten, which differs from the one that was read where a key was renamed earlier on the same line. The line is the same. Cost, on the 28 MB released aggregate, best of 30: the rewrite is 17.6 ms when it has 11,714 keys to rewrite and 14.4 ms when it has none, next to 41.6 ms to parse into a `Forge`. The whole `--target ts` run goes from about 252 ms to 262 ms for a normalized document and to 270 ms for a raw one, which is 7%.
…out a file named normalize Review feedback on the README and the RFC. - What no native key says is now explained with the case it is about: a skip on a property is rewritten to `x-forge-skip` with the same value, but `x-forge-skip` is for every target, so a skip for only some targets, written as a list, has no native form and is left as it is written, in both modes. On an operation that key is not read at all. The RFC and the vendor README say it with the keys; the package README, which does not name other tools' keys, says it without them. - The README and `--help` no longer say how to pass a document that is itself called `normalize`.
Review feedback: there is no need to feature gate it. `serde_json` and `memchr` are dependencies, `forge::normalize` and `forge normalize` are always built, and the `normalize` feature, with the `cfg`s of the binary, the library, the native key tables and the test target that went with it, is gone. The `cli` feature still decides whether there is a `forge` command.
Review feedback: `forge` reads native keys only, so there is one key for each thing an extension says, and no need to remember which gave a value. `Val`, its `slot` and its `set` are gone, and the fields of a `Definition` are plain values. An error that names a key names the native key of the field, which `metadata::definition` now exposes, so the messages are the same. The released aggregate still writes the same 3,243 files of `--target ts`, byte for byte.
Review feedback: expanding a glob is a shell concern, and what to skip is the caller's to say. The inputs are files, or `-` for standard input to standard output. Folder expansion, glob expansion and the exclusion of hidden directories and `node_modules` are gone, and so is the `glob` dependency. A pattern the shell did not expand is a path that is not there, and a folder is an error that says so. A file named twice, or by two names of the same file, is still rewritten once.
Review feedback: a command that modifies files should say so. Without `--write`, `forge normalize` prints one document to standard output, rewritten, and leaves the file as it is; with several files it fails and says to use `--write` or `--check`. `--write` rewrites each file in place, only if something in it changes, as it did before. `--check` still writes nothing and fails if anything would change, and `-` still reads standard input and prints. `--write` cannot be given with `--check`, nor with `-`, which is not a file to rewrite. What it says when it prints is that the keys are "rewritten in the output", so that it does not sound like a file was changed.
An alternative to the byte-level edit of the branch this is cut from, for comparison: the document is read with serde_json, the keys are rewritten in the tree, and it is written again, with its key order (`preserve_order`) and the indentation of its first indented line kept. The scanner, `Edited` and the vectored writer are gone, and so is what only they could do. It is simple, and it is not a minimal edit. What it gets wrong is in the README of the package under "Known gaps", most serious first: - a key written twice loses its first copy, with no error: `forge` itself rejects one it reads, and no longer does in a document that has a key to rewrite - a number that does not fit in 64 bits is rounded, and `1E5` and `0.10` are written `100000.0` and `0.1` - a document nested deeper than 128 levels is an error - `\r\n` is written as `\n`, an array on one line is written on several, and escapes are written as the characters - a key written with an escape is not found by the step before `forge` reads a document, which only reads a document that has the text of a key to rewrite in it - JSON with comments is an error, as is a byte order mark - about 5 times the time and 6 times the memory of the byte scanner for `forge normalize` (100 ms and 185 MB, against 20 ms and 32 MB, for a document of 28 MB), and 90 ms and 120 MB more for a run of `forge` `Report::left` and `Left` are gone: nothing can be left as it is, as the document is written again, and a key written twice is not seen. Each gap is pinned by a test in `tests/normalize/known_gaps.rs`, and the tests of what was meant to be are ignored, with `known gap` as the reason. The SDK built from the 28 MB aggregate is byte for byte the one the engine built before, in 3,243 files.
Downstream code reads the extensions that other tools write directly, so the in-memory rewrite that `forge` does before it reads a document now does what `forge normalize --leave-old-extensions` does: each key that is rewritten stays, beside the native key, as an ordinary extension that plugins are given. It was dropping them, which is what the engine did before this series: the keys it read were left out of the extensions of a method, and are now four that plugins are given (`group-name`, `method-name`, `availability` and `ignore` of the old keys). That is a change, and it is meant: it is what plugins are given once the step is gone, when the keys of other tools are ordinary extensions, and what is built on them needs them now. The step moves into the library as `normalize::before_reading`, so that `tests/normalize/` can test what a plugin is given, which the binary may not, as it may not name the keys of other tools. The binary calls it. The SDK built from the 28 MB aggregate is still byte for byte the one the engine built before, in 3,243 files, and the step costs the same: 330 ms and 284 MB for a run that takes 240 ms and 153 MB without it. `arbitrary_precision`, to keep the text of every number, was tried and is not turned on: with it, the engine's own number visitors are given a map and not a number, so `maxItems: 2.0` is an error, a number enum is not read, a default is a map, and the extensions plugins are given have the map in them. The engine has to read it first.
Found in a review of the branch. An error that `forge` finds in a document that has a key to rewrite is at a line and a column of the document as it is written again. For a file with arrays or objects on one line that is a line past the end of the file: an error `forge` gave at line 9 of an 11-line file, on main, was given at line 27. The existing test of positions only used documents on one line, which stay on one line, so it did not see it. `before_reading` now says whether it rewrote the document, and `forge` says, after the position, that it is of the document with the keys rewritten in memory, which `forge normalize --leave-old-extensions` prints. A test checks that the line it gives is a line of that document that has the key in it, and that a document that was not rewritten has no such note. The README says that the key order is kept, and no test did: the tests that were changed to compare documents as JSON do not care about order. Two tests do now, for a key that is rewritten and for one that is left, and each fails if the keys are sorted. A value that cannot be read is written as the native key, and `forge` says so: it was left as it was before. A test for that, as the ones of the old behavior were removed. Two more gaps are in the README and pinned by a test: the positions, and the memory of files that are read at once, which are held as trees: 6 files of 28 MB took 1.1 GB. A stale mention of the `normalize` feature is removed from the tests.
An audit of what is known against what is written down found gaps that
were only in conversation:
- whitespace: the indentation of the first indented line is used for the
whole file, blank lines, spaces at the end of a line and what is before
the first `{` are dropped, and the newlines at the end become one
- `-0` is written `-0.0`, which is a float and not an integer
- why the number gaps are not closed: `arbitrary_precision` makes the
engine read a number as a map
- the step before `forge` reads a document reads one that has the text of
a key of another tool that no table reads, and changes nothing
- plugins are given four extensions that the engine left out before it
read native keys only
Each one that is a gap has a test in `tests/normalize/known_gaps.rs` that
passes now, and fails when the gap is fixed. A comment says that two keys
of other tools for one native field would be won by the last, as no table
has two today.
Found in a review of what the branch still carries from the version it replaced: - the binary had a function that only called `before_reading` of the library: it calls it - the text that the step before `forge` reads a document looks for was written out a second time, apart from the prefixes that the adapters have: it is now made from them, so a new adapter is looked for - `before_reading` and `mentions_old_keys` are only for a step that is temporary, and are not in the documentation of the library
…nter The walk was written for a scanner that read a document once, and so had to walk what was in an object before it could decide what to do to the object's keys. A tree has no such need. It is now one function over the kind of object, `Kind`, and one place, `child`, that says what is read below each kind by each key: the shapes that `forge` reads are in a table and not in nine functions that call each other through function pointers. An object's own keys are rewritten before what is in it is walked. The JSON Pointer that a conflict says is kept in one buffer that is added to and cut back, and not a string made for each member that is visited, which was kept for the one conflict in a document. What `forge normalize` writes for the 28 MB aggregate is the same, byte for byte, in both modes, and so is the SDK built from it. It takes as long and as much memory.
- the nine tests of what the byte edit did, which were ignored with a reason, are removed: each is pinned by a test in `known_gaps.rs` that passes now and fails when the gap is fixed, and one more, for `forge` not rejecting a key written twice, was missing and is added - the test that fed damaged documents to the walk, written to find a scanner that panicked, is removed: what it tests now is `serde_json` - `forge normalize` reads one file at a time, as it was read many at a time because reading was cheap, and is not now: 6 files of 28 MB took 1.1 GB and now take 184 MB, in 1.1 s and not 0.3 s
Swimburger
marked this pull request as ready for review
October 28, 2026 20:45
The section on `forge normalize` had become a log of how it was built: a table of twelve gaps with the test that pins each, the cost against a scanner that is no longer in the tree, and a read that changes nothing. It now has the commands first, what is rewritten, and the step `forge` does in memory, and the known limitations are one list that is folded away, of what a user can run into: a key written twice, numbers, formatting, error positions, what is not read, and memory. The tests that pin the limitations, and the cost, are in the code and the pull request, where they belong.
srikrsna
reviewed
Oct 29, 2026
srikrsna
left a comment
srikrsna
left a comment
Collaborator
There was a problem hiding this comment.
I think it will be best if we split this into separate PRs:
- The first one should be to add the normalize command. Completely isolated change and that doesn't change existing code except the command parsing code. This can come with the RFC too. We can merge this as is and CF and others will continue to work without any changes.
- The second one should be just renaming the current set of vendor extensions that forge parses to the forge ones. This will need an accompanying PR for cf and also WIT change for wasm plugins which cf uses.
| pub(super) ignore: bool, | ||
| /// `x-forge-hidden`. | ||
| /// Whether the definition makes no method. | ||
| pub(super) skip: bool, |
Collaborator
There was a problem hiding this comment.
This will need a change in the WIT for the wasm plugin.
Contributor
Author
There was a problem hiding this comment.
We don't need to change it in WIT, but ideally we do, but in a separate breaking change.
Comment on lines
-1185
to
-1196
| keys! { | ||
| /// The keys of a [`Definition`], in an Operation Object or an alias. | ||
| DefinitionKey { | ||
| MethodName = "x-fern-sdk-method-name", | ||
| GroupNames = "x-fern-sdk-group-name", | ||
| Ignore = "x-fern-ignore", | ||
| Hidden = "x-forge-hidden", | ||
| Confirmation = "x-forge-require-confirmation", | ||
| Availability = "x-fern-availability", | ||
| } | ||
| } | ||
|
|
Collaborator
There was a problem hiding this comment.
I don't follow the changes here, why isn't this a simple rename?
Comment on lines
+142
to
+161
| if json.starts_with(b"\xEF\xBB\xBF") { | ||
| return Err(Error( | ||
| "the document starts with a byte order mark, which JSON does not have: save it as UTF-8 without one" | ||
| .to_owned(), | ||
| )); | ||
| } | ||
| if json.starts_with(b"\xFF\xFE") || json.starts_with(b"\xFE\xFF") { | ||
| return Err(Error("the document is UTF-16, and has to be UTF-8".to_owned())); | ||
| } | ||
| // Without this, a document that is not JSON, YAML for one, would be | ||
| // reported as the error of a parser that expected something else. | ||
| match json.iter().find(|byte| !matches!(byte, b' ' | b'\t' | b'\n' | b'\r')) { | ||
| None => return Err(Error("the document is empty".to_owned())), | ||
| Some(b'{') => {} | ||
| Some(_) => { | ||
| return Err(Error( | ||
| "the document is not a JSON object: it does not start with `{`".to_owned(), | ||
| )); | ||
| } | ||
| } |
Collaborator
There was a problem hiding this comment.
Why is this needed? Won't serde_json catch this?
| /// what is meant to come first. This keeps a document that has not been through | ||
| /// it building as it did, until the release that stops doing it. | ||
| #[doc(hidden)] | ||
| pub fn before_reading(json: Vec<u8>) -> Result<(Vec<u8>, bool), Error> { |
Collaborator
There was a problem hiding this comment.
We should remove this as this is not in use and trivial to add if needed
Comment on lines
+99
to
+105
| /// Returns whether `json` has the text of a key that another tool writes. A | ||
| /// document that does not has nothing to rewrite, which is told without | ||
| /// reading it. A key written with an escape is not found. | ||
| #[doc(hidden)] | ||
| pub fn mentions_old_keys(json: &[u8]) -> bool { | ||
| vendor::mentions(json) | ||
| } |
Collaborator
There was a problem hiding this comment.
Same goes for this one. See below.
Comment on lines
+60
to
+63
| /// A key that says something other than the native key beside it, which | ||
| /// wins. | ||
| #[derive(Debug)] | ||
| pub struct Conflict { |
Collaborator
There was a problem hiding this comment.
We don't have to worry about such cases because we only ever read the native keys
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
An alternative to #73, for comparison. It makes the same change, and
forge normalizedoes the same thing from the command line, but the document is read withserde_jsonand written again, in place of the byte-level edit of #73. It is simpler, and it is not a minimal edit. Which of the two to take is the decision this PR is for: if this one is taken, #73 should be closed, and if not, this one.This PR contains the commits of #73, followed by the ones that change
forge normalize. To review only the difference between the two: Swimburger/forge@forge-native-keys-only...forge-native-keys-only-serdeWhy
Review of #73 asked for no hand-written JSON scanner and a design that could take YAML later. A parser that reports where each value is, which is what editing bytes needs for YAML, has no dependency that meets what a dependency of this project has to be, so the choice for now is between #73 and this.
What changes from #73
forge normalizereads the document withserde_jsonand writes it again, with its key order (preserve_order) and the indentation of its first indented line kept. A document with nothing to rewrite is never written again.scan.rs,Editedand the vectored writer are gone, and about 900 lines with them.forgereads in a table, in place of the walk of a scanner.forgekeeps the old keys in memory: the step before it reads a document does what--leave-old-extensionsdoes, so plugins, and what is built on them, read the keys of other tools as ordinary extensions. The step isforge::normalize::before_reading, so that it can be tested.forge normalizereads one file at a time. Each file is held as a tree of about 6.5 times its size, and six files of 28 MB took 1.1 GB when they were read together.Report::leftis gone: nothing can be left as it is when the document is written again.forgefinds in a document that has a key to rewrite says that its line and column are of the document with the keys rewritten, whichforge normalize --leave-old-extensionsprints.What it costs
On the 28 MB released aggregate:
forge normalize --checkforge --target ts, document with old keysforge --target ts, nothing to rewriteWhat it gets wrong
Each of these is in the README of the package, under "Known gaps", and is pinned by a test in
tests/normalize/known_gaps.rsthat passes now and fails when the gap is fixed:forgeitself rejects one that it reads, and does not in a document that has a key to rewrite1E5is written100000.0and-0is written-0.0\r\nbecomes\n, an array on one line is written on several (3.6% of the lines of a formatted document in a test), blank lines and spaces at the end of a line are dropped, and escapes are written as the charactersforgesaysforgereads a document, and JSON with comments, or a byte order mark, is an errorKeeping the text of every number needs
arbitrary_precision, which is not on: with it the engine's own number visitors are given a map, somaxItems: 2.0is an error. The engine has to read that first, in a PR of its own.Behavior to look at
group-name,method-name,availabilityandignorekeys, which the engine left out of a method's extensions when it read them itself. That is what plugins are given once the in-memory step is gone.forgeno longer rejects a key written twice in a document that has a key to rewrite.Verification
oxfmt, clippy with-D warningsfor all features and for none, the Windows target, and 153 tests ofnormalize, with 386 ofparseforge normalizewrites the same bytes, in both modes, as before the walk was rewritten--target tsgives the same files onmain, on feat!: forge reads native extension keys only (FORGE-1) #73 and on this branch, for the cf spec (3,265 files) and for the aggregate (3,243)generate.tson the spec version that repo pins, gives the same 7,226 files, with the same hash of the tree, onmain, on feat!: forge reads native extension keys only (FORGE-1) #73 and on this branchmain, on feat!: forge reads native extension keys only (FORGE-1) #73 and on this branch when the old keys are dropped, and differs only by the four old-key extensions when they are kept