Skip to content

fix: include transitive deps in transitive_files for packages with lifecycle hooks - #3067

Open
pgoldberg wants to merge 3 commits into
aspect-build:mainfrom
pgoldberg:fix/lifecycle-transitive-files
Open

pgoldberg wants to merge 3 commits into
aspect-build:mainfrom
pgoldberg:fix/lifecycle-transitive-files

Conversation

@pgoldberg

@pgoldberg pgoldberg commented Oct 28, 2026 •

Copy link
Copy Markdown

Since #2559, packages with lifecycle hooks only get direct dependencies in deps, but npm_package_store assumes that any package whose store target doesn't have src (which still are both packages with lifecycle hooks and packages with circular deps) have their entire transitive closure in deps. It only sets transitive_files to the direct dependencies' files for those packages, which means that now, packages with lifecycle hooks have incomplete transitive_files.


Changes are visible to end-users: yes

  • Searched for relevant documentation and updated as needed: already documented, this is just fixing behavior
  • Breaking change (forces users to change their own code or config): no
  • Suggested release notes appear below: yes

NpmPackageStoreInfo.transitive_files now includes the transitive deps of npm packages that have lifecycle hooks.

Test plan

  • New test cases added: See commits 2 (adds an e2e test) and 3 (adds tests for transitive_files on NpmPackageStoreInfo) on this branch
  • Manual testing; please provide instructions so we can reproduce:

minimal repro here: https://github.com/pgoldberg/rules_js_lifecycle_transitive_repro

@chatgpt-codex-connector

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 ⚠️ Failed 2026-10-28T14:31:44.248395Z
We couldn’t complete this review. Comment @codex review to try again.
61dc823 PR opened
ℹ️ 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

Copy link
Copy Markdown

Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits.
Credits must be used to enable repository wide code reviews.

@CLAassistant

CLAassistant commented Oct 28, 2026 •

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

@acozzette

Copy link
Copy Markdown
Contributor

@pgoldberg Thank you for this fix. Are you OK with signing the CLA?

@pgoldberg

Copy link
Copy Markdown
Author

Hey @acozzette, just signed it, thanks!

@acozzette

Copy link
Copy Markdown
Contributor

@pgoldberg I'm not able to see your repro, I wonder if it needs to be made publicly visible?

@pgoldberg

Copy link
Copy Markdown
Author

@acozzette Sorry about that — I just made it public!

@jbedard

jbedard commented Oct 29, 2026

Copy link
Copy Markdown
Contributor

Can we get that scenario as a test case within this PR? If something is broken it needs a test case to ensure it doesn't break again...

Note we need to be vary cautious touching this stuff and I'll have to take a deeper look next week once you have a test in this PR...

@pgoldberg

Copy link
Copy Markdown
Author

Hey @jbedard, thanks for taking a look! I added a new e2e test that mirrors the setup in my minimal repro, as well as a more direct test that transitive_files includes indirect dependencies (and more broadly checks that transitive_files is the same set of files as npm_sources for third-party packages).

The e2e test and lifecycle_transitive_files_test both fail on main today. basic_transitive_files_test and circular_transitive_files_test do pass on main today, but I added them for completeness/as a check that this change doesn't regress anything there.

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