Repository navigation
feat: install a JavaScript workspace's dependencies before the analysis #144
+197
−2
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,37 @@ | ||
| #!/usr/bin/env bash | ||
| # Turns on Core's install of a JavaScript workspace's dependencies and points every package | ||
| # manager's download cache at one store, which the dependency cache keeps between runs. | ||
| # | ||
| # Why Core decides the rest: it checks the head and the merge-base worktree each on its own, and | ||
| # fetches the package manager and version each one pins. Never for a fork's pull request: its | ||
| # package manager would run with this job's credentials in reach. | ||
| # | ||
| # Output: lockfile=true when the head has a lockfile, so the cache steps have something to key on. | ||
| set -euo pipefail | ||
|
|
||
| if [ "${INSTALL_DEPENDENCIES:-true}" != true ]; then | ||
| echo "Dependencies: not installed (install_dependencies is false)." | ||
| exit 0 | ||
| fi | ||
| if [ "${IS_FORK:-false}" = true ]; then | ||
| echo "Dependencies: not installed (a fork's pull request)." | ||
| exit 0 | ||
| fi | ||
|
|
||
| mkdir -p "$STORE_DIR" | ||
| { | ||
| echo "CODEBOARDING_INSTALL_DEPENDENCIES=1" | ||
| echo "npm_config_cache=$STORE_DIR/npm" | ||
| echo "npm_config_store_dir=$STORE_DIR/pnpm" | ||
| echo "BUN_INSTALL_CACHE_DIR=$STORE_DIR/bun" | ||
| echo "YARN_CACHE_FOLDER=$STORE_DIR/yarn" | ||
| echo "YARN_GLOBAL_FOLDER=$STORE_DIR/yarn-berry" | ||
| } >> "$GITHUB_ENV" | ||
|
|
||
| for name in bun.lock bun.lockb pnpm-lock.yaml yarn.lock package-lock.json npm-shrinkwrap.json; do | ||
| if [ -f "$CHECKOUT_DIR/$name" ]; then | ||
| echo "lockfile=true" >> "$GITHUB_OUTPUT" | ||
| break | ||
| fi | ||
| done | ||
| echo "Dependencies: Core installs a JavaScript workspace's dependencies, without install scripts." |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,106 @@ | ||
| """The dependency step turns Core's install on, except for a fork, and keys the cache on a lockfile.""" | ||
|
|
||
| from __future__ import annotations | ||
|
|
||
| import os | ||
| import re | ||
| import subprocess | ||
| import tempfile | ||
| import unittest | ||
| from pathlib import Path | ||
|
|
||
| ROOT = Path(__file__).resolve().parent.parent | ||
| SCRIPT = ROOT / "scripts" / "action" / "prepare-dependencies.sh" | ||
|
|
||
|
|
||
| def prepare(files: dict[str, str], install: str = "true", is_fork: str = "false") -> tuple[dict, dict]: | ||
| """Runs the step on a checkout holding *files*; returns its outputs and its exported env.""" | ||
| with tempfile.TemporaryDirectory() as tmp: | ||
| checkout = Path(tmp) / "checkout" | ||
| checkout.mkdir() | ||
| for name, text in files.items(): | ||
| (checkout / name).write_text(text, encoding="utf-8") | ||
| output, env_file = Path(tmp) / "output", Path(tmp) / "env" | ||
| output.write_text("", encoding="utf-8") | ||
| env_file.write_text("", encoding="utf-8") | ||
| subprocess.run( | ||
| [str(SCRIPT)], | ||
| env={ | ||
| "PATH": os.environ["PATH"], | ||
| "GITHUB_OUTPUT": str(output), | ||
| "GITHUB_ENV": str(env_file), | ||
| "CHECKOUT_DIR": str(checkout), | ||
| "STORE_DIR": str(Path(tmp) / "store"), | ||
| "INSTALL_DEPENDENCIES": install, | ||
| "IS_FORK": is_fork, | ||
| }, | ||
| check=True, | ||
| capture_output=True, | ||
| ) | ||
| outputs = dict(line.split("=", 1) for line in output.read_text(encoding="utf-8").splitlines()) | ||
| exported = dict(line.split("=", 1) for line in env_file.read_text(encoding="utf-8").splitlines()) | ||
| return outputs, exported | ||
|
|
||
|
|
||
| class PrepareDependenciesTests(unittest.TestCase): | ||
| def test_core_is_asked_to_install_and_every_cache_points_at_the_store(self) -> None: | ||
| outputs, env = prepare({"package.json": "{}", "pnpm-lock.yaml": ""}) | ||
| self.assertEqual(outputs, {"lockfile": "true"}) | ||
| self.assertEqual(env["CODEBOARDING_INSTALL_DEPENDENCIES"], "1") | ||
| store = env["npm_config_cache"].rsplit("/", 1)[0] | ||
| for variable in ("npm_config_store_dir", "BUN_INSTALL_CACHE_DIR", "YARN_CACHE_FOLDER", "YARN_GLOBAL_FOLDER"): | ||
| self.assertTrue(env[variable].startswith(store), variable) | ||
|
|
||
| def test_without_a_lockfile_core_still_checks_the_merge_base(self) -> None: | ||
| outputs, env = prepare({"main.py": ""}) | ||
| self.assertEqual(outputs, {}) | ||
| self.assertEqual(env["CODEBOARDING_INSTALL_DEPENDENCIES"], "1") | ||
|
|
||
| def test_a_forks_pull_request_installs_nothing(self) -> None: | ||
| self.assertEqual(prepare({"bun.lock": ""}, is_fork="true"), ({}, {})) | ||
|
|
||
| def test_turning_it_off_installs_nothing(self) -> None: | ||
| self.assertEqual(prepare({"bun.lock": ""}, install="false"), ({}, {})) | ||
|
|
||
|
|
||
| class ActionWiringTests(unittest.TestCase): | ||
| """The steps around the script: the store is restored for every run and saved only by sync.""" | ||
|
|
||
| def setUp(self) -> None: | ||
| self.action = (ROOT / "action.yml").read_text(encoding="utf-8") | ||
|
|
||
| def _step(self, name: str) -> str: | ||
| return self.action.split(f"- name: {name}", 1)[1].split("- name:", 1)[0] | ||
|
|
||
| def test_the_input_defaults_to_installing(self) -> None: | ||
| block = re.search(r"^ install_dependencies:\n((?: .*\n)+)", self.action, re.MULTILINE) | ||
| assert block is not None | ||
| self.assertIn("default: 'true'", block.group(1)) | ||
|
|
||
| def test_the_step_knows_a_fork(self) -> None: | ||
| self.assertIn("IS_FORK: ${{ steps.guard.outputs.is_fork }}", self._step("Prepare dependency install")) | ||
|
|
||
| def test_only_sync_saves_the_store(self) -> None: | ||
| save = self._step("Save dependency cache") | ||
| self.assertIn("steps.guard.outputs.mode == 'sync'", save) | ||
| self.assertIn("actions/cache/save@", save) | ||
|
|
||
| def test_every_lockfile_keys_the_cache(self) -> None: | ||
| key = self._step("Restore dependency cache") | ||
| for pattern in ("*.lock", "*.lockb", "*-lock.yaml", "*-lock.json", "npm-shrinkwrap.json"): | ||
| self.assertIn(f"'.codeboarding-target/{pattern}'", key) | ||
|
|
||
| def test_an_install_is_part_of_the_analysis_identity(self) -> None: | ||
| """A fork's review installs nothing, so it must not reuse a base analyzed with an install.""" | ||
| self.assertIn( | ||
| "INSTALL_DEPENDENCIES: ${{ steps.deps.outputs.lockfile == 'true' }}", | ||
| self._step("Resolve analysis cache keys"), | ||
| ) | ||
|
|
||
| def test_the_store_is_ready_before_the_engine_runs(self) -> None: | ||
| for step in ("Generate sync baseline", "Generate review baseline"): | ||
| self.assertLess(self.action.index("- name: Restore dependency cache"), self.action.index(f"- name: {step}")) | ||
|
|
||
|
|
||
| if __name__ == "__main__": | ||
| unittest.main() |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Once the required engine pin enables this default-on path, a trusted
/codeboardingcommand on a fork lets the fork control package-manager startup whilewith-auth.shhas exported direct-provider credentials. Disabling lifecycle scripts is insufficient: a checked-in Yarn plugin from.yarnrc.ymlexecutes while Yarn starts, before install-script policy applies, and analogous manager hooks can inspect the environment or the restored dependency cache. Fork reviews should skip dependency installation or run it in an isolated environment without analysis/private-registry credentials or trusted caches.Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in 68d0aef. A fork's pull request installs nothing: the step skips before it exports anything. Core also removes model-provider keys and the runner's OIDC and runtime tokens from the install's environment, and runs pnpm with
--ignore-pnpmfile(CodeBoarding#639). Same-repository pull requests still run a repository's own Yarn plugins, but that code comes from someone with write access.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Revised in 2e29a1d. Forks still install nothing. Core no longer scrubs the environment (it wasn't a real boundary): the repository's package-manager setup runs as in a build, and only dependency install scripts are blocked.