diff --git a/libs/cli/langgraph_cli/config.py b/libs/cli/langgraph_cli/config.py index 57b225187..56d34f6b6 100644 --- a/libs/cli/langgraph_cli/config.py +++ b/libs/cli/langgraph_cli/config.py @@ -1162,7 +1162,39 @@ def _build_runtime_env_vars(config: Config) -> list[str]: return env_vars -def _get_node_pm_install_cmd(project_dir: pathlib.Path) -> str: +# These run as part of the install step, before the source would be copied. +_NODE_INSTALL_HOOKS = ("preinstall", "install", "postinstall", "prepare") + + +def _splittable_node_manifests( + project_dir: pathlib.Path, lockfile: str | None +) -> list[str] | None: + """Return manifests to copy before installing, or None if unsafe to split. + + A lockfile is required: without one the install resolves versions at build + time, so a cached layer could pin an older resolution than a clean build. + """ + if lockfile is None: + return None + manifest = project_dir / "package.json" + try: + if not manifest.is_file(): + return None + with open(manifest) as f: + package_json = json.load(f) + except (OSError, ValueError): + return None + if not isinstance(package_json, dict): + return None + scripts = package_json.get("scripts") or {} + if any(hook in scripts for hook in _NODE_INSTALL_HOOKS): + return None + return ["package.json", lockfile] + + +def _get_node_pm_install_cmd(project_dir: pathlib.Path) -> tuple[str, str | None]: + """Return the install command and the lockfile it was chosen from.""" + def test_file(file_name): full_path = project_dir / file_name try: @@ -1201,13 +1233,19 @@ def _get_node_pm_install_cmd(project_dir: pathlib.Path) -> str: if yarn: install_cmd = "yarn install --frozen-lockfile" + lockfile = "yarn.lock" elif pnpm: install_cmd = "pnpm i --frozen-lockfile" + lockfile = "pnpm-lock.yaml" elif npm: install_cmd = "npm ci" + lockfile = "package-lock.json" elif bun: install_cmd = "bun i" + lockfile = "bun.lockb" else: + # No lockfile, so the install resolves versions at build time. + lockfile = None pkg_manager_name = get_pkg_manager_name() if pkg_manager_name == "yarn": @@ -1219,7 +1257,7 @@ def _get_node_pm_install_cmd(project_dir: pathlib.Path) -> str: else: install_cmd = "npm i" - return install_cmd + return install_cmd, lockfile semver_pattern = re.compile(r":(\d+(?:\.\d+)?(?:\.\d+)?)(?:-|$)") @@ -1423,7 +1461,7 @@ ADD {relpath} /deps/{name} "# -- Installing JS dependencies --", f"ENV NODE_VERSION={config.get('node_version') or DEFAULT_NODE_VERSION}", f"WORKDIR {local_deps.working_dir}", - f"RUN {_get_node_pm_install_cmd(config_path.parent)} && tsx /api/langgraph_api/js/build.mts", + f"RUN {_get_node_pm_install_cmd(config_path.parent)[0]} && tsx /api/langgraph_api/js/build.mts", "# -- End of JS dependencies install --", ] ) @@ -1492,7 +1530,9 @@ def node_config_to_docker( install_root = ( pathlib.Path(build_context).resolve() if build_context else config_path.parent ) - install_cmd = install_command or _get_node_pm_install_cmd(install_root) + detected_cmd, detected_lockfile = _get_node_pm_install_cmd(install_root) + install_cmd = install_command or detected_cmd + relative_workdir = "" if build_context: relative_workdir = _calculate_relative_workdir(config_path, build_context) container_name = pathlib.Path(build_context).name @@ -1530,16 +1570,41 @@ def node_config_to_docker( else: build_workdir = faux_path + source_root = faux_path if not build_context else container_root + + # Excluded: a custom install command may read files we have not copied yet, + # and a nested config means workspace manifests the root copy would miss. + manifests = ( + _splittable_node_manifests(install_root, detected_lockfile) + if install_command is None and not relative_workdir + else None + ) + + if manifests: + add_steps = [ + *(f"ADD {name} {source_root}/{name}" for name in manifests), + "", + f"WORKDIR {install_workdir}", + "", + install_step, + "", + f"ADD . {source_root}", + ] + else: + add_steps = [ + f"ADD . {source_root}", + "", + f"WORKDIR {install_workdir}", + "", + install_step, + ] + docker_file_contents = [ f"FROM {image_str}", "", os.linesep.join(config["dockerfile_lines"]), "", - f"ADD . {faux_path if not build_context else container_root}", - "", - f"WORKDIR {install_workdir}", - "", - install_step, + *add_steps, "", os.linesep.join(env_vars), "", diff --git a/libs/cli/langgraph_cli/uv_lock.py b/libs/cli/langgraph_cli/uv_lock.py index 1e33d16d7..453812e6f 100644 --- a/libs/cli/langgraph_cli/uv_lock.py +++ b/libs/cli/langgraph_cli/uv_lock.py @@ -1019,7 +1019,7 @@ def python_config_to_docker_uv_lock( docker_plan.add_instruction("WORKDIR", plan.working_dir) docker_plan.add_instruction( "RUN", - f"{_get_node_pm_install_cmd(plan.target_root)} && " + f"{_get_node_pm_install_cmd(plan.target_root)[0]} && " "tsx /api/langgraph_api/js/build.mts", ) docker_plan.add_raw("# -- End of JS dependencies install --") diff --git a/libs/cli/tests/unit_tests/test_config.py b/libs/cli/tests/unit_tests/test_config.py index c8b24020c..c6957b0a2 100644 --- a/libs/cli/tests/unit_tests/test_config.py +++ b/libs/cli/tests/unit_tests/test_config.py @@ -3424,3 +3424,134 @@ class TestHasDisallowedBuildCommandContent: ) def test_valid_commands_allowed(self, cmd: str) -> None: assert not has_disallowed_build_command_content(cmd) + + +class TestNodeDependencyLayerOrdering: + """Dependency manifests are copied before source so the install layer caches. + + Without this the first source change invalidates the install, and a JS + deployment reinstalls every dependency on every push. + """ + + def _project( + self, + tmp_path: pathlib.Path, + *, + lockfile: str | None, + scripts: dict[str, str] | None = None, + ) -> pathlib.Path: + package_json: dict = {"name": "agent"} + if scripts: + package_json["scripts"] = scripts + (tmp_path / "package.json").write_text(json.dumps(package_json)) + if lockfile: + (tmp_path / lockfile).write_text("") + (tmp_path / "graphs").mkdir() + (tmp_path / "graphs" / "agent.js").write_text("") + config_path = tmp_path / "langgraph.json" + config_path.write_text("{}") + return config_path + + def _dockerfile(self, config_path: pathlib.Path, **kwargs) -> str: + actual, _ = config_to_docker( + config_path, + validate_config( + {"node_version": "20", "graphs": {"agent": "./graphs/agent.js:graph"}} + ), + base_image="langchain/langgraphjs-api", + **kwargs, + ) + return clean_empty_lines(actual) + + def test_manifests_copied_before_install(self, tmp_path: pathlib.Path) -> None: + config_path = self._project(tmp_path, lockfile="package-lock.json") + lines = self._dockerfile(config_path).splitlines() + + add_manifest = lines.index( + f"ADD package.json /deps/{tmp_path.name}/package.json" + ) + add_lock = lines.index( + f"ADD package-lock.json /deps/{tmp_path.name}/package-lock.json" + ) + install = lines.index("RUN npm ci") + add_source = lines.index(f"ADD . /deps/{tmp_path.name}") + + assert add_manifest < install + assert add_lock < install + assert install < add_source + + def test_lockfile_choice_follows_package_manager( + self, tmp_path: pathlib.Path + ) -> None: + config_path = self._project(tmp_path, lockfile="pnpm-lock.yaml") + dockerfile = self._dockerfile(config_path) + + assert f"ADD pnpm-lock.yaml /deps/{tmp_path.name}/pnpm-lock.yaml" in dockerfile + assert "package-lock.json" not in dockerfile + + def test_no_lockfile_keeps_source_first(self, tmp_path: pathlib.Path) -> None: + # No lockfile means the install resolves at build time, so caching it is wrong. + config_path = self._project(tmp_path, lockfile=None) + lines = self._dockerfile(config_path).splitlines() + + assert lines.index(f"ADD . /deps/{tmp_path.name}") < lines.index("RUN npm i") + assert not any(line.startswith("ADD package.json") for line in lines) + + def test_nested_config_keeps_source_first(self, tmp_path: pathlib.Path) -> None: + # A workspace keeps manifests in subdirectories the root copy would miss. + root = tmp_path / "repo" + root.mkdir() + (root / "package.json").write_text(json.dumps({"name": "root"})) + (root / "package-lock.json").write_text("") + pkg = root / "packages" / "agent" + pkg.mkdir(parents=True) + (pkg / "graphs").mkdir() + (pkg / "graphs" / "agent.js").write_text("") + config_path = pkg / "langgraph.json" + config_path.write_text("{}") + + lines = self._dockerfile(config_path, build_context=str(root)).splitlines() + + assert lines.index("ADD . /deps/repo") < lines.index("RUN npm ci") + assert not any(line.startswith("ADD package.json") for line in lines) + + def test_custom_install_command_keeps_source_first( + self, tmp_path: pathlib.Path + ) -> None: + # A custom command may read files the manifest copy would not include. + config_path = self._project(tmp_path, lockfile="package-lock.json") + lines = self._dockerfile( + config_path, + install_command="npm run bootstrap", + build_context=str(tmp_path), + ).splitlines() + + assert lines.index(f"ADD . /deps/{tmp_path.name}") < lines.index( + "RUN npm run bootstrap" + ) + + @pytest.mark.parametrize( + "hook", ["preinstall", "install", "postinstall", "prepare"] + ) + def test_install_hook_keeps_source_first( + self, tmp_path: pathlib.Path, hook: str + ) -> None: + # A hook referencing a project file would hit ENOENT: source is not copied yet. + config_path = self._project( + tmp_path, + lockfile="package-lock.json", + scripts={hook: "node scripts/setup.js"}, + ) + lines = self._dockerfile(config_path).splitlines() + + assert lines.index(f"ADD . /deps/{tmp_path.name}") < lines.index("RUN npm ci") + assert not any(line.startswith("ADD package.json") for line in lines) + + def test_only_the_chosen_lockfile_is_copied(self, tmp_path: pathlib.Path) -> None: + # Install picks yarn, so copying the npm lockfile would bust the cache for nothing. + config_path = self._project(tmp_path, lockfile="yarn.lock") + (tmp_path / "package-lock.json").write_text("") + dockerfile = self._dockerfile(config_path) + + assert f"ADD yarn.lock /deps/{tmp_path.name}/yarn.lock" in dockerfile + assert "ADD package-lock.json" not in dockerfile