Skip to content

Conversation

juxtin
Copy link
Contributor

@juxtin juxtin commented Jun 27, 2025

This is a big change from how we've been doing this, but it's much more likely to be accurate.

We typically consider a dependency direct iff it is explicitly listed as a dependency at the top level. The explicitlyReferencedComponentIds field in the Component Detection output corresponds to this, so it should be the only thing we need to check when determining if a dependency is direct or transitive.

The main risk here as I see it is an unfortunate difference in the way paths are represented in that section (dependencyGraphs) vs later on in the components:

  "dependencyGraphs": {
    "/workspaces/component-detection-dependency-submission-action/test/package.json": {
      "graph": {
        "Conda-dependency-submission-action 1.0.0 - Npm": null
      },
      "explicitlyReferencedComponentIds": [],
      "developmentDependencies": [],
      "dependencies": []
    },

vs

    {
      "locationsFoundAt": [
        "/package.json"
      ],
      "component": {
        "name": "Conda-dependency-submission-action",
        "version": "1.0.0",

So most of the time, manifest paths are given relative to the repository root. In dependencyGraphs, they're given as absolute filesystem paths. That creates the risk that we'll mess up the mapping from one to the other. For now, I think the way I've written this is likely to work, but it's a little risky still.

@juxtin juxtin requested review from a team as code owners June 27, 2025 20:33
@juxtin juxtin requested review from adrienpessu and aegilops and removed request for a team June 27, 2025 20:33
@juxtin
Copy link
Contributor Author

juxtin commented Jun 27, 2025

In my test repo, this works as expected 🎉

What I'm still concerned about:

  1. Situations where the filePath is overridden to some special value.
  2. Ecosystems other than Python/uv.

Copy link

@Ahmed3lmallah Ahmed3lmallah left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Contributor

@ljones140 ljones140 left a comment

Choose a reason for hiding this comment

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

Nice

This does feel a bit more robust that the top level referrers logic we previously went with. The trade off being now we have to worry about file path conversions

Comment on lines +131 to +134
if (referrerPackage === pkg) {
core.debug(`Skipping self-reference for package: ${pkg.id}`);
return; // Skip self-references
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

New code to eliminate self-referential paths

Copy link

@Ahmed3lmallah Ahmed3lmallah left a comment

Choose a reason for hiding this comment

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

New changes look good. Thanks!

@juxtin juxtin merged commit fc216b2 into main Jul 2, 2025
5 checks passed
@juxtin juxtin deleted the juxtin/direct-vs-transitive branch July 2, 2025 19:32
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.

4 participants