Refactor 922 cowork cleanups

[abhinavgautam01]

Description

Cleans up the remaining accepted cowork internal refactor items from #922. This routes skill deployment path construction through TargetProfile.deploy_path(...), preserves primitive deploy-root overrides such as .agents/skills and makes config unset helpers use the shared update_config() read-modify-write path.

The sync_remove_files cowork-root caching item from #922 was already present on current main, so this PR does not touch base_integrator.py.

Fixes #922

Type of change

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

Testing

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

Validation run:

uv run --python 3.12 ruff check src/apm_cli/config.py src/apm_cli/integration/targets.py src/apm_cli/integration/skill_integrator.py tests/unit/
test_config.py tests/unit/integration/test_copilot_cowork_target.py tests/unit/integration/test_skill_integrator_hermetic.py tests/unit/integration/
test_skill_integrator_phase3w4.py
uv run --python 3.12 pytest tests/unit -q
git diff --check

Results:

• Ruff passed
• Full unit suite: 16607 passed, 2 skipped, 21 xfailed, 19 warnings, 40 subtests passed
• Diff check passed

Note: uv run --python 3.12 pytest -q exited 0 locally, but integration/live suites were environment-gated (26704 skipped, 173 deselected), so the meaningful broad local signal is the full unit suite above.

Spec conformance (OpenAPM v0.1)

