feat(compile): explicit apm compile -g + install-time hint for global root context#1632
feat(compile): explicit apm compile -g + install-time hint for global root context#1632danielmeppiel wants to merge 4 commits into
apm compile -g + install-time hint for global root context#1632Conversation
There was a problem hiding this comment.
Pull request overview
Adds a user-scope “global compile” path so packages installed with apm install -g can surface global (no apply_to) instructions into user-level root context files (~/.claude/CLAUDE.md, ~/.copilot/AGENTS.md, ~/.gemini/GEMINI.md, etc.), and introduces apm compile --global/-g to re-run that compilation without reinstalling.
Changes:
- Adds
compile_user_root_contexts()engine to compile global instructions from the user package tree into per-target user root context files (with overwrite protection via the APM marker). - Hooks global compilation into the install finalize phase for
InstallScope.USER. - Adds
--global/-ghandling toapm compile, plus unit tests and documentation updates.
Show a summary per file
| File | Description |
|---|---|
| tests/unit/install/test_finalize_phase.py | Extends the finalize-phase test context with a scope field. |
| tests/unit/install/phases/test_finalize_user_compile.py | Adds unit tests for the finalize hook that triggers user-scope compilation. |
| tests/unit/compilation/test_user_root_context.py | Adds unit tests for the new user-root compilation engine. |
| tests/unit/compilation/test_compile_global_flag.py | Adds unit tests for the new apm compile --global/-g CLI behavior. |
| src/apm_cli/install/phases/finalize.py | Calls a new post-install hook to compile user-scope root context files after -g installs. |
| src/apm_cli/compilation/user_root_context.py | Implements the engine that writes user-scope root context files from global instructions. |
| src/apm_cli/compilation/init.py | Exposes compile_user_root_contexts at the package level. |
| src/apm_cli/commands/compile/cli.py | Adds --global/-g flag and handler to compile user-scope root context files. |
| packages/apm-guide/.apm/skills/apm-usage/commands.md | Updates CLI usage docs to mention apm compile --global/-g. |
| docs/src/content/docs/producer/compile.md | Documents global compilation behavior, overwrite protection, and constraints. |
Copilot's findings
- Files reviewed: 10/10 changed files
- Comments generated: 6
| from apm_cli.compilation import compile_user_root_contexts | ||
| from apm_cli.core.scope import InstallScope, get_source_root | ||
| from apm_cli.integration.targets import KNOWN_TARGETS | ||
|
|
||
| source_root = get_source_root(InstallScope.USER) | ||
| targets = list(KNOWN_TARGETS.values()) |
| from ...compilation import compile_user_root_contexts | ||
| from ...core.scope import InstallScope, get_source_root | ||
| from ...integration.targets import KNOWN_TARGETS | ||
| from ...utils.console import _rich_error, _rich_info, _rich_success | ||
|
|
||
| source_root = get_source_root(InstallScope.USER) | ||
| apm_modules = source_root / "apm_modules" | ||
| if not apm_modules.is_dir(): |
| source_root = tmp_path / "source" | ||
| source_root.mkdir() | ||
| # apm_modules does NOT exist | ||
|
|
||
| mock_rich_error = MagicMock() | ||
|
|
||
| with ( | ||
| patch( | ||
| "apm_cli.core.scope.get_source_root", | ||
| return_value=source_root, | ||
| ), |
| source_root = Path.home() | ||
| ctx = _make_install_context(scope=InstallScope.USER) | ||
|
|
||
| mock_compile = MagicMock(return_value=[]) | ||
|
|
||
| with ( | ||
| patch( | ||
| "apm_cli.core.scope.get_source_root", | ||
| return_value=source_root, | ||
| ), |
| ## Global compilation (-g) | ||
|
|
||
| By default, `apm compile` reads instructions from your workspace and | ||
| writes root context files to `.github/`, `.claude/`, etc. For distributing | ||
| instructions to all AI tools on your user's machine (not scoped to a project), | ||
| use the `--global` or `-g` flag: |
| def compile_user_root_contexts( | ||
| targets, | ||
| source_root: Path, | ||
| *, | ||
| dry_run: bool = False, | ||
| logger=None, | ||
| ) -> list[dict]: |
APM Review Panel: Advisory RecommendationPR #1632 -- feat(install): compile global instructions into user-scope root context files on install -g Headline: Blocking path-resolution bug (get_source_root vs get_apm_dir) makes global compile silently no-op; shepherd has folded the fix before ship. Panel: python-architect, cli-logging-expert, devx-ux-expert, supply-chain-security-expert, oss-growth-hacker, auth-expert (inactive -- no auth surface), doc-writer, test-coverage-expert ArbitrationAll six active panelists converge on a single critical finding: Strategically, the design is sound and the reservations from the strategic-alignment gate are fully addressed: project-scope install/compile is untouched, the user-scope write is correctly gated behind The remaining recommended findings cluster around three themes: (1) exit-code correctness for CI consumers, (2) discoverability in docs, and (3) deploy-root path containment as a forward-safety measure. Exit-code fix and docs gap have been folded. Path containment is deferred. Principle Alignment
Growth NoteThe oss-growth-hacker correctly identifies this as a funnel-closing feature: global install now "just works" for AI tool configuration. The post-install moment is a prime delight hook. README/quickstart mention should land in the same release cycle to capture the adoption signal. Recommended Follow-ups (prioritized)
Ship Recommendation
The architecture is correct, the scope guards work, and the design delivers on the user promise. The blocking bug (path-resolution) has been folded in this shepherd run along with exit-code fixes, docs update, type annotations, and symbol= API usage. The feature is shippable. README discoverability and path containment guard are tracked as follow-ups for the next release cycle. Advisory only -- no merge gate. Maintainer decides. |
APM Review Panel:
|
| Persona | B | R | N | Takeaway |
|---|---|---|---|---|
| Python Architect | 0 | 2 | 2 | Clean new module; one structural gap (InstallLogger API mismatch silently swallowed) and one minor abstraction leak worth addressing before merge. |
| CLI Logging Expert | 0 | 2 | 3 | skipped-hand-authored shown as [i] info not warning; install path silently drops error results; missing symbol on apm_modules-not-found error. |
| DevX UX Expert | 0 | 3 | 2 | Flag ergonomics and happy-path UX are solid; two gaps: mutual-exclusion errors exit(2) without hinting the fix, and silent auto-compile on install -g has no discoverable signal on the non-verbose path. |
| Supply Chain Security Expert | 2 | 3 | 1 | Package instruction content flows verbatim into AI root context files; no sanitization, size cap, or path containment guard on output_path. |
| OSS Growth Hacker | 0 | 2 | 2 | Global compile is a killer distribution story but docs bury the lede -- the 'publish once, land in every AI tool' hook is missing from both pages. |
| Doc Writer | 0 | 1 | 3 | New global-compile docs are structurally sound and code-accurate; one consumer-page gap and two small inaccuracies need fixing. |
| Test Coverage Expert | 0 | 2 | 1 | 39 unit tests cover the engine well; the core user promise (apm install -g writes CLAUDE.md) has no integration-with-fixtures test, and CLAUDE_CONFIG_DIR is untested in the new engine. |
B = blocking-severity findings, R = recommended, N = nits.
Counts are signal strength, not gates. The maintainer ships.
Top 5 follow-ups
- [Supply Chain Security Expert] (blocking-severity) Add content sanitization: hard size cap per instruction (8 KB) and rejection of instruction content containing the APM marker string, before any write to AI root context files -- verbatim package content in
~/.claude/CLAUDE.mdenables supply-chain prompt injection via compromised transitive dependencies; existing package trust model does not mitigate this because instruction content is never inspected before install - [Supply Chain Security Expert] (blocking-severity) Call
ensure_path_within(output_path, deploy_root)frompath_security.pybefore any write incompile_user_root_contexts-- project's own security chokepoint contract requires all filesystem writes derived from dynamic inputs to pass throughensure_path_within;resolved_deploy_rootis dynamically computed and the guard is missing - [Test Coverage Expert] (blocking-severity) Add an integration-with-fixtures test that drives a real
apm install -gagainst a fixture package tree and asserts~/.claude/CLAUDE.mdis written with the APM marker -- evidenceoutcome: missingonintegration-with-fixturestier for the PR's core user promise; unit tests mockcompile_user_root_contextsat the boundary and will not catch install-pipeline wiring regressions - [DevX UX Expert] Emit a non-verbose output line when
apm install -gwrites root context files; side-effects outside the project tree must be visible on the default output path -- silent writes to~/.claude/CLAUDE.mdon install -g violate the pragmatic-as-npm ergonomic bar; users cannot audit what install did to their AI configuration - [CLI Logging Expert] Surface error results from
_compile_user_root_contexts_after_install; current code only messages onwrittenentries, silently discarding OS write failures -- a user whose CLAUDE.md was not updated due to a write failure sees a successful install summary with no indication that the root context step failed
Architecture
classDiagram
direction LR
class TargetProfile {
<<ValueObject>>
+name str
+compile_family str|None
+resolved_deploy_root Path|None
+for_scope(user_scope) TargetProfile|None
}
class compile_user_root_contexts {
<<Pure>>
+targets Iterable
+source_root Path
+dry_run bool
+logger Logger|None
+returns list[dict]
}
class AgentsCompiler {
<<Compiler>>
+_COPILOT_ROOT_GENERATED_MARKER str
}
class _handle_global_flag {
<<CLIHandler>>
+dry_run bool
+returns int
}
class finalize_run {
<<InstallPhase>>
+ctx InstallContext
+returns InstallResult
}
class InstallContext {
<<Context>>
+scope InstallScope
+logger InstallLogger|None
}
class InstallLogger {
<<CommandLogger>>
+verbose_detail(msg)
}
class discover_primitives {
<<IOBoundary>>
+root str
+returns PrimitivesResult
}
class compile_user_root_contexts:::touched
class _handle_global_flag:::touched
class finalize_run:::touched
compile_user_root_contexts ..> TargetProfile : iterates
compile_user_root_contexts ..> AgentsCompiler : reads marker constant
compile_user_root_contexts ..> discover_primitives : calls
_handle_global_flag ..> compile_user_root_contexts : calls
finalize_run ..> compile_user_root_contexts : calls via helper
finalize_run ..> InstallContext : reads scope
InstallContext *-- InstallLogger : optional
note for compile_user_root_contexts "Collect-then-render: discovers all\nglobal instructions, then writes per-target"
note for finalize_run "Logger API mismatch: passes logger=None\nbecause InstallLogger != stdlib Logger"
classDef touched fill:#fff3b0,stroke:#d47600
flowchart TD
A(["apm install -g"]) --> B["finalize.run()\ninstall/phases/finalize.py"]
B --> C{"ctx.scope == USER?"}
C -- No --> Z(["InstallResult"])
C -- Yes --> D["_compile_user_root_contexts_after_install(ctx)\nfinalize.py:22"]
D --> E["[I/O] get_apm_dir(USER)\ncore/scope.py"]
E --> F["compile_user_root_contexts(targets, source_root)\ncompilation/user_root_context.py:100"]
F --> G["[FS] check apm_modules dir exists\n~/.apm/apm_modules"]
G -- Not found --> H(["return empty list"])
G -- Found --> I["[I/O] discover_primitives(apm_modules)\nprimitives/discovery.py"]
I --> J["filter: instructions with no apply_to"]
J --> K["for each target in KNOWN_TARGETS"]
K --> L["target.for_scope(user_scope=True)"]
L -- None --> K
L -- Profile --> M{"compile_family in _ROOT_FILENAME?"}
M -- No --> K
M -- Yes --> N{"global_instructions empty?"}
N -- Yes --> O["status: skipped-no-instructions"] --> K
N -- No --> P["_resolve_deploy_root(scoped)\nresolved_deploy_root or home/root_dir"]
P --> Q["_generate_content(instructions)\n_finalize_build_id + marker"]
Q --> R{"[I/O] output_path.exists()?"}
R -- No --> S["[FS] mkdir + write_text\nstatus: written"]
R -- Yes --> T["[I/O] read_text existing file"]
T --> U{"APM marker present?"}
U -- No --> V["status: skipped-hand-authored"]
U -- Yes --> W{"content unchanged?"}
W -- Yes --> X["status: unchanged"]
W -- No --> S
S --> K
K -- done --> Y["return results list"]
Y --> Z
A2(["apm compile --global"]) --> H2["_handle_global_flag(dry_run)\ncompile/cli.py:323"]
H2 --> E
H2 --> YY["render per-target status to console"]
sequenceDiagram
actor User
participant CLI as apm CLI
participant Finalize as finalize.run()
participant Engine as compile_user_root_contexts()
participant FS as Filesystem
User->>CLI: apm install -g pkg
CLI->>Finalize: run(ctx) [scope=USER]
Finalize->>Engine: compile_user_root_contexts(KNOWN_TARGETS, ~/.apm)
Engine->>FS: discover_primitives(~/.apm/apm_modules)
FS-->>Engine: primitives
loop per target (claude, copilot, cursor, ...)
Engine->>FS: read existing root file (if any)
FS-->>Engine: existing content or not-found
alt hand-authored (no APM marker)
Engine-->>Engine: skip
else APM-owned or new
Engine->>FS: write CLAUDE.md / AGENTS.md / GEMINI.md
end
end
Engine-->>Finalize: results list
Finalize-->>CLI: InstallResult
CLI-->>User: success message + verbose_detail count
Recommendation
Hold this PR for three targeted fixes before merge: (1) add minimum content sanitization -- per-instruction size cap and marker-string rejection -- in compile_user_root_contexts before any write; (2) add ensure_path_within(output_path, deploy_root) immediately after output_path is resolved; (3) add at least one integration-with-fixtures test that asserts the core promise end-to-end. These are all small, scoped changes that do not require design discussion. Once those three land, the remaining recommended findings (non-verbose install output, error surfacing, logger API bridge, doc additions) should be addressed in the same PR rather than deferred -- they are the difference between a feature that feels polished at launch and one that accumulates a tail of follow-up issues. The strategic framing and code structure are excellent; this is worth the extra day to harden.
Full per-persona findings
Python Architect
-
[recommended] Logger API mismatch is silently swallowed by passing logger=None from finalize.py at
src/apm_cli/install/phases/finalize.py:32
compile_user_root_contexts accepts a stdlib logging.Logger, but ctx.logger is an InstallLogger with a different API (verbose_detail, not debug/info). All compilation debug/warning output during install goes to the root stdlib logger, silently bypassing the structured CLI output pipeline.
Suggested: Add a thin property to InstallContext (or InstallLogger) that returns a stdlib logging.Logger wrapping verbose_detail/warn calls, then pass that logger to compile_user_root_contexts. -
[recommended] discover_primitives called once per compile_user_root_contexts call but targets loop iterates inside; primitive discovery design is fragile if a future caller wraps in a loop at
src/apm_cli/compilation/user_root_context.py:100
The primitives object is never type-hinted; the function signature uses object for TargetProfile instances, eroding IDE support and the architecture's own contract. -
[nit] _ROOT_FILENAME map uses 'vscode' key but copilot target returns compile_family='vscode' at user scope -- add clarifying comment at
src/apm_cli/compilation/user_root_context.py:35 -
[nit] finalize.py imports InstallScope twice: once under TYPE_CHECKING and once as a lazy runtime import inside run() at
src/apm_cli/install/phases/finalize.py:104
Move InstallScope to a top-level runtime import and remove the duplicate lazy import.
CLI Logging Expert
-
[recommended] skipped-hand-authored uses _rich_info/symbol=info but should warn the user at
src/apm_cli/commands/compile/cli.py:365
A hand-authored file being silently skipped is a yellow-flag condition: the user's global instructions were NOT applied to that target. Rendering it as blue [i] buries an actionable message.
Suggested: Use _rich_warning(f"{tname}: skipped (hand-authored) {path} -- add APM marker or use --force to overwrite", symbol="warning") -
[recommended] install path swallows error results from _compile_user_root_contexts_after_install silently at
src/apm_cli/install/phases/finalize.py:33
finalize.py only calls verbose_detail when results contain 'written' entries. If every result is an error (OS write failure), the function returns silently. The user sees a successful install summary but root context files were not updated. -
[nit] _rich_error at line 337 has no symbol= so the apm_modules-not-found error lacks the [x] prefix at
src/apm_cli/commands/compile/cli.py:337 -
[nit] _rich_info at line 351 (no results path) has no symbol= while all per-entry info lines do at
src/apm_cli/commands/compile/cli.py:351 -
[nit] symbol=light_bulb and symbol=page used elsewhere in file are not in STATUS_SYMBOLS and are silently dropped (pre-existing, not introduced by this PR) at
src/apm_cli/commands/compile/cli.py:872
DevX UX Expert
-
[recommended] Mutual-exclusion errors print the problem but not the fix at
src/apm_cli/commands/compile/cli.py:552
'--global cannot be combined with --watch' emits the error then exit(2). npm/pip pattern is to append a one-line hint: e.g. "Drop --watch and re-run, or use 'apm compile --global' separately." -
[recommended] Auto-compile triggered by apm install -g is invisible on the default (non-verbose) output path at
src/apm_cli/install/phases/finalize.py:34
finalize.py gates the message on verbose_detail. A user running plain 'apm install -g some-pkg' sees no indication that ~/.claude/CLAUDE.md was just written. Silent side-effects to files outside the project tree are especially surprising. -
[recommended] Empty-modules info message does not explain that only instructions without applyTo: are compiled globally at
src/apm_cli/commands/compile/cli.py:351
A user who installed a skills-only package globally will hit 'No user-scope targets produced output' and not understand why nothing happened.
Suggested: "No global instructions found in ~/.apm/apm_modules. Only instructions without an applyTo: field are compiled globally. Skills and scoped instructions are skipped." -
[nit] Help text for --global lists only ~/.claude/CLAUDE.md with 'etc.' -- spell out the full list or point to --dry-run at
src/apm_cli/commands/compile/cli.py:484 -
[nit] skipped-hand-authored output uses 'info' symbol rather than a warning symbol at
src/apm_cli/commands/compile/cli.py:366
Supply Chain Security Expert
-
[blocking] Unfiltered package content written verbatim into AI root context files enables supply-chain prompt injection at
src/apm_cli/compilation/user_root_context.py:93
compile_user_root_contexts() concatenates instruction.content.strip() directly from any installed package into ~/.claude/CLAUDE.md, ~/.codex/AGENTS.md, etc. A compromised package maintainer or dependency-confusion package publishes adversarial AI directives. The install hook fires automatically without user acknowledgement. There is no allowlist, no size cap, and no pre-write confirmation.
Suggested: Apply a hard size cap per instruction (8 KB) and a total cap per generated file (64 KB). Reject instruction content containing the APM marker string. Consider a pre-write summary prompt for --yes-less invocations. -
[blocking] output_path is not validated with ensure_path_within; a TargetProfile with an attacker-influenced root_dir could write outside the expected directory at
src/apm_cli/compilation/user_root_context.py:188
_resolve_deploy_root() returns profile.resolved_deploy_root which is dynamically set, potentially by a resolver callback. output_path = deploy_root / root_filename is written without calling ensure_path_within or validate_path_segments, violating the project's path_security.py chokepoint contract.
Suggested: Call ensure_path_within(output_path, deploy_root) from src/apm_cli/utils/path_security.py before any write. -
[recommended] TOCTOU race between output_path.exists() check and output_path.write_text() allows hand-authored file to be overwritten at
src/apm_cli/compilation/user_root_context.py:193
Sequence: (1) exists(), (2) read_text(), (3) marker check, (4) write_text(). A concurrent process could swap a hand-authored file between steps (3) and (4).
Suggested: Write to output_path.with_suffix('.tmp') then atomically rename with output_path.replace(). -
[recommended] APM marker is a plain human-readable string; a package can embed it to confuse future overwrite checks at
src/apm_cli/compilation/user_root_context.py:203
The marker '' is checked with a substring test. Instruction content containing the marker string will degrade the integrity signal and could enable social-engineering attacks.
Suggested: Detect and reject instruction content containing the marker string before compilation, logging a warning with the offending package name. -
[recommended] Install hook fires unconditionally for every USER-scope install, even when no newly installed package provides global instructions at
src/apm_cli/install/phases/finalize.py:107
The attack window opens for ANY user-scope install, including unrelated ones that coincidentally pull in a transitive dependency with global instructions. -
[nit] logger=None passed from finalize.py loses structured install-context logging; stdlib root logger may emit to unexpected handlers at
src/apm_cli/install/phases/finalize.py:32
OSS Growth Hacker
-
[recommended] Producer compile.md lacks the distribution hook for package authors at
docs/src/content/docs/producer/compile.md
The Global compilation section reads as an ops procedure. It never states the compelling consequence: a package author publishes once, and every user who runs 'apm install -g (pkg)' gets instructions surfaced in ALL their AI tools automatically. That is the 'npm install -g' moment. Without that sentence, a package author has no reason to invest in global instructions.
Suggested: Add a 1-2 sentence framing hook: "Publishing a package with global instructions means every user who runs 'apm install -g (your-pkg)' gets your context surfaced in Claude, Codex, Copilot, Cursor, Gemini, and Windsurf automatically -- no project setup required." -
[recommended] Reference CLI page buries -g in a table with no example; the feature deserves a runnable snippet at
docs/src/content/docs/reference/cli/compile.md
A developer scanning for 'how do I make my package affect all AI tools' will not stop at the one-row table. An 'Install globally and verify' example block under Examples would make the flow copy-pasteable. -
[nit] Constraints bullet 'Skills-only packages do not write root files' is user-hostile without a reason at
docs/src/content/docs/producer/compile.md
Suggested: "Skills-only packages (no global instructions) do not write root files -- skills are deployed directly into harness directories by apm install, not compiled into root context files." -
[nit] The closing sentence of producer/compile.md misses a callout to -g as a publish motivation at
docs/src/content/docs/producer/compile.md
Auth Expert -- inactive
PR does not touch auth flows, token management, credential resolution, or AuthResolver inputs; compile engine reads local ~/.apm/apm_modules only.
Doc Writer
-
[recommended] install-packages.md does not mention that apm install -g auto-compiles global root context files at
docs/src/content/docs/consumer/install-packages.md
A consumer reading install-packages.md sees 'apm install -g (package)' but has no signal that root context files (/.claude/CLAUDE.md etc.) are written as a side effect. This is a behavioural change visible to users./.claude/CLAUDE.md, ~/.codex/AGENTS.md, etc.) from any global instructions in the installed packages. Re-run manually with 'apm compile -g'. See Global compilation."
Suggested: Add a note under the 'apm install -g' section: "After a global install, APM automatically compiles user-scope root context files ( -
[nit] producer/compile.md Global section uses apply_to: but the rest of the page uses applyTo: (camelCase frontmatter convention) at
docs/src/content/docs/producer/compile.md
Suggested: Change 'apply_to:' to 'applyTo:' on the relevant line. -
[nit] producer/compile.md global paths list silently omits opencode (~/.config/opencode/AGENTS.md) at
docs/src/content/docs/producer/compile.md
The source docstring explicitly lists opencode with its non-standard XDG path; leaving it out makes the 'etc.' harder to interpret. -
[nit] reference/cli/compile.md global flag hardcodes
/.apm/apm_modules but the path is dynamically resolved via get_apm_dir(InstallScope.USER) which may honour XDG_DATA_HOME at/.apm/apm_modules' with 'the user-scope apm_modules directory (default: ~/.apm/apm_modules)'.docs/src/content/docs/reference/cli/compile.md
Suggested: Replace '
Test Coverage Expert
-
[recommended] No integration-with-fixtures test proves apm install -g writes CLAUDE.md to the user home directory
All 39 new tests mock compile_user_root_contexts at the boundary -- none drive a real apm install -g invocation against a fixture package tree and assert the file appears on disk. The install pipeline tier floor is integration-with-fixtures.
Proof (missing):tests/integration/test_global_install_e2e.py::test_install_global_writes_user_root_context_claude_md-- proves: Running 'apm install -g' with a package that has global instructions writes ~/.claude/CLAUDE.md containing the APM-generated marker [secure-by-default]
assert (fake_home / '.claude' / 'CLAUDE.md').exists() and '<!-- Generated by APM CLI' in (fake_home / '.claude' / 'CLAUDE.md').read_text() -
[recommended] CLAUDE_CONFIG_DIR env-var override path in _resolve_deploy_root has no test in the new compilation engine at
tests/unit/compilation/test_user_root_context.py
Grepped tests/unit/compilation/test_user_root_context.py for 'CLAUDE_CONFIG_DIR' -- zero hits. If CLAUDE_CONFIG_DIR handling in for_scope silently regresses, no test in the new suite would catch it.
Proof (missing):tests/unit/compilation/test_user_root_context.py::test_claude_config_dir_respected_as_deploy_root-- proves: When CLAUDE_CONFIG_DIR is set, compile_user_root_contexts writes CLAUDE.md into the custom directory
monkeypatch.setenv('CLAUDE_CONFIG_DIR', str(custom_dir)); assert result[0]['path'] == custom_dir / 'CLAUDE.md' -
[nit] Overwrite-guard test does not assert that APM-generated file is updated when instructions change (idempotency round-trip) at
tests/unit/compilation/test_user_root_context.py
Proof (missing):tests/unit/compilation/test_user_root_context.py::test_apm_generated_file_updated_when_instructions_change-- proves: A file written by APM on a first compile is overwritten on a second compile when instructions change
assert second_result[0]['status'] == 'written' and output_path.read_text() != first_content
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 #1632 · sonnet46 9M · ◷
1 similar comment
APM Review Panel:
|
| Persona | B | R | N | Takeaway |
|---|---|---|---|---|
| Python Architect | 0 | 2 | 2 | Clean new module; one structural gap (InstallLogger API mismatch silently swallowed) and one minor abstraction leak worth addressing before merge. |
| CLI Logging Expert | 0 | 2 | 3 | skipped-hand-authored shown as [i] info not warning; install path silently drops error results; missing symbol on apm_modules-not-found error. |
| DevX UX Expert | 0 | 3 | 2 | Flag ergonomics and happy-path UX are solid; two gaps: mutual-exclusion errors exit(2) without hinting the fix, and silent auto-compile on install -g has no discoverable signal on the non-verbose path. |
| Supply Chain Security Expert | 2 | 3 | 1 | Package instruction content flows verbatim into AI root context files; no sanitization, size cap, or path containment guard on output_path. |
| OSS Growth Hacker | 0 | 2 | 2 | Global compile is a killer distribution story but docs bury the lede -- the 'publish once, land in every AI tool' hook is missing from both pages. |
| Doc Writer | 0 | 1 | 3 | New global-compile docs are structurally sound and code-accurate; one consumer-page gap and two small inaccuracies need fixing. |
| Test Coverage Expert | 0 | 2 | 1 | 39 unit tests cover the engine well; the core user promise (apm install -g writes CLAUDE.md) has no integration-with-fixtures test, and CLAUDE_CONFIG_DIR is untested in the new engine. |
B = blocking-severity findings, R = recommended, N = nits.
Counts are signal strength, not gates. The maintainer ships.
Top 5 follow-ups
- [Supply Chain Security Expert] (blocking-severity) Add content sanitization: hard size cap per instruction (8 KB) and rejection of instruction content containing the APM marker string, before any write to AI root context files -- verbatim package content in
~/.claude/CLAUDE.mdenables supply-chain prompt injection via compromised transitive dependencies; existing package trust model does not mitigate this because instruction content is never inspected before install - [Supply Chain Security Expert] (blocking-severity) Call
ensure_path_within(output_path, deploy_root)frompath_security.pybefore any write incompile_user_root_contexts-- project's own security chokepoint contract requires all filesystem writes derived from dynamic inputs to pass throughensure_path_within;resolved_deploy_rootis dynamically computed and the guard is missing - [Test Coverage Expert] (blocking-severity) Add an integration-with-fixtures test that drives a real
apm install -gagainst a fixture package tree and asserts~/.claude/CLAUDE.mdis written with the APM marker -- evidenceoutcome: missingonintegration-with-fixturestier for the PR's core user promise; unit tests mockcompile_user_root_contextsat the boundary and will not catch install-pipeline wiring regressions - [DevX UX Expert] Emit a non-verbose output line when
apm install -gwrites root context files; side-effects outside the project tree must be visible on the default output path -- silent writes to~/.claude/CLAUDE.mdon install -g violate the pragmatic-as-npm ergonomic bar; users cannot audit what install did to their AI configuration - [CLI Logging Expert] Surface error results from
_compile_user_root_contexts_after_install; current code only messages onwrittenentries, silently discarding OS write failures -- a user whose CLAUDE.md was not updated due to a write failure sees a successful install summary with no indication that the root context step failed
Architecture
classDiagram
direction LR
class TargetProfile {
<<ValueObject>>
+name str
+compile_family str|None
+resolved_deploy_root Path|None
+for_scope(user_scope) TargetProfile|None
}
class compile_user_root_contexts {
<<Pure>>
+targets Iterable
+source_root Path
+dry_run bool
+logger Logger|None
+returns list[dict]
}
class AgentsCompiler {
<<Compiler>>
+_COPILOT_ROOT_GENERATED_MARKER str
}
class _handle_global_flag {
<<CLIHandler>>
+dry_run bool
+returns int
}
class finalize_run {
<<InstallPhase>>
+ctx InstallContext
+returns InstallResult
}
class InstallContext {
<<Context>>
+scope InstallScope
+logger InstallLogger|None
}
class InstallLogger {
<<CommandLogger>>
+verbose_detail(msg)
}
class discover_primitives {
<<IOBoundary>>
+root str
+returns PrimitivesResult
}
class compile_user_root_contexts:::touched
class _handle_global_flag:::touched
class finalize_run:::touched
compile_user_root_contexts ..> TargetProfile : iterates
compile_user_root_contexts ..> AgentsCompiler : reads marker constant
compile_user_root_contexts ..> discover_primitives : calls
_handle_global_flag ..> compile_user_root_contexts : calls
finalize_run ..> compile_user_root_contexts : calls via helper
finalize_run ..> InstallContext : reads scope
InstallContext *-- InstallLogger : optional
note for compile_user_root_contexts "Collect-then-render: discovers all\nglobal instructions, then writes per-target"
note for finalize_run "Logger API mismatch: passes logger=None\nbecause InstallLogger != stdlib Logger"
classDef touched fill:#fff3b0,stroke:#d47600
flowchart TD
A(["apm install -g"]) --> B["finalize.run()\ninstall/phases/finalize.py"]
B --> C{"ctx.scope == USER?"}
C -- No --> Z(["InstallResult"])
C -- Yes --> D["_compile_user_root_contexts_after_install(ctx)\nfinalize.py:22"]
D --> E["[I/O] get_apm_dir(USER)\ncore/scope.py"]
E --> F["compile_user_root_contexts(targets, source_root)\ncompilation/user_root_context.py:100"]
F --> G["[FS] check apm_modules dir exists\n~/.apm/apm_modules"]
G -- Not found --> H(["return empty list"])
G -- Found --> I["[I/O] discover_primitives(apm_modules)\nprimitives/discovery.py"]
I --> J["filter: instructions with no apply_to"]
J --> K["for each target in KNOWN_TARGETS"]
K --> L["target.for_scope(user_scope=True)"]
L -- None --> K
L -- Profile --> M{"compile_family in _ROOT_FILENAME?"}
M -- No --> K
M -- Yes --> N{"global_instructions empty?"}
N -- Yes --> O["status: skipped-no-instructions"] --> K
N -- No --> P["_resolve_deploy_root(scoped)\nresolved_deploy_root or home/root_dir"]
P --> Q["_generate_content(instructions)\n_finalize_build_id + marker"]
Q --> R{"[I/O] output_path.exists()?"}
R -- No --> S["[FS] mkdir + write_text\nstatus: written"]
R -- Yes --> T["[I/O] read_text existing file"]
T --> U{"APM marker present?"}
U -- No --> V["status: skipped-hand-authored"]
U -- Yes --> W{"content unchanged?"}
W -- Yes --> X["status: unchanged"]
W -- No --> S
S --> K
K -- done --> Y["return results list"]
Y --> Z
A2(["apm compile --global"]) --> H2["_handle_global_flag(dry_run)\ncompile/cli.py:323"]
H2 --> E
H2 --> YY["render per-target status to console"]
sequenceDiagram
actor User
participant CLI as apm CLI
participant Finalize as finalize.run()
participant Engine as compile_user_root_contexts()
participant FS as Filesystem
User->>CLI: apm install -g pkg
CLI->>Finalize: run(ctx) [scope=USER]
Finalize->>Engine: compile_user_root_contexts(KNOWN_TARGETS, ~/.apm)
Engine->>FS: discover_primitives(~/.apm/apm_modules)
FS-->>Engine: primitives
loop per target (claude, copilot, cursor, ...)
Engine->>FS: read existing root file (if any)
FS-->>Engine: existing content or not-found
alt hand-authored (no APM marker)
Engine-->>Engine: skip
else APM-owned or new
Engine->>FS: write CLAUDE.md / AGENTS.md / GEMINI.md
end
end
Engine-->>Finalize: results list
Finalize-->>CLI: InstallResult
CLI-->>User: success message + verbose_detail count
Recommendation
Hold this PR for three targeted fixes before merge: (1) add minimum content sanitization -- per-instruction size cap and marker-string rejection -- in compile_user_root_contexts before any write; (2) add ensure_path_within(output_path, deploy_root) immediately after output_path is resolved; (3) add at least one integration-with-fixtures test that asserts the core promise end-to-end. These are all small, scoped changes that do not require design discussion. Once those three land, the remaining recommended findings (non-verbose install output, error surfacing, logger API bridge, doc additions) should be addressed in the same PR rather than deferred -- they are the difference between a feature that feels polished at launch and one that accumulates a tail of follow-up issues. The strategic framing and code structure are excellent; this is worth the extra day to harden.
Full per-persona findings
Python Architect
-
[recommended] Logger API mismatch is silently swallowed by passing logger=None from finalize.py at
src/apm_cli/install/phases/finalize.py:32
compile_user_root_contexts accepts a stdlib logging.Logger, but ctx.logger is an InstallLogger with a different API (verbose_detail, not debug/info). All compilation debug/warning output during install goes to the root stdlib logger, silently bypassing the structured CLI output pipeline.
Suggested: Add a thin property to InstallContext (or InstallLogger) that returns a stdlib logging.Logger wrapping verbose_detail/warn calls, then pass that logger to compile_user_root_contexts. -
[recommended] discover_primitives called once per compile_user_root_contexts call but targets loop iterates inside; primitive discovery design is fragile if a future caller wraps in a loop at
src/apm_cli/compilation/user_root_context.py:100
The primitives object is never type-hinted; the function signature uses object for TargetProfile instances, eroding IDE support and the architecture's own contract. -
[nit] _ROOT_FILENAME map uses 'vscode' key but copilot target returns compile_family='vscode' at user scope -- add clarifying comment at
src/apm_cli/compilation/user_root_context.py:35 -
[nit] finalize.py imports InstallScope twice: once under TYPE_CHECKING and once as a lazy runtime import inside run() at
src/apm_cli/install/phases/finalize.py:104
Move InstallScope to a top-level runtime import and remove the duplicate lazy import.
CLI Logging Expert
-
[recommended] skipped-hand-authored uses _rich_info/symbol=info but should warn the user at
src/apm_cli/commands/compile/cli.py:365
A hand-authored file being silently skipped is a yellow-flag condition: the user's global instructions were NOT applied to that target. Rendering it as blue [i] buries an actionable message.
Suggested: Use _rich_warning(f"{tname}: skipped (hand-authored) {path} -- add APM marker or use --force to overwrite", symbol="warning") -
[recommended] install path swallows error results from _compile_user_root_contexts_after_install silently at
src/apm_cli/install/phases/finalize.py:33
finalize.py only calls verbose_detail when results contain 'written' entries. If every result is an error (OS write failure), the function returns silently. The user sees a successful install summary but root context files were not updated. -
[nit] _rich_error at line 337 has no symbol= so the apm_modules-not-found error lacks the [x] prefix at
src/apm_cli/commands/compile/cli.py:337 -
[nit] _rich_info at line 351 (no results path) has no symbol= while all per-entry info lines do at
src/apm_cli/commands/compile/cli.py:351 -
[nit] symbol=light_bulb and symbol=page used elsewhere in file are not in STATUS_SYMBOLS and are silently dropped (pre-existing, not introduced by this PR) at
src/apm_cli/commands/compile/cli.py:872
DevX UX Expert
-
[recommended] Mutual-exclusion errors print the problem but not the fix at
src/apm_cli/commands/compile/cli.py:552
'--global cannot be combined with --watch' emits the error then exit(2). npm/pip pattern is to append a one-line hint: e.g. "Drop --watch and re-run, or use 'apm compile --global' separately." -
[recommended] Auto-compile triggered by apm install -g is invisible on the default (non-verbose) output path at
src/apm_cli/install/phases/finalize.py:34
finalize.py gates the message on verbose_detail. A user running plain 'apm install -g some-pkg' sees no indication that ~/.claude/CLAUDE.md was just written. Silent side-effects to files outside the project tree are especially surprising. -
[recommended] Empty-modules info message does not explain that only instructions without applyTo: are compiled globally at
src/apm_cli/commands/compile/cli.py:351
A user who installed a skills-only package globally will hit 'No user-scope targets produced output' and not understand why nothing happened.
Suggested: "No global instructions found in ~/.apm/apm_modules. Only instructions without an applyTo: field are compiled globally. Skills and scoped instructions are skipped." -
[nit] Help text for --global lists only ~/.claude/CLAUDE.md with 'etc.' -- spell out the full list or point to --dry-run at
src/apm_cli/commands/compile/cli.py:484 -
[nit] skipped-hand-authored output uses 'info' symbol rather than a warning symbol at
src/apm_cli/commands/compile/cli.py:366
Supply Chain Security Expert
-
[blocking] Unfiltered package content written verbatim into AI root context files enables supply-chain prompt injection at
src/apm_cli/compilation/user_root_context.py:93
compile_user_root_contexts() concatenates instruction.content.strip() directly from any installed package into ~/.claude/CLAUDE.md, ~/.codex/AGENTS.md, etc. A compromised package maintainer or dependency-confusion package publishes adversarial AI directives. The install hook fires automatically without user acknowledgement. There is no allowlist, no size cap, and no pre-write confirmation.
Suggested: Apply a hard size cap per instruction (8 KB) and a total cap per generated file (64 KB). Reject instruction content containing the APM marker string. Consider a pre-write summary prompt for --yes-less invocations. -
[blocking] output_path is not validated with ensure_path_within; a TargetProfile with an attacker-influenced root_dir could write outside the expected directory at
src/apm_cli/compilation/user_root_context.py:188
_resolve_deploy_root() returns profile.resolved_deploy_root which is dynamically set, potentially by a resolver callback. output_path = deploy_root / root_filename is written without calling ensure_path_within or validate_path_segments, violating the project's path_security.py chokepoint contract.
Suggested: Call ensure_path_within(output_path, deploy_root) from src/apm_cli/utils/path_security.py before any write. -
[recommended] TOCTOU race between output_path.exists() check and output_path.write_text() allows hand-authored file to be overwritten at
src/apm_cli/compilation/user_root_context.py:193
Sequence: (1) exists(), (2) read_text(), (3) marker check, (4) write_text(). A concurrent process could swap a hand-authored file between steps (3) and (4).
Suggested: Write to output_path.with_suffix('.tmp') then atomically rename with output_path.replace(). -
[recommended] APM marker is a plain human-readable string; a package can embed it to confuse future overwrite checks at
src/apm_cli/compilation/user_root_context.py:203
The marker '' is checked with a substring test. Instruction content containing the marker string will degrade the integrity signal and could enable social-engineering attacks.
Suggested: Detect and reject instruction content containing the marker string before compilation, logging a warning with the offending package name. -
[recommended] Install hook fires unconditionally for every USER-scope install, even when no newly installed package provides global instructions at
src/apm_cli/install/phases/finalize.py:107
The attack window opens for ANY user-scope install, including unrelated ones that coincidentally pull in a transitive dependency with global instructions. -
[nit] logger=None passed from finalize.py loses structured install-context logging; stdlib root logger may emit to unexpected handlers at
src/apm_cli/install/phases/finalize.py:32
OSS Growth Hacker
-
[recommended] Producer compile.md lacks the distribution hook for package authors at
docs/src/content/docs/producer/compile.md
The Global compilation section reads as an ops procedure. It never states the compelling consequence: a package author publishes once, and every user who runs 'apm install -g (pkg)' gets instructions surfaced in ALL their AI tools automatically. That is the 'npm install -g' moment. Without that sentence, a package author has no reason to invest in global instructions.
Suggested: Add a 1-2 sentence framing hook: "Publishing a package with global instructions means every user who runs 'apm install -g (your-pkg)' gets your context surfaced in Claude, Codex, Copilot, Cursor, Gemini, and Windsurf automatically -- no project setup required." -
[recommended] Reference CLI page buries -g in a table with no example; the feature deserves a runnable snippet at
docs/src/content/docs/reference/cli/compile.md
A developer scanning for 'how do I make my package affect all AI tools' will not stop at the one-row table. An 'Install globally and verify' example block under Examples would make the flow copy-pasteable. -
[nit] Constraints bullet 'Skills-only packages do not write root files' is user-hostile without a reason at
docs/src/content/docs/producer/compile.md
Suggested: "Skills-only packages (no global instructions) do not write root files -- skills are deployed directly into harness directories by apm install, not compiled into root context files." -
[nit] The closing sentence of producer/compile.md misses a callout to -g as a publish motivation at
docs/src/content/docs/producer/compile.md
Auth Expert -- inactive
PR does not touch auth flows, token management, credential resolution, or AuthResolver inputs; compile engine reads local ~/.apm/apm_modules only.
Doc Writer
-
[recommended] install-packages.md does not mention that apm install -g auto-compiles global root context files at
docs/src/content/docs/consumer/install-packages.md
A consumer reading install-packages.md sees 'apm install -g (package)' but has no signal that root context files (/.claude/CLAUDE.md etc.) are written as a side effect. This is a behavioural change visible to users./.claude/CLAUDE.md, ~/.codex/AGENTS.md, etc.) from any global instructions in the installed packages. Re-run manually with 'apm compile -g'. See Global compilation."
Suggested: Add a note under the 'apm install -g' section: "After a global install, APM automatically compiles user-scope root context files ( -
[nit] producer/compile.md Global section uses apply_to: but the rest of the page uses applyTo: (camelCase frontmatter convention) at
docs/src/content/docs/producer/compile.md
Suggested: Change 'apply_to:' to 'applyTo:' on the relevant line. -
[nit] producer/compile.md global paths list silently omits opencode (~/.config/opencode/AGENTS.md) at
docs/src/content/docs/producer/compile.md
The source docstring explicitly lists opencode with its non-standard XDG path; leaving it out makes the 'etc.' harder to interpret. -
[nit] reference/cli/compile.md global flag hardcodes
/.apm/apm_modules but the path is dynamically resolved via get_apm_dir(InstallScope.USER) which may honour XDG_DATA_HOME at/.apm/apm_modules' with 'the user-scope apm_modules directory (default: ~/.apm/apm_modules)'.docs/src/content/docs/reference/cli/compile.md
Suggested: Replace '
Test Coverage Expert
-
[recommended] No integration-with-fixtures test proves apm install -g writes CLAUDE.md to the user home directory
All 39 new tests mock compile_user_root_contexts at the boundary -- none drive a real apm install -g invocation against a fixture package tree and assert the file appears on disk. The install pipeline tier floor is integration-with-fixtures.
Proof (missing):tests/integration/test_global_install_e2e.py::test_install_global_writes_user_root_context_claude_md-- proves: Running 'apm install -g' with a package that has global instructions writes ~/.claude/CLAUDE.md containing the APM-generated marker [secure-by-default]
assert (fake_home / '.claude' / 'CLAUDE.md').exists() and '<!-- Generated by APM CLI' in (fake_home / '.claude' / 'CLAUDE.md').read_text() -
[recommended] CLAUDE_CONFIG_DIR env-var override path in _resolve_deploy_root has no test in the new compilation engine at
tests/unit/compilation/test_user_root_context.py
Grepped tests/unit/compilation/test_user_root_context.py for 'CLAUDE_CONFIG_DIR' -- zero hits. If CLAUDE_CONFIG_DIR handling in for_scope silently regresses, no test in the new suite would catch it.
Proof (missing):tests/unit/compilation/test_user_root_context.py::test_claude_config_dir_respected_as_deploy_root-- proves: When CLAUDE_CONFIG_DIR is set, compile_user_root_contexts writes CLAUDE.md into the custom directory
monkeypatch.setenv('CLAUDE_CONFIG_DIR', str(custom_dir)); assert result[0]['path'] == custom_dir / 'CLAUDE.md' -
[nit] Overwrite-guard test does not assert that APM-generated file is updated when instructions change (idempotency round-trip) at
tests/unit/compilation/test_user_root_context.py
Proof (missing):tests/unit/compilation/test_user_root_context.py::test_apm_generated_file_updated_when_instructions_change-- proves: A file written by APM on a first compile is overwritten on a second compile when instructions change
assert second_result[0]['status'] == 'written' and output_path.read_text() != first_content
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 #1632 · sonnet46 9M · ◷
github-actions
Bot
removed
the
panel-review
APM Review Panel:
|
| Persona | B | R | N | Takeaway |
|---|---|---|---|---|
| Python Architect | 0 | 0 | 2 | Clean new compilation module; frozen dataclass compatibility shim is harmless. |
| CLI Logging Expert | 0 | 0 | 2 | Output UX is solid; direct rich-helper usage is a bounded future cleanup. |
| DevX UX Expert | 0 | 0 | 1 | Familiar -g semantics, actionable errors, quiet install auto-compile. |
| Supply Chain Security Expert | 0 | 0 | 0 | Path containment hardening folded; marker overwrite protection is sound. |
| OSS Growth Hacker | 0 | 0 | 1 | Strong story-shaped feature; quickstart cross-link can follow later. |
| Doc Writer | 0 | 0 | 0 | Compile/install docs are now accurate across producer, reference, and consumer ramps. |
| Test Coverage Expert | 0 | 0 | 0 | Critical surfaces have unit plus integration coverage; mutation-break confirmed. |
| Performance Expert | 0 | 0 | 2 | Pure local filesystem work; no network or hot-path regression. |
B = blocking-severity findings, R = recommended, N = nits.
Counts are signal strength, not gates. The maintainer ships.
Recommendation
Merge as-is. CI is green, mergeability is clean aside from normal review requirements, and every in-scope panel follow-up was folded into this PR.
Folded in this run
- (panel) Added integration-with-fixtures coverage for
apm compile --global-- resolved in 2b04778. - (panel) Switched invalid
--globalflag combinations to ClickUsageError-- resolved in 2b04778. - (panel) Introduced typed
UserRootCompileResultfor user-root compile results -- resolved in 2b04778. - (panel) Documented
install -gauto-compilation in the install reference -- resolved in 2b04778. - (panel) Reframed the producer global-compile section with the user benefit first -- resolved in 2b04778.
- (panel) Added quick-copy
apm compile -greference example -- resolved in 90dc37b. - (panel) Named written targets in verbose install finalize output -- resolved in 90dc37b.
- (panel) Added user-root path containment before root-file writes -- resolved in 14564c4.
- (panel) Added symlink-escape regression coverage for user-root writes -- resolved in 14564c4.
- (panel) Surfaced global install auto-compilation in the consumer install guide -- resolved in 14564c4.
Copilot signals reviewed
No copilot-pull-request-reviewer[bot] inline comments were present after two fetch rounds.
Deferred (out-of-scope follow-ups)
- (panel) Cross-link first-package/quickstart to the new global workflow -- scope boundary: PR scope is the compile/install -g behavior and directly related docs; quickstart funnel reshaping is a separate onboarding conversion pass.
Regression-trap evidence (mutation-break gate)
tests/integration/test_compile_global.py::test_compile_global_writes_claude_md_from_real_fixtures-- deleted_handle_global_flagcall tocompile_user_root_contexts; test FAILED as expected; guard restored.tests/unit/compilation/test_user_root_context.py::TestSymlinkEscape::test_output_symlink_escape_returns_error-- deletedensure_path_within(deploy_root / root_filename, deploy_root)guard; test FAILED as expected; guard restored.
Lint contract
uv run --extra dev ruff check src/ tests/ and
uv run --extra dev ruff format --check src/ tests/ both silent.
CI
All PR checks green on head 14564c4c58e8a9106158f0ed833fc38908fbf724: Lint, Build & Test shards, APM Self-Check, PR Binary Smoke, CodeQL, Spec conformance, NOTICE drift, Merge Gate, and CLA all succeeded (deploy skipped as expected) after 0 CI fix iteration(s).
Mergeability status
Captured from gh pr view 1632 --json mergeable,mergeStateStatus,statusCheckRollup immediately after the last push of this run.
| PR | head SHA | CEO stance | iters | folds | defers | Copilot rounds | CI | mergeable | mergeStateStatus | notes |
|---|---|---|---|---|---|---|---|---|---|---|
| #1632 | 14564c4 |
ship_now | 2 | 10 | 1 | 2 | green | MERGEABLE | BLOCKED | pending required review |
Convergence
2 outer iteration(s); 2 Copilot round(s). Final panel verdict: ship_now.
Ready for maintainer review.
Full per-persona findings
Python Architect
- [nit] UserRootCompileResult carries dict-style compatibility methods that can be simplified to attribute access in a future cleanup.
- [nit] Duplicate lazy import in finalize.py can be de-duplicated later.
CLI Logging Expert
- [nit]
_handle_global_flagbypasses CommandLogger; acceptable as a small early-return branch. - [nit] Default install output remains quiet about root-file writes; docs now cover the side effect.
DevX UX Expert
- [nit] Empty global-compile output could later mention scoped-vs-global instructions.
Supply Chain Security Expert
No findings after path-containment hardening was folded.
OSS Growth Hacker
- [nit] Quickstart cross-link to global compilation would improve second-use conversion.
Doc Writer
No findings after consumer guide coverage was folded.
Test Coverage Expert
No findings.
Performance Expert
- [nit] Content generation could be hoisted per target in a micro-optimization.
- [nit] Install finalize could avoid re-discovery in a future perf cleanup.
This panel is advisory. It does not block merge. Re-apply the panel-review label after addressing feedback to re-run.
APM Review Panel:
|
| Persona | B | R | N | Takeaway |
|---|---|---|---|---|
| Python architect | 0 | 1 | 0 | Clean module decomposition; project-scope marker alignment is a follow-up, not this PR. |
| CLI logging expert | 0 | 0 | 0 | Output surface is clean after CommandLogger, dry-run, and symbol folds. |
| DevX UX expert | 0 | 0 | 0 | Command surface is predictable after allowlist and --verbose folds. |
| Supply chain security expert | 0 | 0 | 1 | User-scope writes now scan content, validate paths, and use anchored marker ownership. |
| OSS growth hacker | 0 | 0 | 0 | Trust story is strong: install never silently mutates machine-wide context. |
| Doc writer | 0 | 0 | 0 | Docs are concise and truthful after final security wording patch. |
| Test coverage expert | 0 | 0 | 0 | Critical surfaces have unit and integration regression traps; mutation-break confirmed. |
| Performance expert | 0 | 0 | 0 | No meaningful install hot-path regression; compile -g is explicit and bounded. |
B = highest signal, R = recommended, N = nits. Counts are signal strength, not gates. The maintainer ships.
Top 1 follow-up
- [Python architect] Align project-scope marker ownership to the stricter anchored first-nonblank-line check introduced here -- this reduces contributor confusion and closes a safety-contract consistency gap outside this PR's user/global compile scope.
Recommendation
Ship with that follow-up tracked separately. The current PR fixes the issue, preserves user consent for global writes, defends hand-authored root context files, and is green on CI.
Reservations carried from strategic-alignment
- PR is large (+2091/-1 across 15 files) for a -g compile gap; watch for scope-creep beyond the install-time instruction-compile fix -- addressed by keeping folds inside the explicit compile/install-hint surface and deferring project-scope marker alignment.
- Root context file overwrite safety: the generated-marker rule must prevent clobbering a hand-authored CLAUDE.md/AGENTS.md/GEMINI.md -- addressed by
669ec87with anchored marker ownership plus regression tests.
Folded in this run
- (panel) Rebuilt the PR on current
origin/mainafter historical add/add conflicts made the original rebase intractable -- resolved ine06c62b. - (panel) Changed dry-run global compile summary to info styling and added the missing error symbol -- resolved in
669ec87. - (panel) Replaced the
--globaldenylist with an explicit allowlist and updated help text -- resolved in669ec87andfc9609f. - (panel) Routed the global compile handler through
CommandLoggerfrom the CLI path and allowed--verbose-- resolved infc9609f. - (panel) Added
SecurityGate.scan_textbefore user root context writes and documented the global compile security behavior -- resolved in669ec87andfc9609f. - (panel) Anchored generated-marker ownership to the first nonblank line so quoted markers do not clobber hand-authored files -- resolved in
669ec87. - (panel) Added unit and integration regression traps for hand-authored root context preservation -- resolved in
669ec87.
Copilot signals reviewed
review:4416207964-- NOT-LEGIT: summary-only review body had no actionable inline finding after the final diff; inline comment list was empty.
Deferred (out-of-scope follow-ups)
- (panel) Align project-scope
agents_compilermarker ownership to the stricter anchored check -- scope boundary: this PR is the user/global compile and install-hint surface; project-scope managed-section semantics are a separate existing contract.
Regression-trap evidence (mutation-break gate)
tests/unit/compilation/test_user_root_context.py::TestSkippedHandAuthored::test_hand_authored_marker_mention_not_first_line-- deletedexisting.lstrip().startswith(_COPILOT_ROOT_GENERATED_MARKER)by replacing it with substring membership; test FAILED as expected; guard restored.tests/integration/test_compile_global.py::test_compile_global_preserves_hand_authored_claude_md-- deletedexisting.lstrip().startswith(_COPILOT_ROOT_GENERATED_MARKER)by replacing it with substring membership; test FAILED as expected; guard restored.
Lint contract
uv run --extra dev ruff check src/ tests/ and uv run --extra dev ruff format --check src/ tests/ both silent. Additional guards also passed: pylint R0801 rated 10.00/10 and scripts/lint-auth-signals.sh reported clean.
CI
gh pr checks 1632 --repo microsoft/apm --watch observed all checks pass on fc9609f: CI shards, Lint, CodeQL, docs build, NOTICE drift, spec conformance, merge gate, binary smoke, APM self-check, coverage combine, and CLA (0 CI fix iterations).
Mergeability status
Captured from gh pr view 1632 --json mergeable,mergeStateStatus,statusCheckRollup immediately after the last push of this run.
| PR | head SHA | CEO stance | iters | folds | defers | Copilot rounds | CI | mergeable | mergeStateStatus | notes |
|---|---|---|---|---|---|---|---|---|---|---|
| #1632 | fc9609f |
ship_with_followups | 2 | 7 | 1 | 2 | green | MERGEABLE | BLOCKED | pending required review |
Convergence
2 outer iteration(s); 2 Copilot round(s). Final panel stance: ship_with_followups.
Ready for maintainer review.
Full per-persona findings
- Python architect: recommended follow-up to align project-scope marker ownership later; not in this PR scope.
- CLI logging expert: no remaining findings after folds.
- DevX UX expert: no remaining findings after folds.
- Supply chain security expert: nit-level project-scope marker consistency follow-up; user-scope path is safe.
- OSS growth hacker: no ship-time finding; release comms can amplify explicit-consent global writes.
- Auth expert: inactive; no auth/token/credential/remote-host surface touched.
- Doc writer: no remaining findings after final security wording patch.
- Test coverage expert: no remaining findings; 70 targeted tests passed and mutation-break failed as expected.
- Performance expert: no install hot-path concern.
This panel is advisory. It does not block merge. Re-apply the panel-review label after addressing feedback to re-run.
…ile hint Narrow PR #1632 per maintainer triage on #1485. apm compile is a cross-harness transform of the instructions primitive into a root context file (AGENTS.md/CLAUDE.md/GEMINI.md). Auto-running it on apm install -g is rejected: it mutates global agent context for every future run on the machine without explicit consent. - Remove the auto-compile call from the install finalize phase. - Add a read-only, target-aware hint: when global instructions land on a root-context-only target (agents/gemini/claude families), print one [i] line pointing at 'apm compile -g'. No file is written on install. - Directory-native targets (Cursor/Windsurf/Kiro/Claude rules) and Copilot already surface global instructions natively, so the hint is suppressed for them. - Extract discover_global_instructions() shared by the compile engine and the install hint to avoid duplication. - Keep the explicit apm compile --global/-g flag and engine intact. - Update tests, docs, and CHANGELOG to describe explicit compile + the install hint, not auto-compile. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
danielmeppiel
changed the title
apm compile -g + install-time hint for global root context
Tightens the explicit global compile surface after panel review: dry-run output now uses info styling, --global rejects every explicitly supplied project-output flag via an allowlist, user root context writes scan generated content before writing, and marker ownership is anchored to the file header so quoted markers cannot clobber hand-authored files. Also updates security/install docs and adds regression traps for hand-authored root context preservation. Addresses panel follow-ups from cli-logging, DevX UX, supply-chain security, doc-writer, and test coverage. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
danielmeppiel
force-pushed
the
danielmeppiel/issue-1485
branch
from
25ea83b to
669ec87
Compare
Allows --verbose with the explicit global compile path, routes the handler through CommandLogger when called from the CLI, and keeps the security documentation precise about compile -g scan reporting. Addresses final panel follow-ups from CLI logging, DevX UX, and doc-writer. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Block critical hidden-character output in the new global compile path before writing root context files, reduce default output noise, and keep install hints scoped to verified root-context targets. Adds regression coverage for the security, dry-run, missing apm_modules, and logger-routing contracts raised by the shepherd panel. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
APM Review Panel: ship_now
cc @danielmeppiel @sergio-sisternes-epam -- a fresh advisory pass is ready for your review. The panel's highest-signal concern was the global compile security path: critical hidden-character findings were previously warning-only. That is now folded in this PR: global compile scans before write, stops critical output, and exits non-zero. The install hint remains read-only and scoped to verified root-context targets; directory-native/native-user targets no longer get a misleading hint. Output noise was reduced by moving unchanged/no-instruction per-target lines to verbose detail. Aligned with: secure by default: critical findings stop user root-context writes; pragmatic as npm: Panel summary
Folded in this run
Copilot signals reviewedNo Copilot inline comments were present in either shepherd fetch round. The existing Copilot review body was stale from the earlier auto-compile version and had no actionable inline findings to fold. DeferredNone. Validation
Mergeability
This panel is advisory. It does not block merge. Re-apply the |
TL;DR
Adds an explicit
apm compile --global/-gtransform that compiles global (apply_to-less) instructions into user-scope root context files (~/.claude/CLAUDE.md,~/.codex/AGENTS.md,~/.gemini/GEMINI.md, ...). Onapm install -g, when global instructions land on a root-context-only target, install prints a single read-only[i]hint pointing atapm compile -g. No root context file is written on install -- compilation stays explicit.Resolves #1485.
Problem (WHY)
apm compileis a cross-harness transform of theinstructionsprimitive into a single root context file (AGENTS.md / CLAUDE.md / GEMINI.md). An earlier revision of this PR ran that transform automatically onapm install -g. Per maintainer triage on #1485, that install-time side effect is rejected:Two facts narrow the real need:
.cursor/rules/), Windsurf (.windsurf/rules/), Kiro (steering), and Claude (.claude/rules/) surface global instructions onapm install -gvia per-file instruction transforms. Copilot deploys global instructions natively at user scope too.Approach (WHAT)
Keep the explicit transform.
apm compile --global/-g(mutually exclusive with--watch/--root/ project-output flags) and thecompile_user_root_contextsengine are retained.Drop auto-compile-on-install. The finalize phase no longer compiles root context files.
Add a target-aware, read-only hint. On
apm install -g, when both (a) at least one installed package carries global instructions and (b) at least one active target is root-context-only, print exactly one line:[i] Global instructions installed. Run 'apm compile -g' to surface them in root context files (AGENTS.md/CLAUDE.md/GEMINI.md) for: <comma-separated target names>.The hint is suppressed when only directory-native targets are active, when no global instructions were installed, and on
--dry-run.Implementation (HOW)
src/apm_cli/compilation/user_root_context.py: extracteddiscover_global_instructions(source_root, *, logger=None)-- shared by the compile engine and the install hint (avoids R0801 duplication). Engine behavior unchanged.src/apm_cli/install/phases/finalize.py: removed_compile_user_root_contexts_after_install; added_hint_global_root_context(ctx)gated on_ROOT_CONTEXT_ONLY_FAMILIES = {agents, claude, gemini}(vscode/copilot deliberately excluded -- native user-scope deploy). Imports kept lazy to avoid import cycles.run()only hints for USER scope); rewrote the first integration test to assertapm install -g --target claudewrites noCLAUDE.mdand prints the hint; kept theapm compile --globalintegration test; added adiscover_global_instructionsunit test.consumer/install-packages.md,reference/cli/install.md,reference/cli/compile.md,producer/compile.md,CHANGELOG.md) to describe explicitapm compile -g+ the install hint.Behavior
flowchart TD A["apm install -g <pkg>"] --> B{Global instructions installed?} B -- No --> Z[No hint] B -- Yes --> C{Any active target root-context-only?<br/>agents / gemini / claude} C -- No (directory-native / copilot only) --> Z C -- Yes --> D["Print one [i] hint:<br/>Run 'apm compile -g' ..."] D --> E[No file written on install] F["apm compile -g (explicit)"] --> G[Write user-scope root context files]Trade-offs
Validation evidence
CI-mirror lint chain green locally:
ruff check,ruff format --check, pylint R0801 (10.00/10), andscripts/lint-auth-signals.shall pass. Affected suites:pytest tests/unit/compilation tests/unit/install tests/integration/test_compile_global.py-> 2825 passed.How to test
apm-spec-waiver: install finalize hint is read-only console output (writes no artifact); PR net-removes the auto-compile install write and adds no normative install-path behaviour
Mode B detector waiver: the install-scope
_hint_global_root_contextnudge prints one[i]line and writes nothing. This PR's net effect on the OpenAPMinstallcritical path is the removal of the prior auto-compile write, so there is no observable normative behaviour delta to cite. Reviewer-auditable per CONTRIBUTING.md "Mode B (silent extension)".