docs: rewrite how-resolver-works.md to STE rules by elharo · Pull Request #2046 · apache/maven-resolver

elharo · 2026-08-08T11:43:00Z

Along with the usual style improvements:

Much more precise and careful definitions of terms. The previous definition of artifact was simply wrong.

elharo · 2026-08-08T11:43:43Z

CI seems borked:

[13](https://github.com/apache/maven-resolver/actions/runs/31255258160/job/93097599126?pr=2046#step:12:914)
Error: [ERROR] /Users/runner/work/maven-resolver/maven-resolver/maven-resolver-impl/src/test/java/org/eclipse/aether/internal/impl/DefaultRepositorySystemReentrancyTest.java:[361,17] incompatible types: no instance(s) of type variable(s) T exist so that java.util.List<T> conforms to java.util.Map<java.lang.String,org.eclipse.aether.spi.validator.ValidatorFactory>
Error: [ERROR] /Users/runner/work/maven-resolver/maven-resolver/maven-resolver-impl/src/test/java/org/eclipse/aether/internal/impl/DefaultRepositorySystemReentrancyTest.java:[398,17] incompatible types: no instance(s) of type variable(s) T exist so that java.util.List<T> conforms to java.util.Map<java.lang.String,org.eclipse.aether.spi.validator.ValidatorFactory>
Error: [ERROR] /Users/runner/work/maven-resolver/maven-resolver/maven-resolver-impl/src/test/java/org/eclipse/aether/internal/impl/DefaultRepositorySystemReentrancyTest.java:[457,17] incompatible types: no instance(s) of type variable(s) T exist so that java.util.List<T> conforms to java.util.Map<java.lang.String,org.eclipse.aether.spi.validator.ValidatorFactory>
[INFO] 3 errors

gnodet

The prose improvements are welcome, but there are several factual accuracy issues that need fixing before this can be merged:

resolveArtifacts description is wrong — it does not build a flattened list; flattening is done by doFlattenDependencyNodes inside resolveDependencies
TODO left in published docs — "TODO: do they give the same result?" on line 119
Grammar error — "also be resolved" missing "can" on line 68
"binary resource" is inaccurate — POMs are text/XML; the Artifact Javadoc says "identifying coordinates and optionally a file"
Dirty graph omits cycles — CollectResult explicitly tracks List<DependencyCycle>
<aside> HTML tag — not used anywhere else in site markdown, rendering risk with Doxia

This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.

Claude Code on behalf of gnodet

gnodet · 2026-08-08T13:13:00Z

-* Method `DependencyResult resolveDependencies(RepositorySystemSession session, DependencyRequest request)` performs both 
-collection and resolution steps.
+* `CollectResult collectDependencies(RepositorySystemSession session, CollectRequest request)` performs the dependency collection step. It builds the dependency graph and resolves conflicts in that graph before returning.
+* `List<ArtifactResult> resolveArtifacts(RepositorySystemSession session, Collection<? extends ArtifactRequest> requests)` performs the artifact resolution step. It builds a flattened list and downloads artifacts before returning.


This description is factually incorrect. resolveArtifacts takes a pre-supplied Collection<? extends ArtifactRequest> and resolves their paths — it does not build a flattened list. DefaultRepositorySystem.resolveArtifacts (lines 308-334) delegates directly to artifactResolver.resolveArtifacts(session, requests). Flattening is done only inside resolveDependencies via doFlattenDependencyNodes (line 421-422).

Suggested:

Suggested change

* `List<ArtifactResult> resolveArtifacts(RepositorySystemSession session, Collection<? extends ArtifactRequest> requests)` performs the artifact resolution step. It builds a flattened list and downloads artifacts before returning.

* `List<ArtifactResult> resolveArtifacts(RepositorySystemSession session, Collection<? extends ArtifactRequest> requests)` performs only the artifact resolution step. It resolves the paths for a collection of artifacts, downloading them from remote repositories if necessary.

Notice above where it says, "Resolver also usually resolves artifacts as it builds the flattened list.
The name for this combined step is artifact resolution." Is this also wrong? (it might be.) This was originally "The "flattening" and "artifact resolving" are also usually done together, and we use for those the term "artifact resolution"."

gnodet · 2026-08-08T13:13:01Z

+Resolver 2.x has two conflict resolution implementations.
+The legacy implementation does multiple graph passes.
+The faster path-based implementation does a single graph pass.
+TODO: do they give the same result?


This TODO should not appear in published documentation. Either answer the question and replace with a factual statement, or remove it.

Co-authored-by: Guillaume Nodet <gnodet@gmail.com>

gnodet

Thanks for fixing the grammar error and adding cycles to the dirty graph description. Four issues from the previous review remain:

TODO in published docs (line 119) — still present
resolveArtifacts description (line 148) — still says "builds a flattened list"; it takes a pre-supplied Collection and resolves paths, no flattening
"binary resource" (line 34) — POMs are text/XML, not binary
<aside> tag (line 67) — not used elsewhere in site markdown

This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.

Claude Code on behalf of gnodet

gnodet

Thanks for removing the TODO. Three issues remain from previous reviews:

resolveArtifacts description — still says "builds a flattened list"; it takes a pre-supplied Collection and resolves each artifact in it
"binary resource" — POMs are text/XML artifacts, not binary; suggest dropping "binary"
<aside> tag — not used elsewhere in site markdown; a blockquote (> ...) would be safer for Doxia rendering

This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.

Claude Code on behalf of gnodet

gnodet

The STE rewrite improves prose quality throughout. Several factual inaccuracies remain — see inline comments.

Additional notes:

"you can collect the dirty graph for inspection and skip conflict resolution" (line 169) contradicts the earlier statement that collectDependencies "resolves conflicts in that graph before returning" (line 160). To get the unresolved graph, you would need to configure a null DependencyGraphTransformer — this is not explained.
"Resolver also usually resolves artifacts as it builds the flattened list" (line 149) implies interleaving, but in DefaultRepositorySystem.resolveDependencies(), flattening completes first (doFlattenDependencyNodes at line 422), then artifact resolution runs on the result (line 438). They are sequential steps.

This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.

Claude Code on behalf of Guillaume Nodet

gnodet · 2026-08-15T14:04:03Z

-* Method `DependencyResult resolveDependencies(RepositorySystemSession session, DependencyRequest request)` performs both 
-collection and resolution steps.
+* `CollectResult collectDependencies(RepositorySystemSession session, CollectRequest request)` performs the dependency collection step. It builds the dependency graph and resolves conflicts in that graph before returning.
+* `List<ArtifactResult> resolveArtifacts(RepositorySystemSession session, Collection<? extends ArtifactRequest> requests)` performs the artifact resolution step. It builds a flattened list and downloads artifacts before returning.


resolveArtifacts does not build a flattened list. Its signature takes a pre-supplied Collection<? extends ArtifactRequest> and resolves the path for each artifact in that collection. Flattening is done by doFlattenDependencyNodes inside resolveDependencies, not resolveArtifacts. The Javadoc says: "Resolves the paths for a collection of artifacts."

This has been flagged in previous reviews.

Suggested change

* `List<ArtifactResult> resolveArtifacts(RepositorySystemSession session, Collection<? extends ArtifactRequest> requests)` performs the artifact resolution step. It builds a flattened list and downloads artifacts before returning.

* `List<ArtifactResult> resolveArtifacts(RepositorySystemSession session, Collection<? extends ArtifactRequest> requests)` performs the artifact resolution step. It resolves the path for each artifact in the provided collection, downloading from remote repositories if necessary.

Notice lines 148-149, or line 114 in the original. This might be wrong, it probably is, but it's not enough to just change it here. What else needs to be done?

gnodet · 2026-08-15T14:04:03Z

-Furthermore, there are extension points like `WorkspaceReader` that can make artifacts resolvable 
-without installing or deploying them, but that is an integration detail (like Maven does by exposing reactor projects).
+**Artifacts** and **repositories** are at the core of Resolver.
+An *artifact* is a binary resource with Maven coordinates.


"binary resource" is inaccurate — POM files are text/XML artifacts. The Artifact Javadoc says: "an artifact has identifying coordinates and optionally a file that denotes its data." Dropping "binary" would fix this.

Suggested change

An *artifact* is a binary resource with Maven coordinates.

An *artifact* is a resource identified by Maven coordinates.

It is a binary resource. Maven loads it as bytes without processing or decoding text.

gnodet · 2026-08-15T14:04:03Z

+To make an artifact resolvable from the local repository, you install it.
+To make an artifact resolvable from a remote repository, you deploy it.
+
+<aside>


The <aside> HTML tag is not used anywhere else in the project's site markdown. Doxia (Maven's site renderer) may not render it properly. A standard markdown blockquote would be safer and more consistent.

