fix(init): run cookiecutter template hooks from the packaged binary - #9206
fix(init): run cookiecutter template hooks from the packaged binary#9206roger-zhangg wants to merge 5 commits into
Conversation
Cookiecutter runs a Python hook as [sys.executable, script]. In a PyInstaller bundle sys.executable is sam itself, so the hook became 'sam /tmp/xxxx.py', which printed help and exited 0 -- cookiecutter read that as success and the hook was silently skipped. Supply a real interpreter instead: a system python3 when present, else this executable re-launched in hook mode. Also correct INCOMPATIBLE_PARAM_MESSAGE, which still expected the parameter order from before #9176 generated the hint from the enforced combinations.
![aws-sam-tooling-bot[bot]](https://avatars.githubusercontent.com/b/1290?s=60&v=4){kind=small}
…template path The re-launched bundle never reaches the CLI callback that undoes the bootloader's library paths, so hooks shelling out to git/npm/pip inherited them. Also wrap the second cookiecutter call site, reachable via sam pipeline init with a custom template location.
…auncher runpy leaves our second argument in sys.argv, so a hook saw [script, script] instead of [script]. Also add py3/py to interpreter discovery, matching _get_python_command_name, since a default python.org Windows install puts only py.exe on PATH.
| # Set on the hook subprocess so a re-launched bundle runs the script instead of parsing a command name. | ||
| HOOK_SCRIPT_ENV_VAR = "SAM_CLI_RUN_HOOK_SCRIPT" | ||
|
|
||
| # A bundle ships no interpreter of its own, so a system one is preferred: it gives hooks the same |
There was a problem hiding this comment.
[GENERAL] The rationale for preferring a system interpreter is inverted, and the resulting order is the one that diverges most from a pip install.
The comment says a system python "gives hooks the same environment they get from a pip install, rather than this bundle's Python and dependencies." Under a pip install, sys.executable is the interpreter that has SAM CLI's dependencies importable — PyYAML~=6.0 is a direct dependency (pyproject.toml:41), and jinja2 comes in with cookiecutter. So a hook containing import yaml or import jinja2 succeeds today for pip users, and would also succeed on the re-launch fallback (which runs under the bundle's sys.path), but fails against a bare /usr/bin/python3 — the path this code prefers.
Net effect: the preferred branch is the only one of the three that cannot import SAM's dependencies, and the failure surfaces as a ModuleNotFoundError inside a FailedHookException for custom --location templates. Either make the re-launch path the primary and the system interpreter the fallback, or keep the current order and correct the comment to say the system interpreter is preferred for isolation, not fidelity, so the next reader does not rely on a guarantee that is not there.
There was a problem hiding this comment.
You are right, and the comment was mine to get wrong — fixed in c137500.
Verified the premise: PyYAML~=6.0 is a direct dependency (pyproject.toml:41) and jinja2 arrives with cookiecutter, so under a pip install sys.executable is an interpreter that can import them, and the re-launch path can too. A bare /usr/bin/python3 cannot. So the branch I called the faithful one is the only one of the three that is not.
I took your second option and kept the order, correcting the rationale to say the system interpreter is preferred for isolation — so SAM's bundled dependency set does not quietly become an implicit contract for template authors — rather than for fidelity.
I did not flip the order, for two reasons. Making the bundle primary would promise that hooks can import SAM's dependencies, which is incidental under pip rather than a contract, and would make a PyYAML major bump able to break someone's template. It would also promote the arbitrary-file execution path from rarely-used fallback to the common case. That said, the ordering was chosen by the PR author on the strength of the rationale you just corrected, so I have flagged the trade-off to them; if they would rather have fidelity than isolation, flipping is a one-line change.
…rationale The probe accepted any Python 3.x, so a 3.6 /usr/bin/python3 could be preferred over the bundled 3.11 even though the project requires >=3.10. Also the comment claimed a system interpreter matches a pip install; it does not, since a pip install can import SAM's dependencies and a bare system python cannot.
| # cookiecutter returns the directory it created, which is the only reliable way to know | ||
| # where the project landed when the template chooses its own project directory name. | ||
| project_directory = cookiecutter(**params) | ||
| with patched_hook_runner(cookiecutter_hooks): |
There was a problem hiding this comment.
[GENERAL] The two call sites this fix depends on have no test guarding them, and the failure mode is silent.
patched_hook_runner() is a no-op unless is_pyinstaller_bundle() is true, so every existing unit test in tests/unit/lib/init/test_init.py and tests/unit/lib/cookiecutter/test_template.py passes identically whether or not the with wrapper is present. The new tests/unit/lib/utils/test_hook_script.py covers the helper thoroughly but never asserts that anything applies it. That leaves the wiring — the part that actually fixes the bug — unverified at both samcli/lib/init/__init__.py:124 and samcli/lib/cookiecutter/template.py:172.
This is not hypothetical: the wrapper was initially applied to only one of the two cookiecutter() calls, and it took a review pass to catch it. A future refactor that drops or reorders the with block reintroduces the original silent bug (hook skipped, project generated wrong, exit 0) with no failing test, and the bundle path is not exercised by CI — validate_pyinstaller.yml only builds the binary.
A cheap regression guard in each existing test module:
@patch("samcli.lib.init.cookiecutter")
@patch("samcli.lib.init.patched_hook_runner")
def test_hook_runner_is_patched_around_cookiecutter(self, patched_hook_runner_mock, cookiecutter_patch):
generate_project(location="/path", output_dir=".", name="sam-app")
patched_hook_runner_mock.assert_called_once_with(cookiecutter_hooks)An equivalent test against samcli.lib.cookiecutter.template.patched_hook_runner covers the sam pipeline init path. If you want the assertion to also prove ordering (that the patch is active while cookiecutter() runs, not merely entered), have the mock's __enter__ record into a list that the cookiecutter mock's side_effect also appends to.
There was a problem hiding this comment.
Valid, fixed in 8a2fb4b. Your framing of the risk was right and the evidence you cite is fair — the missed second call site was caught by review, not by a test.
Added a guard to each existing module: test_hook_runner_is_active_while_cookiecutter_runs in tests/unit/lib/init/test_init.py and in tests/unit/lib/cookiecutter/test_template.py. I took the stronger ordering variant you suggested rather than a bare assert_called_once_with, so it proves the patch is active while cookiecutter() runs: __enter__, __exit__ and the cookiecutter mock all append to one list, asserted as ["enter", "cookiecutter", "exit"].
Confirmed the guards actually guard, by simulating the refactor you describe and deleting the with block at both call sites:
FAILED tests/unit/lib/init/test_init.py::TestInit::test_hook_runner_is_active_while_cookiecutter_runs
FAILED tests/unit/lib/cookiecutter/test_template.py::TestTemplate::test_hook_runner_is_active_while_cookiecutter_runs
2 failed, 20 passed
The 20 passed is your point restated: every other test in those modules is indifferent to the wrapper being present.
You are also right that CI never exercises the bundle path — validate_pyinstaller.yml only builds the binary. I verified the two interpreter paths locally against a simulated bundle instead, but that is not a substitute for CI coverage, and closing that gap is worth its own issue.
There was a problem hiding this comment.
Correction to the reply above: the commit is a7b00a0fc, not 8a2fb4b.
…ites patched_hook_runner is a no-op outside a bundle, so dropping the wrapper left every test passing. Each guard asserts the context manager is entered, that cookiecutter runs inside it, and that it is exited.
| # A hook launched by a real interpreter sees only its own path in argv; run_path fixes argv[0] | ||
| # but would leave our second argument behind, so give the hook the argv it expects. | ||
| with _replaced_attribute(sys, "argv", [arguments[0]]): | ||
| runpy.run_path(arguments[0], run_name="__main__") |
There was a problem hiding this comment.
[GENERAL] The two interpreter paths give hooks different import environments, which makes the bundled dependencies an implicit contract on exactly the hosts the fallback serves.
The constant block states the intent for preferring a system interpreter:
# A bundle ships no interpreter of its own. A system one is preferred for isolation, so that SAM's
# bundled dependencies do not become an implicit contract for template authors ...The fallback grants precisely that access. runpy.run_path executes the hook in-process inside the frozen interpreter, whose sys.path is the bundle, so the frozen importer resolves anything PyInstaller collected — yaml, jinja2, click, boto3, and samcli itself. Under the system-interpreter branch the hook runs out-of-process against a bare /usr/bin/python3, where those imports fail.
The observable consequence is that the same template behaves differently on two hosts running the same sam build, keyed on something the template author cannot see: whether a system Python ≥ 3.10 happens to be on PATH. A hook containing import yaml authored and tested on a host with only the native installer succeeds; the same hook on a host with python3 installed fails with an ImportError surfacing as cookiecutter's generic FailedHookException (exit status 1), with nothing pointing at the interpreter choice as the cause.
Two ways to close the gap, both consistent with the stated intent:
- Have the fallback reduce the hook's import surface before executing it — for example running
runpy.run_pathwithsys.pathrestricted to the script's own directory plus the stdlib, so a hook cannot reach bundled third-party packages regardless of which branch ran. Note this narrows but does not fully close the difference, since modules already insys.modulesfrom importingsamcli.cli.mainstay importable. - Alternatively, if the divergence is acceptable, say so in the module docstring and drop the isolation claim from the constant comment, so the next reader does not treat isolation as a guarantee the fallback silently breaks.
Either way the current pairing is self-contradictory: the comment promises a property that the code below it does not hold on the fallback path.
|
Closing in favour of a much smaller change. The packaged macOS/Linux SAM CLI cannot execute a template's Python hooks, and the fix here is to say so clearly rather than to emulate an interpreter. Reopening as a fresh PR to keep the diff and its review history readable. |
1 participant
Which issue(s) does this change fix?
N/A — found via the 2026-08-28 nightly (run 33161056863), 5 failures in
tests/integration/init/test_init_command.py.Why is this change necessary?
Two unrelated causes.
1. Template hooks never run in the packaged binary (pre-existing). Cookiecutter runs a Python hook as
[sys.executable, script](hooks.py#L84). In a PyInstaller bundlesys.executableissamitself, so it becomessam /tmp/xxxx.py. Because that path starts with/, click treats it as option-like, re-entersparse_argswith no args, hitsno_args_is_helpand callsctx.exit()— printing help and exiting 0 (core.py#L1753, #L1409-L1411). Cookiecutter reads 0 as success, so the hook is silently skipped and the project is generated wrong.Reproduced on released 1.154.0 (installed Feb 2026, six months before the test existed): exit 0, hook marker never written, sam's help text in the output.
sam boguscorrectly exits 2; only paths starting with a non-alphanumeric character hit this. Official app templates contain zero hooks, so the defaultsam initis unaffected — this hits custom--locationtemplates.2. A stale expected message. #9176 generated
INCOMPATIBLE_PARAMS_HINTfromNON_INTERACTIVE_PARAM_COMBINATIONSso the hint cannot drift from the enforced check, which changed the order to--dependency-manager, --app-template. The test'sMISSING_REQUIRED_PARAM_MESSAGEalready matched;INCOMPATIBLE_PARAM_MESSAGEdid not. It escaped PR CI because that class ispr_skip.How does it address the issue?
Supply a real interpreter for
.pyhooks whenis_pyinstaller_bundle():python3/python, probed by executing it (Windows ships apythonApp Execution Alias that resolves on PATH without being an interpreter), so hooks get the same environment as a pip install;Implemented by wrapping
cookiecutter.hooks.run_script, which readssys.executableat call time, so upstream'sFailedHookExceptionhandling,make_executableand Windows shell behaviour are all reused rather than reimplemented.cookiecutter.hooksis passed in as an argument sosamcli/lib/utils/hook_script.pystays importable from__main__without paying for a cookiecutter import on everysaminvocation.Applied at both
cookiecutter()call sites:sam init(samcli/lib/init/__init__.py) andTemplate.generate_project(samcli/lib/cookiecutter/template.py), the latter reachable viasam pipeline initwith a custom template location. In hook mode the re-launched process also callsisolate_library_paths_for_subprocess()before running the hook, because the bootloader re-points library paths into the bundle and the CLI callback that normally undoes that is never reached — hooks routinely shell out to git, npm and pip. Plus the one-line message correction.What side effects does this change have?
Behaviour change worth a release note: hooks that silently no-op today will start executing, so a template whose hook fails will now fail
sam initwhere it previously appeared to succeed. That is the correct behaviour, but it is a visible change for custom-template users.Non-frozen installs are untouched — the wrapper is a no-op, verified that
run_scriptandsys.executableare both left unmodified.PYINSTALLER_RESET_ENVIRONMENTis deliberately not set: the hook child is short-lived with the parent waiting, so worker semantics are correct. The env var is popped before running the hook so a hook that shells out tosamgets the normal CLI.Verified against a simulated bundle (real cookiecutter, real
generate_project,sys.executablepointed at a stand-in for the frozen binary):No new integration test: the one #9176 added is already the regression test, and it is the only place that exercises hooks against a real binary.
Mandatory Checklist
PRs will only be reviewed after checklist is complete
make prpassesmake update-reproducible-reqsif dependencies were changedBy submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.