fix(applyTo): fold #1387 panel follow-ups (docs + DRY + YAML escape)#1449
Open
danielmeppiel wants to merge 1 commit into
Open
fix(applyTo): fold #1387 panel follow-ups (docs + DRY + YAML escape)#1449danielmeppiel wants to merge 1 commit into
danielmeppiel wants to merge 1 commit into
Conversation
Lands the panel recommendations on #1387 (closes #1366) that were filed as nits after the merge: - CHANGELOG: add Fixed entry under [Unreleased] documenting the comma-separated applyTo fix across Claude/Cursor/Windsurf, with the Copilot-verbatim carve-out spelled out (#1387, closes #1366). - docs(instructions-and-agents): demonstrate the comma-list contract with a worked example, document the brace-alternation carve-out, and clarify per-target rendering in "What compiles where". - docs(apm-usage/package-authoring): mirror the comma-list syntax note in the apm-usage skill resource (linting rule 4). - refactor(patterns): move _has_top_level_comma from context_optimizer into utils/patterns.py as has_top_level_comma (DRY: single source of truth for the brace-depth state machine). - security(integrators): emit YAML scalars via a new yaml_double_quote() helper that escapes backslashes, quotes, and control characters. Defence-in-depth against adversarial or copy-paste-mangled applyTo values; risk is LOW today (parse_apply_to strips whitespace and Windsurf strips newlines) but producer output is now well-formed even on hostile input. Added 11 unit tests including yaml.safe_load round-trip and a has_top_level_comma brace-aware suite. Lint silent; 175 tests pass (26 in test_patterns.py). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Folds review-panel follow-ups from #1387 by tightening applyTo comma-list behavior documentation, de-duplicating comma-vs-brace detection logic, and hardening YAML emission for instruction target frontmatter.
Changes:
- Document
applyTocomma-list vs brace-alternation semantics and per-target rendering (Copilot vs Claude/Cursor/Windsurf). - Refactor comma-discrimination into
utils.patterns.has_top_level_comma()and add unit coverage. - Add
yaml_double_quote()and apply it in instruction integrator YAML outputs to prevent malformed frontmatter on special characters.
Show a summary per file
| File | Description |
|---|---|
tests/unit/utils/test_patterns.py |
Adds unit tests for has_top_level_comma() and yaml_double_quote() (incl. yaml.safe_load round-trip). |
src/apm_cli/utils/patterns.py |
Introduces has_top_level_comma() and yaml_double_quote() alongside parse_apply_to(). |
src/apm_cli/integration/instruction_integrator.py |
Uses yaml_double_quote() when emitting globs: / paths: YAML for Cursor/Windsurf/Claude conversions. |
src/apm_cli/compilation/context_optimizer.py |
Replaces local _has_top_level_comma with shared has_top_level_comma. |
packages/apm-guide/.apm/skills/apm-usage/package-authoring.md |
Updates apm-usage package authoring docs to clarify comma-list semantics and target behavior. |
docs/src/content/docs/producer/author-primitives/instructions-and-agents.md |
Adds a worked comma-list example + clarifies per-target compilation behavior. |
CHANGELOG.md |
Adds an Unreleased Fixed entry describing the applyTo comma-list behavior across harnesses. |
Copilot's findings
- Files reviewed: 7/7 changed files
- Comments generated: 3
Comment on lines
+16
to
+18
| Single source of truth for the comma-vs-brace discrimination; the | ||
| placement optimizer and the integrators both consume this so the | ||
| semantics of ``parse_apply_to`` and its callers stay in lock-step. |
Comment on lines
+32
to
+40
| def yaml_double_quote(value: str) -> str: | ||
| """Escape ``value`` for embedding inside a YAML double-quoted scalar. | ||
|
|
||
| Defence-in-depth for the instruction integrators that emit YAML | ||
| frontmatter via f-strings (``f' - "{g}"'``). A glob containing a | ||
| literal backslash, double-quote, or control character would break | ||
| the surrounding YAML if inlined verbatim; this helper escapes the | ||
| minimal set needed for the YAML 1.2 double-quoted form. Returns the | ||
| value already wrapped in the surrounding double quotes. |
|
|
||
| ### Fixed | ||
|
|
||
| - Comma-separated `applyTo` globs now correctly emit a YAML list under `globs:` / `paths:` in Claude, Cursor, and Windsurf instruction outputs (Copilot output unchanged -- it already preserves `applyTo` verbatim and handles comma-lists natively). The shared `parse_apply_to()` helper is brace-aware, so `**/*.{css,scss},**/*.py` is split on the top-level comma only. Closes a documented promise that was silently broken on the three non-Copilot harnesses. (#1387, closes #1366) |
This was referenced May 22, 2026
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.
Summary
Follow-up to #1387 (which fixed comma-separated
applyToglobs in Claude/Cursor/Windsurf, closes #1366). The APM review panel postedship_with_followupswith 0 blocking and 5 nit-level items; this PR folds all 5.What lands
Fixedentry under[Unreleased]with the per-harness behavior spelled out and the Copilot-verbatim carve-out called out. (fix(integration): respect comma-separated applyTo globs in Claude/Cursor/Windsurf #1387, closes [BUG] applyTo comma-separated globs are not split ? Claude target emits broken paths: and compiler warns "matches no files" #1366)_has_top_level_commamoves fromcontext_optimizer.pyintoutils/patterns.pyashas_top_level_comma. Single source of truth for the brace-depth state machine; eliminates the DRY violation flagged by python-architect.yaml_double_quote()helper escapes backslashes, double-quotes, newlines, CR, and tab before emission. Defence-in-depth against adversarial or copy-paste-mangledapplyTovalues; risk is LOW today (parse_apply_to already strips whitespace; Windsurf already strips newlines) but producer output is now well-formed even on hostile input.Validation
uv run --extra dev ruff check src/ tests/-- silent.uv run --extra dev ruff format --check src/ tests/-- silent.tests/unit/utils/test_patterns.py,tests/unit/compilation/test_context_optimizer.py,tests/unit/integration/test_instruction_integrator.py,tests/integration/test_apply_to_comma_e2e.py).TestHasTopLevelComma(6) andTestYamlDoubleQuote(5 +yaml.safe_loadround-trip). Mutation-break verified -- dropping the escape inyaml_double_quotefails all 5 escape tests plus the round-trip.References
apm-review-paneladvisory comments on fix(integration): respect comma-separated applyTo globs in Claude/Cursor/Windsurf #1387 (doc-writer, python-architect, supply-chain-security, oss-growth-hacker convergence).