Remote ingot dependencies by g-r-a-n-t · Pull Request #1158 · argotorg/fe

g-r-a-n-t · 2025-11-17T15:43:37Z

Added remote dependency support.

Introduced resolver/git plumbing (crates/resolver/src/git.rs, git_stub.rs) with new fixtures; extended
resolver graph/ingot logic.
Expanded common dependency graph and cache to track remote locations; reorganized display/tree code under
common/src/dependencies/tree.rs.
Driver now handles remote ingots (new ingot_handler), with updated dependency tree diagnostics and config handling.
fe tree simplified.

micahscopes · 2025-11-25T14:28:55Z

Taking a look now!

micahscopes · 2025-11-25T22:01:31Z

can you think of any obvious ways to break up tree.rs into a few smaller modules just to make it easier to navigate? not super important

micahscopes · 2025-11-25T22:02:08Z

small thought on the graph struct: the git_locations / reverse_git_map pair could be a BiHashMap to keep them in sync automatically. not blocking, just a nice-to-have

micahscopes · 2025-12-04T19:24:33Z

How's this coming since I last looked? Want me to take another look?

Cargo.toml

crates/common/src/dependencies/tree.rs

crates/driver/Cargo.toml

g-r-a-n-t · 2025-12-04T23:46:06Z

crates/common/src/ingot.rs

