Add new version of forge by srikrsna · Pull Request #28 · cloudflare/forge

srikrsna · 2026-10-12T16:49:59Z

Add a new version of forge.

ematipico · 2026-10-13T10:09:57Z

@@ -0,0 +1,50 @@
+[workspace]
+resolver = "3"


I already envision the use of many crates, so I suggest switching right now to the use of cargo workspaces.

Usually there's a top level folder called crates, then each folder is a crate.

Here's how you can set them up https://github.com/biomejs/biome/blob/39d7595be26634ffe27e05de3011e4dc312abdb8/Cargo.toml#L3-L4

bin/forge can simply be forge or forge_cli, which is simply the main binary.

I don't see us having many crates so I used the root.

ematipico · 2026-10-13T10:13:47Z

+    /// Corresponds to `info.title`, `info.summary` and `info.description` in
+    /// the [Info Object](https://spec.openapis.org/oas/latest.html#fixed-fields-0),
+    /// joined as paragraphs with a blank line between them.
+    pub description: String,


Shouldn't this be a Info struct? Reason mostly due to the fact that we should lint the title as mandatory

ematipico · 2026-10-13T10:17:50Z

+    pub fn models(&self) -> impl ExactSizeIterator<Item = Model<'_>> {
+        self.top_models.iter().map(|&id| self.model(id))
+    }


This isn't very Rust idiomatic. Usually you have two options:

return a &[Model] and let callers use an iterator if they need

return your own iterator e.g. ModelIterator

Considering we're using a map, I suggest the latter solution. Avoid returning things that depend on trait such as ExactSizeIterator. 9 times out of 10 it's wrong

I'll use the latter, the only reason I used the trait directly is because we can swap the implementation but I don't see much use for it anyway.

Considering we're using a map,

Did you mean the .map?

The only problem I see is we'd end up with ~8 new types. Also just for my understanding can you point me to an example that illustrates why this is considered bad?

ematipico · 2026-10-13T10:22:30Z

+
+    /// Returns the groups `ids` and every group inside them, depth first,
+    /// keeping the order of `ids` and of each group's subgroups.
+    fn depth_first<'f>(&'f self, ids: &'f [GroupId]) -> impl Iterator<Item = Group<'f>> {


Ditto. Return a struct iterator that does this. Some psuedo code

struct GroupIterator<'a> { stack: &'a DoubleEndedIterator<Group> ids: &'a [GroupId] } impl<'a> Iter for GroupIterator<'a> { type Item = Group fn next() -> Option<Self::Item> { } }

You have to deal with lifetimes, but you use less memory

ematipico · 2026-10-13T10:27:27Z

+    }
+
+    fn data(self) -> &'f GroupData {
+        &self.forge.groups[self.id.index()]


This is an unsafe operation. You have two options:

use .get and return Option<GroupData>

Add a // SAFETY: ... comment that explains why it's safe to execute this accessor. You need to explain why the index doesn't overflow groups

I'll add the comment, it is safe because this is never accessed directly outside of the crate and we only loop over the ids that were derived from the vector. I'll look at the rest and decide although most of them will fall under the same category.

Reading a bit more on this. The Safety comments are meant to add reason for unsafe blocks that may otherwise cause undefined behavior. This however is not undefined behavior, it will panic. I'll leave it as is, let me know if I am missing something

It's a bit nuanced, actually. Generally it's good to leave a command, it tells other developers why it's safe to use something that could, in theory, cause a panic.

ematipico · 2026-10-13T10:36:42Z

