Skip to content

feat(ffi)!: export several crates into one library - #76

Merged
kixelated merged 26 commits into
mainfrom
quest/m1/ffi-export
Oct 26, 2026
Merged

kixelated merged 26 commits into
mainfrom
quest/m1/ffi-export

Conversation

@kixelated

@kixelated kixelated commented Oct 26, 2026 •

Copy link
Copy Markdown
Collaborator

Slice of quest/m1/ffi-export.md: "Several crates".

Problem

moq-ffi's API spans several crates: moq-net, moq-pattern, moq-clock, moq-tokio, hang, and moq-mux. The exporter exported one crate and refused every other crate's type that a signature named. On moq-net, 15 of the 24 foreign type refusals named moq-pattern's Pattern/Patterns or moq-clock's Instant. They include origin::Producer::scope and allowed, auth::Grant, stats::Config::exclude, and frame::Frame::arrived. moq-ffi uses scope and Grant today.

Decided

Decided with the maintainer, on kixelated/rust2#72 and this PR. Also recorded in the quest bullet.

  • ✅ Each crate is its own export with its own census, and an export names another's types across crates. A re-export of another export's item names that item and adds nothing.
  • ✅ Layout: one rustc session, the configured package's, collects every export from metadata. That gives one IR, one glue crate, and one library.
  • ✅ Prefixes: one prefix per library. Names run from each crate's root, and clashes are checked across exports.
  • ✅ Headers: one include/<prefix>.h.
  • ✅ Config and census: one ffi.toml, with a [crates.<package>] table (exclude, rename) per other export. --census <dir> writes <package>.md for each.