If this PR changes behaviour that an OpenAPM v0.1 req-XXX covers,
confirm the three-step ritual (see CONTRIBUTING.md "Adding or
changing a normative requirement"):

• Spec edit: docs/src/content/docs/specs/openapm-v0.1.md updated
  (new/changed anchor + prose + Appendix C
  row).

• Manifest edit: docs/src/content/docs/specs/manifests/openapm-v0.1.requirements.yml
  updated.

• Test edit: a @pytest.mark.req("req-XXX") test under
  tests/spec_conformance/ added or extended.

• CONFORMANCE.{md,json} regenerated via
  uv run --extra dev python -m tests.spec_conformance.gen_statement
  and committed.

• N/A -- this PR does not change OpenAPM-observable behaviour.

[Copilot]

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

This PR centralizes config key removal through the update_config() write path and extends target deployment path resolution to support primitive-specific deploy_root overrides (used by skills), updating skill integration logic and tests accordingly.

Changes:

• Add remove_keys support to update_config() and route _unset_config_key() through it.
• Extend TargetProfile.deploy_path() with an optional primitive parameter to honor primitive-level deploy_root/subdir.
• Refactor skill integrator path building to use target.deploy_path(...) and expand tests for these behaviors.

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
tests/unit/test_config.py	Adds unit tests ensuring unset helpers call the shared update_config() path.
tests/unit/integration/test_skill_integrator_phase3w4.py	Updates test target construction to provide a deploy_path() matching new primitive-aware behavior.
tests/unit/integration/test_skill_integrator_hermetic.py	Same as above for hermetic integration coverage.
tests/unit/integration/test_copilot_cowork_target.py	Adds coverage for primitive deploy-root override behavior on the copilot target.
src/apm_cli/integration/targets.py	Implements deploy_path(..., primitive=...) and removes prior Path typing workarounds.
src/apm_cli/integration/skill_integrator.py	Replaces bespoke “skills root” resolution with TargetProfile.deploy_path().
src/apm_cli/config.py	Adds remove_keys to update_config() and routes unsets through it.

[github-actions]

APM Review Panel: ship_with_followups

Bounded internal refactor with verified path containment and correct API shape; ship with followups to restore the absent-key write guard, delegate the test helper, and confirm integration coverage.

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

PR #1667 is a clean, purposeful internal refactor confined to three bounded changes: update_config gains a remove_keys consolidation path, deploy_path gains primitive-aware routing, and a shared test helper is extracted. The supply-chain reviewer algebraically verified path containment for all KNOWN_TARGETS across both deploy_root=None and deploy_root='.agents' configurations -- the primitive kwarg adds zero new path-traversal surface. The python-architect confirms that the keyword-only primitive placement is correct and backward-compatible. No user-facing CLI surface, flag, or documented API is altered; auth-expert, doc-writer, and performance-expert all self-excluded with clean negative findings.

Three panelists converge on the same behavioral regression in the config consolidation: the old _unset_config_key guarded open()+json.dump() behind if key in config:, making absent-key calls true I/O-free no-ops. The new path through update_config({}, remove_keys=(key,)) always reaches json.dump regardless of key presence -- spurious mtime bumps, unintended indent=2 reformatting on absent-key calls, and a previously silent no-op that now raises OSError when the config file is read-only and the key is absent. The public wrappers unset_temp_dir, unset_allow_protocol_fallback, and unset_prefer_ssh all carry docstrings saying 'No-op if the key is not present', which is now factually wrong at the I/O level. The same three panelists -- and supply-chain -- also converge on the test-helper sync hazard: tests/unit/_skill_integrator_target_helpers.py:12-21 reproduces TargetProfile.deploy_path's 8-line branch tree verbatim, creating a silent drift risk for any future path hardening; the python-architect's lambda delegation fix is clean and makes the mock exercise the real method with zero structural change to existing test setup.

The highest-uncertainty signal is the test-coverage reviewer's finding that tests/integration/test_skill_integrator_hermetic.py and test_skill_integrator_phase3w4.py were not updated alongside the unit-test counterparts. The evidence carries outcome: unknown because pytest was unavailable at review time; per protocol this is weighted as opinion only. The reasoning chain is structurally sound: the refactor moves inline path computation into target.deploy_path() calls at skill_integrator.py:914-915; bare MagicMock targets constructed without attach_skill_deploy_path will return a truthy MagicMock from deploy_path(), trigger the is_symlink() guard, and fail with PathTraversalError. All three open items are small, localized fixes requiring no design discussion.

Dissent. test-coverage-expert finding on tests/integration/test_skill_integrator_phase3w4.py:568 carries outcome: unknown because pytest was not available; per arbitration protocol this is weighted as opinion only. No substantive disagreement between any two active panelists on any finding; multi-panelist convergence on both the unconditional-write regression and the test-helper drift is consistent across all active reviewers.

Aligned with: portable-by-manifest (preserved -- primitive kwarg routes to harness-specific subdirectories without hardcoding), secure-by-default (fully preserved -- path containment algebraically verified for all KNOWN_TARGETS), governed-by-policy (weakened by the unconditional-write regression in _unset_config_key; guard restoration repairs it), multi-harness-multi-host (improved -- deploy_path is now the canonical harness-aware call site for all integrators), oss-community-driven (marginal positive -- noqa removal and shared test helper lower contributor friction), pragmatic-as-npm (preserved -- no consumer-facing migration required).

Growth signal. The oss-growth-hacker surfaces two quiet contributor-funnel positives: all noqa: F821 suppressions are now gone from src/apm_cli/, signaling type-annotation hygiene to contributors scanning the integration layer; and the shared fixture module lowers the first-PR barrier for anyone adding integration-target tests. Neither warrants a CHANGELOG entry but both reduce friction on the contributor path that feeds long-term maintainability.

Panel summary

Persona	B	R	N	Takeaway
Python Architect	0	2	1	Clean, bounded refactor -- two recommended findings, one nit. Main concerns: test helper duplicates deploy_path verbatim (sync hazard); update_config now unconditionally writes even on absent-key unset (behavioral drift).
CLI Logging Expert	0	0	1	No logging or diagnostic regressions; one nit on _unset_config_key docstring accuracy now that it always writes the config file regardless of key presence.
DevX UX Expert	0	1	2	No user-visible CLI surface touched. One behavioral contract breaks quietly: _unset_config_key documented No-op now incurs unconditional disk write. Two nits: test helper duplicates production logic, deploy_path/primitive silent-ignore needs a doc note.
Supply Chain Security	0	1	2	No security regressions. Path containment fully preserved and algebraically verified. Two post-merge nits: unconditional config write (no credentials at risk) and test helper drift risk for future path hardening.
OSS Growth Hacker	0	0	0	Pure internal refactor; no conversion surface touched and no growth action required.
Test Coverage Expert	0	1	1	Two gaps: mock-only routing test for remove_keys (nit; indirect coverage exists); tests/integration/ hermetic+phase3w4 not updated for deploy_path refactor (recommended).

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

Top 5 follow-ups

1. [Python Architect (converged: cli-logging-expert, devx-ux-expert)] Restore absent-key guard in update_config: add if not updates and all(k not in config for k in remove_keys): return before the json.dump path, and update docstrings for the three public wrappers. -- Three panelists converge on this behavioral regression. An absent key that was a silent no-op before now raises OSError; mtime stamps and indent-2 reformatting change on every absent-key unset call.
2. [Test Coverage Expert] Add from tests.unit._skill_integrator_target_helpers import attach_skill_deploy_path and attach_skill_deploy_path(fake_target) to both tests/integration/test_skill_integrator_hermetic.py and tests/integration/test_skill_integrator_phase3w4.py. -- The refactor moved inline path computation into target.deploy_path() calls; integration tests constructing bare MagicMock targets without the helper will call deploy_path() on a raw mock, trigger the is_symlink() guard, and fail with PathTraversalError.
3. [Python Architect (converged: devx-ux-expert, supply-chain-security-expert)] Replace the verbatim branch-tree copy in tests/unit/_skill_integrator_target_helpers.py:12-21 with lambda delegation: target.deploy_path = lambda *a, **kw: TargetProfile.deploy_path(target, *a, **kw). -- Three panelists flag the sync hazard. Future path hardening in TargetProfile.deploy_path will silently diverge from the test helper, making unit tests validate stale behavior while production ships new logic.
4. [Python Architect (converged: devx-ux-expert)] Add a one-line docstring note to deploy_path stating that the primitive kwarg is silently ignored when resolved_deploy_root is set. -- Now that deploy_path is the canonical call site for all integrators, authors adding new integration targets will have no indication that their primitive argument is a no-op on cowork targets.
5. [Supply Chain Security Expert] Add an explanatory comment at skill_integrator.py:929 documenting why the cowork target bypasses ensure_path_within. -- The bypass is pre-existing and correct, but future security auditors will see an ensure_path_within skip with no rationale and flag it; a single inline comment collapses that future review cost to zero.

Architecture

classDiagram
    direction LR
    class PrimitiveMapping {
        <<ValueObject>>
        +subdir str
        +extension str
        +format_id str
        +deploy_root str
    }
    class TargetProfile {
        <<ValueObject>>
        +name str
        +root_dir str
        +primitives dict
        +resolved_deploy_root Path
        +deploy_path(project_root, parts, primitive) Path
        +supports(primitive) bool
        +for_scope(user_scope) TargetProfile
    }
    class BaseIntegrator {
        <<Abstract>>
    }
    class SkillIntegrator {
        +_integrate_native_skill()
        +_integrate_skill_bundle()
    }
    class ConfigModule {
        <<Module>>
        +update_config(updates, remove_keys)
        +_unset_config_key(key)
        +unset_temp_dir()
        +unset_copilot_cowork_skills_dir()
    }
    class SkillIntegratorTargetHelpers {
        <<TestDouble>>
        +attach_skill_deploy_path(target)
    }
    note for TargetProfile "primitive kwarg added; silently ignored when resolved_deploy_root is set"
    note for SkillIntegratorTargetHelpers "DRY risk: mirrors TargetProfile.deploy_path branch tree verbatim"
    note for ConfigModule "Single write path; always writes, no absent-key guard"
    BaseIntegrator <|-- SkillIntegrator
    TargetProfile *-- PrimitiveMapping : primitives
    SkillIntegrator ..> TargetProfile : deploy_path(root, primitive)
    SkillIntegratorTargetHelpers ..> TargetProfile : mirrors logic verbatim
    classDef touched fill:#fff3b0,stroke:#d47600
    class TargetProfile:::touched
    class SkillIntegrator:::touched
    class ConfigModule:::touched
    class SkillIntegratorTargetHelpers:::touched

Loading

flowchart TD
    subgraph deploy["deploy_path - targets.py:241"]
        dp1["SkillIntegrator._integrate_native_skill<br/>skill_integrator.py:914-915"]
        dp2["mapping = primitives.get(primitive)<br/>None when primitive is None"]
        dp3{"resolved_deploy_root not None?"}
        dp4["return resolved_deploy_root.joinpath(parts)<br/>primitive silently ignored here"]
        dp5["deploy_root = mapping.deploy_root or None"]
        dp6["base = project_root / (deploy_root or root_dir)"]
        dp7{"mapping and mapping.subdir?"}
        dp8["base = base / mapping.subdir"]
        dp9["return base.joinpath(parts)"]
        dp1 --> dp2 --> dp3
        dp3 -- "YES cowork" --> dp4
        dp3 -- "NO static target" --> dp5 --> dp6 --> dp7
        dp7 -- "YES" --> dp8 --> dp9
        dp7 -- "NO" --> dp9
    end
    subgraph unset["config unset - config.py:151-160"]
        us1["unset_temp_dir or unset_copilot_cowork_skills_dir"]
        us2["_unset_config_key(key)"]
        us3["update_config remove_keys=(key,)"]
        us4["_invalidate_config_cache()"]
        us5["[I/O] get_config reads CONFIG_FILE"]
        us6["config.pop(key, None) - no-op if key absent"]
        us7["config.update - no-op for empty dict"]
        us8["[FS] json.dump to CONFIG_FILE<br/>ALWAYS written even if key was absent"]
        us9["_invalidate_config_cache()"]
        us1 --> us2 --> us3 --> us4 --> us5 --> us6 --> us7 --> us8 --> us9
    end

Loading

Recommendation

Merge is defensible: path containment is algebraically verified, API shape is correct, and no user-facing surface changes. The two convergent recommended findings (absent-key write guard, test-helper delegation) and the unconfirmed integration-test gap are all addressed by small, localized changes requiring no design discussion. Ship and open a single companion issue tracking all five followups, with the absent-key guard restoration and integration-test verification as first-to-close.

---

Full per-persona findings

Python Architect

• [recommended] Test helper duplicates TargetProfile.deploy_path branch tree verbatim -- sync hazard at tests/unit/_skill_integrator_target_helpers.py
  tests/unit/_skill_integrator_target_helpers.py:12-21 reproduces the exact 8-line branch tree of TargetProfile.deploy_path (targets.py:263-272) line-for-line. If the production method gains a new case -- e.g. primitive-aware routing when resolved_deploy_root is set -- the helper silently diverges and unit tests validate stale behavior while production ships the new path. Minimal fix: target.deploy_path = lambda *a, **kw: TargetProfile.deploy_path(target, *a, **kw). The mock already carries .primitives, .resolved_deploy_root, and .root_dir so the real method works unmodified.
  Suggested: Use lambda delegation to real TargetProfile.deploy_path method.
• [recommended] update_config({}, remove_keys=(key,)) always writes the config file -- absent-key guard was dropped at src/apm_cli/config.py
  Old _unset_config_key (pre-PR) guarded the disk write behind if key in config:, making absent-key unset calls true no-ops with zero disk I/O. The new path always reaches json.dump regardless of whether remove_keys mutated anything: _invalidate_cache + get_config (read) + pop (no-op when key absent) + update({}) (no-op) + json.dump (always). Effects: spurious FS write, mtime bump, and indent=2 reformatting on every absent-key unset call. Environments with file-system watchers or hook-triggered config reloads see unexpected mtime changes. The new TestUnsetConfigHelpers test patches update_config entirely so it verifies routing only -- it does not catch this regression. Minimal fix: if not updates and all(k not in config for k in remove_keys): return.
  Suggested: Add short-circuit guard before json.dump, or document unconditional-write behavior explicitly in update_config docstring.
• [nit] deploy_path docstring omits that primitive is silently ignored when resolved_deploy_root is set at src/apm_cli/integration/targets.py
  targets.py:264-267: when resolved_deploy_root is set the method returns immediately after joinpath(*parts), never consulting the primitive mapping. One sentence covers it: 'When resolved_deploy_root is set, primitive is not consulted -- the dynamic root already encodes the complete deploy directory.'

CLI Logging Expert

• [nit] _unset_config_key and its callers document 'No-op when key not present' but the new implementation always writes the config file at src/apm_cli/config.py:151
  Public wrappers (unset_temp_dir, unset_allow_protocol_fallback, unset_prefer_ssh) all carry docstrings saying 'No-op if the key is not present', which is now factually wrong at the I/O level. No log message is emitted in either path so there is zero user-visible output change, but a read-only config file combined with an absent key would have silently succeeded before and now raises an OSError.
  Suggested: Either update the docstrings to say 'Logical no-op when key is not present; config file is still rewritten on every call', or add a short-circuit guard before delegating: if key not in get_config(): return to restore the conditional-write semantics.

DevX UX Expert

• [recommended] _unset_config_key docstring says 'No-op when key is not present' but new implementation unconditionally writes to disk at src/apm_cli/config.py:151
  Old code guarded the open()+json.dump() inside if key in config:, making the absent-key path truly I/O-free. New path delegates to update_config({}, remove_keys=(key,)), which always rewrites the file after pop(key, None). In environments with filesystem watches, audited I/O, or CI setups where the config file is read-only when no keys need unsetting, callers relying on the docstring guarantee now see unexpected side effects or errors.
• [nit] attach_skill_deploy_path re-implements TargetProfile.deploy_path verbatim -- drift risk if production method gains new branching condition at tests/unit/_skill_integrator_target_helpers.py:12
  TestDeployPath in test_copilot_cowork_target.py already shows the right pattern: use replace() on a real KNOWN_TARGETS profile for the path-resolution boundary, reserve MagicMock for integrator surrounding logic.
• [nit] deploy_path silently ignores primitive when resolved_deploy_root is set -- warrants a docstring note at src/apm_cli/integration/targets.py:264
  With deploy_path now the canonical calling convention across all integrator callers, the silent ignore is more exposed. A single docstring note prevents future callers from assuming primitive-routed paths for cowork targets.

Supply Chain Security Expert

• [nit] update_config({}, remove_keys=(key,)) unconditionally writes config.json even when the key was absent at src/apm_cli/config.py:160
  No credentials live in config.json so there is no confidentiality risk, but restoring the short-circuit (e.g. if key in config or updates: write) eliminates the spurious disk write.
• [recommended] Test helper mirrors TargetProfile.deploy_path logic; future path hardening will not be reflected in the mock at tests/unit/_skill_integrator_target_helpers.py
  Any future change to the real deploy_path (extra containment check, symlink guard, normalisation step) will not be reflected here, allowing tests to pass against an older, potentially weaker path shape. Fix: either delegate to the real method or add a convergence property test.
• [nit] Pre-existing ensure_path_within bypass for cowork targets needs an explanatory comment for future auditors at src/apm_cli/integration/skill_integrator.py:929
  validate_path_segments and symlink rejection still run so there is no regression. The omission is safe (deploy_path with resolved_deploy_root always returns resolved_deploy_root.joinpath(skill_name), so the containment check would trivially pass). A one-line comment to that effect would aid future auditors.

OSS Growth Hacker

No findings.

Auth Expert -- inactive

None of the canonical auth files are changed (auth.py, token_manager.py, azure_cli.py, github_downloader.py, marketplace/client.py, github_host.py, validation.py, pipeline.py, registry_proxy.py). Fallback self-check also negative: the diff is confined to config read-modify-write consolidation and deploy-path primitive-aware routing -- no token handling, credential resolution, host classification, or AuthResolver semantics are touched.

Doc Writer -- inactive

All changes are internal implementation details with no user-facing surface. update_config(remove_keys=()) and deploy_path(primitive=...) are private Python helpers not exposed in any CLI command, flag, config key, or documented API. Doc drift check: negative.

Test Coverage Expert

• [recommended] tests/integration/ hermetic and phase3w4 not updated for deploy_path refactor; TestIntegrateSkillNameNormalization tests likely broken at tests/integration/test_skill_integrator_phase3w4.py:568
  The refactor replaces inline path computation with target.deploy_path() calls at skill_integrator.py:914-915. Pre-existing tests in tests/integration/ construct fake_target=MagicMock() without attach_skill_deploy_path. After the refactor, target.deploy_path(project_root, skill_name, primitive='skills') returns a truthy MagicMock; target_skill_dir.is_symlink() returns another truthy MagicMock; the guard at skill_integrator.py:925 fires and the test fails with PathTraversalError. The PR correctly updated tests/unit/integration/ but did not update tests/integration/ counterparts. Outcome: unknown (pytest unavailable).
  Suggested: Add from tests.unit._skill_integrator_target_helpers import attach_skill_deploy_path and attach_skill_deploy_path(fake_target) in both tests/integration/ test files, mirroring the change already applied to tests/unit/integration/ in this PR.
  Proof (unknown): tests/integration/test_skill_integrator_phase3w4.py::TestIntegrateSkillNameNormalization::test_invalid_name_normalized_with_diagnostics -- proves: skill install correctly deploys a skill with a normalized name to the configured target directory [multi-harness-support, devx]
• [nit] update_config(remove_keys=...) new API exercised by routing-only mock test; no direct round-trip with isolated_config at tests/unit/test_config.py
  Indirect coverage exists through TestAuditOnInstallConfig.test_unset_falls_back_to_default which exercises the full write path with a real temp file. Adequate for a routing refactor, but a direct test would be preferred for the new public parameter.
  Proof (passed): tests/unit/test_config.py::TestAuditOnInstallConfig::test_unset_falls_back_to_default -- config_mod.set_audit_on_install('block'); config_mod.unset_audit_on_install(); assert config_mod.get_audit_on_install() == 'off' -- proves: unsetting a config key removes it from the on-disk file and subsequent reads return the default value [devx]

Performance Expert -- inactive

Fast-path and fallback both miss. deploy_path() refactor adds ~70-130ns method-call overhead per integration target during the integrate phase -- noise against disk-write latency. The unconditional config write on absent-key unset is cold-path only (confirmed absent from install/, deps/, cache/** call graphs). No action required from the performance perspective.

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

Generated by PR Review Panel for issue #1667 · sonnet46 16.1M · ◷

[abhinavgautam01]

Addressed the review follow-ups and the lint failure.

Changes:

• restored absent-key no-op behavior for update_config(remove_keys=...) so absent-key unsets avoid disk rewrites
• added direct tests for absent-key no-write and present-key removal
• changed the shared skill-integrator target helper to delegate to TargetProfile.deploy_path()
• updated both tests/integration skill-integrator copies to use the shared helper
• documented that primitive is ignored for resolved_deploy_root targets
• added the cowork containment-bypass rationale comment
• ran ruff format, fixing the targets.py format failure

Validation passed locally:

• ruff format --check src/ tests/
• ruff check on touched files
• focused pytest: 210 passed
• full unit suite: 16611 passed, 2 skipped, 21 xfailed
• git diff --check