diff --git a/README.md b/README.md index 760aa5e..da0604b 100644 --- a/README.md +++ b/README.md @@ -104,6 +104,16 @@ A pull request whose changed files are all outside what the engine analyses (doc Every review comment ends with a machine-readable HTML comment, ``, for readers that should not parse the prose or the diagram. The progress comment carries the platform link from the start, so a pull request can be opened there while the run is still going. +### JavaScript workspaces + +In a JavaScript or TypeScript monorepo (npm, pnpm, Yarn or Bun workspaces), a call from one package into another only resolves once the packages are installed, so a fresh checkout shows no calls between them. With `install_dependencies: true` (the default), CodeBoarding installs the analyzed repository's dependencies before analyzing it, for the pull request and for its merge base alike: + +- **With the lockfile frozen and without dependency install scripts.** The repository's own package-manager setup (`.npmrc`, `.yarnrc.yml`) applies, as it would in a build. +- **With the package manager the repository pins** in `packageManager`, else the one its lockfile belongs to, fetched through npm when the runner lacks that version. +- **From a cached store.** Downloads are cached once per lockfile, about 1 GB for a large monorepo, against the repository's 10 GB of free Actions cache. Only sync saves it, so pull request runs read it without adding to the repository's cache use. + +A pull request from a fork is analyzed without an install: its package manager would run with the job's credentials in reach. A repository that is not a workspace, has no lockfile or uses Yarn Plug'n'Play is analyzed without one too, and so is one whose install fails. For a private registry, set the token your `.npmrc` reads in the job's `env` (for example `NPM_TOKEN: ${{ secrets.NPM_TOKEN }}`). Set `install_dependencies: false` to skip installing entirely. + ## Authentication and providers The `llm` input is required and says where analysis credentials come from. There are diff --git a/action.yml b/action.yml index 82793b6..587e88f 100644 --- a/action.yml +++ b/action.yml @@ -153,6 +153,10 @@ inputs: in_place: committed as .codeboarding/ on the synced branch itself. required: false default: 'codeboarding_branch' + install_dependencies: + description: "Install the analyzed repository's JavaScript workspace dependencies before the analysis (no install scripts, frozen lockfile), so calls between its packages resolve. Never for a fork's pull request. Set to false to skip." + required: false + default: 'true' warmstart_retention_days: description: 'Days to keep the reusable analysis. Only the next run reads it.' required: false @@ -399,6 +403,28 @@ runs: distribution: temurin java-version: '21' + - name: Prepare dependency install + id: deps + if: steps.guard.outputs.skip != 'true' + shell: bash + env: + CHECKOUT_DIR: ${{ github.workspace }}/.codeboarding-target + STORE_DIR: ${{ runner.temp }}/cb-deps + INSTALL_DEPENDENCIES: ${{ inputs.install_dependencies }} + IS_FORK: ${{ steps.guard.outputs.is_fork }} + run: "$GITHUB_ACTION_PATH/scripts/action/prepare-dependencies.sh" + + # Only sync saves the store, on the branch it runs on, which PR runs can read; a cache saved + # inside one pull request is visible to no other, so it would only use up the repository's quota. + - name: Restore dependency cache + id: deps_cache + if: steps.guard.outputs.skip != 'true' && steps.deps.outputs.lockfile == 'true' + uses: actions/cache/restore@v4 + with: + path: ${{ runner.temp }}/cb-deps + key: cb-deps-${{ runner.os }}-${{ hashFiles('.codeboarding-target/*.lock', '.codeboarding-target/*.lockb', '.codeboarding-target/*-lock.yaml', '.codeboarding-target/*-lock.json', '.codeboarding-target/npm-shrinkwrap.json') }} + restore-keys: cb-deps-${{ runner.os }}- + - name: Install CodeBoarding if: steps.guard.outputs.skip != 'true' shell: bash @@ -435,6 +461,8 @@ runs: MODEL: ${{ inputs.model }} AGENT_MODEL_INPUT: ${{ inputs.agent_model }} PARSING_MODEL_INPUT: ${{ inputs.parsing_model }} + # A fork's review installs nothing, so it never reuses an analysis made with an install. + INSTALL_DEPENDENCIES: ${{ steps.deps.outputs.lockfile == 'true' }} run: "$GITHUB_ACTION_PATH/scripts/action/resolve-cache-keys.sh" # Both lookups are best effort. Without them the run derives the base from @@ -546,6 +574,14 @@ runs: retention-days: 30 if-no-files-found: ignore + - name: Save dependency cache + if: steps.guard.outputs.skip != 'true' && steps.guard.outputs.mode == 'sync' && steps.deps.outputs.lockfile == 'true' && steps.deps_cache.outputs.cache-hit != 'true' && steps.sync_analyze.outcome == 'success' + continue-on-error: true + uses: actions/cache/save@v4 + with: + path: ${{ runner.temp }}/cb-deps + key: ${{ steps.deps_cache.outputs.cache-primary-key }} + - name: Write sync summary if: always() && steps.guard.outputs.skip != 'true' && steps.guard.outputs.mode == 'sync' shell: bash diff --git a/scripts/action/prepare-dependencies.sh b/scripts/action/prepare-dependencies.sh new file mode 100755 index 0000000..70f9428 --- /dev/null +++ b/scripts/action/prepare-dependencies.sh @@ -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." diff --git a/scripts/action/resolve-cache-keys.sh b/scripts/action/resolve-cache-keys.sh index b658ba6..feb7611 100755 --- a/scripts/action/resolve-cache-keys.sh +++ b/scripts/action/resolve-cache-keys.sh @@ -33,8 +33,10 @@ ignore_file="$CHECKOUT_DIR/.codeboarding/.codeboardingignore" # does. It carries no key: rotating a secret must not throw away reusable analysis. model_digest="$(printf '%s\n%s\n%s\n%s\n%s\n' \ "${LLM_PROVIDER:-}" "${BACKEND_ID:-}" "${MODEL:-}" "${AGENT_MODEL_INPUT:-}" "${PARSING_MODEL_INPUT:-}" | digest)" -cfg="$(printf '%s\n%s\n%s\n%s\n%s\n' \ - "$STATE_SCHEMA" "$engine_version" "$ignore_digest" "$model_digest" "${DEPTH_CAP:-2}" | digest)" +# Installed dependencies decide which calls between workspace packages resolve. +cfg="$(printf '%s\n%s\n%s\n%s\n%s\n%s\n' \ + "$STATE_SCHEMA" "$engine_version" "$ignore_digest" "$model_digest" "${DEPTH_CAP:-2}" \ + "${INSTALL_DEPENDENCIES:-false}" | digest)" { echo "engine_version=$engine_version" diff --git a/tests/test_action_state.py b/tests/test_action_state.py index 0a02ea4..fe51235 100644 --- a/tests/test_action_state.py +++ b/tests/test_action_state.py @@ -170,6 +170,10 @@ def test_analysis_scope_and_engine_version_change_the_identity(self) -> None: (self.checkout / ".codeboarding" / ".codeboardingignore").write_text("docs/\n", encoding="utf-8") self.assertNotEqual(baseline["cfg_hash"], self._run()["cfg_hash"]) + def test_installing_dependencies_changes_the_identity(self) -> None: + # Installed packages decide which calls between workspace packages resolve. + self.assertNotEqual(self._run()["cfg_hash"], self._run(INSTALL_DEPENDENCIES="true")["cfg_hash"]) + def test_model_selection_changes_the_identity(self) -> None: baseline = self._run() diff --git a/tests/test_prepare_dependencies.py b/tests/test_prepare_dependencies.py new file mode 100644 index 0000000..4e62de2 --- /dev/null +++ b/tests/test_prepare_dependencies.py @@ -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()