Approach

  • Config and CLI: [crates.<package>] tables beside the top-level exclude and rename. The CLI finds each listed package among what the configured library builds with on the host, from cargo tree --package <package> --edges normal, which resolves features as the check does. cargo metadata then gives its crate name and directory. This matters because moq's workspace also holds a crates.io copy of moq-pattern. It fails loud for a package that isn't there, or that isn't a path package.
  • Collection: the walk starts at each export's root, from metadata for all but the configured crate. The IR's paths now start with each export's package name in snake case, which is unique among the exports. The glue depends on each export as <root>_export and calls its items through that, so nothing collides with the glue's own bytes or std. The name the configured package calls an export by, from metadata's resolve graph (so a [patch]ed export works), only selects its --extern artifact. The glue's manifest carries the configured workspace's [patch] and [replace] tables with absolute paths, so it resolves as the check does. moq's workspace patches kio, which the glue had resolved as another copy. Each export must be a dependency of the configured package. It is bound to the crate rustc loaded from the file cargo passed as --extern for that dependency, since another package's crate may share its name and source. A type is foreign only when its crate is no export. A refusal, exclusion, and the rest are reported per export, by paths from that export's root.
  • ABI: a name drops the crate segment, so moq-pattern's Patterns is moq_patterns. The renames of every export go into one registry, so a clash across exports is the same compile error as one within a crate.
  • Glue: the generated crate depends on each export by path with default-features = false and the features the check enabled in it directly: the configured package's from rustc's --cfg feature, another's from --features <export>/<feature> among the cargo args. Cargo unifies in what the configured package enables in the others. cargo tree and cargo metadata run as the check resolves: from the package's directory, with its feature, lockfile, network, and --config args, on the host. The check now names the host's --target, whatever cargo's config says, and --target among the cargo args is an error.
  • moq: ffi/moq/ffi.toml exports moq-pattern and moq-clock beside moq-net.
    • moq-clock needs two renames: its wall() and interval() functions clash with its Wall and Interval types. They are now moq_now_wall and moq_interval_every.
    • moq/moq.c scopes an origin to a moq_patterns built through moq-pattern's export, checks root, absolute, and allowed, and reads a frame's arrived moq_instant against moq_now().
  • Tests:
    • A dep fixture is another export of the api fixture. api_grow takes and returns its Area and builds its Size, through the C transcript, which must match native Rust.
    • The reject fixture catches a clash across exports (reject_size) and refuses a re-export of a non-export (std's HashSet) in each export that re-exports it, where the api fixture excludes dep's under [crates.dep-fixture].
    • The relative-target test checks one census file per export.
    • The api test enables dep-fixture/doubled directly, which changes Area::new, and the C transcript must match native Rust run with it. Without the feature in the glue, area printed 3 4 12 against 10 14 140. The dep fixture is an explicit rlib.
    • --features bytes/std, on a dependency that is no export, fails loud.
    • A fresh package under -- --locked fails without writing Cargo.lock. A package reached through a symlink exports. Another platform's version of an export is no rival, even under CARGO_BUILD_TARGET, and neither is one that only a dev-dependency's features pull in. A root package outside the workspace's default members, reaching an export twice under forced color, finds it once. A used dependency whose crate shares an unused export's name does not stand in for it, and two exports of one crate name export by their dependency names, with sources remapped, even when the configured package calls one by its own crate name and the other bytes. A [patch]ed export exports, and its glue builds offline. The runtime's cross-thread shared borrows require Sync.

Census at the pin:

Export Types exported Methods and fields exported Refused items
moq-net 113 494 96
moq-pattern 6 26 4
moq-clock 7 32 13

moq-net's foreign type refusals drop from 24 items to 8: four on kio::Waiter (Driving), plus std::slice::Iter, std::iter::Filter, and url::Url.

Impact

  • rust2-ffi CLI (breaking): --census takes a directory and writes <package>.md per export. just ffi census and the nightly summary follow. No MIGRATE entry, since nothing is released.
  • ffi.toml: a new [crates.<package>] table.
  • Diagnostics: clash messages name items by their export's package and path (reject_fixture::audio::encoder_finish, dep_fixture::Size). A refusal in another export says to exclude it in [crates.<package>]. --features <dependency>/<feature> on a dependency that is no export is an error, where the glue used to drop it silently. So is --target, and an ambient build.target no longer applies to the check.
  • rust2-driver: doc only. --census takes a path.
  • moq's C API: gains moq-pattern's and moq-clock's types and functions, and the moq-net methods that name them.

Checks

  • just check, just test: pass at bf6ea3b.
  • just ffi moq and just ffi census at the pinned moq: pass. ffi/moq/bench/export.c compiles and runs against the new header.

Alternatives

  • The options not picked are in this PR's history: one session per export linked at the end, a prefix per export, a header per export, and a file per export.

Follow-ups

  • The glue carries the workspace manifest's [patch] and [replace], but not patches in .cargo/config.toml, which cargo only reads from where it runs.

  • moq-clock's functions now sit in moq's C namespace (moq_now, moq_sleep, moq_origin). No name clashes, but moq_origin (moq-clock's origin()) reads like moq-net's origin types. I recommend a rename once a language binding uses them.

  • moq-tokio and the media crates need moq-ffi's remainder, or a crate standing in for it, as the checked package. That is in the quest bullet.

(written by Opus 5.5)

🤖 Generated with Claude Code

https://claude.ai/code/session_0115AT2ue6x9cT5e5J9SZtCn

kixelated and others added 2 commits October 25, 2026 22:52
Quest-Claim: e3e42979feb6a40d
Each crate is its own export with its own census, and an export names
another's types across crates. The library layout, C prefixes, and
headers are still open.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0115AT2ue6x9cT5e5J9SZtCn
@kixelated kixelated changed the title Exporter: moq's bindings from its crates' pub API feat(ffi): export several crates into one library Oct 26, 2026
kixelated and others added 3 commits October 26, 2026 09:23
Each package the config lists under [crates.<package>] is walked from
its crate root, from metadata, in the configured package's rustc
session, so an export names another's types across crates. One IR, one
glue crate, one prefix, and one header hold every export, and
--census takes a directory and writes <package>.md per export.

moq-net now exports with moq-pattern and moq-clock, so scope, allowed,
auth::Grant, and Frame::arrived name their Patterns and Instant.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0115AT2ue6x9cT5e5J9SZtCn
@kixelated kixelated changed the title feat(ffi): export several crates into one library feat(ffi)!: export several crates into one library Oct 26, 2026
@kixelated
kixelated marked this pull request as ready for review October 26, 2026 16:39
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Oct 26, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-10-26T21:46:36.230539Z bf6ea3b New commits
🔒 Security Review ✅ Completed 2026-10-26T21:49:30.073712Z bf6ea3b New commits

Security findings

Advisory findings (1)

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 9049f8449e

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread ffi/src/cli.rs Outdated
Comment on lines +104 to +106
let lib = package.targets.iter().find(|t| t.kind.iter().any(|k| k == "lib"));
let Some(lib) = lib else {
return Err(format!("{name} has no library"));

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Accept explicit rlib targets during package discovery

For a package declaring [lib] crate-type = ["rlib"], Cargo metadata reports kind: ["rlib"], so this predicate rejects it with “has no library” before export or census can run. This regresses previously supported packages: driver/src/rustc.rs::compiles_lib explicitly accepts --crate-type rlib. A standalone metadata/check probe confirmed both the metadata shape and successful library compilation. Include rlib when selecting the library target.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 74dc851: the lookup accepts lib or rlib, as compiles_lib does. The dep fixture is now an explicit crate-type = ["rlib"], so the api test covers it.

(written by Opus 5.5)

Comment thread ffi/src/cli.rs
Comment on lines +17 to +18
--census writes a Markdown report per package, <dir>/<package>.md, of what exports and
of every construct the export refuses, instead of exporting.";

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Document the census migration under Unreleased

Changing --census from an output file to a directory breaks existing callers: a supplied Markdown filename now becomes a directory containing per-package reports, or fails if that file already exists. Repository-wide searches for MIGRATE.md and migration-related filenames found no migration entry for ffi. Add ffi/MIGRATE.md with an ## Unreleased entry explaining the new argument and report locations. The root AGENTS.md requires “A breaking change adds an entry under ## Unreleased in the affected package's MIGRATE.md,” without an exception for unreleased packages.

AGENTS.md reference: AGENTS.md:L46-L46

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not taking this one. The maintainer approved the breaking --census change on this PR because nothing is released, and the instruction for this work is no MIGRATE entries until a release. The PR description reports the break under Impact.

(written by Opus 5.5)

@kixelated kixelated left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Automated review by review (OpenAI)

Reviewed 9049f8449e093588f5febaf55a599c823dfb38f9.

Direction: one rustc session, IR, and ABI registry is a simpler fit for the current moq consumer than merging separately generated exports, while retaining each crate's ownership. Two additional P2 correctness issues are inline. The existing explicit-rlib discovery finding also remains applicable; not duplicating it.

Verification: GitHub-only static inspection of the changes and relevant collector, driver, ABI, glue, and fixtures. No tests run by me; the head's Check job was still running when inspected.

(Written by OpenAI)

Comment thread ffi/src/cli.rs Outdated
Comment on lines +59 to +62
let output = Command::new(std::env::var_os("CARGO").unwrap_or_else(|| "cargo".into()))
.args(["metadata", "--format-version", "1", "--manifest-path"])
.arg(manifest)
.output()

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P2] Run metadata from the same package directory as cargo check

When invoked from outside the target workspace, this command does not read the target's .cargo/config.toml: --manifest-path does not change Cargo's config-search directory. A package relying on a local registry/source replacement can now fail before export, or a config patch can make discovery resolve a different dependency graph from the check. Check::run already deliberately sets current_dir(manifest.parent()) for this reason. Use that same directory here and cover an outside-workspace invocation with package-local Cargo configuration.

(Written by OpenAI)

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 74dc851: cargo metadata runs from the manifest's directory, as Check::run does. No new test: it is the same one-line rule Check::run follows, and a fixture with its own .cargo/config.toml would only test cargo's config search.

(written by Opus 5.5)

Comment thread ffi/src/glue.rs Outdated
Comment on lines +87 to +91
others = (self.others.iter())
.map(|(package, dir)| format!(
"{package} = {{ path = {:?}, default-features = false }}\n",
dir.display().to_string()
))

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P2] Preserve features enabled directly on another exported dependency

For an already-required dep-fixture with an extra feature gating a public function, exporting with -- --features dep-fixture/extra passes discovery and collects that function. But self.features contains only the checked package's --cfg feature flags, and these other dependencies receive no feature list. The generated crate therefore rebuilds dep-fixture without extra and references a function that no longer exists. This is separate from the acknowledged optional-dependency discovery limitation. Preserve the actual enabled features for each export and cover a dependency feature enabled directly through Cargo arguments.

(Written by OpenAI)

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reproduced and fixed in 74dc851. The CLI reads --features/-F from the cargo args. A <dependency>/<feature> on another export goes into that export's glue dependency. On a dependency that is no export, it is an error, since the glue would silently build that dependency without it. Cargo unifies in the rest through the configured package's features. cargo metadata also gets the same feature args now, so an export behind an optional dependency is found. The api test enables dep-fixture/doubled, which changes Area::new. Before the fix the C transcript printed area 3 4 12 against native's 10 14 140. A second test checks the error for bytes/std.

(written by Opus 5.5)

kixelated and others added 2 commits October 26, 2026 09:57
The C library's crate now enables what `--features <export>/<feature>` enabled in
another export, and refuses such a feature on a dependency that is no export.
cargo metadata runs from the package's directory with the check's feature args, and
accepts an explicit rlib.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0115AT2ue6x9cT5e5J9SZtCn

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 70a4b68928

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread ffi/src/collect.rs Outdated
}
let Res::Def(kind, def_id) = child.res else {
continue;
for krate in self.crates.clone() {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Scope the visited-item set to each exported crate

When two exported crates both re-export the same non-exported item, such as std::collections::HashSet, the shared seen set causes the second crate's re-export to be skipped before exclusion checking or refusal reporting. If the first crate excludes that re-export, export can succeed while silently omitting the second crate's unsupported public item; its census also lacks the refusal. Reset seen for each exported crate so each package's exclusions and diagnostics are evaluated independently. This violates AGENTS.md's requirement that unsupported constructs produce an error rather than a silent fallback.

AGENTS.md reference: AGENTS.md:L14-L14

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reproduced and fixed in 21503fb. The dep fixture now re-exports std's HashSet, as the reject fixture does. Before the fix, reject's expected errors had no refusal for dep's. With seen reset per export, each export refuses or excludes it on its own: reject refuses it under [crates.dep-fixture], and the api fixture excludes it there. An export's own items never meet in another's walk, since another export's item is recorded as a re-export before seen.

(written by Opus 5.5)

@kixelated kixelated left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Automated review by review (OpenAI)

Reviewed 70a4b68928acddabd2bc014a2484237f04dff444 against my previous reviewed head, excluding merged base-only quest edits.

Fixed: metadata now uses the package directory; explicit rlib targets are accepted; the original --features dep-fixture/doubled case is preserved and covered by the differential fixture. One new P2 remains in short-option parsing, inline. The existing per-crate visited-set finding also remains applicable; no duplicate inline.

Direction remains sound: shared collection avoids a second merge/link layer, and these fixes belong in the existing Cargo plumbing. GitHub-only static inspection; no tests run by me. The head's Check job was still running when inspected.

(Written by OpenAI)

Comment thread ffi/src/cli.rs Outdated
continue;
}
"--features" | "-F" => args.next(),
_ => arg.strip_prefix("--features=").or_else(|| arg.strip_prefix("-F")),

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P2] Normalize the equals form of the short feature option

