Skip to content

fix(hooks): deploy hook script runtime context#2028

Open
WilliamK112 wants to merge 1 commit into
microsoft:mainfrom
WilliamK112:codex/apm-hook-script-runtime-2023
Open

fix(hooks): deploy hook script runtime context#2028
WilliamK112 wants to merge 1 commit into
microsoft:mainfrom
WilliamK112:codex/apm-hook-script-runtime-2023

Conversation

@WilliamK112

@WilliamK112 WilliamK112 commented Jul 4, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Expands deployed hook script copies from only the command entrypoint to the entrypoint runtime subtree, so required sibling modules like ./ponytail-config are present after install.
  • Copies the nearest source package.json into the deployed hook runtime directory so hook scripts keep the package module semantics instead of inheriting the consumer project's package.json type.
  • Adds a Node-backed regression for both Copilot and Claude deployments in a consumer project with type=module, proving the deployed CommonJS hook can run and load its sibling module.

Closes #2023.

Preflight

Validation

  • uv run --extra dev pytest tests/integration/test_hook_script_runtime_deployment_e2e.py -q (2 passed)
  • uv run --extra dev pytest tests/unit/integration/test_hook_integrator.py tests/integration/test_hook_script_runtime_deployment_e2e.py tests/integration/test_hook_routing_combined_manifest_e2e.py tests/integration/test_hook_integrator_copilot_casing_e2e.py tests/integration/test_integrators_hooks_execution.py tests/integration/test_integrators_hooks_phase3c.py -q (444 passed)
  • uv run --extra dev ruff check src tests
  • uv run --extra dev ruff format --check src tests
  • git diff --check
  • uv run --extra dev pytest -q was also attempted; this repo's default marker configuration skipped 29592 tests and deselected 173, so the targeted hook suite above is the relevant behavior evidence.

Copilot AI review requested due to automatic review settings July 4, 2026 23:53

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@sergio-sisternes-epam

Copy link
Copy Markdown
Collaborator

APM Review Panel -- PR #2028

Ship recommendation: APPROVE


What this PR does

Closes a runtime crash: hook scripts that require('./sibling-module') failed in consumer projects
with "type": "module" in their package.json because the deployer only copied the entrypoint.
This PR copies the entrypoint's directory subtree and the nearest source package.json alongside
it, so the deployed CommonJS hook runs with its original module semantics.


Correctness

The fix is implemented across 6 focused private helpers feeding two call sites
(integrate_package_hooks line 1399, _integrate_merged_hooks line 1625). Both sites now
expand script lists before copying. The fallback contract is conservative at every layer -- if
anything fails (path traversal, rglob error, missing package.json), the function returns
the original single-entrypoint pair, preserving pre-PR behaviour rather than crashing.


Security

Path traversal is guarded at every materialisation point via ensure_path_within. Symlinks are
skipped before is_file() checks throughout _iter_hook_runtime_files and
_nearest_package_json_for_script. No concerns.


Test evidence

Two new integration tests run node as a subprocess and assert stdout == "PONYTAIL MODE ACTIVE",
proving the deployed script can load its sibling module inside a "type": "module" consumer project.
scripts_copied >= 3 asserts expansion happened. 170 pre-existing hook tests continue to pass.


Minor suggestions (non-blocking)

  1. _hook_runtime_target_root: add a one-line comment on the len(target_path.parts) <= len(child_parts) guard explaining the invariant.
  2. _expand_hook_runtime_scripts: add a brief comment on the {target: source} dict inversion before the list comprehension to make the swap explicit.
  3. Consider a Cursor or Codex runtime-subprocess test to match the Copilot/Claude pair -- not required, the shared code path is covered, but it would complete the cross-target runtime proof.

Panel: python-architect, supply-chain-security, test-coverage, performance, devx-ux, cli-logging, ceo-synthesizer

@sergio-sisternes-epam sergio-sisternes-epam left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@WilliamK112 Panel review complete. All findings are non-blocking nits. The PR needs rebase and fix CI and potential conflicts

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG] Deployed hook scripts crash at runtime: required sibling modules are not copied, and the consumer's package.json module type is applied to them

3 participants