Skip to content

fix: process windows hook key for script copying and path rewriting#521

Open
Copilot wants to merge 4 commits intomainfrom
copilot/fix-windows-property-processing
Open

fix: process windows hook key for script copying and path rewriting#521
Copilot wants to merge 4 commits intomainfrom
copilot/fix-windows-property-processing

Conversation

Copy link
Copy Markdown
Contributor

Copilot AI commented Mar 31, 2026

Description

_rewrite_hooks_data() iterated over a hardcoded tuple ("command", "bash", "powershell") at two sites — the windows key was missing, so scripts referenced in it were never copied and paths were never rewritten.

Fix: Extract a single HOOK_COMMAND_KEYS class constant on HookIntegrator so both loops share one source of truth. Future platform keys require a one-line change.

# Before — duplicated in two places, easy to miss a key
for key in ("command", "bash", "powershell"):

# After — single constant, includes windows
HOOK_COMMAND_KEYS: Tuple[str, ...] = ("command", "bash", "powershell", "windows")

for key in self.HOOK_COMMAND_KEYS:

Also adds logging.debug() in the rewrite loops so verbose output shows which keys were transcribed and how many scripts were extracted per key.

Type of change

  • Bug fix
  • New feature
  • Documentation
  • Maintenance / refactor

Testing

  • Tested locally
  • All existing tests pass
  • Added tests for new functionality (if applicable)

Four new tests: test_rewrite_windows_key, test_rewrite_hooks_data_windows_flat_format, test_rewrite_hooks_data_windows_nested_format, test_rewrite_hooks_data_all_platform_keys.

Copilot AI and others added 3 commits March 31, 2026 21:54
Agent-Logs-Url: https://github.com/microsoft/apm/sessions/8ea8e0b4-eccd-436e-92c2-fefdbd55ba3c

Co-authored-by: danielmeppiel <51440732+danielmeppiel@users.noreply.github.com>
… hook transcription

Add class-level HOOK_COMMAND_KEYS tuple as single source of truth for
hook command keys that contain script-path references. This replaces
the two hardcoded tuples in _rewrite_hooks_data() and includes the
previously missing "windows" key.

Also adds Python logging (debug level) for hook transcription so
verbose output reports which keys were rewritten and how many scripts
were extracted per key.

Fixes #311

Agent-Logs-Url: https://github.com/microsoft/apm/sessions/8ea8e0b4-eccd-436e-92c2-fefdbd55ba3c

Co-authored-by: danielmeppiel <51440732+danielmeppiel@users.noreply.github.com>
… add changelog entry

Agent-Logs-Url: https://github.com/microsoft/apm/sessions/8ea8e0b4-eccd-436e-92c2-fefdbd55ba3c

Co-authored-by: danielmeppiel <51440732+danielmeppiel@users.noreply.github.com>
Copilot AI changed the title [WIP] Fix bug: Process 'windows' property in hook integrator fix: process windows hook key for script copying and path rewriting Mar 31, 2026
Copilot AI requested a review from danielmeppiel March 31, 2026 22:00
@danielmeppiel danielmeppiel marked this pull request as ready for review March 31, 2026 22:01
Copilot AI review requested due to automatic review settings March 31, 2026 22:01
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Fixes hook script copying/path rewriting for the windows hook key by centralizing the set of hook command keys and extending rewrite logic to include windows, with accompanying unit tests and a changelog entry.

Changes:

  • Introduce HookIntegrator.HOOK_COMMAND_KEYS and iterate over it in _rewrite_hooks_data() so windows is processed consistently.
  • Add debug logging when hook keys are rewritten and scripts are extracted.
  • Add unit tests covering windows in both flat and nested hook formats, plus update CHANGELOG.md.

Reviewed changes

Copilot reviewed 3 out of 4 changed files in this pull request and generated 1 comment.

File Description
src/apm_cli/integration/hook_integrator.py Adds HOOK_COMMAND_KEYS (including windows) and uses it in both rewrite loops; adds debug logging.
tests/unit/integration/test_hook_integrator.py Adds coverage for windows hook rewriting in multiple formats and combined-key scenarios.
CHANGELOG.md Documents the bug fix under ## [Unreleased].
Comments suppressed due to low confidence (1)

src/apm_cli/integration/hook_integrator.py:274

  • Same as above: extending all_scripts without de-duplication can return duplicate script copy entries, which leads to redundant copies and potential false collision warnings/skips during install. Consider de-duplicating by target_rel before returning.
                            all_scripts.extend(scripts)

package_name, event_name, key, len(scripts),
)
matcher[key] = new_cmd
all_scripts.extend(scripts)
Copy link

Copilot AI Mar 31, 2026

Choose a reason for hiding this comment

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

_rewrite_hooks_data() can accumulate duplicate (source_file, target_rel) entries when multiple keys reference the same script (e.g., command and bash pointing to the same ./scripts/run.sh). Those duplicates later trigger redundant copy attempts and can produce false "local file exists" collision skips because managed_files is not updated during the integration loop. Consider de-duplicating scripts by target_rel (or source+target) before extending/returning.

This issue also appears on line 274 of the same file.

Copilot uses AI. Check for mistakes.
@danielmeppiel danielmeppiel added the bug Something isn't working label Mar 31, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Bug: Hook integrator does not process 'windows' property – scripts not copied and paths not rewritten

3 participants