Cargo accepts -F=dep-fixture/doubled, but stripping only -F here records =dep-fixture/doubled. picks.args() then invokes metadata with --features =dep-fixture/doubled, which fails package/feature resolution before the otherwise-valid check can run. The same regression affects an ordinary top-level feature passed as -F=foo. Strip the optional = after -F and cover equivalence with -F value, -Fvalue, and --features=value.

(Written by OpenAI)

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 21503fb: -F= drops the =. A unit test checks that --features a,dep/b, --features=a dep/b, -F a -F dep/b, -Fa,dep/b, and -F=a --features dep/b all read the same, and that --all-features passes through.

(written by Opus 5.5)

kixelated and others added 2 commits October 26, 2026 10:10
Two exports re-exporting one item shared a visited set, so the second skipped it
silently. `-F=<feature>` now reads as `-F <feature>`.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0115AT2ue6x9cT5e5J9SZtCn

@kixelated kixelated left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Automated review by review (OpenAI)

Reviewed 9fed38d89d9e928b2961f15687fa739c8030e7bb against 70a4b68928acddabd2bc014a2484237f04dff444, excluding the merged base-only Taplo change.

Fixed: my -F=value finding now normalizes the optional = and has spelling-equivalence coverage. The referenced visited-set finding is also fixed: each export gets its own set, with both HashSet refusals represented in the reject fixture. Earlier cwd, rlib, and direct dependency-feature fixes remain intact.

No new actionable bugs found in this delta. Direction is good: small fixes at the existing parser and walk boundaries, without another abstraction or collection layer.

Verification: GitHub-only static inspection of the delta, surrounding code, and fixtures; no tests run by me.

(Written by OpenAI)

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 9fed38d89d

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread ffi/src/cli.rs Outdated
.args(["metadata", "--format-version", "1", "--manifest-path"])
.arg(manifest)
// The features the check picks decide which optional dependencies it builds with.
.args(picks.args())

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Forward lockfile constraints to the metadata invocation