+                    let url = match &dependency.location {
+                        DependencyLocation::Remote(remote) => db
+                            .dependency_graph()
+                            .local_for_remote_git(db, remote)


This should be tracked in workspace.

@micahscopes I think little organizational things like this can wait for a later pr.

sounds good to me!

crates/driver/src/lib.rs

cburgdorf

Looks great. Just left one comment for a potential source of problems you might want to double check.

crates/resolver/src/git.rs

Copilot

Pull request overview

This PR adds comprehensive support for remote git dependencies to the Fe language compiler, enabling projects to depend on packages hosted in git repositories rather than only local filesystem paths.

Key Changes:

Introduced git resolution infrastructure with platform-specific implementations (native git2 and WASM stub)
Extended the dependency graph resolver to support priority-based resolution (local vs remote dependencies)
Refactored diagnostic handling to use callback-based patterns instead of accumulating diagnostics in resolvers
Reorganized dependency tree display logic and added remote edge visualization

Reviewed changes

Copilot reviewed 73 out of 76 changed files in this pull request and generated 6 comments.

Show a summary per file

File	Description
`crates/resolver/src/git.rs`	Core git repository cloning and checkout logic using libgit2
`crates/resolver/src/git_stub.rs`	WASM-compatible stub that returns unsupported errors
`crates/resolver/src/ingot.rs`	Unified resolver combining local files and remote git sources
`crates/resolver/src/graph.rs`	Added priority-based resolution and removed diagnostics field
`crates/resolver/src/lib.rs`	Refactored Resolver trait with callback-based diagnostics
`crates/driver/src/ingot_handler.rs`	New handler implementing resolution callbacks and path conversions
`crates/driver/src/lib.rs`	Simplified init_ingot to use boolean return instead of diagnostic vec
`crates/common/src/dependencies/graph.rs`	Added remote checkout tracking with bidirectional maps
`crates/common/src/dependencies/tree.rs`	Extracted tree display logic with remote edge visualization
`crates/common/src/config.rs`	Added git dependency parsing (source, rev, path fields)
`crates/common/src/cache.rs`	Platform-aware cache directory resolution for git checkouts
`crates/fe/src/tree.rs`	Simplified by delegating to DependencyTree infrastructure
Test snapshots	Updated for new tree display format and diagnostic messages

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-12-07T17:37:51Z

crates/resolver/src/ingot.rs

+        if self.git.has_valid_cached_checkout(description)
+            && let Ok((result, _)) =
+                self.load_remote_files(handler, description, checkout_path.as_path(), true)
+        {
+            return Ok(result);


Performance: The code attempts to resolve from cache first (line 320-324), then falls back to network resolution. However, when the cache check succeeds, on_resolution_start is never called (unlike the fallback path on line 328). This inconsistency could confuse handlers expecting symmetric lifecycle callbacks.

Copilot · 2025-12-07T17:37:51Z

crates/resolver/src/ingot.rs

+pub trait RemoteProgress {
+    fn start(&mut self, description: &GitDescription);
+    fn success(&mut self, description: &GitDescription, ingot_url: &Url);
+    fn error(&mut self, description: &GitDescription, error: &IngotResolutionError);
+}


Missing documentation: The RemoteProgress trait lacks documentation explaining when each method is called and what implementers should do. This is a public API (pub trait) that external code may implement.

Copilot · 2025-12-07T17:37:51Z

crates/resolver/src/git.rs

+        let repo = Repository::clone(&remote, checkout_path.as_std_path()).map_err(|error| {
+            GitResolutionError::CloneRepository {
+                source: description.source.clone(),
+                error,
+            }
+        })?;
+        self.checkout_revision(&repo, description)?;
+        Ok(CheckoutStatus::Fresh)
+    }
+
+    fn checkout_revision(
+        &self,
+        repo: &Repository,
+        description: &GitDescription,
+    ) -> Result<(), GitResolutionError> {
+        let oid = git2::Oid::from_str(&description.rev).map_err(|error| {
+            GitResolutionError::InvalidRevision {
+                rev: description.rev.clone(),
+                error,
+            }
+        })?;
+        let object =
+            repo.find_object(oid, None)
+                .map_err(|error| GitResolutionError::RevisionLookup {
+                    rev: description.rev.clone(),
+                    error,
+                })?;
+        let mut builder = CheckoutBuilder::new();
+        builder.force();
+        repo.checkout_tree(&object, Some(&mut builder))
+            .map_err(|error| GitResolutionError::Checkout {
+                rev: description.rev.clone(),
+                error,
+            })?;
+        repo.set_head_detached(oid)
+            .map_err(|error| GitResolutionError::Checkout {
+                rev: description.rev.clone(),
+                error,
+            })?;
+        Ok(())


The git2 checkout operations (clone, checkout_tree, set_head_detached) have no timeout mechanism. A malicious or very large repository could cause the process to hang indefinitely, creating a potential denial-of-service vector.

Copilot · 2025-12-07T17:37:52Z

crates/resolver/src/git.rs

+impl GitDescription {
+    pub fn new(source: Url, rev: impl Into<String>) -> Self {
+        Self {
+            source,
+            rev: rev.into(),
+            path: None,
+        }


[nitpick] The description field rev accepts any string, but the code only validates it when converting to an OID. This allows invalid revisions to be stored and propagated through the system until resolution time. Consider validating that rev looks like a valid git revision (commit hash, branch, tag) at construction time to fail fast.

Copilot · 2025-12-07T17:37:52Z

crates/common/src/cache.rs

+    env_path("FE_REMOTE_CACHE_DIR")
+        .or_else(|| {
+            env_path("FE_CACHE_DIR").map(|path| {
+                if path.ends_with("git") {


The ends_with("git") check is unreliable for determining if a path is a git cache directory. This could lead to incorrect behavior if a user sets FE_CACHE_DIR to a path ending in "git" (e.g., /home/user/my-git). Consider checking if the last component equals "git" rather than using ends_with().

Suggested change

if path.ends_with("git") {

if path.file_name().map_or(false, |name| name == "git") {

Copilot · 2025-12-07T17:37:52Z

crates/resolver/src/git.rs

+                    error,
+                })?;
+        let mut builder = CheckoutBuilder::new();
+        builder.force();


Security concern: Using .force() on CheckoutBuilder can overwrite local modifications without warning. While this may be intentional for a cache, it could lead to data loss if users mistakenly modify checked-out dependencies. Consider documenting this behavior or adding a safety mechanism.

g-r-a-n-t force-pushed the remote-graph branch 4 times, most recently from deff46d to 5331856 Compare November 22, 2025 02:03

g-r-a-n-t marked this pull request as draft November 24, 2025 18:42

g-r-a-n-t marked this pull request as ready for review November 24, 2025 19:07

g-r-a-n-t force-pushed the remote-graph branch from 53df86e to 5447788 Compare December 1, 2025 22:55

g-r-a-n-t force-pushed the remote-graph branch 2 times, most recently from 387c5e6 to 56cba26 Compare December 4, 2025 21:41

g-r-a-n-t changed the title ~~remote graph~~ Remote ingot dependencies Dec 4, 2025

g-r-a-n-t commented Dec 5, 2025

View reviewed changes

Remote ingot dependencies.

f12f566

g-r-a-n-t force-pushed the remote-graph branch from 0b4bda2 to f12f566 Compare December 5, 2025 20:47

g-r-a-n-t requested review from cburgdorf and micahscopes December 5, 2025 20:55

cburgdorf requested a review from Copilot December 7, 2025 17:27

Copilot started reviewing on behalf of cburgdorf December 7, 2025 17:27 View session

cburgdorf approved these changes Dec 7, 2025

View reviewed changes

crates/resolver/src/git.rs Show resolved Hide resolved

Copilot AI reviewed Dec 7, 2025

View reviewed changes

fetch origin when checkout lacks rev

bd2c0ac

g-r-a-n-t merged commit dbd3f3a into argotorg:master Dec 8, 2025
5 checks passed

	if path.ends_with("git") {
	if path.file_name().map_or(false, \|name\| name == "git") {

Conversation

g-r-a-n-t commented Nov 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

micahscopes commented Nov 25, 2025

Uh oh!

micahscopes commented Nov 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

micahscopes commented Nov 25, 2025

Uh oh!

micahscopes commented Dec 4, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

g-r-a-n-t Dec 4, 2025

Choose a reason for hiding this comment

Uh oh!

g-r-a-n-t Dec 5, 2025

Choose a reason for hiding this comment

Uh oh!

micahscopes Dec 8, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

cburgdorf left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Dec 7, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 7, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 7, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 7, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 7, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 7, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

g-r-a-n-t commented Nov 17, 2025 •

edited

Loading

micahscopes commented Nov 25, 2025 •

edited

Loading