Repository navigation
feat: install a JavaScript workspace's dependencies before the analysis - #144
Svilen-Stefanov wants to merge 2 commits into
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
A call from one package of a JavaScript monorepo into another resolves only once the packages are installed, so a fresh checkout showed no calls between them. Core now installs them itself when CODEBOARDING_INSTALL_DEPENDENCIES is set (no install scripts, frozen lockfile, hard timeout), for the head checkout and the merge-base worktree alike. The action provides the rest: - `install_dependencies` (default true) turns it on. - prepare-dependencies.sh detects a workspace and its package manager (the `packageManager` pin, else the lockfile's), skips Yarn Plug'n'Play, and points every package manager's cache at one store under RUNNER_TEMP. - Bun comes from oven-sh/setup-bun at the pinned version; pnpm and Yarn from corepack. - The store is restored per lockfile, falling back to the previous one, and saved only by sync: a cache saved inside a pull request is visible to no other, so it would only use up the repository's quota. The install needs the Core release that adds it; with the current pin the variable is ignored. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
062a1a9 to
bebc977
Compare
CodeBoarding reviewStatus: 0 changed components (no analysed file changed) See the full change in CodeBoarding. graph LR
n_action_scripts["action_scripts"]
classDef added fill:#1f883d,stroke:#0b5d23,color:#ffffff;
classDef modified fill:#bf8700,stroke:#7d4e00,color:#ffffff;
classDef deleted fill:#cf222e,stroke:#82071e,color:#ffffff,stroke-dasharray:5 3;
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 062a1a9448
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
|
||
| mkdir -p "$STORE_DIR" | ||
| { | ||
| echo "CODEBOARDING_INSTALL_DEPENDENCIES=1" |
There was a problem hiding this comment.
This export is ignored by the still-pinned codeboarding==0.14.5, so the default-on input, package-manager setup, and cache steps never cause dependencies to be installed and the advertised workspace-call improvement is absent in every run. Ship this only with the Core release that consumes CODEBOARDING_INSTALL_DEPENDENCIES, updating the pin in action.yml; this repository explicitly notes that engine changes reach users only through such a pin bump.
AGENTS.md reference: AGENTS.md:L13-L17
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed. This ships with the Core release that includes CODEBOARDING_INSTALL_DEPENDENCIES (CodeBoarding#639), together with the codeboarding== pin bump in this PR. I've moved it to draft until that release exists.
| 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. The package store is cached per lockfile. Set to false to skip." | ||
| required: false | ||
| default: 'true' |
There was a problem hiding this comment.
Once the required engine pin enables this default-on path, a trusted /codeboarding command on a fork lets the fork control package-manager startup while with-auth.sh has exported direct-provider credentials. Disabling lifecycle scripts is insufficient: a checked-in Yarn plugin from .yarnrc.yml executes 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.
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.
| 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. The package store is cached per lockfile. Set to false to skip." | ||
| required: false | ||
| default: 'true' |
There was a problem hiding this comment.
Changing install_dependencies from false to true does not change the configuration hash constructed by resolve-cache-keys.sh, so a review or sync can reuse analysis state produced without node_modules. When no analyzed source changed, the incremental run can retain the old graph and continue omitting workspace calls even though installation is now enabled. Include this setting—and any dependency-resolution fingerprint required by Core—in state compatibility, or force a full analysis when it changes.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Handled in Core (CodeBoarding#639, 2b3c542e). The static-analysis cache now records the installed dependency state, and a warm start whose state differs re-reads every file while keeping the cache as the incremental base. Turning the input on, and any later lockfile change, forces that full pass without any config-hash change here.
| declared = manifest.get("packageManager") if isinstance(manifest, dict) else None | ||
| match = re.match(r"^(bun|pnpm|yarn|npm)@([0-9][^+\s]*)", declared) if isinstance(declared, str) else None | ||
| if match: | ||
| manager, version = match.group(1), match.group(2) |
There was a problem hiding this comment.
For a workspace declaring packageManager: "npm@<version>", this records the requested version but no later step consumes it: Bun alone receives steps.deps.outputs.version, and the corepack step is restricted to pnpm and Yarn. The engine therefore uses the runner's preinstalled npm, which can reject or reinterpret a frozen lockfile created by the pinned version. Set up the declared npm version as well, or do not claim that npm pins are honored.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Done in 68d0aef: a pinned npm is installed with npm install --global npm@<version> before the analysis.
| } | ||
|
|
||
| [ "${INSTALL_DEPENDENCIES:-true}" = true ] || skip "install_dependencies is false" | ||
| [ -f "$CHECKOUT_DIR/package.json" ] || skip "no package.json" |
There was a problem hiding this comment.
When a pull-request head removes its root package.json or otherwise stops being a workspace, this check disables dependency installation globally before the merge-base worktree is created. If no published base is reusable, the later baseline analysis therefore examines a merge base that may still be a JavaScript workspace without installing its dependencies, omitting the very cross-package calls this change is meant to expose and producing an inaccurate comparison. Make the install decision for each checkout independently, or let Core detect the workspace in each worktree.
AGENTS.md reference: AGENTS.md:L48-L51
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Done in 68d0aef. CODEBOARDING_INSTALL_DEPENDENCIES is now exported whenever the input is on and the pull request isn't from a fork, and Core decides for each checkout. The head's detection only chooses which package manager to set up and cache. A merge base that uses another package manager falls back to Core's npm exec path.
| berry = version != "-" and not version.startswith("1.") | ||
| linker = re.search(r"^\s*nodeLinker:\s*['\"]?([\w-]+)", yarnrc.read_text(errors="ignore"), re.M) if yarnrc.is_file() else None | ||
| if (linker and linker.group(1) == "pnp") or (not linker and (yarnrc.is_file() or berry)): |
There was a problem hiding this comment.
A Yarn 1 workspace can enable Plug'n'Play through installConfig.pnp in package.json (and commonly has a .pnp.js) without any .yarnrc.yml. For that valid configuration berry is false and linker is absent, so this branch enables an install even though it creates no node_modules, contrary to the documented promise to skip Plug'n'Play repositories. Check the Yarn Classic PnP manifest setting or marker file before preparing the install.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Done in 68d0aef, and in Core: installConfig.pnp, .pnp.cjs or .pnp.js now skip the setup.
| } | ||
|
|
||
| [ "${INSTALL_DEPENDENCIES:-true}" = true ] || skip "install_dependencies is false" | ||
| [ -f "$CHECKOUT_DIR/package.json" ] || skip "no package.json" |
There was a problem hiding this comment.
A pnpm-workspace.yaml is sufficient to define a pnpm workspace, and pnpm install works from such a root even when only the member packages have manifests. This early exit makes the later explicit pnpm-workspace.yaml check unreachable for that valid layout, so its dependencies are never installed and cross-package calls remain unresolved. Treat the workspace file plus pnpm-lock.yaml as sufficient when the root manifest is absent.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Done in 68d0aef, and in Core: pnpm-workspace.yaml plus pnpm-lock.yaml is now enough.
…e, set up a pinned npm - a fork's pull request installs nothing: its package manager would start with the analysis credentials in reach - CODEBOARDING_INSTALL_DEPENDENCIES is exported whenever the input is on, so Core decides for the head and the merge base on their own; the head's detection only picks what to set up - a pinned npm is installed over the runner's - npm-shrinkwrap.json, pnpm workspaces without a root package.json, and Yarn 1 Plug'n'Play (installConfig.pnp, .pnp.cjs) are detected as Core detects them Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 68d0aefca5
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| run: "$GITHUB_ACTION_PATH/scripts/action/prepare-dependencies.sh" | ||
|
|
||
| - name: Setup Bun for dependency install | ||
| if: steps.guard.outputs.skip != 'true' && steps.deps.outputs.manager == 'bun' |
There was a problem hiding this comment.
When the PR head removes its workspace or changes from Bun to another manager, steps.deps.outputs.manager is empty or names only the head manager, so this condition skips Bun setup even though Core subsequently analyzes a Bun-based merge-base worktree; the baseline install then lacks the required binary and its cross-package calls can disappear from the comparison. The updated no_setup path is fresh evidence that the earlier issue remains: it now exports CODEBOARDING_INSTALL_DEPENDENCIES, but still emits an empty manager that skips every setup step. Determine and provision the manager for each checkout, or otherwise ensure all managers Core may select are available.
AGENTS.md reference: AGENTS.md:L49-L52
Useful? React with 👍 / 👎.
1 participant
Stacked on #143. Needs the Core release that contains CodeBoarding #639; with the current
codeboarding==0.14.5pin, the variable it sets is ignored.Why
A call from one package of a JavaScript monorepo into another resolves only once the packages are installed, so the Action's fresh checkout showed no calls between them. On opencode, installing took package pairs with calls from 1 to 45.
What changes
Core now does the install itself when
CODEBOARDING_INSTALL_DEPENDENCIESis set, for the head checkout and the merge-base worktree alike. The Action provides the rest:install_dependencies(defaulttrue).scripts/action/prepare-dependencies.sh:packageManagerpin, else the lockfile's), using the same rules as Core;BUN_INSTALL_CACHE_DIR,npm_config_store_dir,YARN_CACHE_FOLDER,npm_config_cache) at one store underRUNNER_TEMP;oven-sh/setup-bun@v2at the pinned version; pnpm and Yarn fromcorepack enable.actions/cache/restore@v4, keyed by the lockfile's hash, falling back to the previous lockfile's store.actions/cache/save@v4). A cache saved inside a pull request is visible to no other pull request, so saving there would only use up the repository's quota..npmrcreads in the job'senv) and the off switch.Impact
Tests
tests/test_prepare_dependencies.py(12 tests): detection per package manager and pin, single packages, missing lockfiles, Plug'n'Play and the off switch; plus the wiring (input default, only sync saves, store restored before both analysis steps).shellcheckis clean.test_provider_table_driftfailures compare against the Core installed on this machine, fail without this change too, and pass in thecore-compatibilityjob against the pin.🤖 Generated with Claude Code