This is not a block quote. It is an aside. We may not use this particular tag yet, but I think we do use pass-through HTML and Doxia does support it.

Co-authored-by: Guillaume Nodet <gnodet@gmail.com>

gnodet

Re-review (2nd pass): Thank you for addressing the "resolving" definition (now correctly says "storing it locally"). One high-severity factual issue remains from the previous review, plus a minor rendering note.

This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.

Claude Code on behalf of Guillaume Nodet

gnodet · 2026-08-18T14:34:53Z

-* Method `DependencyResult resolveDependencies(RepositorySystemSession session, DependencyRequest request)` performs both 
-collection and resolution steps.
+* `CollectResult collectDependencies(RepositorySystemSession session, CollectRequest request)` performs the dependency collection step. It builds the dependency graph and resolves conflicts in that graph before returning.
+* `List<ArtifactResult> resolveArtifacts(RepositorySystemSession session, Collection<ArtifactRequest> requests)` performs the artifact resolution step. It builds a flattened list and downloads artifacts before returning.


This description is still inaccurate. resolveArtifacts does NOT build a flattened list — it takes a pre-supplied Collection<? extends ArtifactRequest> and resolves the path for each artifact, downloading from remote repositories if necessary.

The source code at DefaultRepositorySystem.java:308-334 confirms: resolveArtifacts simply validates requests and delegates to artifactResolver.resolveArtifacts(session, requests). Flattening is performed by doFlattenDependencyNodes inside resolveDependencies (a different method).