+
+impl Name {
+    /// Returns the name written as `s`.
+    pub fn new(s: &str) -> Name {


Suggested change

pub fn new(s: &str) -> Name {

pub fn new(s: &str) -> Self {

This is the idiomatic way. Using Self helps with maintenance

ematipico · 2026-10-13T10:37:49Z

+    pub fn new(s: &str) -> Name {
+        let mut words = Vec::new();
+        // Where the word being read starts in `s`, and its last character.
+        let mut word: Option<(usize, char)> = None;


Suggested change

let mut word: Option<(usize, char)> = None;

let mut word = None;

Generally, it's idiomatic not doing early typing. The rust compiler is very good at inferring types. You add types only when the compilers tells you to.

ematipico · 2026-10-13T10:40:59Z

+pub type Err = Box<dyn std::error::Error + Send + Sync + 'static>;
+
+/// A plugin that generates files.
+pub trait Plugin: Sync {


You might want to add why a plugin is only Sync. Generally, Sync and Send go in pair, but since we added only one, it's worth the explanation. This is an architectural decision we must document.

Also, I would consider adding https://doc.rust-lang.org/std/panic/struct.AssertUnwindSafe.html

If a plugin panics, we need to be able to recover from it

ematipico · 2026-10-13T10:46:59Z

+        .iter()
+        .filter_map(|(file, _)| file.path().rsplit_once('/').map(|(parent, _)| parent))
+        .collect();
+    fs::create_dir_all(dir).map_err(|err| format!("{}: {err}", dir.display()))?;


I would start thinking about abstracting the file system. This will be very useful when running e2e tests via CLI. I have some some prior work with Biome, here's how we do it.

The trait https://github.com/biomejs/biome/blob/main/crates/biome_fs/src/fs.rs#L105

The implementations: memory and OS

The CLI tests pass e memory file system to the CLI, so that it's possible to inspect what's been written. Not a blocker, however I think it's something we must do sooner rather than later.

I thought of that, abstracting it is also quite useful to run it environments like the browser. This function is isolated and the only place where we write out the files. As soon as we need we can definitely add it.

ematipico · 2026-10-13T10:49:23Z

+    fn generate(&self, forge: &Forge) -> Result<impl Iterator<Item = File>, Err> {
+        let mut files = files(Node::Root(forge))?;
+        files.extend(RUNTIME.iter().map(|&(path, source)| File::Runtime {
+            path: RUNTIME_DIR.sub(path).as_str().to_owned(),


Suggested change

path: RUNTIME_DIR.sub(path).as_str().to_owned(),

path: RUNTIME_DIR.sub(path).as_str().to_string(),

Generally you want to avoid the use of to_owned if there's a better function that shows intent

srikrsna · 2026-10-13T11:16:41Z

@ematipico Thank you for the thorough review! Really valuable. Would you be okay to merge this as is and address these in followup PRs? CF already switched to using this version: cloudflare/cf#279 and I would like to merge as is and followup immediately.

Add new version of forge

3996601

srikrsna added this pull request to stack #29 October 12, 2026 16:50

srikrsna requested review from dimitropoulos and ematipico October 12, 2026 16:50

srikrsna added 2 commits October 12, 2026 22:28

Upgrade node version

4762fa9

Restore style of existing packages

fc5e7c9

github-advanced-security Bot found potential problems Oct 12, 2026

View reviewed changes

Comment thread src/bin/forge/ts/lib/src/transport.ts Dismissed

Comment thread src/bin/forge/ts/lib/src/transport.ts Dismissed

Comment thread src/bin/forge/ts/lib/src/transport.ts Dismissed

srikrsna requested a review from teamchong October 13, 2026 03:30

ematipico reviewed Oct 13, 2026

View reviewed changes

srikrsna mentioned this pull request Oct 13, 2026

Exclude forge pacakges from the update forge workflow cloudflare/cf#292

Closed

ematipico approved these changes Oct 13, 2026

View reviewed changes

srikrsna merged commit d738df1 into main Oct 13, 2026
7 checks passed

srikrsna deleted the sk/readd-forge branch October 13, 2026 11:45

sansynx mentioned this pull request Oct 14, 2026

fix: preserve declared response headers in operation metadata #14

Closed

	let mut word: Option<(usize, char)> = None;
	let mut word = None;

	path: RUNTIME_DIR.sub(path).as_str().to_owned(),
	path: RUNTIME_DIR.sub(path).as_str().to_string(),

Conversation

srikrsna commented Oct 12, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ematipico Oct 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

srikrsna commented Oct 13, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ematipico Oct 13, 2026 •

edited

Loading