Repository navigation
feat: install a JavaScript workspace's dependencies before the analysis - #144
Svilen-Stefanov wants to merge 1 commit 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. |
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.
There was a problem hiding this comment.
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.
| 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.
There was a problem hiding this comment.
Revised in 2e29a1d: now handled here, as you first suggested. An install is part of cfg_hash, so an analysis is never reused across a change in whether the run installs, a fork's review included. Core's static-cache fingerprint is gone.
| 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.
There was a problem hiding this comment.
Superseded in 2e29a1d: Core fetches the pinned npm (or any pinned manager) when the one on PATH reports another version, so the Action sets up none.
| } | ||
|
|
||
| [ "${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.
There was a problem hiding this comment.
Still holds in 2e29a1d: the variable is exported whenever the input is on and the PR isn't a fork, and Core decides for each checkout.
| 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.
There was a problem hiding this comment.
Superseded in 2e29a1d: the Action no longer detects anything, and Core skips Yarn 1 Plug'n'Play.
| } | ||
|
|
||
| [ "${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.
There was a problem hiding this comment.
Superseded in 2e29a1d: the Action no longer detects anything, and Core handles a pnpm workspace without a root package.json.
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 👍 / 👎.
There was a problem hiding this comment.
Fixed in 2e29a1d by removing the setup from the Action. Core now fetches the package manager and version each checkout pins (npm exec, version-checked against PATH), so the merge base gets its own. The Action only turns the install on and keeps the download cache.
68d0aef to
2e29a1d
Compare
With install_dependencies (default true), the action asks Core to install a JavaScript workspace's dependencies, so calls between its packages resolve. Core checks the head and the merge base each on its own and fetches the package manager each one pins; the action only turns it on and keeps the package managers' downloads in one cache, saved by sync. - never for a fork's pull request: its package manager would run with the job's credentials - an install is part of the analysis identity (cfg hash), so an analysis made without one is never reused by a run that installs, or the other way round Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
2e29a1d to
1d8adad
Compare
2 participants
Stacked on #143. Draft until the Core release that contains CodeBoarding #639 exists and the
codeboarding==pin here moves to it. With the current pin, 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 does the install itself when
CODEBOARDING_INSTALL_DEPENDENCIESis set (CodeBoarding #639). It checks the head and the merge-base worktree each on its own, and fetches the package manager each one pins from the public npm registry, outside the repository. The Action only:install_dependencies(defaulttrue);scripts/action/prepare-dependencies.sh). A fork's package manager would run with the job's credentials in reach;RUNNER_TEMP.actions/cacherestores it, keyed by the head's lockfiles, and only sync saves it: a cache saved inside a pull request is visible to no other pull request;cfg_hash). A fork's review installs nothing, so it never reuses a base analysed with an install; it re-derives one without. Repositories without a lockfile keep sharing bases with forks;Impact
Tests
tests/test_prepare_dependencies.py, 10 tests: what the step exports, fork and off switch, lockfile detection, and the wiring (input default, fork flag, only sync saves, cache key patterns, the identity input, store ready before both analysis steps).tests/test_action_state.py: an install changes the analysis identity.🤖 Generated with Claude Code