feat(install, compile): --root DIR to redirect writes while sources resolve from $PWD

[srid]

Closes #888.

Downstream consumer validating this PR end-to-end: juspay/kolu#716 — collapses a ~60-line rsync + mkdir + cp scratch-staging workaround in their CI pipeline to a four-line apm install --root + apm compile --root sequence.

Why

apm install and apm compile always write relative to $PWD, which forces any scratch-directory workflow to physically stage every input file (manifest, .apm/, lockfile, project tree) before cd-ing in. The juspay/kolu CI pipeline hit this in a sharp way (#684): running apm install on the live worktree was briefly destroying .claude/ while a Claude Code session had an open file handle there. The workaround exists purely because there was no --target-style flag.

This PR adds that flag: apm install --root DIR and apm compile --root DIR. Writes go under DIR; sources (apm.yml, .apm/, local-path packages, the project tree used for distributed compile placement scoring) continue reading from $PWD. Semantics deliberately mirror pip install --target and npm install --prefix.

With both flags in place the kolu workaround collapses to:

[[ ! -e apm.lock.yaml ]] || cp apm.lock.yaml "$scratch/apm.lock.yaml"
apm install --root "$scratch"
apm compile --root "$scratch" --target codex,opencode

How

The implementation picks a chdir + source-root pin model over refactoring the long tail of Path.cwd() / os.getcwd() call sites (notably the MCP adapters in apm_cli.adapters.client.*, the OpenCode/Cursor/VSCode adapters, and mcp_integrator.py). A new context manager apm_cli.install.root_redirect.install_root_redirect does two things:

1. os.chdir(root) so every site that hardcodes Path.cwd() auto-resolves to the deploy root.
2. set_source_root_override($PWD) (process-global) so helpers that need the user's source tree (get_source_root, get_manifest_path) still point at the original working directory.

Both are restored in a finally so the context doesn't leak across invocations — important for test runners, REPL sessions, and embedded callers. compile_root_redirect is a one-line alias for the same implementation.

get_manifest_path is decoupled from get_apm_dir: the manifest is a source, so it follows get_source_root, while the lockfile / apm_modules/ / deploy dirs follow get_deploy_root. That distinction is the design crux — the issue body explicitly called out "sources continue resolving from $PWD" as the intended semantics.

Install side

• InstallContext gains source_root (defaults to project_root for back-compat via __post_init__).
• install/pipeline.py computes source_root = get_source_root(scope) alongside project_root = get_deploy_root(scope).
• Four places that previously conflated read-from-$PWD with write-to-$PWD now split: _project_has_root_primitives (scans source root for .apm/), integrate_local_content's synthetic _local package (install_path = source_root), the dep resolver's apm.yml lookup, and local-path package resolution in phases/resolve.py + sources.py.

The resolver fix is its own commit because the tree-shaped symptom ("running apm install --root silently resolved zero direct dependencies") is worth a standalone reviewable change.

Compile side

AgentsCompiler and DistributedAgentsCompiler gain an optional source_dir parameter (defaults to base_dir for back-compat). base_dir keeps driving placement targets and write paths; source_dir drives primitive discovery, ContextOptimizer, and constitution lookup. DistributedAgentsCompiler._source_to_base translates placement-map keys from source-rooted to base-rooted so the AGENTS.md writes land at the deploy root even though the placement decisions were scored against the source tree. template_builder.build_conditional_sections picks up a matching source_dir parameter so <!-- Source: ... --> display paths render relative to $PWD — without this, scratch-compiled output diverges from in-place output by embedding absolute scratch paths.

What's included / what's not

Source resolution for apm.yml is tied to get_source_root, which currently means the original CWD. Adding a separate --manifest PATH flag for reading the manifest from an arbitrary location (called out as future work in the issue body) is deliberately not in this PR — the current design accommodates it cleanly by having get_manifest_path consult an additional override, but that's a future increment.

The compiler's source_dir is CLI-aware by necessity because distributed-compile placement scoring scans the project tree; a pre-resolved path from the caller would be fine except that ContextOptimizer is constructed inside DistributedAgentsCompiler.__init__ and already took a directory. Threading two paths through is the minimal change.

Verification

• tests/unit/install/ (63 tests) + tests/unit/compilation/ + tests/unit/commands/ (344 tests combined) all pass on this branch.
• The full tests/unit/ suite passes (5430 / 5430 excluding 4 environment-dependent failures unrelated to this change: test_is_tool_available("python") and three _real_system checks — none reference any code I touched).
• The install.py LOC budget in test_architecture_invariants.py bumps 1700 → 1725 with rationale in the same comment-based tradition as prior PRs (Enforce apm-policy.yml at apm install time, not only in apm audit --ci #827, feat(policy): enforce apm-policy.yml at install time #832, Azure DevOps authentication via Entra ID (AAD) bearer tokens #852, feat(auth): Azure DevOps authentication via Entra ID (AAD) bearer tokens #856); the pending --mcp extraction that's already tracked in that test will recover the budget.

End-to-end validated against juspay/kolu#716: apm audit --ci passes 7/7 and every AGENTS.md output (root + packages/ + packages/client/src/ + packages/tests/features/ + packages/server/src/) is byte-identical between an apm install --root "$scratch" && apm compile --root "$scratch" run and the in-place equivalent.

Draft status

Drafted so the design can be sanity-checked before it leaves draft. In particular, feedback welcome on:

• The chdir-vs-refactor tradeoff. The Path.cwd() long tail is real and this PR sidesteps it; the alternative is a sweep through the adapters to use scope helpers, which is a larger and probably-separate change. Is that acceptable?
• The source_dir leak into the compiler classes. Principled ("the compiler already knew about roots") or a smell ("CLI-level concept leaking into a pure library")? I picked principled-enough-for-now.
• Process-global override vs plumbed parameter. A global is ugly but the chdir is already global. Open to being talked out of it.

🤖 Generated with Claude Code

[srid]

Pre-merge structural review (Hickey + Lowy)

I ran two structural-review passes on this branch — one Hickey-style ("Simple Made Easy", looking for complecting and missed deduplication) and one Lowy-style (volatility-based decomposition, after Parnas '72). Background on what each pass checks: kolu.dev/blog/hickey-lowy.

Posting both before maintainer review since the two passes disagree on the central design choice and I'd rather hear your opinion than commit to one direction blindly.

Where they converge

Correctness bug under --root (Hickey, must-fix). AgentsCompiler relativises primitive/instruction file paths against self.base_dir at three sites — agents_compiler.py:695, agents_compiler.py:707, agents_compiler.py:872. When --root is active, those source files live under source_dir and base_dir is the deploy root; the two don't share a common parent, so portable_relpath either produces a confusing ../../… path or falls back to absolute. DistributedAgentsCompiler:594 already correctly uses self.source_dir — the asymmetry is the smoking gun. Will fix regardless of which design direction you prefer.

Missing cross-reference docstring on get_lockfile_dir (Hickey, must-fix). core/scope.py:154 is the boundary where a future maintainer will pick the wrong root. get_manifest_path has the right docstring; get_lockfile_dir doesn't. Easy fix.

Where they disagree

The headline: chdir + process-global override vs. just-the-override-and-refactor-cwd-callsites.

• Hickey considers the braid "deliberate, justified" (the docstring explains it; chdir handles the long tail of Path.cwd() callsites in MCP adapters; the override handles scope helpers — two mechanical parts of one coherent strategy). Optional finding only.

• Lowy considers it the must-fix problem: "two separate mechanisms for what should be one concept", non-hermetic, and creates a coupling debt where future MCP-adapter additions silently won't see _SOURCE_ROOT_OVERRIDE. Recommends eliminating chdir() and routing every Path.cwd() callsite through get_source_root() or an explicit parameter — a sweep through apm_cli.adapters.client.* and mcp_integrator.py.

This is the tradeoff I called out in the PR description's "draft status" section; the two reviewers come down on opposite sides. I lean Hickey's way (smaller diff, contained surface) but Lowy's argument that the long tail is the volatility worth encapsulating is real. Maintainer call: which?

Lowy's other architectural pushes

• InstallContext.source_root Optional default is backwards (should-fix). Resolve at the CLI boundary, make the field non-optional, drop the ctx.source_root or ctx.project_root fallback patterns in phases/resolve.py and pipeline.py. I think this is right and would do it.

• Compiler source_dir leaks CLI concept into domain (should-fix). The compiler should receive pre-resolved paths and not know about --root. The _source_to_base() helper is "a guard against the feature, not a design". Counterpoint: ContextOptimizer is constructed inside DistributedAgentsCompiler.__init__ and already takes a directory, so threading a second one is the minimal change. Genuinely uncertain — open to either direction.

Hickey's other style pushes

• Manual __enter__/__exit__ instead of with (should-fix). The handlers hold a _root_redirect reference and unwind in finally. with would be cleaner — install_root_redirect is already @contextmanager. Trivial to fix; the original avoidance was about not re-indenting the existing large try body, which is a weak reason.

• Convention enforced by naming alone, not types (should-fix). The "use source_root to read, project_root to write" rule is convention; a phase can read the wrong field and the bug is silent in the no---root case. Suggested fix: convention note at the top of install/pipeline.py. Cheaper than a full type-level fix.

Optional / defer

• compile_root_redirect = install_root_redirect alias should carry a comment explaining the intentional sharing and split conditions (Lowy).
• A future --manifest <path> flag would re-thread cleanly if source_root becomes non-Optional (Lowy) — argument for landing the should-fix above.

Asking

The MUST-FIX correctness bug + docstring will get fixed regardless. For the rest, especially the chdir-vs-refactor call, I'd rather have your direction before I rewrite. Happy to push either way; the kolu downstream consumer (juspay/kolu#716) is in draft pending this PR's resolution and isn't blocking.

🤖 Reviews generated with structural-analysis subagents via Claude Code (background: kolu.dev/blog/hickey-lowy).

[srid]

Pushed fix for the two MUST-FIX items (commit 19c20e1):

• AgentsCompiler relpath fix — validate_primitives + verbose-output helper now use self.source_dir for primitive/instruction paths, matching DistributedAgentsCompiler:594. Verified end-to-end: apm install --root "$scratch" && apm compile --root "$scratch" --target codex,opencode against the juspay/kolu worktree produces AGENTS.md + packages/AGENTS.md that are byte-identical to an in-place compile. 617 unit tests across tests/unit/{compilation,commands,install} all pass.

• get_lockfile_dir docstring — now parallels get_manifest_path's, spelling out the source-vs-deploy routing convention so future metadata helpers pick the right root deliberately.

The should-fix / chdir-vs-refactor items still await your call. Draft stays draft until then.

[srid]

Pushed 24c38bf: merged origin/main and folded in the Hickey/Lowy review findings.

Conflict resolution — followed upstream's PEP 604 / Ruff convention; threaded --root plumbing through the new _install_apm_packages / _post_install_summary decomposition; preserved source_dir end-to-end in the compiler so primitive paths in validation warnings, source-attribution comments, and verbose-trace lines render against the source root even under --root.

Addressed findings:

• Lowy / should-fix — InstallContext.source_root is now required (no Optional[Path] = None default). Resolved at the CLI boundary in run_install_pipeline. Dropped the ctx.source_root or ctx.project_root and ctx.source_root or ctx.apm_dir fallbacks in phases/resolve.py and install/sources.py — they masked bugs whenever source_root != project_root.
• Hickey / should-fix — replaced manual __enter__ / finally: __exit__ pairs with with install_root_redirect(...) / with compile_root_redirect(...) in both commands. Re-indents the handler bodies but unwinds correctly even on early return or uncaught exceptions, and drops the _root_redirect reference from the function frame.
• Hickey / should-fix — added a "Source-vs-deploy root convention" section to install/pipeline.py's module docstring spelling out which root each phase reads vs writes. Future field additions must pick a side deliberately.
• Lowy / optional — expanded the comment on compile_root_redirect = install_root_redirect to document why the alias exists and when it should split.

The chdir-vs-refactor design call is still open for maintainer direction — that's the bigger sweep through Path.cwd() callsites in apm_cli.adapters.client.* / mcp_integrator.py. Happy to do it if you'd prefer Lowy's direction; otherwise the current chdir + _SOURCE_ROOT_OVERRIDE model is documented and contained.

Verified end-to-end: apm install --root SCRATCH + apm compile --root SCRATCH --target codex,opencode produces byte-identical AGENTS.md files to an in-place compile (modulo the non-deterministic Build ID). All install + compilation unit tests pass (656 + arch-invariant suite).

The kolu downstream consumer (juspay/kolu#716) will be re-pointed at this commit next.

[Copilot]

Pull request overview

Adds a new --root DIR flag to apm install and apm compile to redirect deployment writes into DIR while continuing to resolve sources (e.g., apm.yml, .apm/, local-path deps, and compile placement scanning) from the original working directory ($PWD). This is implemented via a shared chdir + source-root pin mechanism and by threading a distinct “source vs deploy root” through the install and compilation pipelines.

Changes:

• Add --root DIR to apm install and apm compile, backed by a shared install_root_redirect context manager (compile reuses it via alias).
• Split source-vs-deploy path semantics in install (InstallContext.source_root, resolver and local-path handling) so --root doesn’t break dependency resolution or local content integration.
• Extend compiler internals to accept a separate source_dir for discovery/scoring while still writing outputs under the deploy root, keeping attribution paths stable.

Show a summary per file

File	Description
tests/unit/install/test_no_policy_flag.py	Updates InstallContext construction to include the new required source_root.
tests/unit/install/test_direct_dep_failure.py	Updates minimal InstallContext fixture to pass source_root.
tests/unit/install/test_architecture_invariants.py	Updates install.py LOC-budget rationale to account for --root plumbing.
src/apm_cli/install/sources.py	Ensures local package copying uses ctx.source_root (source tree) rather than deploy root.
src/apm_cli/install/services.py	Adds source_root to local-content integration so synthetic _local reads from sources, not deploy root.
src/apm_cli/install/root_redirect.py	Introduces the shared chdir + source-root override context manager used by install/compile --root.
src/apm_cli/install/pipeline.py	Computes both deploy root and source root; documents the convention for phases.
src/apm_cli/install/phases/resolve.py	Resolves local-path deps relative to source_root and points the resolver at the source root (manifest location).
src/apm_cli/install/phases/integrate.py	Passes ctx.source_root into local-content integration.
src/apm_cli/install/context.py	Adds required source_root field to enforce explicit root selection in phases.
src/apm_cli/core/scope.py	Adds process-global source_root override and updates get_manifest_path() to follow source root.
src/apm_cli/compilation/template_builder.py	Adds source_dir parameter for stable <!-- Source: ... --> relative path rendering.
src/apm_cli/compilation/distributed_compiler.py	Adds source_dir distinct from base_dir, translating placement keys so writes land under deploy root.
src/apm_cli/compilation/agents_compiler.py	Threads source_dir through primitive discovery, warnings, and distributed compilation.
src/apm_cli/commands/install.py	Adds --root option and wraps command execution in install_root_redirect.
src/apm_cli/commands/compile/cli.py	Adds --root option and wraps command execution in compile_root_redirect, using source_root for reads.

Copilot's findings

• Files reviewed: 16/16 changed files
• Comments generated: 8

[srid]

Pushed a2186450: merged latest origin/main (--frozen, local-bundle install, --refresh, --as, --legacy-skill-paths, --all for compile, parse_targets_field, install_started_at timing, etc.) and addressed Copilot's automated review.

Review findings — addressed:

1. compile/cli.py:396 — --watch under --root scans the wrong tree. Now rejected upfront: if root and watch: raise click.UsageError("--root is not valid with --watch"). The flag combination has no real use case (watch is interactive dev; --root is for CI scratch-dir verification).
2. root_redirect.py:45 — dry-run leaks a mkdir. Threaded dry_run through the context manager. With --dry-run, the redirect refuses to create the target directory and raises a click.UsageError instead — preview cannot mutate the filesystem.
3. commands/install.py:1019 — logger UnboundLocalError. Already addressed upstream (logger = None initialized before try; except Exception guards if logger:). Preserved through the merge.
4. template_builder.py:20 — E305 lint. Added the missing second blank line between TemplateData and render_instructions_block.
   5./6. CLI doc drift. Added --root to docs/src/content/docs/reference/cli-commands.md (both apm install and apm compile sections) and to packages/apm-guide/.apm/skills/apm-usage/commands.md. Called out --global / --watch incompatibility and the --dry-run mkdir-refusal in both surfaces.
5. root_redirect.py:32 — no unit tests. New tests/unit/install/test_root_redirect.py covers: no-op when None/empty, chdir + override on entry, restore-on-exception, mkdir-on-write, dry_run refuses-mkdir, dry_run with existing dir, compile-alias-identity. 8 tests, all passing.
6. core/scope.py:150 — USER-branch duplicates get_apm_dir. get_manifest_path now delegates: if scope is InstallScope.USER: return get_apm_dir(scope) / APM_YML_FILENAME.

Verified end-to-end:

• 1039 install + compilation unit tests pass (656 baseline + new --root tests + the larger upstream surface).
• Ruff clean on every touched file.
• apm install --root SCRATCH && apm compile --root SCRATCH --target codex,opencode produces byte-identical AGENTS.md to an in-place compile (Build ID modulo).
• New CLI rejections: --root + --watch, --root + --global, and --dry-run + --root with a missing DIR all emit click.UsageError (exit 2) before any chdir/mkdir runs.

The chdir-vs-refactor design call is still open for maintainer direction (the bigger sweep through Path.cwd() callsites in apm_cli.adapters.client.* / mcp_integrator.py) — happy to do it if you'd prefer Lowy's direction.

[github-actions]

APM Review Panel: ship_with_followups

Ship --root with immediate followups on path containment and integration test coverage; concurrency risk is latent but must be documented before any server-mode work begins.

cc @srid @danielmeppiel -- a fresh advisory pass is ready for your review.

The feature is behaviorally correct and the read/write split root model is architecturally sound. The panel converged on four distinct risk tiers. The highest-signal items are two test-coverage-expert MISSING findings: the --global+--root mutual exclusion guard has no executable test, and the core user promise of --root (apm_modules/, lockfile, and runtime dirs land under DIR while sources resolve from $PWD) has no integration-with-fixtures coverage. Both are regression traps on a surface that must remain stable across every future install-phase refactor. Supply-chain's path containment finding ranks immediately below: --root / or --root ~ silently creates .claude/, .codex/, and apm_modules/ under the filesystem root or user home directory without confirmation. validate_path_segments() already exists; wiring it here is a one-liner with outsized protection. Auth-expert's finding on git credential fill losing per-repo .git/config helpers after chdir is a real correctness regression for users with per-repo credential config and should be filed immediately.

The concurrency/global-state concern raised by both python-architect and supply-chain is accurate in principle but not firing in practice today: the ThreadPoolExecutor in the download phase performs network I/O only and does not call set_source_root_override. A docstring warning on install_root_redirect costs zero runtime and eliminates the latent trap. UX gaps (no confirmation line, misleading relative paths in summary after chdir, --dry-run DIR-must-exist constraint) and the missing CHANGELOG entry are real but addressed in a followup PR -- they do not block behavioral correctness. The doc-writer found the primary CLI reference surfaces consistent; only nits remain.

Aligned with: Portability by manifest (upheld: sources resolve from $PWD, preserving manifest-relative portability). Pragmatic as npm (upheld: mirrors pip --target / npm --prefix as claimed; --dry-run DIR-must-exist is a minor deviation worth relaxing). Secure by default (partially upheld: no path containment guard means --root accepts / and ~ without confirmation -- one near-term gap).

Growth signal. The --root flag unlocks Docker multi-stage builds, CI scratch-dir verification, and monorepo fixture generation as three distinct adopter segments with pip/npm muscle memory. The Docker snippet (apm install --root /layer && COPY /layer . in a multi-stage Dockerfile) is the single highest-ROI conversion artifact this flag enables and should land in the advanced install docs before the first public release. The absence of a CHANGELOG entry is an immediate fix: this flag closes a concrete GitHub issue (#888) and the changelog is the primary artifact external contributors consult.

Panel summary

Persona	B	R	N	Takeaway
Python Architect	0	2	1	Pragmatic chdir+global pattern works for the CLI single-invocation case but carries two latent correctness faults: non-reentrant context manager and unguarded global under threads.
CLI Logging Expert	0	2	2	No user-visible signal that --root is active; silent chdir means users cannot confirm where files landed without checking the filesystem.
DevX UX Expert	0	2	2	Docs updated, help text solid; two ergonomic hazards: --dry-run DIR-must-exist constraint inverts expectations, and the split read/write root silently misfires when apm.yml is absent from $PWD.
Supply Chain Security Expert	0	2	1	chdir+global-override pair is not concurrency-safe; --root itself has no path containment guard, enabling silent writes to filesystem root or home.
OSS Growth Hacker	0	2	2	--root flag unlocks Docker, CI matrix, and monorepo installs; strong story beat but needs a CHANGELOG entry and one Docker snippet to convert.
Auth Expert	0	1	0	chdir to --root before lazy git credential fill may silently drop repo-local credential helpers; env-var and gh-auth paths unaffected.
Doc Writer	0	0	3	--root is documented in both the CLI reference and the apm-usage skill; instructions and SKILL.md are consistent and accurate.
Test Coverage Expert	0	2	1	Unit coverage for root_redirect context manager is solid (8 tests, state-unwind verified); --global+--root UsageError has no executable test; install --root deploy-path promise lacks integration-tier coverage.

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

Top 5 follow-ups

1. [Test Coverage Expert] Add integration-with-fixtures test: apm install --root (dir) writes apm_modules/, lockfile, and runtime dirs under DIR; sources resolve from $PWD -- Missing-evidence finding on the core user promise. No unit test on root_redirect.py can verify the cross-module resolve->integrate->lockfile-write flow. This is a regression trap on the primary behavioral contract of the flag.
2. [Supply Chain Security Expert] Add path containment guard before mkdir in root_redirect.py; call validate_path_segments and warn/require --yes when target resolves to $HOME directly -- A user typo (--root / or --root ~) silently creates .claude/, .codex/, and apm_modules/ under filesystem root or user home. validate_path_segments() already exists; wiring it is a one-liner with outsized blast-radius protection.
3. [Test Coverage Expert] Add unit test: apm install --global --root (dir) exits non-zero with click.UsageError matching 'not valid with --global' -- Missing-evidence finding on a guard that must hold permanently. If this mutual exclusion regresses, users mixing --global and --root get silent misbehavior with no diagnostic.
4. [Auth Expert] Capture cwd before os.chdir and pass cwd=original to resolve_credential_from_git subprocess.run; or set GIT_DIR/GIT_WORK_TREE env vars pointing at the original repo -- Post-chdir, git credential fill runs with cwd=root. If root is outside the original git repo, per-repo .git/config credential helpers are silently skipped. This breaks private local-package resolution for any user with repo-local credential config.
5. [OSS Growth Hacker] Add CHANGELOG entry for --root and a Docker multi-stage build snippet to the advanced install docs -- The flag closes a concrete issue (enhancement: apm install --root <dir> to target a directory other than $PWD #888) and opens three adopter segments. A missing CHANGELOG entry breaks the community trust contract. The Docker snippet is the single highest-ROI conversion artifact this flag enables.

Architecture

classDiagram
    direction LR

    class InstallScope {
        <<Enum>>
        PROJECT
        USER
    }

    class scope {
        <<Module>>
        -_SOURCE_ROOT_OVERRIDE Path
        +set_source_root_override(path)
        +get_source_root_override() Path
        +get_source_root(scope) Path
        +get_deploy_root(scope) Path
        +get_apm_dir(scope) Path
        +get_manifest_path(scope) Path
        +get_lockfile_dir(scope) Path
    }

    class install_root_redirect {
        <<ContextManager>>
        +root str
        +dry_run bool
        +__enter__() captures cwd, chdir to root
        +__exit__() chdir original, clear override
    }

    class compile_root_redirect {
        <<Alias>>
    }

    class InstallContext {
        <<Dataclass>>
        +source_root Path
        +project_root Path
        +scope InstallScope
        +expected_hash_change_deps set
        +direct_dep_failed bool
    }

    class InstallPipeline {
        <<Pure>>
        +run(ctx) void
    }

    class DownloadPhase {
        <<IOBoundary>>
        +uses ThreadPoolExecutor
    }

    class install_cmd {
        <<CLICommand>>
        +root Path
        +scope InstallScope
    }

    class compile_cmd {
        <<CLICommand>>
        +root Path
    }

    install_root_redirect ..> scope : calls set_source_root_override
    compile_root_redirect --|> install_root_redirect : alias
    install_cmd ..> install_root_redirect : wraps body
    compile_cmd ..> compile_root_redirect : wraps body
    InstallPipeline *-- InstallContext : reads source_root
    InstallPipeline *-- DownloadPhase : spawns threads
    InstallContext ..> scope : get_source_root()
    DownloadPhase ..> scope : Path.cwd() in thread workers

    class scope:::touched
    class install_root_redirect:::touched
    class compile_root_redirect:::touched
    class InstallContext:::touched
    class InstallPipeline:::touched
    class install_cmd:::touched
    class compile_cmd:::touched

    classDef touched fill:#fff3b0,stroke:#d47600

Loading

flowchart TD
    A(["apm install --root DIR"]) --> B["install_cmd: click option parsed\nsrc/apm_cli/commands/install.py"]
    B --> C{"root set?"}
    C -- No --> D["[FS] normal pipeline, Path.cwd()=PWD"]
    C -- Yes --> E["install_root_redirect(root, dry_run)\nsrc/apm_cli/install/root_redirect.py"]
    E --> F["[FS] target.mkdir(parents=True)\ncreate deploy root if missing"]
    F --> G["original = Path.cwd()\ncapture source root"]
    G --> H["set_source_root_override(original)\nsrc/apm_cli/core/scope.py"]
    H --> I["[FS] os.chdir(root)\nprocess cwd = deploy root from here"]
    I --> J["get_source_root(PROJECT) -> _SOURCE_ROOT_OVERRIDE\nreturns original PWD for apm.yml reads"]
    I --> K["get_deploy_root(PROJECT) -> Path.cwd()\nreturns deploy root for writes"]
    J --> L["pipeline: source_root=get_source_root()\nsrc/apm_cli/install/pipeline.py"]
    K --> M["get_apm_dir -> Path.cwd()=deploy root\nlockfile, apm_modules written there"]
    L --> N["[NET+THREADS] DownloadPhase ThreadPoolExecutor\nWARNING: threads share mutated cwd + global"]
    N --> O["[FS] IntegratePhase\nwrites .claude/ .codex/ .agents/ to deploy root"]
    O --> P{"exception?"}
    P -- Yes --> Q["finally: os.chdir(original)"]
    P -- No --> Q
    Q --> R["finally: set_source_root_override(None)"]
    R --> S(["control returned, state restored"])

Loading

Recommendation

The feature is behaviorally correct, the primary context manager has solid unit coverage (state-unwind on exception verified), and the read/write split root model is architecturally sound. The five followups above are real gaps but none is a blocking regression on the current implementation; they are regression traps and hardening items that should be filed as tracked issues and resolved in the next patch window. The path containment guard and the integration test are the two highest-urgency items and should ideally land before the first public release that includes this flag.

---

Full per-persona findings

Python Architect

• [recommended] _SOURCE_ROOT_OVERRIDE global has no lock; ThreadPoolExecutor in download phase creates a race window at src/apm_cli/core/scope.py:60
  install/phases/download.py spins a ThreadPoolExecutor during the same install run that holds the chdir + _SOURCE_ROOT_OVERRIDE mutation. os.chdir() is process-wide and not thread-safe; _SOURCE_ROOT_OVERRIDE is a bare module-level global with no threading.Lock. In the current pipeline the threads only do network I/O and do not call set_source_root_override, so no race fires today. But the invariant is undocumented: nothing prevents a future phase or server-mode CLI from touching scope helpers inside a worker.
  Suggested: Add threading.Lock around reads/writes in scope.py, or add a docstring warning on install_root_redirect that it must only be entered from the main thread before any ThreadPoolExecutor is live.

• [recommended] install_root_redirect is non-reentrant: nested entry silently corrupts restore path at src/apm_cli/install/root_redirect.py:68
  The context manager computes original = Path.cwd() at entry. If a caller is already inside an install_root_redirect (cwd is the deploy root), a nested entry records the deploy root as 'original', then on exit os.chdir(original) leaves the process in the deploy root. No guard exists. compile_root_redirect shares the same code.
  Suggested: At the top of install_root_redirect, assert get_source_root_override() is None, or raise RuntimeError('install_root_redirect is not reentrant') when _SOURCE_ROOT_OVERRIDE is already set.

• [nit] get_lockfile_dir docstring chain is misleading about how the override is consulted at src/apm_cli/core/scope.py:162
  Docstring says it 'honours set_source_root_override via Path.cwd() after install_root_redirect chdirs'. The override is NOT consulted by get_apm_dir or get_lockfile_dir -- only the chdir makes cwd() point to the deploy root.

CLI Logging Expert

• [recommended] No confirmation message when --root redirects writes at src/apm_cli/install/root_redirect.py
  install_root_redirect() does os.chdir(root) and set_source_root_override() with zero user-visible output. A user running 'apm install --root /tmp/scratch my-pkg' sees the normal install summary with no indication that apm_modules/ and .claude/ landed under /tmp/scratch rather than $PWD.
  Suggested: Emit an [i] line at the top of install_root_redirect() before yield, e.g. _rich_info(f'[i] Deploy root: {target} (sources: {original})') when root is set and not dry_run.

• [recommended] Install summary does not name the deploy root; relative paths in summary are misleading after chdir at src/apm_cli/commands/install.py
  After os.chdir(root), any relative path the InstallLogger/CommandLogger renders looks like it belongs to $PWD. A user diffing two runs sees identical-looking summaries pointing to paths that resolve to different absolute locations.
  Suggested: When root is set, append the absolute deploy root to the summary line, e.g. '[+] Installed 3 packages -> /abs/path/to/root'.

• [nit] --root/--global conflict error omits the fix at src/apm_cli/commands/install.py
  Suggested: Change to: '--root is not valid with --global (user scope). Drop --global to install into DIR, or drop --root to install into the user scope.'

• [nit] dry-run UsageError for missing root dir uses f-string path without quoting at src/apm_cli/install/root_redirect.py
  Suggested: Change to: f"--root '{target}' does not exist. Create the directory before --dry-run, or drop --dry-run to let install/compile create it."

DevX UX Expert

• [recommended] --dry-run refuses to run when DIR does not exist, which inverts the expected dry-run contract
  Every major package manager lets --dry-run run against a hypothetical state. Requiring DIR to already exist for --dry-run means a CI author must mkdir first, defeating the 'safe preview' mental model.
  Suggested: For --dry-run, treat a missing DIR as a preview-only no-op (log 'would create DIR') rather than a hard error.

• [recommended] Split read/write root will produce confusing apm.yml-not-found error when invoked from the wrong directory
  A user who runs 'apm install --root /tmp/verify' from a directory lacking apm.yml gets a source-resolution error with no mention of --root or $PWD. The help text documents the split, but the failure mode does not surface it.
  Suggested: When source resolution fails and --root is active, prepend: 'apm.yml not found in $PWD ({cwd}). With --root, sources always resolve from $PWD, not from DIR.'

• [nit] --root vs --prefix naming: help text analogue list ('Mirrors pip install --target / npm install --prefix') must not be shortened in future edits.

• [nit] --root is not valid with --watch error message is terse; no recovery hint.

Supply Chain Security Expert

• [recommended] os.chdir() and _SOURCE_ROOT_OVERRIDE are process-global with no lock; concurrent callers corrupt each other at src/apm_cli/install/root_redirect.py
  root_redirect.py uses os.chdir(target) and set_source_root_override() which both mutate process-global state. Parallel pytest-xdist workers, embedded library use, or future async install flows will race: one thread's chdir stomps another's cwd.
  Suggested: Add a module-level threading.Lock around the chdir+set_source_root_override pair, held for the entire duration of the context manager body.

• [recommended] --root accepts any filesystem path with no containment guard; apm install --root / silently creates agent dirs at filesystem root at src/apm_cli/install/root_redirect.py
  root_redirect.py calls target.mkdir(parents=True, exist_ok=True) on an unvalidated Path(root). No call to validate_path_segments or ensure_path_within. A user typo like --root / or --root ~ causes APM to create and populate .claude/, .codex/, apm_modules/ under / or $HOME without confirmation.
  Suggested: Before mkdir, call validate_path_segments(str(target)) and warn/require --yes when target resolves to $HOME directly.

• [nit] target is not resolved before os.chdir; a symlink as --root is followed silently at src/apm_cli/install/root_redirect.py
  Suggested: Add target = Path(root).resolve() to make the physical path explicit and consistent with set_source_root_override which already calls .resolve().

OSS Growth Hacker

• [recommended] Add a CHANGELOG entry for this feature -- it is absent from CHANGELOG.md
  The --root flag opens three distinct use-case categories (scratch verification, Docker layer builds, CI isolation). A changelog entry is the minimum for issue enhancement: apm install --root <dir> to target a directory other than $PWD #888 close visibility.
  Suggested: 'apm install --root DIR redirects apm_modules/, lockfile, and all runtime deploy dirs under a custom root while sources resolve from $PWD -- enables Docker multi-stage builds, CI scratch-dir verification, and fixture generation. (closes enhancement: apm install --root <dir> to target a directory other than $PWD #888)'

• [recommended] Add a Docker multi-stage build snippet to the advanced install docs -- highest-ROI conversion surface this flag enables
  A three-line Dockerfile snippet (apm install --root /layer && COPY /layer .) makes this flag immediately actionable for CI/Docker adopters.

• [nit] --root naming: add a one-line callout in docs pointing to pip --target / npm --prefix analogues.

• [nit] Tweet-length hook is not written anywhere: 'apm install --root /tmp/env -- test your agent setup in total isolation, like pip install --target but for AI agents.'

Auth Expert

• [recommended] git credential fill inherits post-chdir cwd, losing local .git/config credential helper config at src/apm_cli/core/token_manager.py
  install_root_redirect calls os.chdir(root) before yielding. AuthResolver resolves credentials lazily, so any try_with_fallback call that reaches git credential fill runs with cwd=root. If root is outside the original git repo, git will not find the local .git/config credential.helper entry, silently skipping per-repo credential helpers.
  Suggested: Capture the original cwd before chdir and pass cwd=original to the subprocess.run call in resolve_credential_from_git, or set GIT_DIR/GIT_WORK_TREE env vars pointing at the original repo.

Doc Writer

• [nit] linting.instructions.md is missing the applyTo frontmatter field that tests.instructions.md uses for scope binding at .apm/instructions/linting.instructions.md
  Suggested: Add applyTo: '**/*.py' if intent is Python sources only, or document global scope is deliberate.

• [nit] SKILL.md doc-writer activation list does not include .apm/instructions/** as a trigger path at .apm/skills/apm-review-panel/SKILL.md
  Suggested: Add .apm/instructions/**/*.md to the Doc Writer activation file list.

• [nit] cli-commands.md --root description lists .opencode/ but apm-usage commands.md entry omits it at packages/apm-guide/.apm/skills/apm-usage/commands.md
  Suggested: Expand the --root DIR description to enumerate the same directories as cli-commands.md.

Test Coverage Expert

• [recommended] --global + --root mutual exclusion (UsageError) has no executable test at src/apm_cli/commands/install.py
  src/apm_cli/commands/install.py raises click.UsageError but no test invokes the install command with both flags and asserts the error. If this guard regresses, users mixing --global and --root get silent misbehavior.
  Proof (missing at unit): tests/unit/install/test_root_redirect.py::test_global_and_root_are_mutually_exclusive -- proves: apm install --global --root DIR must exit non-zero with a clear error message [devx]
  with pytest.raises(click.UsageError, match='not valid with --global'): runner.invoke(install_cmd, ['--global', '--root', str(tmp_path)])

• [recommended] No integration-with-fixtures test exercises apm install --root DIR end-to-end deploy path at src/apm_cli/commands/install.py
  The core user promise of --root is that apm_modules/, lockfile, and runtime dirs land under DIR while sources resolve from $PWD. This cross-module flow (resolve->integrate->lockfile write) cannot be verified by unit tests on root_redirect.py alone. Grepped tests/integration/ for '--root', 'root_redirect', 'source_root': no match on install flow.
  Proof (missing at integration-with-fixtures): tests/integration/test_install_root_e2e.py::test_install_root_writes_outputs_under_root_dir -- proves: apm install --root DIR writes apm_modules/, apm.lock.yaml, and runtime dirs under DIR; sources resolve from $PWD [portability-by-manifest,devx]
  assert (root_dir / 'apm_modules').exists(); assert not (pwd / 'apm_modules').exists()

• [nit] Unit tests for root_redirect pass; state-unwind on exception is covered at tests/unit/install/test_root_redirect.py
  Proof (passed at unit): tests/unit/install/test_root_redirect.py::test_restore_on_exception -- proves: install_root_redirect clears source_root override and restores cwd even when an exception propagates [devx]
  assert Path.cwd() == cwd_before\nassert get_source_root_override() is None

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

Warning

Firewall blocked 1 domain

The following domain was blocked by the firewall during workflow execution:

• pypi.org

To allow these domains, add them to the network.allowed list in your workflow frontmatter:

network:
  allowed:
    - defaults
    - "pypi.org"

See Network Configuration for more information.

Note

🔒 Integrity filter blocked 2 items

The following items were blocked because they don't meet the GitHub integrity level.

• #928 pull_request_read: has lower integrity than agent requires. The agent cannot read data with integrity below "approved".
• feat(install, compile): --root DIR to redirect writes while sources resolve from $PWD #928 pull_request_read: has lower integrity than agent requires. The agent cannot read data with integrity below "approved".

To allow these resources, lower min-integrity in your GitHub frontmatter:

tools:
  github:
    min-integrity: approved  # merged | approved | unapproved | none

Generated by PR Review Panel for issue #928 · ● 5.3M · ◷