Skip to content

fix(install): restore static-target deploy path; tighten test fixtures for #1154 strict harness detection#1176

Merged
danielmeppiel merged 1 commit intomainfrom
fix/skill-deploy-path-and-target-test-fixtures
May 7, 2026
Merged

fix(install): restore static-target deploy path; tighten test fixtures for #1154 strict harness detection#1176
danielmeppiel merged 1 commit intomainfrom
fix/skill-deploy-path-and-target-test-fixtures

Conversation

@danielmeppiel
Copy link
Copy Markdown
Collaborator

TL;DR

Two CI failures on the v0.12.3 retag exposed (1) a real source regression where skills deployed to the wrong path, and (2) a fleet of integration/release tests that hadn't been updated for the post-#1154 strict harness-detection regime. This PR fixes both so the full integration AND release-validation suites are green locally end-to-end.

Problem (WHY)

After cutting v0.12.3 (#1170, #1171, #1173) the integration job kept failing. Investigation surfaced two distinct issues:

  1. Source regression in PR fix(targets): explicit, auditable target resolution (closes #1154) #1165: phases/targets.py started setting resolved_deploy_root = project_root / target.root_dir on EVERY v2-resolved static target. But that field was originally set only by cowork's dynamic-root for_scope(user_scope=True), and downstream readers (skill_integrator.py:751-753, 887-889, 985-986, base_integrator.py:237-242) treat resolved_deploy_root != None as "this is a dynamic-root cowork target where the resolved path is the FINAL deploy destination". Result: skills landed at <root_dir>/<skill_name>/ (e.g. .github/brand-guidelines/SKILL.md) instead of <deploy_root>/<subdir>/<skill_name>/ (e.g. .agents/skills/brand-guidelines/SKILL.md). Reproduced manually:

    |-- Skill integrated -> .agents/skills/    # what the log claims
    ./.github/brand-guidelines/SKILL.md         # what actually happens (pre-fix)
    ./.agents/skills/brand-guidelines/SKILL.md  # post-fix
    
  2. Test/script fixtures lacking target:: After Claude skills silently skipped when CLAUDE.md exists but .claude/ directory does not #1154, apm install exits 2 with "No harness detected" when the project has neither a harness signal nor target: in apm.yml nor --target on the CLI. A wide swath of integration tests, release-validation scripts, and even the gh-aw compat fixture (test-release-validation.sh) were still relying on the old "default to copilot" behavior.

Approach (WHAT)

Source fix (smallest possible surface): stop setting resolved_deploy_root on static targets. Static targets fall back to the standard primitive-mapping path (deploy_root override + subdir), which is the correct logic. resolved_deploy_root remains reserved for true dynamic-root targets (cowork via for_scope).

Fixture fixes: add target: copilot to every test/script apm.yml literal that drives apm install without a harness signal. For tests that intentionally exercise multi-target behavior (link_rewrite_e2e::TestMultiTargetLinkRewriting), pass an explicit list.

Unit-test alignment: test_targets_phase_v2.py::test_three_guard_collapse_no_skip was added in #1165 and used resolved_deploy_root as a proxy for "target was resolved". Updated the helper to accept either resolved_deploy_root (cowork) or root_dir (static), preserving the semantic intent of the test ("target is in ctx.targets with a real on-disk dir") without leaking the proxy detail.

Implementation (HOW)

File Change
src/apm_cli/install/phases/targets.py Remove two resolved_deploy_root=_target_dir assignments (lines ~285, ~424). Drop now-unused dataclasses.replace imports.
tests/unit/install/phases/test_targets_phase_v2.py _resolved_dirs -> _target_root_dirs(ctx, project_root) that falls back to root_dir for static targets.
tests/integration/test_drift_check.py Default target arg on _make_apm_project flipped from None -> "copilot".
tests/integration/test_drift_check_e2e.py _make_project adds target: copilot.
tests/integration/test_link_rewrite_e2e.py _make_consumer defaults to target: copilot; MultiTargetLinkRewriting now passes targets=["copilot","claude"]; switch from plural targets: to canonical singular target: (CSV when multiple).
tests/integration/test_transitive_chain_e2e.py Two consumer apm.yml literals add target: copilot.
scripts/test-release-validation.sh gh-aw compat fixture (apm-action's generateManifest format) adds target: copilot.
scripts/test-dependency-integration.sh + Windows .ps1 mirror Both real-dep and multi-dep apm.yml fixtures add target: copilot.

Other test-fixture target-additions and one token-prefix relaxation (test_apm_dependencies.py accepts gho_ from gh CLI OAuth in addition to github_pat_*) carried over from earlier work in this batch.

Validation

$ bash scripts/test-integration.sh --use-existing-binary
... Integration validation complete - COMPREHENSIVE TESTING ... ✅ Ready for release validation!

$ bash scripts/test-release-validation.sh /Users/.../.venv/bin/apm
... Results: 6/6 tests passed ✅ RELEASE VALIDATION PASSED!

$ uv run pytest tests/unit -q
7779 passed, 1 warning, 30 subtests passed in 90.39s

$ uv run --extra dev ruff check src/ tests/ && uv run --extra dev ruff format --check src/ tests/
All checks passed! ; 705 files already formatted

Trade-offs

  • Source fix is surgical (remove 2 lines + 2 unused imports) instead of guarding every reader with target.user_root_resolver is not None. Both restore the same invariant; the surgical version preserves the cowork-only contract that downstream code already encodes.
  • The gh-aw compat scenario change in the script makes a soft assumption that apm-action's generateManifest() will be updated to include target: once it consumes >= v0.12.3. Until then, gh-aw users on older apm-action will hit the same exit-2; this is the expected new contract from Claude skills silently skipped when CLAUDE.md exists but .claude/ directory does not #1154 and not introduced by this PR.

How to test

# repro skill deploy regression on main pre-fix
git checkout main
mkdir /tmp/skill-test && cd /tmp/skill-test
printf 'name: t\nversion: 1.0.0\ntarget: copilot\n' > apm.yml && mkdir .github
apm install anthropics/skills/skills/brand-guidelines
find . -name 'SKILL.md'   # outside apm_modules: was .github/brand-guidelines/, now .agents/skills/brand-guidelines/

# verify suites
bash scripts/test-integration.sh --use-existing-binary
bash scripts/test-release-validation.sh

Co-authored-by: Copilot 223556219+Copilot@users.noreply.github.com

…s for #1154

PR #1165 set resolved_deploy_root on every v2-resolved static target
(phases/targets.py:285, :424). Downstream readers in skill_integrator
and base_integrator treat that field as the FINAL deploy destination
(it was originally only set by cowork's dynamic-root for_scope), so
skills landed at <root_dir>/<skill_name>/ (e.g. .github/<name>/)
instead of <deploy_root>/skills/<name>/ (e.g. .agents/skills/<name>/).

Source fix:
  - Stop setting resolved_deploy_root on static targets in both
    branches of phases/targets.py. Static targets fall back to the
    standard primitive-mapping path (deploy_root + subdir).

Test fixture fixes (#1154 strict harness detection):
  - integration: drift_check, drift_check_e2e, link_rewrite_e2e,
    transitive_chain_e2e all needed target: copilot in their
    apm.yml literals.
  - integration: link_rewrite_e2e MultiTarget test now passes
    target=[copilot,claude] explicitly.
  - release: test-dependency-integration.sh, test-release-validation.sh
    (gh-aw compat fixture), and the Windows mirror all add target: copilot.
  - unit: test_targets_phase_v2 was using resolved_deploy_root as a
    proxy for resolution; updated helper to also accept root_dir for
    static targets, so it asserts the actual semantic.

Local verification:
  - bash scripts/test-integration.sh --use-existing-binary -> green
  - bash scripts/test-release-validation.sh -> 6/6 green
  - uv run pytest tests/unit -> 7779 passed
  - ruff check + format --check -> silent

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings May 6, 2026 23:22
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

Restores correct deployment routing for static targets by reserving resolved_deploy_root for true dynamic-root targets (cowork), and updates integration/unit fixtures to explicitly declare target: copilot under the post-#1154 strict harness-detection contract.

Changes:

  • Stop setting resolved_deploy_root for static v2-resolved targets so integrators honor deploy_root + subdir mappings (e.g., skills to .agents/skills/...).
  • Update unit/integration tests and validation scripts to include target: copilot (and explicit multi-target CSV where needed) to avoid exit-2 “No harness detected”.
  • Adjust a unit-test helper to treat static targets as resolved via root_dir when resolved_deploy_root is absent.
Show a summary per file
File Description
src/apm_cli/install/phases/targets.py Removes resolved_deploy_root assignment for static targets to restore correct deploy-path mapping behavior.
tests/unit/install/phases/test_targets_phase_v2.py Updates helper to derive target root dirs from root_dir when resolved_deploy_root is not set.
tests/integration/test_transitive_chain_e2e.py Adds target: copilot to consumer fixtures to satisfy strict harness detection.
tests/integration/test_skill_integration.py Adds target: copilot to the temp project manifest.
tests/integration/test_skill_install.py Adds target: copilot to manifests used by skill-install tests.
tests/integration/test_mixed_deps.py Adds target: copilot to the temp project manifest.
tests/integration/test_link_rewrite_e2e.py Defaults consumer manifest to target: copilot and sets explicit multi-target CSV for the multi-target scenario.
tests/integration/test_drift_check.py Defaults _make_apm_project(..., target=...) to "copilot" to prevent harness-detection failures.
tests/integration/test_drift_check_e2e.py Writes target: copilot into the minimal apm.yml fixture.
tests/integration/test_deps_update_e2e.py Adds target: copilot to manifests used in deps-update flows.
tests/integration/test_cache_lockfile_parity.py Adds target: copilot to the parity fixture manifest.
tests/integration/test_apm_dependencies.py Relaxes token-prefix assertions to accept additional valid GitHub token prefixes.
scripts/test-release-validation.sh Adds target: copilot to the gh-aw compatibility apm.yml fixture.
scripts/test-dependency-integration.sh Adds target: copilot to dependency-integration apm.yml fixtures.
scripts/windows/test-dependency-integration.ps1 Mirrors the shell script fixture updates by adding target: copilot.

Copilot's findings

Comments suppressed due to low confidence (1)

tests/integration/test_link_rewrite_e2e.py:387

  • The comment above the manual .claude mkdir is now misleading: this fixture is using an explicit target: copilot,claude in apm.yml, so the install no longer relies on auto-detection needing both target directories present. Please update/remove the comment so future readers don't assume the behavior is auto-detect-driven.
        consumer = _make_consumer(ws, targets=["copilot", "claude"])
        # Auto-detect needs both target dirs present.  Copilot
        # instructions deploy to .github/instructions/; Claude
        # instructions deploy to .claude/rules/.
        (consumer / ".claude").mkdir()
  • Files reviewed: 15/15 changed files
  • Comments generated: 0

@danielmeppiel danielmeppiel merged commit 634ba5f into main May 7, 2026
21 checks passed
@danielmeppiel danielmeppiel deleted the fix/skill-deploy-path-and-target-test-fixtures branch May 7, 2026 06:30
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.

2 participants