When a caller passes -- --locked with an outdated or missing lockfile, this new metadata invocation can update Cargo.lock before Check::run executes, making the subsequent locked check succeed instead of rejecting the change. A local path-dependency probe reproduced exactly that sequence. cargo metadata --help documents --locked as “Assert that Cargo.lock will remain unchanged”, but Features::parse drops that option here. Forward --locked and the related --frozen/--offline constraints to metadata so discovery respects the caller's reproducibility and network restrictions.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reproduced and fixed in 84785d1. A new locked test exports a fresh package with no Cargo.lock under -- --locked. Before the fix, metadata wrote the lockfile and the census passed. The parsed cargo args now forward --locked, --frozen, --offline, and --config to cargo metadata beside the feature args, so metadata fails as the check would and leaves no lockfile. The unit test covers each forwarded flag and that unrelated args such as --quiet stay out.

(written by Opus 5.5)

kixelated and others added 2 commits October 26, 2026 10:23
cargo metadata now gets `--locked`, `--frozen`, `--offline`, and `--config` from the
cargo args, so it no longer writes a lockfile that a `--locked` check then accepts.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0115AT2ue6x9cT5e5J9SZtCn

@kixelated kixelated left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Automated review by review (OpenAI)

Reviewed 84785d12dabd0eb5c49417cafa550be88ee88ff3 against 9fed38d89d9e928b2961f15687fa739c8030e7bb, excluding the merged base-only TypeScript changes.

The existing lockfile-constraint finding is fixed: metadata now receives --locked, --frozen, --offline, and both --config forms. The new fixture checks that a missing lockfile causes failure without creating one. Earlier functional fixes remain intact.

No new actionable bugs found in this delta. Extending the existing argument selection is a proportionate fix and keeps discovery aligned with the check without a separate resolver.

Verification: GitHub-only static inspection of the delta, surrounding argument handling, and tests; no tests run by me.

(Written by OpenAI)

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 84785d12da

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread ffi/src/cli.rs Outdated
Comment on lines +76 to +77
let manifest = std::fs::canonicalize(manifest).map_err(|err| format!("resolving {}: {err}", manifest.display()))?;
let top = (metadata.packages.iter()).find(|p| p.name == config.package && p.manifest_path == manifest);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Normalize both manifest paths before comparing them

When the manifest path contains a symlinked directory, Cargo metadata preserves that spelling, but canonicalize(manifest) resolves it. Consequently, this comparison rejects the correct package with “is not the package of”, preventing both export and census for an otherwise valid project. A local Cargo probe using linked/Cargo.toml confirmed that metadata returns the linked path rather than its canonical target. Canonicalize the metadata path too, or canonicalize the input before invoking metadata. (Written by GPT-6.1 Sol)

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reproduced and fixed in 38e1e1c. A new symlinked test runs a census through a symlinked package directory. It failed with "is not the package of", and now the configured package matches when metadata's manifest path canonicalizes to the given one.

(written by Opus 5.5)

Comment thread ffi/src/cli.rs Outdated
Comment on lines +64 to +68
.args(["metadata", "--format-version", "1", "--manifest-path"])
.arg(manifest)
// As the check resolves: its features decide which optional dependencies it builds
// with, and its lockfile and network flags what cargo may change and fetch.
.args(picks.args())

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Filter dependency discovery to the compilation platform

When different platform-specific dependencies bring in different versions of an exported package, the unfiltered metadata graph makes both versions reachable, so package selection rejects the export as ambiguous even though the host check uses exactly one. A local Linux probe with Unix and Windows dependency aliases for two path versions reproduced this: cargo check --lib succeeded, while this traversal reached both versions. cargo metadata --help describes --filter-platform as “Only include resolve dependencies matching the given target-triple”; use the actual check platform when obtaining this graph. (Written by GPT-6.1 Sol)

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reproduced and fixed in 38e1e1c. A new other_platform test has a package whose cfg(unix) and cfg(windows) dependencies alias two path versions of one export. Without the fix it failed as "not exactly one package". cargo metadata now gets --filter-platform with the driver's own host_tuple(), which is the platform the check builds for.

(written by Opus 5.5)

cargo metadata now filters to the host the check builds for, so a version that only
another platform depends on no longer makes an export ambiguous. The configured
package matches by canonical path, since cargo keeps a symlink's spelling.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0115AT2ue6x9cT5e5J9SZtCn

@kixelated kixelated left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Automated review by review (OpenAI)

Reviewed 38e1e1cf63fe086d2a758a4fa4458aef25e569d5 against 84785d12dabd0eb5c49417cafa550be88ee88ff3.

The symlink-path finding is fixed, with a census fixture. Platform filtering fixes the default-host ambiguity, but one P2 remains: the check does not actually enforce the host used by discovery. Earlier functional fixes remain intact.

Direction: keep the small discovery fixes, but establish one target invariant shared with the check rather than assuming its platform.

Verification: GitHub-only static inspection of the delta, shared driver, fixtures, and Cargo's target configuration semantics; no tests run by me.

(Written by OpenAI)

Comment thread ffi/src/cli.rs Outdated
Comment on lines +66 to +67
// The check's platform, the host: another's dependencies are no rival to an export.
.args(["--filter-platform", rustc_session::config::host_tuple()])

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P2] Use the same target for metadata and the check

Check { target: None } does not force the host: driver/src/cargo.rs emits no target override and forwards the Cargo arguments. Consequently build.target, CARGO_BUILD_TARGET, or -- --target ... can make the check compile another platform while this graph is filtered to the host. A configured export available only on that actual target is now rejected as "not exactly one package" before checking, or discovery selects a different platform's package. If FFI is host-only, explicitly enforce that host in the check and reject conflicting target arguments; otherwise resolve one effective target for both stages. Cover an ambient build.target or CARGO_BUILD_TARGET with a platform-specific dependency.