The Javadoc says: "Resolves the paths for a collection of artifacts."

Suggested fix:

Suggested change

* `List<ArtifactResult> resolveArtifacts(RepositorySystemSession session, Collection<ArtifactRequest> requests)` performs the artifact resolution step. It builds a flattened list and downloads artifacts before returning.

* `List<ArtifactResult> resolveArtifacts(RepositorySystemSession session, Collection<ArtifactRequest> requests)` performs the artifact resolution step. It resolves the path for each artifact in the provided collection, downloading from remote repositories if necessary.

gnodet · 2026-08-18T14:34:53Z

+To make an artifact resolvable from the local repository, you install it.
+To make an artifact resolvable from a remote repository, you deploy it.
+
+<aside>


Minor: <aside> is not used in any other Markdown file in the project's site. The Maven site skin likely has no CSS styling for it, so it would render identically to surrounding text with no visual distinction. A blockquote (> ...) would be safer and consistent with the rest of the docs.

elharo

Is there a way to stop Claude from reviewing this? At this point it is just repeating itself and not responsive to comments. Human review and discussion on the remaining points is needed.

AI BOT, human not responding

docs: rewrite how-resolver-works.md to STE rules

1d7b06b

elharo added the documentation Improvements or additions to documentation label Aug 8, 2026

elharo added 9 commits August 8, 2026 12:00

Update how-resolver-works.md

077e251

Update how-resolver-works.md

fd0d4db

Update how-resolver-works.md

91f8c4c

Update how-resolver-works.md

48132ab

Update how-resolver-works.md

c3d4c41

Update how-resolver-works.md

b1b8bf7

Update how-resolver-works.md

c7fa016

Update how-resolver-works.md

11d8bb2

Update how-resolver-works.md

9a6dc9b

elharo marked this pull request as ready for review August 8, 2026 13:03

elharo requested a review from cstamas August 8, 2026 13:03

Merge branch 'master' into docs/ste-how-resolver-works

cf7da4f

gnodet reviewed Aug 8, 2026

View reviewed changes

elharo and others added 2 commits August 8, 2026 10:29

Update how-resolver-works.md

fc11af2

Apply suggestion from @gnodet

dd61dce

Co-authored-by: Guillaume Nodet <gnodet@gmail.com>

gnodet reviewed Aug 8, 2026

View reviewed changes

remove todo

cd188ef

gnodet reviewed Aug 8, 2026

View reviewed changes

gnodet previously requested changes Aug 15, 2026

View reviewed changes

elharo and others added 3 commits August 18, 2026 10:17

Apply suggestion from @gnodet

41148b0

Co-authored-by: Guillaume Nodet <gnodet@gmail.com>

Update how-resolver-works.md

fef43a0

Update how-resolver-works.md

aa80239

gnodet reviewed Aug 18, 2026

View reviewed changes

elharo commented Aug 18, 2026

View reviewed changes

	* `List<ArtifactResult> resolveArtifacts(RepositorySystemSession session, Collection<? extends ArtifactRequest> requests)` performs the artifact resolution step. It builds a flattened list and downloads artifacts before returning.
	* `List<ArtifactResult> resolveArtifacts(RepositorySystemSession session, Collection<? extends ArtifactRequest> requests)` performs only the artifact resolution step. It resolves the paths for a collection of artifacts, downloading them from remote repositories if necessary.

	An artifact is a binary resource with Maven coordinates.
	An artifact is a resource identified by Maven coordinates.

Uh oh!

Conversation

elharo commented Aug 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elharo commented Aug 8, 2026

Uh oh!

gnodet left a comment

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!

Uh oh!

Uh oh!

Uh oh!

gnodet left a comment

Choose a reason for hiding this comment

Uh oh!

gnodet left a comment

Choose a reason for hiding this comment

Uh oh!

gnodet left a comment

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!

Uh oh!

gnodet left a comment

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!

elharo left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

elharo commented Aug 8, 2026 •

edited

Loading