fix(compile): live-reload apm.yml and warn on --clean --watch

[edenfunf]

fix(compile): live-reload apm.yml and warn on --clean --watch

TL;DR

Two behaviors #1349 left on the table when it closed #1345, picked up at @danielmeppiel's invitation in #1351 (comment):

1. apm compile --watch re-reads apm.yml when apm.yml itself is modified. Editing target: / targets: mid-watch now takes effect on the next file event instead of needing a watcher restart. Pre-fix the value resolved at startup was reused for every recompile, so mid-session edits silently did nothing.
2. apm compile --watch --clean prints an explicit [!] warning that --clean is ignored, then continues. Pre-fix the flag was silently dropped. Running --clean on every recompile would surprise users mid-session by deleting orphans; running it only on the initial compile would re-introduce a watcher-specific code path — the same trap fix(compile): forward target to watch-mode recompile (closes #1345) #1349 exists to remove. To clean orphaned outputs, run apm compile --clean separately between watch sessions.

Design notes I expect a reviewer to ask about

• Re-resolution is gated to the file that can change the answer. _recompile only calls _resolve_effective_target when os.path.basename(changed_file) == APM_YML_FILENAME. Instruction-file edits keep using the startup snapshot, so .instructions.md edits don't pay an extra resolver round-trip. Basename equality (not endswith) so a stray backup_apm.yml cannot masquerade as the project root manifest.
• CLI --target still wins over apm.yml. Mid-session edits to apm.yml's targets: are ignored when the watcher was launched with --target X, matching the one-shot path's priority order. The resolver receives the raw cli_target and applies the same precedence rules. Pinned by test_recompile_on_apm_yml_change_with_cli_target_keeps_cli_priority.
• target_label_user and cli_target carry the same value at the call site but have different roles. target_label_user (paired with target_label_config) feeds the startup Compiling for ... label; cli_target is the resolver input on apm.yml change. Kept distinct so the label path and the re-resolve path don't accidentally fuse.
• No new per-recompile log line. When apm.yml changes and the resolved target shifts, the watcher does not print a "now compiling for X" diff. Surfacing the change would require diffing the previous resolution and add visual noise on every recompile. Users see the change via the output files appearing / disappearing. If reviewers want a target-changed signal we can add it as a follow-up; deferred to keep this PR minimal.
• Lazy import (from .cli import _resolve_effective_target) inside _recompile to break the cli.py → watcher.py → cli.py cycle. Same approach fix(compile): --watch path honors apm.yml targets and --target flag #1351 documented.

How to test

--clean --watch warning

apm compile --watch --clean
# Prints: [!] --clean is ignored in watch mode; run 'apm compile --clean'
#         separately to remove orphaned outputs.
# ...then proceeds with normal watch (no destructive cleanup mid-session).

Mid-session apm.yml reload (both directions)

mkdir -p /tmp/repro/.apm/instructions
cd /tmp/repro
cat > apm.yml <<'EOF'
name: Repro
version: 1.0.0
targets:
- claude
EOF
cat > .apm/instructions/style.instructions.md <<'EOF'
---
description: style
applyTo: "**/*.py"
---
snake_case.
EOF

apm compile --watch &
# 1. Initial: only CLAUDE.md exists.
# 2. Edit apm.yml to `targets: [claude, gemini]` (atomic save, e.g. PowerShell
#    Set-Content or `vim :w`).  Watcher logs `File changed: ./apm.yml` and the
#    next recompile emits AGENTS.md + CLAUDE.md + GEMINI.md.
# 3. Edit apm.yml back to `targets: [claude]`.  Next recompile emits only
#    CLAUDE.md; the previously-written AGENTS.md / GEMINI.md are left on
#    disk untouched (--clean is intentionally separate; see above).

End-to-end run on Windows 11 + watchdog confirmed the watcher log shows [>] File changed: .\apm.yml followed by the correct emission set in both directions; mtimes confirm only the freshly-relevant outputs are rewritten.

Files changed

• src/apm_cli/commands/compile/cli.py — --clean --watch warning in the if watch: branch; forward raw cli_target=target into _watch_mode.
• src/apm_cli/commands/compile/watcher.py — APMFileHandler stores cli_target; _recompile re-resolves when the changed file's basename is apm.yml; _watch_mode plumbs cli_target through; docstring updated to describe the snapshot-vs-fresh contract.
• tests/unit/commands/compile/test_watch_live_reload_and_clean_warning.py — six tests:
  • test_recompile_on_apm_yml_change_reresolves_against_current_file — core reload behavior.
  • test_recompile_on_instruction_file_change_uses_snapshot — non-apm.yml edits skip the resolver round-trip.
  • test_recompile_on_lookalike_filename_does_not_reresolve — backup_apm.yml and friends must NOT trigger reload (basename, not endswith).
  • test_recompile_on_apm_yml_change_with_cli_target_keeps_cli_priority — explicit --target outranks mid-session apm.yml edits.
  • test_clean_watch_emits_warning_and_does_not_run_clean — warning fires, watcher still launches.
  • test_watch_without_clean_does_not_emit_clean_warning — positive control.
  • Toggle-verified: reverting either fix on the current branch makes the corresponding tests fail with assertion messages that name the regression they pin.
• CHANGELOG.md — ### Changed entries under [Unreleased].

Related

• Follow-up to fix(compile): forward target to watch-mode recompile (closes #1345) #1349 (closed [BUG] apm compile --watch regenerates GEMINI.md when targets is [claude, cursor] — regression of #1019 in watch code path #1345).
• Behaviors invited explicitly by @danielmeppiel after closing fix(compile): --watch path honors apm.yml targets and --target flag #1351.

[Copilot]

Pull request overview

This PR improves apm compile --watch parity with one-shot compile by (1) re-resolving targets when apm.yml itself changes and (2) surfacing an explicit warning when --clean is used with --watch (since --clean is ignored in watch mode).

Changes:

• Re-resolve the effective compile target when the changed file is apm.yml, so mid-session targets: edits can take effect.
• Emit a warning for apm compile --watch --clean instead of silently dropping --clean.
• Add unit tests covering live reload gating behavior and the --clean warning, plus update the Unreleased changelog.

Reviewed changes

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

File	Description
src/apm_cli/commands/compile/cli.py	Adds a --clean+--watch warning and plumbs raw --target (cli_target) into watch mode.
src/apm_cli/commands/compile/watcher.py	Stores cli_target and conditionally re-resolves the effective target when apm.yml changes.
tests/unit/commands/compile/test_watch_live_reload_and_clean_warning.py	New regression tests for apm.yml-triggered re-resolution and the --clean watch-mode warning behavior.
CHANGELOG.md	Adds two Unreleased “Changed” entries describing the new watch-mode behavior.

Comments suppressed due to low confidence (1)

src/apm_cli/commands/compile/watcher.py:167

• The _watch_mode docstring says recompiles re-run the resolver against the current apm.yml, but the implementation only re-resolves when the changed file’s basename is exactly apm.yml (instruction-file events explicitly skip it). Please adjust the wording to reflect the actual gating (and, if you persist the updated target, mention the caching behavior).

    ``cli_target`` is the raw ``--target`` argument; recompiles re-run
    the resolver against the current apm.yml so mid-session edits to
    ``targets:`` take effect on the next file event without restarting
    the watcher.

[github-actions]

APM Review Panel: ship_with_followups

fix(compile): apm.yml target changes now apply instantly mid-watch (no restart), and --clean --watch warns instead of silently misbehaving -- two long-overdue DX correctness fixes.

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

PR #1403 closes two distinct watch-mode defects: the silent target-staleness bug that forced users to kill and restart the watcher after editing apm.yml, and the silent swallow of --clean --watch that left users confused about orphan-output behavior. Both fixes are narrow, well-tested at the unit tier, and the CHANGELOG and docs land alongside the code -- all consistent with APM's ship-fast, communicate-clearly operating principle.

The panel's strongest convergent signal is the missing graceful fallback when apm.yml becomes unparseable mid-watch: three independent panelists (devx-ux-expert, supply-chain-security-expert, and test-coverage-expert backed by a missing-evidence block on a resilience-critical surface) raised it. That convergence elevates it above the individual recommended tier; it is a correctness gap on the watch-loop's resilience contract, not a style concern.

The thread-safety gap (no lock around self.effective_target mutation) is real but structurally low-risk given the existing 1-second debounce serializes the vast majority of rapid-fire events in practice -- it warrants a follow-up issue, not a block. The doc-writer's blocking label on "at startup" ambiguity is a single-word doc-polish fix; it carries no code-correctness weight and should be resolved in a fast follow commit rather than gating the merge.

Dissent. doc-writer declared "at startup" ambiguity severity: blocking; devx-ux-expert classified it severity: nit. The substance is identical -- a three-word wording fix in compile.md. CEO sides with the nit classification for ship-gating purposes: the current wording is imprecise but not misleading enough to cause user harm.

Aligned with: Pragmatic as npm -- live-reload and a visible warning mirror the expectation that the tool does what the user intends without requiring manual intervention; editing apm.yml mid-session just works now. DevX -- both fixes eliminate silent failure modes: the first removes an invisible state-staleness footgun, the second surfaces an incompatibility that was previously swallowed, making the watch loop trustworthy enough to leave running unattended.

Growth signal. The "no restart required" story is a reusable beat with outsized conversion value in release notes and social copy. A short screen capture or GIF showing a target: edit in apm.yml taking effect on the next file save -- with zero terminal interaction -- removes a common objection from evaluators who hit the stale-target bug once and moved on. Recommend seeding this into the next minor release post.

Panel summary

Persona	B	R	N	Takeaway
Python Architect	0	2	1	Lazy import breaks a real cycle cleanly; in-place self.effective_target mutation is correct and tested; basename gate is better than endswith; one thread-safety gap if watchdog delivers concurrent events.
CLI Logging Expert	0	1	1	Warning renders correctly as [!] yellow; missing re-resolve log when apm.yml triggers reload is the only meaningful gap.
DevX UX Expert	0	2	2	Two solid UX fixes; one recommended gap: malformed apm.yml mid-watch has no graceful recovery path in _recompile, risking a silent watcher crash.
Supply Chain Security Expert	0	1	1	No blocking security regressions. basename guard is correct; lazy relative import is safe. One silent fail-open on YAML parse worth hardening.
OSS Growth Hacker	0	1	2	Solid DX fix that removes a frustrating restart-to-reload loop; CHANGELOG is accurate but undersells the "no restart needed" wow moment.
Doc Writer	1	2	1	Two recommended fixes: "at startup" is ambiguous (should be "when the watcher starts"), and --target priority rule needs one word of clarification for first-time readers.
Test Coverage Expert	0	2	1	7 new tests cover all stated promises at unit/CliRunner tier; two gaps remain: apm.yml-parse-error swallowing and list[str] cli_target threading through re-resolve.

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

Top 5 follow-ups

1. [DevX UX Expert + Supply Chain + Test Coverage] Add try/except around _resolve_effective_target in the apm.yml event handler; warn user and keep previous target on parse error; add missing unit test asserting error is surfaced not swallowed. -- Three-panelist convergence on a user-promise gap backed by a missing-evidence block on a resilience-critical surface; silently dropping config mid-watch is the most impactful correctness risk in the PR.
2. [Doc Writer] Change "a [!] warning is printed at startup" to "a [!] warning is printed when the watcher starts" in compile.md. -- Fast single-word fix that removes genuine first-reader ambiguity; labeled blocking by doc-writer and should ship in the same PR or an immediate follow commit.
3. [CLI Logging Expert] Emit a progress line before _resolve_effective_target so users know targets are being re-read when apm.yml changes (not just "Recompiling..."). -- Closes a UX visibility gap that leaves the user unable to confirm live-reload actually fired; directly reinforces the "no restart needed" story.
4. [DevX UX Expert] Extend the --clean --watch warning to include a one-sentence explanation of WHY the two flags are incompatible. -- Without the causal "why", users will keep retrying the combined form; the explanation is also the seed for a FAQ entry.
5. [Python Architect] Add threading.Lock in __init__ and wrap the self.effective_target read-modify-write block; open a follow-up issue tracking the race window. -- Real race exists even with debounce on rapid burst events; low-probability but the fix is two lines and the debt should not accumulate.

Architecture

classDiagram
    direction LR
    class APMFileHandler {
        <<EventHandler>>
        +output str
        +chatmode str|None
        +effective_target CompileTargetType|None
        +cli_target str|list|None
        +debounce_delay float
        +on_modified(event) None
        +_recompile(changed_file) None
    }
    class FileSystemEventHandler {
        <<watchdog>>
        +on_modified(event) None
    }
    class CommandLogger {
        <<Logger>>
        +progress(msg) None
        +warning(msg) None
    }
    class CompilationConfig {
        <<ValueObject>>
        +from_apm_yml(...) CompilationConfig
    }
    class AgentsCompiler {
        +compile(config, logger) CompileResult
    }
    class _resolve_effective_target {
        <<PureFunction>>
        +__call__(cli_target) tuple
    }
    FileSystemEventHandler <|-- APMFileHandler : extends
    APMFileHandler *-- CommandLogger : owns
    APMFileHandler ..> CompilationConfig : creates
    APMFileHandler ..> AgentsCompiler : creates
    APMFileHandler ..> _resolve_effective_target : lazy import on apm.yml event
    class APMFileHandler:::touched
    class _resolve_effective_target:::touched
    classDef touched fill:#fff3b0,stroke:#d47600

Loading

flowchart TD
    A(["apm compile --watch"])
    B{"--clean flag?"}
    C["logger.warning: --clean ignored in watch mode"]
    D["_resolve_effective_target(target)"]
    E["_watch_mode(cli_target=target)"]
    F["APMFileHandler.__init__\nself.effective_target = snapshot\nself.cli_target = cli_target"]
    G["Observer.start() -- watchdog thread"]
    H(["File event arrives"])
    I{"basename(changed_file)\n== apm.yml?"}
    J["lazy import _resolve_effective_target\nre-reads apm.yml from disk"]
    K["self.effective_target = fresh_target\n(no lock -- see finding)"]
    L["CompilationConfig.from_apm_yml(target=effective_target)"]
    M["AgentsCompiler.compile(config)"]
    N(["logger.success / logger.error"])
    A --> B
    B -- yes --> C
    C --> D
    B -- no --> D
    D --> E
    E --> F
    F --> G
    G --> H
    H --> I
    I -- yes --> J
    J --> K
    K --> L
    I -- no --> L
    L --> M
    M --> N

Loading

Recommendation

Ship PR #1403. The two behavioral fixes are correct, tested, documented, and CHANGELOG'd. No panelist identified a blocking code correctness regression. The panel's highest-signal gap -- missing graceful fallback on malformed apm.yml -- is a hardening improvement, not a regression introduced by this PR (the watcher had no apm.yml event handling at all before this change). Track it as a priority follow-up issue alongside the doc wording fix and the --clean warning copy improvement. The thread-safety gap warrants an issue but not a block. Merge, then iterate fast.

---

Full per-persona findings

Python Architect

• [recommended] self.effective_target mutated without a lock; concurrent watchdog events can race at src/apm_cli/commands/compile/watcher.py
  Watchdog dispatches events on a background thread. If two rapid apm.yml events arrive and the debounce guard is satisfied, two _recompile calls can interleave. Python's GIL serialises bytecode but not the read-modify-write sequence across the two attribute accesses.
  Suggested: Add threading.Lock in __init__ and wrap the if os.path.basename(changed_file) == APM_YML_FILENAME: block with with self._target_lock: to serialise target updates.

• [recommended] Lazy import in _recompile re-executes on every apm.yml event; module docstring lacks cycle explanation at src/apm_cli/commands/compile/watcher.py
  Python caches modules after first import so the performance hit is negligible, but the cycle is real and should be documented in the module docstring for future refactors.
  Suggested: Add a module-level note: # cli.py imports this module; _resolve_effective_target is imported lazily in _recompile to break the cycle.

• [nit] os.path.basename could be Path(changed_file).name for pathlib consistency at src/apm_cli/commands/compile/watcher.py
  Minor style nit; both are correct.

CLI Logging Expert

• [recommended] No progress message when apm.yml triggers target re-resolution at src/apm_cli/commands/compile/watcher.py
  User sees "File changed: apm.yml" and "Recompiling..." but never learns that targets were re-read from disk. If the resolve picks up a new target, the output changes with no explanation.
  Suggested: Emit self.logger.progress('apm.yml changed -- re-resolving targets...', symbol='gear') before calling _resolve_effective_target.

• [nit] Warning message leads with a flag name rather than the outcome at src/apm_cli/commands/compile/cli.py
  APM message rule: lead with the outcome.
  Suggested: Rephrase to: "Orphaned outputs will not be removed mid-session -- --clean is ignored in watch mode. Run 'apm compile --clean' separately between sessions."

DevX UX Expert

• [recommended] No graceful fallback when _resolve_effective_target throws mid-watch (malformed apm.yml) at src/apm_cli/commands/compile/watcher.py
  Any parse or FileNotFound error propagates up through the watchdog event handler, likely killing the watcher with a Python traceback. Correct behavior: warn and keep previous target.
  Suggested: Wrap _resolve_effective_target call in try/except, emit logger.warning with error summary and "keeping previous target", continue with self.effective_target unchanged.

• [recommended] Warning message omits WHY --clean is incompatible with --watch at src/apm_cli/commands/compile/cli.py
  Without the "why", users keep retrying the combined form thinking it is a transient issue.
  Suggested: Extend to: "--clean is ignored in watch mode (running it on every recompile would remove outputs mid-session); run 'apm compile --clean' separately between watch sessions."

• [nit] compile.md says "[!] warning is printed at startup" but fires before the watcher loop at docs/src/content/docs/reference/cli/compile.md
  Suggested: Change to "a [!] warning is printed before the watcher starts" for precision.

• [nit] --target priority rule only in watch-mode bullet, not alongside the --target flag description at docs/src/content/docs/reference/cli/compile.md
  Suggested: Add a parenthetical to the --target flag description: "(in --watch mode, outranks apm.yml targets: on every recompile)".

Supply Chain Security Expert

• [recommended] Silent exception swallow on YAML parse in _resolve_effective_target produces fail-open behavior mid-watch at src/apm_cli/commands/compile/cli.py
  cli.py swallows all exceptions during the parse_targets_field / load_yaml path with bare except Exception: pass. If apm.yml is partially written mid-watch, the resolver silently drops config_target to None and falls back to auto-detect with no user warning.
  Suggested: Replace except Exception: pass with except Exception as exc: logger.warning(f'apm.yml target parse failed: {exc}; falling back to auto-detect').

• [nit] on_modified endswith filter admits backup_apm.yml events; basename guard downstream is correct but inconsistent at src/apm_cli/commands/compile/watcher.py
  Suggested: Change to os.path.basename(src_path) == APM_YML_FILENAME to match the basename guard in _recompile.

OSS Growth Hacker

• [recommended] CHANGELOG entry buries the lead -- "no restart needed" should open the sentence, not appear mid-clause
  Power users scanning release notes make a split-second judgment on whether to upgrade.
  Suggested: Rewrite to: "apm compile --watch no longer requires a restart when you edit target: / targets: in apm.yml -- changes take effect on the next file event. Previously the startup value was cached for the whole session. (fix(compile): live-reload apm.yml and warn on --clean --watch #1403)"

• [nit] Docs bullet phrased as caveat, not capability; could use a Starlight tip admonition
  Suggested: Wrap in :::tip admonition: "Tip -- edit apm.yml mid-watch: target: / targets: changes take effect automatically on the next save."

• [nit] --clean warning copy could seed a docs FAQ / error-message index
  Suggested: Add one FAQ bullet under a "Watch mode pitfalls" heading in compile.md.

Auth Expert -- inactive

No auth surface touched; changed files are scoped entirely to the compile --watch live-reload and --clean warning feature.

Doc Writer

• [blocking] "a [!] warning is printed at startup" is ambiguous -- "startup" could mean process startup, not watch invocation at docs/src/content/docs/reference/cli/compile.md
  A first-time reader could interpret "startup" as application/process startup. (Note: CEO weighs this as a nit for ship-gating purposes; it is a fast single-word fix, not a code correctness issue.)
  Suggested: Change to "a [!] warning is printed when the watcher starts".

• [recommended] The --target priority rule omits "over apm.yml" -- the override relationship is implicit at docs/src/content/docs/reference/cli/compile.md
  The word "still" implies the reader already knows the resolution order.
  Suggested: Drop "still" and write: "Passing --target to apm compile --watch takes precedence over apm.yml target:/targets:."

• [recommended] No doc for what happens when apm.yml has a parse/syntax error mid-watch at docs/src/content/docs/reference/cli/compile.md
  The new bullet documents the happy path but omits the error path.
  Suggested: Add: "If apm.yml is unparseable at the time of the file event, the recompile is skipped and an error is printed; the watcher continues running."

• [nit] CHANGELOG --clean entry is slightly longer than surrounding entries; trim remediation sentence at CHANGELOG.md
  Suggested: Trim to: "apm compile --watch --clean now prints a [!] warning that --clean is ignored in watch mode instead of silently dropping the flag. (fix(compile): live-reload apm.yml and warn on --clean --watch #1403)"

Test Coverage Expert

• [recommended] apm.yml syntax-error mid-watch is silently swallowed; no test asserts user sees error message rather than silence at tests/unit/commands/compile/test_watch_live_reload_and_clean_warning.py
  No test exercises the path where _resolve_effective_target raises. Probed tests/unit/commands/compile/ for YAMLError, ParseError, syntax, except -- zero hits.
  Proof (missing at unit): tests/unit/commands/compile/test_watch_live_reload_and_clean_warning.py::test_recompile_on_apm_yml_change_resolver_raises_logs_error_not_silent -- proves: If apm.yml becomes unparseable mid-watch, the user sees an error message rather than a silent no-op.

• [recommended] Live-reload certified only at unit tier; integration-with-fixtures floor not met for CLI-visible watch-mode behavior change at tests/unit/commands/compile/test_watch_live_reload_and_clean_warning.py
  Tests 1-5 mock all module boundaries. Tests 6-7 use CliRunner with real apm.yml but mock _watch_mode entirely.
  Proof (passed at unit): tests/unit/commands/compile/test_watch_live_reload_and_clean_warning.py::test_recompile_on_apm_yml_change_reresolves_against_current_file -- proves: apm.yml change mid-watch causes resolver to be called and fresh target forwarded to CompilationConfig. assert mock_from_apm_yml.call_args.kwargs["target"] == fresh

• [nit] cli_target as list[str] not exercised through the re-resolve path at tests/unit/commands/compile/test_watch_live_reload_and_clean_warning.py
  All five unit tests use cli_target=None or cli_target="claude". The list[str] branch goes through _resolve_effective_target untested.
  Proof (missing at unit): tests/unit/commands/compile/test_watch_live_reload_and_clean_warning.py::test_recompile_on_apm_yml_change_with_cli_target_list_keeps_cli_priority -- proves: apm compile --watch --target claude --target gemini re-resolves with list cli_target, not just single string.

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

Note

🔒 Integrity filter blocked 2 items

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

• #1403 pull_request_read: has lower integrity than agent requires. The agent cannot read data with integrity below "approved".
• fix(compile): live-reload apm.yml and warn on --clean --watch #1403 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 #1403 · ● 3.3M · ◷