fix(hooks): emit required top-level version in Copilot hook JSON (closes #2128)

[danielmeppiel]

Generated Copilot hook JSON copied source top-level fields as-is, so version-absent hook primitives produced non-conformant files without the upstream-required version: 1. This change defaults version to 1 immediately before serialization while preserving explicit source values, and adds regression coverage for a fresh Copilot hook installation.

How to test

• uv run --extra dev python -m pytest tests/unit/integration/test_hook_integrator.py -q
• uv run --extra dev python -m pytest tests/ -k "hook or copilot" -q
• Run the canonical ruff, pylint R0801, and auth-signal lint chain.
• Remove rewritten.setdefault("version", 1) and confirm the new regression test fails with a missing version key.

Closes #2128

[danielmeppiel]

Docs sync advisory

Verdict: no_change * Pages affected: 0 * LLM calls: 0/15 * Took: <1s

No docs impact detected. The change supplies an upstream-required field in generated Copilot hook JSON without changing the documented authoring shape, CLI surface, or target paths.

[Copilot]

Pull request overview

Ensures GitHub Copilot hook JSON emitted by HookIntegrator is always schema-conformant by defaulting the required top-level version field to 1 when the source hook primitive omits it, and adds a regression test covering the Copilot install path.

Changes:

• Default Copilot hook JSON top-level version to 1 immediately before serialization (without overwriting explicitly provided values).
• Add a unit regression test asserting .github/hooks/<pkg>-hooks.json contains version: 1 on a fresh install.

Show a summary per file

File	Description
src/apm_cli/integration/hook_integrator.py	Adds rewritten.setdefault("version", 1) before writing Copilot hook JSON to ensure required schema compliance.
tests/unit/integration/test_hook_integrator.py	Adds a regression test verifying generated Copilot hook JSON includes version: 1.

Review details

• Files reviewed: 2/2 changed files
• Comments generated: 0
• Review effort level: Low

[danielmeppiel]

APM Review Panel: ship_with_followups

One-line setdefault fix makes generated Copilot hook JSON carry the required top-level version field; the panel's only substantive ask is a regression trap for preserving an explicit source version.

cc @sergio-sisternes-epam -- a fresh advisory pass is ready for your review.

The panel converges cleanly: every active reviewer confirms the change is correctly scoped, correctly placed after collision handling and before serialization, and does not mutate user-owned files. The blast radius is as small as the PR claims.

The one finding worth folding came from test coverage and supply-chain review: setdefault has two branches (absent -> inject default; present -> preserve), but only the absent branch is tested. The present branch is the second half of the PR's stated behavior, and the Cursor sibling path already has paired preservation coverage. This is in-scope regression safety, not scope creep.

The remaining nits about a shared defaults helper and release-time changelog entry cross this PR's narrow scope and need no action here.

Dissent. Test coverage rated the missing preservation test as recommended while supply-chain review rated it as a nit. The concrete missing assertion and the PR's own preservation claim support the recommended signal.

Aligned with: Portability by manifest: explicit source versions should survive generation untouched. Multi-harness support: Copilot now receives the same required schema discipline already present for Cursor.

Panel summary

Persona	B	R	N	Takeaway
Python Architect	0	1	1	Correct, well-scoped fix; the sibling declarative defaults mechanism is worth cross-referencing, not refactoring here.
CLI Logging Expert	0	0	0	No CLI-visible output changed; no logging UX concerns.
DevX UX Expert	0	0	1	Narrow conformance fix with no user-facing command or error surface change.
Supply Chain Security Expert	0	0	1	No integrity, path, or token concern; explicit-version preservation lacks a regression trap.
OSS Growth Hacker	0	0	1	Internal reliability fix; mention it at release time.
Test Coverage Expert	0	1	0	Fresh-install default is tested; explicit-source-version preservation is unguarded.

B = blocking-severity findings, R = recommended, N = nits.
Counts are signal strength, not gates. The maintainer ships.

Top 1 follow-ups

1. [Test Coverage Expert] Add a regression test asserting an explicit source version (for example, version=3) is preserved through the setdefault path -- the PR claims both default injection and explicit-value preservation, but only the first branch is tested.

Architecture

classDiagram
 class HookIntegrator
 class BaseIntegrator
 HookIntegrator --|> BaseIntegrator
 class HookIntegrator:::touched
 classDef touched fill:#fff3b0,stroke:#d47600

Loading

flowchart TD
 A[apm install] --> B[HookIntegrator]
 B --> C[rewrite Copilot hooks]
 C --> D[setdefault version 1]
 D --> E[json.dump]

Loading

Recommendation

Ship once the preservation-branch test is folded in. It is a one-test addition mirroring an existing pattern, not a rework. The defaults-helper and changelog-timing nits can remain outside this PR.

---

Full per-persona findings

Python Architect

• [recommended] Copilot version default is a bare literal while an identical declarative mechanism already exists for Cursor at src/apm_cli/integration/hook_integrator.py:1286
  The separate per-package and merged-target paths make inline setdefault defensible, but the two default sites are less discoverable. No correctness issue.
  Suggested: If this area is revisited, cross-reference the sibling mechanism; do not expand this PR into a framework refactor.
• [nit] setdefault is correctly placed after collision handling
  It does not mutate user-owned files and preserves pre-existing values. No action needed.

CLI Logging Expert

No findings.

DevX UX Expert

• [nit] The new setdefault duplicates the existing top_level_defaults mechanism at src/apm_cli/integration/hook_integrator.py:1286
  The separate Copilot write path makes this reasonable; a shared helper would exceed this PR's scope.

Supply Chain Security Expert

• [nit] No test proves setdefault preserves an explicit package-declared version at tests/unit/integration/test_hook_integrator.py
  The code is correct, but the second half of the stated behavior lacks a regression trap.
  Suggested: Add a source version=3 case and assert the emitted value remains 3.

OSS Growth Hacker

• [nit] No Unreleased changelog entry yet at CHANGELOG.md
  Capture this reliability fix during release preparation; no PR change is needed now.

Auth Expert -- inactive

Only hook JSON shape and its test changed; no authentication, token, credential, or host-selection surface is touched.

Doc Writer -- inactive

Only generated internal schema output and its regression test changed; documented authoring shape, CLI surface, and target paths are unchanged.

Test Coverage Expert

• [recommended] No regression trap for explicit source version preservation at tests/unit/integration/test_hook_integrator.py
setdefault has absent and present branches; only the absent branch is tested, while Cursor already has paired preservation coverage.
  Suggested: Add test_copilot_existing_source_version_preserved with a source version=3 and assert the emitted value remains 3.
  Proof (missing at): tests/unit/integration/test_hook_integrator.py::test_copilot_existing_source_version_preserved -- proves: an explicit source schema version is not overwritten. [multi-harness-support,devx]
  assert config["version"] == 3

Performance Expert -- inactive

One dictionary default and one regression test do not touch dependency resolution, cache, materialization, transport, or an install hot path.

_{This panel is advisory. It does not block merge. Re-apply the
panel-review label after addressing feedback to re-run.}

[danielmeppiel]

APM Review Panel: ship_now

Generated Copilot hook JSON now always carries the required top-level version while preserving explicit source values, with direct and install-dispatch regression proof.

cc @sergio-sisternes-epam -- a fresh advisory pass is ready for your review.

This is a clean, narrow reliability fix. The production change is one setdefault immediately before serialization. Tests prove all states that matter: absent input defaults to 1, explicit source value 3 survives unchanged, and the real install-service dispatch writes version: 1.

The mutation gates are load-bearing proof rather than decoration: replacing setdefault with assignment breaks preservation, while deleting version emission breaks the dispatch-tier assertion. Architecture, CLI output, DevX, supply-chain, and test-coverage reviewers found no remaining in-scope concerns. Auth, docs, and performance correctly found no relevant surface.

Aligned with: Portability by manifest: generated hook JSON carries the schema field downstream Copilot requires. Pragmatic as npm: one production line with targeted, mutation-proven regression coverage.

Growth signal. Capture the user-visible reliability fix in release notes during the existing cut-release workflow; no change is needed in this PR.

Panel summary

Persona	B	R	N	Takeaway
Python Architect	0	0	0	Minor, well-tested one-line fix; no architectural concerns.
CLI Logging Expert	0	0	0	No CLI output or logging surface changed.
DevX UX Expert	0	0	0	Copilot schema correctness improved without changing command UX.
Supply Chain Security Expert	0	0	0	No trust, path, credential, or download surface changed.
OSS Growth Hacker	0	1	0	Mention the repaired Copilot-hook symptom at release time.
Test Coverage Expert	0	0	0	Direct and install-dispatch coverage is run and mutation-confirmed.

B = blocking-severity findings, R = recommended, N = nits.
Counts are signal strength, not gates. The maintainer ships.

Architecture

classDiagram
    direction LR
    class BaseIntegrator
    class HookIntegrator {
      +integrate_package_hooks(package_info, project_root) HookIntegrationResult
    }
    class HookConfigDict {
      +hooks dict
      +version int
    }
    BaseIntegrator <|-- HookIntegrator
    HookIntegrator ..> HookConfigDict : rewrites
    class HookIntegrator:::touched
    classDef touched fill:#fff3b0,stroke:#d47600

Loading

flowchart TD
    A[apm install] --> B[HookIntegrator]
    B --> C[read source hooks JSON]
    C --> D[rewrite Copilot events]
    D --> E[setdefault version 1]
    E --> F[write generated hook JSON]

Loading

Recommendation

Ship as-is. The change is minimal, both failure modes are mutation-confirmed, and no in-PR follow-ups remain.

Folded in this run

• (panel) Add a regression trap proving an explicit source version is preserved -- resolved in fbfc9f0d0ea62573d99be613239aa6b293d3c8ec.
• (panel) Assert the required version through the real install-service dispatch -- resolved in 42af7a69a0929c83d1e50a1210a3f18ef1756f24.

Regression-trap evidence (mutation-break gate)

• TestVSCodeIntegration::test_copilot_existing_source_version_preserved -- replaced rewritten.setdefault("version", 1) with assignment; test FAILED as expected (1 != 3); guard restored.
• test_copilot_install_writes_only_camel_case_hook_events -- deleted rewritten.setdefault("version", 1); test FAILED as expected (KeyError: version); guard restored.

Lint contract

The full CI-mirror chain completed with exit code 0: ruff check, ruff format check, pylint R0801, and auth-signal lint. The final targeted suites report 168 passed.

CI

All 13 checks are green on fbc2e84f865efcf092ced18243a174b66c69da3a, including Lint, both Linux test shards, coverage combine, CodeQL, binary smoke, spec conformance, and merge gate: https://github.com/microsoft/apm/actions/runs/29117788172 (0 CI fix iterations).

Mergeability status

PR	head SHA	CEO stance	iters	folds	defers	Copilot rounds	CI	mergeable	mergeStateStatus	notes
#2133	fbc2e84	ship_now	2	2	0	2	green	MERGEABLE	BLOCKED	pending required review

Convergence

2 outer iterations; 2 Copilot rounds with zero inline findings. Final panel stance: ship_now.

Ready for maintainer review.

---

Full per-persona findings

Python Architect

No findings.

CLI Logging Expert

No findings.

DevX UX Expert

No findings.

Supply Chain Security Expert

No findings.

OSS Growth Hacker

• [recommended] Name the repaired Copilot-hook conformance symptom in release notes.
  The existing cut-release workflow owns this release-time communication; it requires no change in this PR.

Auth Expert -- inactive

Only hook JSON generation and regression tests changed; no authentication, credential, token, or host-selection surface is touched.

Doc Writer -- inactive

The generated upstream-required field does not change the documented authoring shape, CLI surface, or target paths.

Test Coverage Expert

No findings.

Performance Expert -- inactive

One dictionary default and regression assertions do not touch dependency resolution, cache, transport, or materialization hot paths.

_{This panel is advisory. It does not block merge. Re-apply the
panel-review label after addressing feedback to re-run.}

[danielmeppiel]

Superseded by #2155. The reliability campaign consolidated all 13 point fixes into a single architectural cure PR that re-homes each fix under its canonical owner module and adds owner-invariant guards. The fix for #2128 is folded into #2155 and re-verified there (full CI green). Closing this point PR in favor of the consolidated cure; the branch is preserved and this can be reopened if #2155 is not merged.