fix(install): support CLAUDE_CONFIG_DIR outside HOME for global Claude install (closes #2129)#2135
fix(install): support CLAUDE_CONFIG_DIR outside HOME for global Claude install (closes #2129)#2135danielmeppiel wants to merge 4 commits into
Conversation
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR fixes global (--global) Claude installs when CLAUDE_CONFIG_DIR is an absolute path outside $HOME, ensuring deployed-file lockfile entries can be serialized and cleanup stays bounded to the Claude-managed config root.
Changes:
- Extend deployed-path lockfile translation to treat absolute static
root_dirtargets (e.g. Claude config outside$HOME) as representable dynamic roots with containment checks. - Allow
BaseIntegrator.validate_deploy_path()to validate absolute Claude-managed paths and boundcleanup_empty_parents()to stop at the resolved external config root. - Add regression tests covering absolute Claude config roots, cleanup boundaries, and symlink-escape rejection.
Show a summary per file
| File | Description |
|---|---|
| tests/unit/integration/test_base_integrator.py | Adds unit coverage for validating absolute Claude config-root paths and bounded cleanup for external roots. |
| tests/unit/install/test_services.py | Adds a regression test ensuring lockfile path translation rejects symlink-based escapes for absolute static roots. |
| tests/integration/test_compile_global.py | Adds integration coverage for apm install -g with CLAUDE_CONFIG_DIR outside $HOME. |
| src/apm_cli/integration/targets.py | Updates comments to reflect that absolute root_dir is now supported by lockfile translation. |
| src/apm_cli/integration/base_integrator.py | Implements absolute-path validation + bounded cleanup behavior for external Claude config roots. |
| src/apm_cli/install/deployed_paths.py | Teaches deployed-path encoding to support absolute static roots with containment enforcement. |
Review details
- Files reviewed: 6/6 changed files
- Comments generated: 1
- Review effort level: Low
| resolved_root = deploy_root.resolve() | ||
| for mapping in target_profile.primitives.values(): | ||
| if not mapping.subdir: | ||
| continue | ||
| primitive_root = (resolved_root / mapping.subdir).resolve() | ||
| if resolved != primitive_root and resolved.is_relative_to(primitive_root): | ||
| return resolved_root |
APM Review Panel:
|
| Persona | B | R | N | Takeaway |
|---|---|---|---|---|
| Python Architect | 0 | 1 | 2 | Sound boundary model; centralize effective managed-root derivation. |
| CLI Logging Expert | 0 | 0 | 2 | No new default output needed; optional diagnostic polish only. |
| DevX UX Expert | 0 | 1 | 2 | Install now succeeds; avoid a hardcoded future-target list. |
| Supply Chain Security Expert | 0 | 2 | 2 | Containment remains sound; align validation primitives and diagnostics. |
| OSS Growth Hacker | 0 | 1 | 1 | Add a concise changelog entry; no docs page needed. |
| Doc Writer | 0 | 1 | 0 | Existing docs stay accurate; changelog entry is required. |
| Test Coverage Expert | 0 | 1 | 0 | Integration coverage passes; add exact serialization-shape coverage. |
B = highest-severity findings, R = recommended, N = nits.
Counts are signal strength, not gates. The maintainer ships.
Top 4 follow-ups
- [Python Architect + DevX UX + Supply Chain] Centralize the effective managed-root heuristic and remove the hardcoded Claude/Hermes fallback - avoids drift when another target gains an absolute managed root.
- [Test Coverage] Assert the successful serialized lockfile path string for an absolute managed root - catches format regressions directly.
- [OSS Growth Hacker + Doc Writer] Add an Unreleased changelog entry - preserves the public record of this reliability fix.
- [Supply Chain] Keep unmanaged cleanup observable at debug level if a suitable logger exists - useful diagnosis without normal-output noise.
Architecture
classDiagram
class TargetProfile {
+root_dir str
+resolved_deploy_root Path
+primitives dict
}
class BaseIntegrator {
+validate_deploy_path()
+cleanup_empty_parents()
}
class DeployedPathEncoder {
+encode()
}
class PathContainmentGuard {
+ensure_path_within()
}
BaseIntegrator --> TargetProfile : resolves managed root
DeployedPathEncoder --> TargetProfile : reads target root
DeployedPathEncoder --> PathContainmentGuard : checks containment
flowchart TD
A[Global Claude install] --> B[Resolve CLAUDE_CONFIG_DIR]
B --> C[Write registered primitive]
C --> D[Check containment]
D --> E[Serialize project-relative deployed path]
E --> F[Bound cleanup at managed root]
Recommendation
Fold the in-scope follow-ups, add an honest apm-spec-waiver trailer stating that this changes Claude harness install reliability but not OpenAPM artifact/wire behavior, then re-run CI. The current implementation is directionally correct and the focused tests passed.
Full per-persona findings
Python Architect
- [recommended] Centralize duplicated effective managed-root derivation in
TargetProfileso deployed-path encoding and integration validation cannot drift. - [nit] Remove the hardcoded Claude/Hermes fallback when all registered targets can be filtered safely.
CLI Logging Expert
- [nit] Consider a debug trace when cleanup intentionally no-ops for an unmanaged absolute path.
- [nit] A positive install-success output assertion would strengthen CLI UX coverage.
DevX UX Expert
- [recommended] Derive eligible targets from the registry rather than a target-name constant.
- [nit] Clarify that no matching managed root means the path is unmanaged.
- [nit] Assert the lockfile serialization result.
Supply Chain Security Expert
- [recommended] Keep absolute-path validation aligned with the sanctioned containment helper.
- [recommended] Make an unmanaged cleanup no-op diagnosable when practical.
- [nit] Avoid a future-target omission through hardcoded names.
OSS Growth Hacker
- [recommended] Add a concise Unreleased changelog entry for enterprise managed Claude roots.
- [nit] No additional Starlight page is needed.
Auth Expert - inactive
No authentication, credential, host, or token surface is touched.
Doc Writer
- [recommended] Add the required Unreleased changelog entry; current Starlight claims remain accurate.
Test Coverage Expert
- [recommended] Add a unit assertion for the successful absolute-root deployed-path serialization shape. Existing integration, containment, validation, and cleanup tests passed.
Performance Expert - inactive
No dependency resolution, transport, cache, materialization, or parallelism hot path is touched.
This panel is advisory. It does not gate merge. Re-apply the panel-review label after addressing feedback to re-run.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Centralize managed deployment-root derivation, keep absolute path validation on the shared containment guard, add direct serialization coverage, and record the fix in the changelog. Addresses panel follow-ups from PR #2135. apm-spec-waiver: harness-install reliability fix for Claude CLAUDE_CONFIG_DIR outside HOME; no OpenAPM artifact or wire behavior change Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Qualify path containment guidance for global installs using configured target roots. Addresses the final supply-chain panel finding for PR #2135. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
APM Review Panel:
|
| Persona | B | R | N | Takeaway |
|---|---|---|---|---|
| Python Architect | 0 | 0 | 1 | Centralized root derivation is the simplest correct shape. |
| CLI Logging Expert | 0 | 0 | 0 | No CLI output surface changed; no in-scope issue remains. |
| DevX UX Expert | 0 | 0 | 0 | The configured Claude root now works without a new flag or workaround. |
| Supply Chain Security Expert | 0 | 0 | 0 | Containment and cleanup boundaries are sound; the doc nit was folded. |
| OSS Growth Hacker | 0 | 0 | 0 | The Unreleased changelog entry accurately records the fix. |
| Doc Writer | 0 | 0 | 0 | Documentation is accurate after the security-guide clarification. |
| Test Coverage Expert | 0 | 0 | 0 | Required behavior tiers are covered and passing. |
B = highest-severity findings, R = recommended, N = nits.
Counts are signal strength, not gates. The maintainer ships.
Architecture
classDiagram
class TargetProfile {
+root_dir str
+resolved_deploy_root Path
+managed_deploy_root Path
+primitives dict
}
class BaseIntegrator {
+validate_deploy_path()
+cleanup_empty_parents()
}
class DeployedPathEncoder {
+encode()
}
class PathContainmentGuard {
+ensure_path_within()
}
BaseIntegrator --> TargetProfile : resolves managed root
DeployedPathEncoder --> TargetProfile : resolves managed root
BaseIntegrator --> PathContainmentGuard : validates primitive path
DeployedPathEncoder --> PathContainmentGuard : validates serialized path
flowchart TD
A[Global Claude install] --> B[Resolve CLAUDE_CONFIG_DIR]
B --> C[Write registered primitive]
C --> D[Check managed-root containment]
D --> E[Record lockfile-safe deployed path]
E --> F[Bound cleanup at managed root]
Recommendation
Ready for maintainer review. The implementation, regression coverage, documentation, lint contract, and all required CI checks are green on the final head.
Folded in this run
- (panel) Centralized resolved and absolute static deployment-root derivation in
TargetProfile.managed_deploy_root- resolved in5c7aa9385f3b8c2b34437cbce0c2c147aa42d499. - (panel) Removed the Claude/Hermes-only helper fallback by deriving eligible static user targets from
KNOWN_TARGETS- resolved in5c7aa9385f3b8c2b34437cbce0c2c147aa42d499. - (panel) Aligned absolute-path validation with
ensure_path_within- resolved in5c7aa9385f3b8c2b34437cbce0c2c147aa42d499. - (panel) Added successful absolute-root deployed-path serialization coverage - resolved in
5c7aa9385f3b8c2b34437cbce0c2c147aa42d499. - (panel) Added the Unreleased changelog entry - resolved in
5c7aa9385f3b8c2b34437cbce0c2c147aa42d499. - (panel) Clarified project-scope and global managed-root containment in the security guide - resolved in
a7af5d8bbad0fc2556a59ca1f67d4ab8818b8eb7. - (panel) Added the honest Mode B waiver trailer for harness-specific reliability with no OpenAPM artifact/wire delta - resolved in
5c7aa9385f3b8c2b34437cbce0c2c147aa42d499.
Copilot signals reviewed
Two fetch rounds found no inline Copilot findings. The existing overview review was informational only.
Deferred (out-of-scope follow-up)
- (panel) Add debug logging when cleanup skips an unmanaged absolute path - scope boundary:
BaseIntegratorhas no debug logger; implementing this requires cross-command logger plumbing unrelated to the Claude root reliability fix.
Regression-trap evidence (mutation-break gate)
tests/unit/install/test_services.py::TestDeployedPathEntry::test_absolute_static_root_returns_portable_relpath- deleted the absolute-static-root serialization branch; the test failed as expected; guard restored.
Lint contract
The full CI mirror was silent before each push: ruff check, ruff format check, pylint R0801, and scripts/lint-auth-signals.sh all exited 0.
CI
All checks passed on the final head, including Spec conformance gate, Lint, both Linux test shards, coverage combine, CodeQL, Merge Gate, docs build, smoke, self-check, NOTICE, and CLA. Final workflow evidence: https://github.com/microsoft/apm/actions/runs/29120285352 (1 CI recovery iteration).
Mergeability status
| PR | head SHA | CEO stance | iters | folds | defers | Copilot rounds | CI | mergeable | mergeStateStatus | notes |
|---|---|---|---|---|---|---|---|---|---|---|
| #2135 | a7af5d8 |
ship_now | 2 | 7 | 1 | 2 | green | MERGEABLE | BLOCKED | awaiting required maintainer review |
Convergence
2 outer iterations; 2 Copilot rounds. Final panel stance: ship_now.
Ready for maintainer review.
Full per-persona findings
Python Architect
- [nit] Informational pattern note only: the value-object property and shared helper correctly centralize root strategy.
CLI Logging Expert
No findings.
DevX UX Expert
No findings.
Supply Chain Security Expert
No findings after the security-guide clarification was folded.
OSS Growth Hacker
No findings.
Auth Expert - inactive
No authentication, credential, token, or remote-host surface changed.
Doc Writer
No findings.
Test Coverage Expert
No findings; focused and CI test evidence passed.
Performance Expert - inactive
No resolution, transport, cache, materialization, or parallelism surface changed.
This panel is advisory. It does not gate merge.
The merged deployed_path_entry owner reads target.managed_deploy_root (the canonical field from #2135). The TestDeployedPathEntry mocks only wired resolved_deploy_root, so managed_deploy_root resolved to an auto-mock and the to_lockfile_path assertion failed under the merge-queue integration job. Mirror the real TargetProfile property in make_target so the mock faithfully represents production. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 8e13654a-d2ea-4310-9261-3a38e47db5f0
|
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 #2129 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. |
Global Claude installs now treat an absolute
CLAUDE_CONFIG_DIRoutsideHOMEas a managed deployment root, serialize its deployed files without failing after the write, and keep cleanup and integrity checks bounded to registered Claude primitive paths. Regression coverage verifies successful installation, path validation, symlink containment, and cleanup boundaries.How to test
uv run --extra dev python -m pytest tests/unit/install/test_services.py tests/unit/integration/test_scope_integration.py tests/unit/integration/test_base_integrator.py tests/unit/integration/test_cleanup_helper.py tests/unit/test_security_file_scanner.py tests/integration/test_compile_global.py -quv 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.shCloses #2129
Co-authored-by: Copilot 223556219+Copilot@users.noreply.github.com