refactor(cowork): DRY internal hot-path cleanup (closes #922)

[danielmeppiel]

refactor(cowork): DRY internal hot-path cleanup

TL;DR

This is the item-1-only refactor accepted for #922: SkillIntegrator no longer open-codes the cowork dynamic-root branch at the three identified call sites. The change is behavior-preserving and keeps static target deploy_root handling intact. Closes #922 under the maintainer's accepted scope.

Note

Scope is intentionally limited to issue item 1: routing skill deployment paths through the target deploy-path abstraction. Items 2 and 3 remain out of scope per the Board comment.

Problem (WHY)

• Issue Cowork internal refactors — DRY and hot-path cleanup #922 item 1 identified three SkillIntegrator call sites that repeated if target.resolved_deploy_root is not None instead of using the target abstraction.
• The maintainer acceptance narrowed the work to "scoped to item 1 only" and required "no public CLI/manifest surface change".
• [!] Repeating the dynamic-root branch makes future cowork path changes easier to miss in hot skill deployment paths.

Approach (WHAT)

#	Fix
1	Add one internal helper for resolving the target skills root.
2	Add one internal helper for resolving a concrete native skill directory.
3	Replace the three item-1 dynamic-root branches with those helpers, which route cowork roots through TargetProfile.deploy_path().
4	Update the existing target test doubles so they model the deploy-path accessor now used by the implementation.

Implementation (HOW)

• src/apm_cli/integration/skill_integrator.py -- adds _target_skills_root() and _target_skill_dir() and uses them in the three item-1 call sites: standalone sub-skill promotion, native skill directory deployment, and native sub-skill promotion. Static targets still use skills_mapping.deploy_root or target.root_dir, preserving existing Copilot/Cursor/OpenCode .agents/skills behavior.
• tests/unit/integration/test_skill_integrator_hermetic.py -- teaches the local _make_target() mock to expose deploy_path() so the dedup tests exercise the same internal accessor shape.
• tests/unit/integration/test_skill_integrator_phase3w4.py -- mirrors the same mock update in the duplicate coverage fixture.

Diagrams

Legend: the diagram shows the three repeated cowork branches collapsing into two internal helpers while cowork roots continue through TargetProfile.deploy_path().

flowchart LR
    subgraph Before[Before]
        B1[standalone sub-skill path]
        B2[native skill path]
        B3[native sub-skill promotion]
        B4[inline resolved_deploy_root branches]
    end
    subgraph After[After]
        A1[_target_skills_root]:::new
        A2[_target_skill_dir]:::new
        A3[TargetProfile.deploy_path]
    end
    B1 --> A1
    B2 --> A2
    B3 --> A1
    A1 --> A3
    A2 --> A1
    classDef new stroke-dasharray: 5 5;
    class A1,A2 new;

Loading

Trade-offs

• Helper extraction instead of broader target API change. Chose a local SkillIntegrator helper; rejected changing TargetProfile.deploy_path() semantics because item 1 is a bounded internal tidy.
• No behavior tests added. This is a pure refactor; existing cowork and skill-integrator tests already cover the affected paths, and the only test changes keep mocks aligned with the production accessor.
• Items 2 and 3 left untouched. The maintainer explicitly scoped this PR to item 1 only.

Benefits

1. Reduces three open-coded cowork dynamic-root branches to one helper path.
2. Keeps cowork deployment rooted through TargetProfile.deploy_path() for the accepted item-1 sites.
3. Preserves static deploy_root behavior for existing non-cowork skill targets.
4. Leaves public CLI flags, manifest schema, and user-visible output unchanged.

Validation

Baseline before refactor:

$ uv run --extra dev pytest tests/ -k cowork -q
224 skipped, 28664 deselected in 28.95s

$ uv run --extra dev pytest tests/unit/integration/test_skill_integrator_cowork.py -q
6 passed in 1.53s

After refactor:

$ uv run --extra dev pytest tests/ -k cowork -q
224 skipped, 28664 deselected in 9.05s

$ uv run --extra dev pytest tests/unit/integration/test_skill_integrator_cowork.py -q
6 passed in 0.79s

$ uv run --extra dev pytest tests/unit/integration/test_skill_integrator.py tests/unit/integration/test_skill_integrator_hermetic.py tests/unit/integration/test_skill_integrator_phase3w4.py -q
280 passed in 4.36s

$ uv run --extra dev pytest tests/ -q
exit 0

Lint:

$ uv run --extra dev ruff check src/ tests/ && uv run --extra dev ruff format --check src/ tests/ && uv run --extra dev python -m pylint --disable=all --enable=R0801 --min-similarity-lines=10 --fail-on=R0801 src/apm_cli/ && bash scripts/lint-auth-signals.sh
All checks passed!
1315 files already formatted

--------------------------------------------------------------------
Your code has been rated at 10.00/10 (previous run: 10.00/10, +0.00)

[*] Rule A: get_bearer_provider boundary (any reference)
[*] Rule B: git ls-remote auth-delegated annotation
[+] auth-signal lint clean

Additional CI lint guards (YAML I/O, file length, raw str(relative_to(...))) were run with BSD-compatible local equivalents and exited 0.

Scenario Evidence

Skipped: pure internal refactor with no observable behavior change.

How to test

• Run uv run --extra dev pytest tests/unit/integration/test_skill_integrator_cowork.py -q and expect 6 passing tests.
• Run uv run --extra dev pytest tests/unit/integration/test_skill_integrator.py tests/unit/integration/test_skill_integrator_hermetic.py tests/unit/integration/test_skill_integrator_phase3w4.py -q and expect 280 passing tests.
• Run the lint chain above and expect ruff, format, pylint R0801, and auth-signal checks to pass.

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

[Copilot]

Pull request overview

Refactors SkillIntegrator to centralize cowork (dynamic-root) vs static target skill-path resolution, removing repeated inline branching in hot deployment paths while keeping static deploy_root behavior intact.

Changes:

• Introduces _target_skills_root() and _target_skill_dir() helpers in SkillIntegrator to DRY the three identified cowork path branches.
• Updates the three call sites to use the new helpers (standalone sub-skill promotion, native skill deployment, native sub-skill promotion).
• Updates unit-test target doubles to include a deploy_path() mock matching the production accessor shape.

Show a summary per file

File	Description
src/apm_cli/integration/skill_integrator.py	Adds internal path-resolution helpers and uses them at the three cowork dynamic-root call sites to remove repeated branching.
tests/unit/integration/test_skill_integrator_hermetic.py	Updates the _make_target() test double to provide deploy_path() used by the refactor.
tests/unit/integration/test_skill_integrator_phase3w4.py	Mirrors the same _make_target() update for the phase3w4 coverage fixture.

Copilot's findings

• Files reviewed: 3/3 changed files
• Comments generated: 0

[danielmeppiel]

APM Review Panel: ship_now

DRY refactor collapses three cowork dynamic-root skill-path branches behind TargetProfile.deploy_path(), reducing maintenance surface with zero behavior change.

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

All nine panelists converge on the same signal: this is a clean, behavior-preserving internal refactor with no externally visible surface delta. The python-architect and supply-chain-security-expert both flagged the same nit - clarifying why effective_root is retained for the static-target containment guard - and the shepherd already folded it as an inline comment. No specialist disagreed with the approach or raised structural concerns.

Strategically, this is textbook maintenance hygiene. The collapsed branches lower the cognitive load for anyone touching skill deployment paths next. The containment invariant remains intact and is now explicitly documented in source, which is the right outcome for a security-relevant code path. Nothing here shifts positioning, naming, or scope.

Aligned with: Secure by default: path-containment guard preserved and now explicitly commented; Pragmatic as npm: internal DRY cleanup reduces contributor friction without changing user-facing behavior.

Panel summary

Persona	B	R	N	Takeaway
Python Architect	0	0	1	Clean DRY refactor; helper extraction preserves dynamic-root and static-root path semantics.
CLI Logging Expert	0	0	0	No CLI output, diagnostic wording, logger, or status-symbol surface changed.
DevX UX Expert	0	0	0	Pure internal refactor; no command surface, error wording, or manifest behavior changes.
Supply Chain Security Expert	0	0	1	Path-containment and symlink safeguards are preserved; folded comment clarifies the static containment invariant.
OSS Growth Hacker	0	0	0	Internal refactor only; no adoption or release-note surface to amplify.
Test Coverage Expert	0	0	0	Relevant cowork and skill-integrator unit coverage passed; no missing regression trap for this pure refactor.
Performance Expert	0	0	0	No material performance delta; helper calls are negligible beside filesystem copy and traversal I/O.

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

Architecture

classDiagram
    direction LR
    class BaseIntegrator {
        +validate_deploy_path()
        +partition_managed_files()
    }
    class SkillIntegrator {
        +_target_skills_root(target, project_root) Path
        +_target_skill_dir(target, project_root, skill_name) Path
        +_integrate_native_skill()
        +_promote_sub_skills_standalone()
    }
    class TargetProfile {
        +resolved_deploy_root Path
        +root_dir str
        +deploy_path(project_root, parts) Path
    }
    BaseIntegrator <|-- SkillIntegrator
    SkillIntegrator ..> TargetProfile : delegates dynamic root lookup

Loading

flowchart TD
    A[Skill deployment call sites] --> B[_target_skills_root]
    B --> C{dynamic root?}
    C -- yes --> D[TargetProfile.deploy_path(project_root)]
    C -- no --> E[project_root / effective_root / skills]
    B --> F[_target_skill_dir adds skill name]
    F --> G[copy native skill]
    B --> H[promote sub-skills]

Loading

Recommendation

CI is green, the only nit was folded in-PR, and all active panelists returned zero required findings. This is ready for maintainer review - no follow-up items to track.

Folded in this run

• (panel) Clarified why effective_root remains after helper extraction: it feeds the static-target ensure_path_within containment guard - resolved in ff0600b22.

Copilot signals reviewed

• Copilot review 4575506222 generated 0 inline comments; no LEGIT items to fold.

Lint contract

uv run --extra dev ruff check src/ tests/, uv run --extra dev ruff format --check src/ tests/, uv run --extra dev python -m pylint --disable=all --enable=R0801 --min-similarity-lines=10 --fail-on=R0801 src/apm_cli/, and bash scripts/lint-auth-signals.sh all passed locally before push.

CI

All PR checks passed on head 38a3a578e9fb1d03d751c9e7f7266a7d086fb20b: Lint, Build & Test Shard 1, Build & Test Shard 2, Coverage Combine, APM Self-Check, PR Binary Smoke, CodeQL, NOTICE Drift Check, Spec conformance gate, gate, and license/cla.

Mergeability status

Captured after the last push of this run.

PR	head SHA	CEO stance	iters	folds	defers	Copilot rounds	CI	mergeable	mergeStateStatus	notes
#1914	38a3a578e	ship_now	1	1	0	2	green	MERGEABLE	BLOCKED	Awaiting human review/merge decision.

Convergence

1 outer iteration; 2 Copilot rounds. Final CEO stance: ship_now.

Ready for maintainer review.

---

Full per-persona findings

Python Architect

• [nit] Clarify retained static effective_root data flow at src/apm_cli/integration/skill_integrator.py:989
  The helper now computes target_skill_dir, but effective_root remains necessary for ensure_path_within on static targets; a short comment helps future readers preserve the containment invariant.
  Suggested: Add a short comment above effective_root explaining it feeds the static containment guard.

CLI Logging Expert

No findings.

DevX UX Expert

No findings.

Supply Chain Security Expert

• [nit] Document the retained containment root computation at src/apm_cli/integration/skill_integrator.py:989
  The security guard still relies on effective_root for static targets after target_skill_dir moved behind a helper; the folded comment makes that invariant explicit.

OSS Growth Hacker

No findings.

Auth Expert -- inactive

PR touches skill integrator path helpers and unit-test doubles only; no auth, token, credential, or remote-host surface is affected.

Doc Writer -- inactive

Touched files are skill integrator internals and unit tests; the refactor causes no documentation drift for CLI, manifest, lockfile, or deployment behavior.

Test Coverage Expert

No findings.

Performance Expert

No findings.

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