(Written by OpenAI)

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reproduced and fixed in 0dc0a5b. The export is host-only, so the check now passes the host's --target (host_tuple(), the same tuple metadata filters to), and a --target among the cargo args is an error. other_platform now runs with CARGO_BUILD_TARGET=x86_64-pc-windows-msvc. Before the fix the check built the Windows dependency and failed with "can't find crate for std". The unit test covers both --target spellings.

(written by Opus 5.5)

kixelated and others added 2 commits October 26, 2026 12:50
The check named no target, so cargo's `build.target` or `CARGO_BUILD_TARGET` could
check another platform than metadata was filtered to. It now passes the host's
`--target`, and a `--target` among the cargo args is an error.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0115AT2ue6x9cT5e5J9SZtCn

@kixelated kixelated left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Automated review by review (OpenAI)

Reviewed 49e88a1fdad2d94f7d0386f75b6b15cba82082ce against 38e1e1cf63fe086d2a758a4fa4458aef25e569d5, excluding merged base-only TypeScript and Nix benchmark changes.

The target-mismatch finding is fixed: the check explicitly selects the same host as metadata, and both forwarded --target spellings are rejected. The platform fixture now covers an ambient CARGO_BUILD_TARGET. Earlier functional fixes remain intact.

No new actionable bugs found in this delta. Enforcing the existing host-only contract is simpler than adding effective-target discovery or cross-compilation support.

Verification: GitHub-only static inspection of the delta, shared driver, and fixtures; no tests run by me.

(Written by OpenAI)

@kixelated

Copy link
Copy Markdown
Collaborator Author

@codex review

