fix(compile): live-reload apm.yml and warn on --clean --watch#1403
Open
edenfunf wants to merge 2 commits into
Open
fix(compile): live-reload apm.yml and warn on --clean --watch#1403edenfunf wants to merge 2 commits into
edenfunf wants to merge 2 commits into
Conversation
Two behaviors microsoft#1349 left on the table: 1. `apm compile --watch` re-runs target resolution against the current `apm.yml` when `apm.yml` itself is the file event source. Pre-fix the value resolved at startup was reused for every recompile, so mid-session edits to `target:` / `targets:` did nothing until the watcher was restarted. Re-resolution is gated to the file that can change the answer (basename match -- not `endswith` -- so a stray `backup_apm.yml` cannot masquerade as the project root manifest); instruction-file edits keep using the startup snapshot. 2. `apm compile --watch --clean` prints an explicit warning that `--clean` is ignored in watch mode, then continues. Pre-fix the flag was silently dropped. CLI `--target X` still outranks mid-session `apm.yml` edits, matching the one-shot path's priority order: the resolver receives the raw `cli_target` on every re-run and applies the same precedence rules. Lazy `from .cli import _resolve_effective_target` inside `_recompile` to break the cli -> watcher -> cli import cycle.
Contributor
There was a problem hiding this comment.
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-sessiontargets:edits can take effect. - Emit a warning for
apm compile --watch --cleaninstead of silently dropping--clean. - Add unit tests covering live reload gating behavior and the
--cleanwarning, 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_modedocstring says recompiles re-run the resolver against the currentapm.yml, but the implementation only re-resolves when the changed file’s basename is exactlyapm.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.
…ent watch contract Three review comments from PR microsoft#1403: 1. After an apm.yml-driven re-resolve, persist the fresh value to `self.effective_target` so subsequent non-apm.yml events do not silently revert to the startup snapshot. Without this, the sequence `apm.yml edit -> instructions edit` emits the new family set on the first recompile and the wrong family set on the second -- AGENTS.md / GEMINI.md written by the apm.yml event become stale until the next apm.yml edit. New test `test_apm_yml_change_persists_fresh_target_for_subsequent_events` pins the sequence end-to-end. 2. Append (microsoft#1403) to both CHANGELOG entries to match the project's one-line-per-PR Keep-a-Changelog convention. 3. Document the two new watch-mode behaviors in the CLI reference: apm.yml `target:` / `targets:` mid-session live-reload (with the CLI `--target` priority note) and the `--clean` warning.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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):
apm compile --watchre-readsapm.ymlwhenapm.ymlitself is modified. Editingtarget:/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.apm compile --watch --cleanprints an explicit[!]warning that--cleanis ignored, then continues. Pre-fix the flag was silently dropped. Running--cleanon 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, runapm compile --cleanseparately between watch sessions.Design notes I expect a reviewer to ask about
_recompileonly calls_resolve_effective_targetwhenos.path.basename(changed_file) == APM_YML_FILENAME. Instruction-file edits keep using the startup snapshot, so.instructions.mdedits don't pay an extra resolver round-trip. Basename equality (notendswith) so a straybackup_apm.ymlcannot masquerade as the project root manifest.--targetstill wins overapm.yml. Mid-session edits toapm.yml'stargets:are ignored when the watcher was launched with--target X, matching the one-shot path's priority order. The resolver receives the rawcli_targetand applies the same precedence rules. Pinned bytest_recompile_on_apm_yml_change_with_cli_target_keeps_cli_priority.target_label_userandcli_targetcarry the same value at the call site but have different roles.target_label_user(paired withtarget_label_config) feeds the startupCompiling for ...label;cli_targetis the resolver input on apm.yml change. Kept distinct so the label path and the re-resolve path don't accidentally fuse.from .cli import _resolve_effective_target) inside_recompileto break thecli.py → watcher.py → cli.pycycle. Same approach fix(compile): --watch path honors apm.yml targets and --target flag #1351 documented.How to test
--clean --watchwarningMid-session
apm.ymlreload (both directions)End-to-end run on Windows 11 + watchdog confirmed the watcher log shows
[>] File changed: .\apm.ymlfollowed 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 --watchwarning in theif watch:branch; forward rawcli_target=targetinto_watch_mode.src/apm_cli/commands/compile/watcher.py—APMFileHandlerstorescli_target;_recompilere-resolves when the changed file's basename isapm.yml;_watch_modeplumbscli_targetthrough; 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.ymland friends must NOT trigger reload (basename, notendswith).test_recompile_on_apm_yml_change_with_cli_target_keeps_cli_priority— explicit--targetoutranks mid-sessionapm.ymledits.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.CHANGELOG.md—### Changedentries under[Unreleased].Related