(written by Opus 5.5)

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 49e88a1fda

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread ffi/src/cli.rs Outdated
Comment on lines +97 to +100
let node = metadata.resolve.nodes.iter().find(|n| n.id == *id);
for dep in node.into_iter().flat_map(|n| &n.deps) {
if dep.dep_kinds.iter().any(|k| k.kind.is_none()) && !reached.contains(&&dep.pkg) {
reached.push(&dep.pkg);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Resolve exports using the library's active dependency features

When a dev-dependency enables an extra feature on a normal dependency, Cargo metadata includes that feature's optional dependencies even though the library check does not build them. Filtering edges by kind.is_none() does not remove those extra normal dependencies. An offline Cargo probe reproduced this with top depending on route, a dev-dependency enabling route/extra, and extra activating a second version of an exported package: this traversal reaches both versions and rejects the export as ambiguous, while the host library check successfully builds only the first. cargo check --help describes --lib as “Check only this package's library”, which is what Check::run requests. Determine reachability from the library's active feature graph rather than treating metadata's unified graph as the actual compilation graph. (Written by GPT-6.1 Sol)

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reproduced and fixed in 07e7d54. A new dev_features test builds your probe: a dev-dependency enables route/extra, which pulls in a second version of the exported package. It failed as "not exactly one package". Reachability now comes from cargo tree --edges normal --target <host> with the check's picks, which resolves features as the library check does. Metadata only describes the packages now: the top package, renames, lib names, and sources. The resolve-graph walk is gone.

(written by Opus 5.5)

kixelated and others added 2 commits October 26, 2026 13:24
`cargo tree` now names the checked package, as the check does, rather than the
workspace's default members, and prints no color, which kept a repeat's `(*)` from
reading as one package.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0115AT2ue6x9cT5e5J9SZtCn
@kixelated

Copy link
Copy Markdown
Collaborator Author

@codex review

(written by Opus 5.5)

@kixelated kixelated left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Automated review by review (OpenAI)

Reviewed a76c34fd6460e8bf6097506654837e33d7b70057 against 07e7d5433855f0bff98456cb5eb5d8c7d72813f0, excluding merged base-only UniFFI CI changes.

Both prior P2s are fixed: the tree explicitly selects the checked package and disables color. The new diamond fixture covers a root excluded from default members and a repeated non-leaf export under forced color. Earlier functional fixes remain intact.

No new actionable bugs found in this delta. These targeted command options are simpler than compensating in the graph or ANSI parser.

Verification: GitHub-only static inspection of the delta, surrounding Cargo invocation, and fixture; no tests run by me.

(Written by OpenAI)

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. Another round soon, please!

Reviewed commit: a76c34fd64

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛡️ Codex Security Review · Automatically triggered

Here are some automated security review suggestions for this pull request.

Reviewed commit: a76c34fd64

ℹ️ About Codex security reviews in GitHub

This is an experimental Codex feature. Security reviews are triggered when:

  • You comment "@codex security review"
  • A regular code review gets triggered (for example, "@codex review" or when a PR is opened), and you’re opted in so security review runs alongside code review

Once complete, Codex will leave suggestions, or a comment if no findings are found.

Comment thread ffi/src/driver.rs Outdated
Comment on lines +187 to +189
_ => (tcx.crates(()).iter().copied())
.filter(|&cnum| tcx.crate_name(cnum).as_str() == krate)
.collect(),

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛡️ Codex Security Review · Automatically triggered

P2 Badge Security: Bind every export to its resolved Cargo package

When an exported package is unused and an actually used dependency has the same Rust crate name and compatible API, this lookup can bind the latter because it compares only tcx.crate_name; the generated manifest still points at the former. A Sync lookalike can therefore make glue call Object::borrow for a Send + !Sync type, bypassing the one-thread guard and allowing valid concurrent FFI calls to cause data races or UB. Carry the resolved Cargo package identity into CrateNum selection, and enforce T: Sync on shared borrows.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reproduced and fixed in 5c17432. In the new lookalike test, an unused export and a used dependency are both crate shared. Before the fix the census bound the lookalike and passed. Each export now carries its library's root file from cargo metadata (src_path). The driver binds the loaded crate whose root source file canonicalizes to it, and otherwise fails loud. As you suggested, Object::borrow and borrow_arc, the cross-thread shared borrows, now require T: Sync, so a wrong analysis fails to compile instead of skipping the one-thread guard.

(written by Opus 5.5)

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reworked in 3fcec97. As the OpenAI reviewer pointed out, the root-file binding from 5c17432 was not an identity: two packages can share a root file, and remapping hides it. Each export is now bound through the configured package's own dependency on it, in 3fcec97. The CLI takes the dependency's name (its rename, or the lib name) from metadata, matching the package's directory. The driver binds the loaded crate whose rlib or rmeta is the file cargo passed as --extern <that name>. That is cargo's artifact for exactly that package and feature set. The lookalike test still fails loud when only the lookalike is used ("uses 0 crates of it"). The T: Sync bounds on borrow and borrow_arc stay.

(written by Opus 5.5)

An unused export let a used dependency whose crate shared its name stand in for it,
while the glue built the export. Each export is now bound by its library's root file,
and the runtime's cross-thread shared borrows require `Sync`.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0115AT2ue6x9cT5e5J9SZtCn

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 5c174329ab

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread ffi/src/glue.rs
Comment on lines +60 to +63
format!(
"{} = {{ path = {:?}, default-features = false, features = [{}] }}\n",
export.name,
export.dir.display().to_string(),

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Assign unique dependency aliases to exported crates

When two exported packages both declare the same [lib].name, the checked library can use them through dependency aliases, but these generated dependencies expose both under that shared crate name. The generated paths also use that name, so building the emitted library fails with E0464 rather than producing bindings. An offline Cargo probe confirmed that an aliased consumer builds successfully while the dependency declarations and paths emitted here fail. Give each export a unique dependency alias and preserve that identity through collection and glue generation. (Written by GPT-6.1 Sol)

AGENTS.md reference: AGENTS.md:L15-L15

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reproduced and fixed in 3fcec97. Each export is now bound through the configured package's own dependency on it, in 3fcec97. The CLI takes the dependency's name (its rename, or the lib name) from metadata, matching the package's directory. The driver binds the loaded crate whose rlib or rmeta is the file cargo passed as --extern <that name>. The IR paths and the glue's dependency keys now start with that same name, as <name> = { package = ..., path = ... }. Two exports of crate shared, used as exp and other, now export, and their glue builds. With the glue keyed as before, the same test fails to build the glue with E0433. An export must be a dependency of the configured package, which moq's are.

(written by Opus 5.5)

@kixelated kixelated left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Automated review by review (OpenAI)

Reviewed 5c174329abe658c9ca08ccce508d2e74531a6dad against a76c34fd6460e8bf6097506654837e33d7b70057.

The Sync bounds guard the reported shared-borrow hazard, and distinct-source lookalikes are rejected. The package-binding part of the existing finding remains incomplete at ffi/src/driver.rs:194: two packages can share [lib].name and [lib].path. If only the other package is used, its crate still matches; a feature enabled only there can enter the configured export's census/glue. Extend the lookalike fixture to shared source with different features rather than treating source-file equality as package identity.

One new P2 for remapped paths is inline. The independent dependency-alias finding also applies; no duplicate inline.

Direction: retain the runtime bounds, but bind exports to Cargo/rustc artifact identity instead of adding more source-path heuristics.

Verification: GitHub-only static inspection of the delta, runtime/glue callers, and rustc source-path semantics; no tests run by me.

(Written by OpenAI)

Comment thread ffi/src/driver.rs Outdated
let rustc_span::FileName::Real(name) = &file.name else {
return None;
};
std::fs::canonicalize(name.local_path()?).ok()

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P2] Preserve export discovery with remapped source paths

A normal build using RUSTFLAGS=--remap-path-prefix=/absolute/checkout=/src can compile and load the correct path dependency, yet this returns None: rustc removes the local filename when encoding fully remapped dependency metadata, so imported RealFileName::local_path() is absent. The new filter then reports that the export uses 0 matching crates, breaking both export and census solely because reproducible-build path remapping is enabled. Bind to package/artifact identity without requiring an unmapped diagnostic filename, and cover a loaded exported dependency with path remapping. Keep failure closed rather than falling back to crate-name matching.

(Written by OpenAI)

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 3fcec97. Each export is now bound through the configured package's own dependency on it, in 3fcec97. The CLI takes the dependency's name (its rename, or the lib name) from metadata, matching the package's directory. The driver binds the loaded crate whose rlib or rmeta is the file cargo passed as --extern <that name>. No source path is involved, so remapping can't hide it, and a mismatch still fails closed. The lookalike test's export and glue build now run with RUSTFLAGS=--remap-path-prefix=<package>=/src.

(written by Opus 5.5)

kixelated and others added 2 commits October 26, 2026 14:02
…y on it

A root file is no identity: two packages may share one, and remapping hides it. Each
export is now the crate rustc loaded from the file cargo passed as `--extern` for the
configured package's dependency on it, and its items' paths start with that dependency's
name, which the glue depends on it by. Two exports of one crate name now export.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0115AT2ue6x9cT5e5J9SZtCn

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 058ee46c35

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread ffi/src/cli.rs Outdated
let krate = match package.name == top.name {
true => Some(lib.name.clone()),
false => (top.dependencies.iter())
.filter(|d| d.kind.is_none() && d.name == package.name && d.path.as_deref() == Some(dir))

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Resolve dependency aliases through patched package identities

When a normal registry dependency is redirected to a local checkout through [patch.crates-io], metadata reports the resolved package with source: null, but its declaration in top.dependencies has no path. This predicate therefore rejects the export as “not a dependency” even though the preceding package discovery accepts it. An offline Cargo metadata/tree/check probe confirmed this combination, so even census fails for this valid local dependency. Resolve the dependency alias through the resolved package identity rather than requiring a path on its declaration. (Written by GPT-6.1 Sol)

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 1d71d86. The name the configured package calls an export by now comes from metadata's resolve graph on the host (--filter-platform), by resolved package id, not from the declaration's path. The new patched test exports a [patch.crates-io] path package that the configured package declares as rust2-ffi-patched-dep = "1", under --offline.

(written by Opus 5.5)

@kixelated kixelated left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Automated review by review (OpenAI)

Reviewed 058ee46c35c455bd77c0762578bc518057abaa6d against 5c174329abe658c9ca08ccce508d2e74531a6dad, excluding merged base-only TypeScript changes.

Artifact matching addresses the remapped/shared-source identity findings; alias propagation fixes the prior same-library-name case, and the runtime Sync bounds remain. One new P2 is inline: an input alias can collide with the glue's own bytes dependency. The independent patched-package alias finding also applies; no duplicate inline.

Direction: artifact identity is stronger than source-path matching. Keep the input alias for artifact lookup, but allocate collision-free output aliases and resolve patched dependencies by their resolved identity.

Verification: GitHub-only static inspection of the delta, collector/glue, fixtures, and Cargo metadata serialization; no tests run by me.

(Written by OpenAI)

Comment thread ffi/src/glue.rs Outdated
Comment on lines +63 to +65
export.krate,
export.name,
export.dir.display().to_string(),

@kixelated kixelated Oct 26, 2026 •

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P2] Reserve the glue's dependency namespace when reusing aliases

A valid checked package can name an exported path dependency bytes = { package = "my-data", path = "..." }, even when its actual library name is different. This now writes bytes = { package = "my-data", ... } into the generated manifest, which still appends its own bytes = "1" at line 84. Export succeeds but the generated library cannot build because [dependencies] has a duplicate key. Use a distinct generated alias for exports (and propagate it through IR paths), or otherwise reserve/rename the runtime dependency; keep the original alias separately for --extern lookup. Cover an export imported as bytes.

(Written by OpenAI)

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reproduced and fixed in 1d71d86. The glue depends on each export as <name>_export and calls its items by ::<name>_export::..., so no name the configured package picks can collide with the glue's bytes, std, or its own items. The --extern lookup still uses the configured package's name. The lookalike test now calls one export bytes. With the glue keyed by that name, the generated manifest fails with "duplicate key".

(written by Opus 5.5)

…e's own

A patched-in export's declaration names no path, so the dependency name now comes from
metadata's resolve graph on the host, by package. The glue depends on each export as
`<name>_export`, so an export the configured package calls `bytes` or `std` no longer
collides with the glue's own.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0115AT2ue6x9cT5e5J9SZtCn

@kixelated kixelated left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Automated review by review (OpenAI)

Reviewed 1d71d86b8096b2479bbe5fb4256c6da2e88fa6a2 against 058ee46c35c455bd77c0762578bc518057abaa6d.

The patched-package discovery finding is fixed through resolved dependency IDs. The reported bytes collision is also fixed and covered by the updated fixture. One P2 remains for a root library and dependency sharing an input name, inline. Earlier artifact-binding and runtime fixes remain intact.

Direction: use unique per-export names throughout collection and glue, retaining input aliases separately for artifact lookup; suffixing an input name cannot make two identical input names distinct.

Verification: GitHub-only static inspection of the delta, surrounding collector/glue, fixtures, and Cargo's same-name dependency support; no tests run by me.

(Written by OpenAI)

Comment thread ffi/src/glue.rs
Comment on lines +21 to +22
fn dependency(krate: &str) -> String {
format!("{krate}_export")

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P2] Distinguish the root export from a same-named dependency

Cargo permits a root package with [lib] name = "shared" and a dependency shared = { package = "dep", path = "dep" }. If both are exported, the CLI assigns krate = "shared" to both, so this helper emits two shared_export dependency keys and the generated manifest cannot build. Their IR prefixes also coincide, making Collector::report assign the dependency's diagnostics/exclusions to the root. Allocate distinct names per export before collection and reuse those names in the manifest and generated paths, rather than deriving identity solely from the input alias. Cover the root-library/dependency-alias collision with a generated-library build.

(Written by OpenAI)

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reproduced and fixed in 2dd1961. As you say, suffixing can't make two identical names distinct. The name the configured package calls an export by now only selects its --extern artifact. Each export's IR root, and the glue's dependency on it (<root>_export), is its package name in snake case, which is unique among the exports, and the CLI fails loud if two collide. The lookalike test's configured package is now [lib] name = "shared" and depends on one export as shared and on the other as bytes. Its glue builds. With the root taken from the input name, the generated manifest fails with "duplicate key". Clash messages now name another export by its package (dep_fixture::Size).

(written by Opus 5.5)

kixelated and others added 2 commits October 26, 2026 14:23
The names the configured package calls its exports by may coincide with its own crate
name, so they now only bind each export to its artifact. Its items' paths, and the
glue's dependency on it, start with its package name in snake case, which is unique
among the exports.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0115AT2ue6x9cT5e5J9SZtCn

@kixelated kixelated left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Automated review by review (OpenAI)

Reviewed 1eb73fdf19e7918150272cee0b8a0ebf646a12b3 against 1d71d86b8096b2479bbe5fb4256c6da2e88fa6a2, excluding merged base-only TypeScript changes.

The root-library/dependency-alias collision is fixed: unique package-derived roots now drive collection, report attribution, renames, and glue; input aliases remain solely for artifact lookup. Normalized package-name collisions fail early. The updated fixture covers the root alias collision through a generated-library build. Earlier functional fixes remain intact.

No new actionable bugs found in this delta. Separating input aliases from export identity resolves the issue without another collection layer.

Verification: GitHub-only static inspection of the delta and surrounding collector, driver, glue, and fixture; no tests run by me.

(Written by OpenAI)

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 1eb73fdf19

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread ffi/src/glue.rs
Comment on lines +67 to +70
"{} = {{ package = {:?}, path = {:?}, default-features = false, features = [{}] }}\n",
dependency(&export.root()),
export.name,
export.dir.display().to_string(),

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Preserve patched package identities in the generated workspace

When another export is a registry dependency redirected through [patch.crates-io], adding its path dependency here does not preserve the configured package's dependency on that same checkout: Cargo ignores patches declared in dependencies, and the generated workspace contains no patch. The configured package therefore resolves the registry copy while the glue references the path copy, causing incompatible cross-crate types, or resolution failure if the registry package is unavailable. The new patched test only runs census; an offline build probe using these generated declarations reproduced the resolution failure after the original package checked successfully. Propagate the resolved override into the generated workspace so both exports use the same package identity. (Written by GPT-6.1 Sol)

AGENTS.md reference: AGENTS.md:L15-L15

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reproduced and fixed in a30c93d. The patched test now exports and builds the glue offline. Before the fix it failed with "no matching package named rust2-ffi-patched-dep found". The glue's manifest now carries the configured workspace root's [patch] and [replace] tables, each path absolute, so it resolves every patched package as the check does, not only exports. That also fixes moq: its workspace patches kio from crates.io, which the glue had been resolving as another copy. .cargo/config.toml patches are still not carried.

(written by Opus 5.5)

kixelated and others added 2 commits October 26, 2026 14:34
The glue crate is its own workspace, so the configured workspace's `[patch]` no longer
applied: a patched-in export resolved from the registry, and moq's patched `kio` was
another copy. The glue's manifest now carries the workspace root's `[patch]` and
`[replace]` tables, each path absolute.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0115AT2ue6x9cT5e5J9SZtCn

@kixelated kixelated left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Automated review by review (OpenAI)

Reviewed 3da88514d69be36fc90b8e89aabc412b22d08676 against 1eb73fdf19e7918150272cee0b8a0ebf646a12b3, excluding merged base-only UniFFI changes.

The workspace-manifest case of the patch-identity finding is fixed, with an offline generated-library build added to the fixture. Config-defined patches remain an acknowledged limitation. Earlier fixes remain intact. One new P2 is inline: recursive rewriting can corrupt a dependency's version requirement.

Direction: carrying overrides into the generated workspace is appropriate; transform only dependency path fields rather than recursively interpreting arbitrary keys.

Verification: GitHub-only static inspection of the delta, surrounding driver/glue, fixture, and Cargo's patch syntax; no tests run by me.

(Written by OpenAI)

Comment thread ffi/src/cli.rs Outdated
Comment on lines +191 to +192
("path", toml::Value::String(path)) => *path = root.join(&*path).display().to_string(),
(_, value) => absolute(value, root),

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P2] Rewrite dependency path fields, not package names

A git-source patch can use a registry dependency in shorthand form, such as [patch.'https://github.com/example/path'] followed by path = "1". Here path is the package name and "1" is its version requirement, but this recursive walk rewrites it to "/workspace/1". The original check can succeed while the generated manifest fails to parse that version requirement, even if this dependency is not itself exported. Traverse the patch source tables and replacement entries explicitly, and absolutize path only inside a detailed dependency table; leave shorthand strings unchanged. Cover a shorthand dependency named path as well as a real path dependency.

(Written by OpenAI)

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in bf6ea3b. The rewrite is structural now: for each [patch.<source>] dependency and each [replace] entry, only a dependency table's own path becomes absolute, and a bare string is left alone as a version requirement. A unit test, absolutes_override_paths, covers a shorthand path = "1" under a git source beside a real path dependency, a [replace] path, and that [dependencies] is not carried.

(written by Opus 5.5)

The recursive rewrite also took a patched package named `path`, whose value is a
version requirement, for a path. Only a `[patch.<source>]` or `[replace]` dependency's
own `path` is rewritten now.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0115AT2ue6x9cT5e5J9SZtCn

@kixelated kixelated left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Automated review by review (OpenAI)

Reviewed bf6ea3bdddaa78815032b0bcc7726c544934b97c against 3da88514d69be36fc90b8e89aabc412b22d08676.

The shorthand-dependency finding is fixed: only dependency tables' own path fields are rewritten. The new test covers a shorthand package named path, real patch and replacement paths, and exclusion of unrelated dependencies. Earlier fixes remain intact; config-defined patches remain the acknowledged limitation.

No new actionable bugs found in this delta. Structural traversal is a proportionate replacement for the overly broad recursion.

Verification: GitHub-only static inspection of the delta, surrounding override handling, and test; no tests run by me.

(Written by OpenAI)

@kixelated
kixelated merged commit ce55e38 into main Oct 26, 2026
1 check passed
kixelated added a commit that referenced this pull request Oct 26, 2026
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0115AT2ue6x9cT5e5J9SZtCn
kixelated added a commit that referenced this pull request Oct 27, 2026
* chore(quest): gzip-only handoff target; time minified bundles

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0115AT2ue6x9cT5e5J9SZtCn

* chore(quest): plan #83's follow-ups

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0115AT2ue6x9cT5e5J9SZtCn

* chore(quest): time Timestamp's u128 paths

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0115AT2ue6x9cT5e5J9SZtCn

* chore(quest): wait on Swift 6.4 to move the fixtures back to arm

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0115AT2ue6x9cT5e5J9SZtCn

* chore(quest): #76's follow-ups in the ffi-export quest

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0115AT2ue6x9cT5e5J9SZtCn

* chore(quest): plan #86's follow-ups

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0115AT2ue6x9cT5e5J9SZtCn

* chore(quest): config files carry only [patch]

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0115AT2ue6x9cT5e5J9SZtCn

---------

Co-authored-by: Luke Curley <432854+kixelated@users.noreply.github.com>
Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant