Skip to content

fix(yaml_io): validate line-1 delimiter before parsing Markdown frontmatter (#2663) - #2666

Open
Manideep Malyala (manideep-malyala) wants to merge 15 commits into
microsoft:mainfrom
manideep-malyala:fix/frontmatter-horizontal-rule-parsing
Open

fix(yaml_io): validate line-1 delimiter before parsing Markdown frontmatter (#2663)#2666
Manideep Malyala (manideep-malyala) wants to merge 15 commits into
microsoft:mainfrom
manideep-malyala:fix/frontmatter-horizontal-rule-parsing

Conversation

@manideep-malyala

@manideep-malyala Manideep Malyala (manideep-malyala) commented Aug 22, 2026

Copy link
Copy Markdown
Contributor

Description

Fixes a high-impact bug in load_frontmatter() (src/apm_cli/utils/yaml_io.py) where standard Markdown horizontal rules (---) in documentation text bodies were misinterpreted as YAML frontmatter fences.

When a .md, .instructions.md, or .agent.md file contains two or more --- section dividers in its body without a Line 1 frontmatter header, PyYAML attempted to parse the entire text body as YAML metadata and raised yaml.scanner.ScannerError or yaml.parser.ParserError. This caused APM to log a warning and silently drop the instruction/skill file during apm install.

Verification & Impact Summary

Metric Before Fix After Fix Empirical Result
Total Scanned Markdown Files 1,758 1,758 All package files scanned
Successfully Parsed Files 1,549 (88.1%) 1,742 (99.1%) +193 Files Restored
Parsing Failures / Dropped Files 209 (11.9%) 16 (0.9%)* 92.3% Error Reduction
APM .instructions.md Files 192 192 (100.0%) 0 Failures across all instructions
Unit Test Suite (tests/unit/utils/) N/A 113 / 113 Passed All tests passing cleanly

*Note: The remaining 16 files are non-APM GitHub Action workflow manifests (e.g. .github/workflows/learning-hub-updater.md), not APM instruction/skill files. 100% of all APM instructions and skills now parse with 0 errors.

Fixes #2663

Type of change

  • Bug fix
  • New feature
  • Documentation
  • Maintenance / refactor

Testing

  • Tested locally
  • All existing tests pass
  • Added focused loader and installed-artifact regression coverage

Scenario Evidence

# Scenario (user promise) Principle(s) Test(s) proving it Type
1 Installing an unfenced instruction containing Markdown horizontal rules preserves and parses its complete content. DevX (pragmatic as npm) tests/integration/test_local_install.py::TestLocalInstall::test_install_local_deploys_instructions (regression-trap for #2663) e2e
2 Four-hyphen frontmatter preserves instruction scope and body across Claude, Cursor, Windsurf, Kiro, and Antigravity installs. Portability by manifest; Multi-harness support tests/integration/test_local_install.py::TestLocalInstall::test_install_four_hyphen_frontmatter_preserves_scope_across_targets e2e
3 Parser-rejected bounded YAML fails package-atomically across a multi-target install without deploying any package primitive, including an earlier valid prompt or either valid/rejected instruction. Secure by default; DevX (pragmatic as npm) tests/integration/test_local_install.py::TestLocalInstall::test_install_rejects_bounded_frontmatter_bomb e2e
4 Cursor descriptions containing newlines, fence text, and decoded control characters remain one valid YAML scalar without dropping scope. Secure by default; Multi-harness support tests/integration/test_local_install.py::TestLocalInstall::test_install_cursor_quotes_multiline_control_characters e2e

Spec conformance (OpenAPM v0.1)

  • Spec edit: docs/src/content/docs/specs/openapm-v0.1.md updated
  • Manifest edit: docs/src/content/docs/specs/manifests/openapm-v0.1.requirements.yml updated.
  • Test edit: a @pytest.mark.req("req-XXX") test under tests/spec_conformance/ added or extended.
  • CONFORMANCE.{md,json} regenerated via uv run --extra dev python -m tests.spec_conformance.gen_statement and committed.
  • N/A -- this PR does not change OpenAPM-observable behaviour.

apm-spec-waiver: Internal parser consolidation restores existing accepted frontmatter grammar across target converters.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR fixes load_frontmatter() to avoid misinterpreting Markdown horizontal rules (---) inside document bodies as YAML frontmatter fences, which previously caused PyYAML parse errors and led to instruction/skill files being silently dropped during apm install.

Changes:

  • Read Markdown content as text and only attempt frontmatter parsing when the file begins with a frontmatter fence.
  • Add a unit test covering the “middle horizontal rules” regression case and a test for valid frontmatter on line 1.

Reviewed changes

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

File Description
src/apm_cli/utils/yaml_io.py Adds a “frontmatter must start at the beginning” guard before parsing, routing through the bounded handler.
tests/unit/utils/test_frontmatter_horizontal_rules.py Adds regression tests for horizontal rules in Markdown bodies vs valid frontmatter on line 1.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread src/apm_cli/utils/yaml_io.py Outdated
Comment thread tests/unit/utils/test_frontmatter_horizontal_rules.py Outdated
@danielmeppiel

Copy link
Copy Markdown
Collaborator

APM Review Panel: needs_rework

This fix prevents ordinary Markdown structure from silently dropping agent instructions, but its line-1 gate should reuse the parser's delimiter grammar.

cc Manideep Malyala (@manideep-malyala) Sergio Sisternes (@sergio-sisternes-epam) -- a fresh advisory pass is ready for your review.

The panel agrees on the user value: unfenced Markdown horizontal rules must remain body content. However, the new guard creates a second, narrower delimiter grammar. The Python architect reports a compatibility regression for supported four-hyphen fences, while the security reviewer identifies an indented-line-1 path that can reactivate body metadata parsing. Delegating detection to _BOUNDED_FRONTMATTER_HANDLER.detect(text) resolves both concerns and removes the performance cost of splitting the entire document.

The missing secure-by-default regression test for indented thematic breaks carries elevated weight. The install-level preservation test is also valuable because silent content loss is ultimately a package-manager failure, not merely a parser defect. After aligning detection and adding focused regression coverage, this becomes a strong reliability fix worth communicating in the changelog.

Dissent. The CLI logging reviewer recommends shipping as proposed, while the Python architect identifies a blocking compatibility regression. I side with the architect because parser grammar must have one authority, and the same change also resolves the security and performance findings.

Aligned with: portable by manifest: instruction and skill Markdown should survive package ingestion without structural content loss; secure by default: one canonical fence detector prevents body text from being interpreted unexpectedly as metadata; pragmatic as npm: ordinary Markdown should not risk silent drops.

Growth signal. APM now preserves ordinary Markdown structure instead of silently losing agent instructions, giving users a concrete reliability reason to upgrade.

Panel summary

Persona B R N Takeaway
Python Architect 1 0 0 Delegate the line-1 gate to the handler detector.
CLI Logging Expert 0 0 0 No CLI logging or diagnostic output concerns.
DevX UX Expert 0 1 0 Add fixture-backed coverage of the user-facing path.
Supply Chain Security Expert 0 1 0 Align the line-1 check with the handler grammar.
OSS Growth Hacker 0 1 0 Capture this reliability fix in the changelog.
Doc Writer 0 2 0 Document the fix and line-1 authoring contract.
Test Coverage Expert 0 2 0 Unit tests pass; install-level coverage is missing.
Performance Expert 0 1 0 Avoid full-document splitlines() allocation.

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

Top 4 follow-ups

  1. [Python Architect] (blocking-severity) Delegate the line-1 gate to the bounded handler detector and cover four-hyphen and indented-line-1 cases -- this removes split parser authority, preserves compatibility, closes the security edge case, and avoids full-document splitting.
  2. [Test Coverage Expert] Add one fixture-backed install regression proving complete unfenced Markdown survives horizontal rules -- unit coverage does not yet protect the installation promise.
  3. [OSS Growth Hacker] Add an Unreleased changelog entry for restored instruction and skill installs -- this user-visible reliability fix gives users a clear reason to upgrade.
  4. [Doc Writer] State once that frontmatter must open on line 1 -- this clarifies that later horizontal rules are body content.

Architecture

classDiagram
    class YAMLIO
    class YAMLHandler
    YAMLIO ..> YAMLHandler : delegates fence detection
Loading
flowchart TD
    A[Markdown input] --> B[YAMLHandler detect]
    B -->|fenced| C[bounded YAML parse]
    B -->|unfenced| D[raw Post]
Loading

Recommendation

Align the preliminary check with the handler detector before shipping, then add focused compatibility and indented-rule regression tests. Track install-level preservation coverage and the changelog entry as the highest-value follow-ups.


Full per-persona findings

Python Architect

  • [blocking] The new guard duplicates and narrows the handler's delimiter grammar at src/apm_cli/utils/yaml_io.py:480.
    YAMLHandler.detect() accepts three or more hyphens, while the literal comparison only accepts three. Reuse the handler detector and add a four-hyphen regression test.

CLI Logging Expert

No findings.

DevX UX Expert

  • [recommended] Prove content preservation through the install or primitive parsing path.
    Add fixture-backed coverage that asserts the complete Markdown content survives.

Supply Chain Security Expert

  • [recommended] Match the preliminary fence check to the parser's delimiter grammar at src/apm_cli/utils/yaml_io.py:480.
    An indented thematic break should remain body content and must not let later rules become metadata boundaries.

OSS Growth Hacker

  • [recommended] Add a user-facing Unreleased entry for restored instruction and skill installs.

Auth Expert -- inactive

Only YAML frontmatter parsing and its tests changed; no authentication surface is touched.

Doc Writer

  • [recommended] Add an Unreleased CHANGELOG entry for the silent content-loss fix.
  • [recommended] State the line-1 frontmatter delimiter contract once in the canonical authoring docs.

Test Coverage Expert

  • [recommended] Add an install-level regression trap for unfenced Markdown.
  • [recommended] Add the Scenario Evidence mapping to the PR body.

Performance Expert

  • [recommended] Avoid splitting the entire document to inspect its first line.
    Reusing the handler detector removes the avoidable allocation.

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

@danielmeppiel

Copy link
Copy Markdown
Collaborator

APM Review Panel: needs_rework

PR #2666 restores affected Markdown content during install; runtime behavior is correct, but the new authority guard and Markdown-scoped docs need tightening.

cc Manideep Malyala (@manideep-malyala) Daniel Meppiel (@danielmeppiel) Sergio Sisternes (@sergio-sisternes-epam) -- a fresh advisory pass is ready for your review.

The runtime signal is strong: eight exact-head regressions passed, and the installed-artifact regression covers the restored behavior. The architecture and security findings address the fitness function, not the runtime fix. Manual mutations showed that parser aliases or handler removal can bypass the checker while exiting successfully. Preserve the runtime fix, tighten these safeguards, and run a converged follow-up pass.

Dissent. There is no substantive dissent: passing runtime tests establish current behavior, while the architecture and security probes expose a future-regression gap in the static authority checker.

Aligned with: portable by manifest through restored Markdown primitives; secure by default through bounded parsing; governed by policy through explicit canonical ownership; pragmatic as npm through predictable install behavior.

Growth signal. Lead with recovered user content and predictable installs. Use a specific restored-file count only after reproducing it at this exact head.

Panel summary

Persona B R N Takeaway
Python Architect 0 2 0 Runtime behavior is sound; strengthen control-flow enforcement and register the owner.
CLI Logging Expert 0 0 0 No CLI output concerns; diagnostics follow ASCII conventions.
DevX UX Expert 0 0 0 The fix restores predictable install behavior.
Supply Chain Security Expert 0 1 0 Bounded runtime parsing is sound; the static guard needs alias coverage.
OSS Growth Hacker 0 1 0 Clarify the Markdown authoring contract.
Doc Writer 0 1 0 Scope the new guidance to Markdown primitives.
Test Coverage Expert 0 0 0 Unit and installed-artifact regressions cover the behavior.
Performance Expert 0 1 0 Runtime remains O(n); prefilter the CI AST scan.

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

Top 4 follow-ups

  1. [Supply Chain Security Expert] Harden the authority checker against parser aliases, handler removal, and paths where detection no longer gates bounded loading -- mutation probes currently evade the checker.
  2. [Python Architect] Register Markdown frontmatter detection in both canonical ownership tables -- the new durable decision needs one discoverable authority record.
  3. [Doc Writer] Scope the callout and preceding guidance to supported Markdown primitives -- hooks and MCP should not inherit a YAML rule.
  4. [Performance Expert] Prefilter source files before AST parsing -- this avoids unconditional parsing of unrelated Python files in CI.

Architecture

classDiagram
    direction LR
    class YamlIO {
      <<Facade>>
      +load_frontmatter(fd, encoding) Post
    }
    class BoundedYAMLHandler {
      <<Adapter>>
      +detect(text) bool
      +load(frontmatter) Any
    }
    class FrontmatterModule {
      <<ExternalModule>>
      +loads(text, handler) Post
      +Post(content) Post
    }
    class PrimitiveParser {
      <<Consumer>>
      +parse_primitive_file(path, source) Primitive
    }
    class FrontmatterAuthorityChecker {
      <<ArchitectureFitnessFunction>>
      +check(root) list
    }
    YamlIO *-- BoundedYAMLHandler : owns singleton
    YamlIO ..> FrontmatterModule : creates or parses Post
    PrimitiveParser ..> YamlIO : delegates parsing
    FrontmatterAuthorityChecker ..> YamlIO : inspects AST boundary
Loading
flowchart TD
    A["Primitive parser"] --> B["load_frontmatter reads text"]
    B --> C{"bounded handler detects line-1 fence"}
    C -->|no| D["Post with unchanged body"]
    C -->|yes| E["bounded frontmatter.loads"]
    E --> F["parsed primitive"]
    D --> F
    G["architecture lint"] --> H["frontmatter authority checker"]
    H --> I{"single gated owner"}
Loading

Recommendation

Keep the validated runtime fix, tighten the authority checker and Markdown-scoped documentation, complete exact-head CI, and then run a converged follow-up panel pass. The AST prefilter is worthwhile but secondary.


Full per-persona findings

Python Architect

  • [recommended] Make the static guard prove that detection controls parsing at scripts/check_frontmatter_authority.py:54.
    The checker counts calls but does not yet prove the bounded load is control-dependent on detection or reject aliases and parser entry points.
  • [recommended] Register Markdown frontmatter detection in the canonical owner table at .apm/instructions/architecture.instructions.md:36.
    Without a row, the checker creates a parallel authority declaration.

CLI Logging Expert

No findings.

DevX UX Expert

No findings.

Supply Chain Security Expert

  • [recommended] Harden the authority guard against parser aliases and handler removal at scripts/check_frontmatter_authority.py:13.
    Manual alias-import and bounded-handler-removal mutations both returned success.

OSS Growth Hacker

  • [recommended] Clarify the supported Markdown frontmatter contract at docs/src/content/docs/producer/author-primitives/index.md:46.
    Broad wording creates authoring ambiguity.

Auth Expert -- inactive

PR #2666 touches frontmatter parsing, tests, docs, and lint scripts, not auth, token, host, or credential semantics.

Doc Writer

  • [recommended] Scope the author-primitives callout to Markdown primitives at docs/src/content/docs/producer/author-primitives/index.md:49.
    The surrounding page also covers hooks and MCP.

Test Coverage Expert

No findings.

Performance Expert

  • [recommended] Prefilter source files before AST parsing at scripts/check_frontmatter_authority.py:79.
    The scan currently parses unrelated files; a substring prefilter preserves correctness and reduces CI allocation.

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

@danielmeppiel

Copy link
Copy Markdown
Collaborator

APM Review Panel: needs_rework

PR #2666 leaves valid four-hyphen frontmatter inconsistent across Claude, Cursor, and Windsurf despite green CI.

cc Manideep Malyala (@manideep-malyala) Daniel Meppiel (@danielmeppiel) Sergio Sisternes (@sergio-sisternes-epam) -- a fresh advisory pass is ready for your review.

The Python Architect's manual exact-head probe is load-bearing: the canonical loader parsed applyTo=src/**, while Claude preserved the header, Cursor emitted description: ----, and Windsurf emitted always_on. Green CI does not rebut this result because existing tests do not exercise four-hyphen conversion. The converter delimiter checks should route through the canonical helper before shipping.

Aligned with: portable by manifest through preserved instruction metadata; multi-harness support through one frontmatter grammar.

Panel summary

Persona B R N Takeaway
Python Architect 1 0 0 Target converters still diverge from the canonical frontmatter grammar.
CLI Logging Expert 0 0 0 No output concerns.
DevX UX Expert 0 0 0 No additional UX concerns.
Supply Chain Security Expert 0 0 0 Bounded parsing and provenance are sound.
OSS Growth Hacker 0 0 0 Docs and changelog are clear.
Doc Writer 0 0 0 Documentation is accurate.
Test Coverage Expert 0 0 0 Existing exact-head regressions pass.
Performance Expert 0 0 0 Runtime remains one read and O(n).

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

Top 1 follow-up

  1. [Python Architect] (blocking-severity) Route converter parsing and stripping through the canonical frontmatter helper, add four-hyphen regressions, and strengthen the static guard -- the exact-head probe demonstrates metadata divergence across supported harnesses.

Architecture

flowchart TD
    A["Instruction Markdown"] --> B["canonical bounded frontmatter parser"]
    B --> C["Instruction metadata and body"]
    C --> D["Claude converter"]
    C --> E["Cursor converter"]
    C --> F["Windsurf converter"]
    G["manual exact-three-hyphen checks"] -. diverge .-> D
    G -. diverge .-> E
    G -. diverge .-> F
Loading

Recommendation

Fold the canonical parsing change and four-hyphen multi-harness regression tests into this PR, then rerun CI before shipping.


Full per-persona findings

Python Architect

  • [blocking] Make the frontmatter authority boundary cover target converters at scripts/check_frontmatter_authority.py:167.
    Six manual delimiter checks in instruction_integrator.py accept only exact three-hyphen fences. A four-hyphen instruction parsed by the canonical loader loses or corrupts applyTo semantics during Claude, Cursor, and Windsurf conversion.
    Suggested: Route conversion through a canonical text parser, add four-hyphen converter regressions, and reject manual detector patterns in the static guard.

CLI Logging Expert

No findings.

DevX UX Expert

No findings.

Supply Chain Security Expert

No findings.

OSS Growth Hacker

No findings.

Auth Expert -- inactive

The diff changes frontmatter parsing, tests, docs, lock provenance, and architecture lint, not authentication semantics.

Doc Writer

No findings.

Test Coverage Expert

No findings.

Performance Expert

No findings.

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

@danielmeppiel

Copy link
Copy Markdown
Collaborator

APM Review Panel: needs_rework

Exact-head review of bd887327 found two fail-open conversion paths and one integration-coverage gap.

cc Manideep Malyala (@manideep-malyala) Daniel Meppiel (@danielmeppiel) Sergio Sisternes (@sergio-sisternes-epam) -- a fresh advisory pass is ready for your review.

The panel supports the move to one bounded frontmatter parser and its canonical-owner guardrails. However, converter exception fallbacks can still turn parser-rejected input into active instruction text, and Cursor descriptions are emitted as unquoted YAML. Those two paths undercut the security boundary this PR introduces.

Aligned with: secure package consumption through fail-closed parsing; multi-harness portability through one frontmatter grammar.

Panel summary

Persona B R N Takeaway
Python Architect 0 0 0 Canonical parser ownership is sound.
CLI Logging Expert 0 0 0 No output concerns.
DevX UX Expert 0 0 0 No additional UX concerns.
Supply Chain Security Expert 2 0 0 Parser failures can fall open; Cursor descriptions can inject frontmatter.
OSS Growth Hacker 0 0 0 No adoption concerns.
Doc Writer 0 0 0 No additional documentation concerns.
Test Coverage Expert 0 1 0 Add real multi-target install coverage.
Performance Expert 0 0 0 No performance concerns.

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

Top follow-ups

  1. [Supply Chain Security] (blocking-severity) Propagate bounded-parser failures from every instruction converter instead of returning original fenced content.
  2. [Supply Chain Security] (blocking-severity) Quote Cursor description values with the existing YAML scalar helper so embedded newlines or --- cannot terminate generated frontmatter or erase scope.
  3. [Test Coverage] (recommended) Prove four-hyphen scope conversion through real installs for Claude, Cursor, Windsurf, Kiro, and Antigravity, and prove parser-rejected YAML is not deployed.

Architecture

flowchart TD
    A["Instruction Markdown"] --> B["bounded frontmatter parser"]
    B --> C["validated metadata and body"]
    C --> D["target converters"]
    D --> E["agent-readable deployed files"]
    B -- "parse rejection" --> F["installation failure"]
    G["exception fallback"] -. "must not bypass" .-> E
Loading

Recommendation

Fold both security fixes and the install-level regression coverage, rerun the mutation, architecture, and CI gates, then request one exact-head convergence pass.


Full per-persona findings

Supply Chain Security Expert

  • [blocking] _parse_frontmatter catches canonical parser exceptions and returns the rejected fenced source as body content; Antigravity carries the same fail-open behavior. Propagate the parser failure and prove hostile or over-budget YAML cannot reach a target file.
  • [blocking] Cursor emits description as an unquoted scalar. An embedded newline followed by --- can close generated frontmatter and discard globs. Use yaml_double_quote() and parse the generated document in a regression test.

Test Coverage Expert

  • [recommended] Existing converter units do not prove the install boundary. Install a four-hyphen instruction into all five converting targets and assert both scope and body; add rejected-YAML no-deployment proof.

Auth Expert -- inactive

The reviewed diff does not change token, credential, host-authentication, or authorization behavior.

Other personas

No additional findings.

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

@danielmeppiel

Copy link
Copy Markdown
Collaborator

APM Review Panel: needs_rework

PR #2666 centralizes fail-closed instruction parsing, but exact-head probes expose invalid Cursor YAML and non-atomic deployment that should be fixed before shipping.

cc Manideep Malyala (@manideep-malyala) Daniel Meppiel (@danielmeppiel) Sergio Sisternes (@sergio-sisternes-epam) -- a fresh advisory pass is ready for your review.

At f1624d05e, green CI does not outweigh two reproducible end-to-end failures. A description escape of \0 becomes U+0000 and is emitted as a raw NUL, producing installed Cursor YAML that cannot be parsed. Separately, sequential conversion leaves an earlier rule deployed when a later instruction fails. The observed failures are direct correctness and supply-chain integrity signals.

The specialists otherwise converge: the canonical parser should remain single-owned and fail closed, with no additional abstraction. The serializer must preserve target validity, and installation must preflight every package rule before writing. Diagnostic and recovery guidance should accompany the fixes so stronger safety remains easy to understand.

Aligned with: portable-by-manifest valid target output; secure-by-default serialization and package-atomic rejection; multi-harness conversion consistency; npm-like actionable install failures.

Panel summary

Persona B R N Takeaway
Python Architect 0 0 1 Canonical parsing is single-owned and fail-closed; no additional abstraction is warranted.
CLI Logging Expert 0 1 0 Fail-closed behavior works, but rejected YAML produces duplicate diagnostics.
DevX UX Expert 0 1 0 Core install promises hold; add a concrete recovery action for rejected YAML.
Supply Chain Security Expert 2 0 0 Cursor control escapes and sequential writes violate fail-closed deployment.
OSS Growth Hacker 0 0 0 Clear user-facing fix and install-level proof support shipping after the security folds.
Doc Writer 0 0 0 Documentation matches the parser contract.
Test Coverage Expert 0 1 0 Cursor multiline-description quoting lacks install-path coverage.
Performance Expert 0 0 0 Conversion remains one bounded O(n) parse per instruction.

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

Top 5 follow-ups

  1. [Supply Chain Security] (blocking-severity) Replace the partial YAML quoting helper with complete scalar serialization and add a control-character install round-trip -- raw NUL currently makes deployed Cursor YAML invalid.
  2. [Supply Chain Security] (blocking-severity) Render every package instruction before writing any target file, with a two-file failure regression -- a later rejection currently leaves earlier output active.
  3. [Test Coverage] Add install-path coverage for Cursor descriptions containing newlines, fence text, and control escapes -- unit proof does not defend the deployment boundary.
  4. [CLI Logging] Emit one structured, package-relative diagnostic for rejected YAML -- the discovery warning duplicates the final install error.
  5. [DevX UX] Include and assert a concrete fix-and-rerun action -- stricter validation should tell package authors how to recover.

Architecture

flowchart TD
    A["Instruction package"] --> B["preflight every bounded parse and target conversion"]
    B --> C{"all rules valid?"}
    C -->|No| D["one actionable install error; no target files"]
    C -->|Yes| E["write target-native rules"]
    E --> F["valid Claude, Cursor, Windsurf, Kiro, and Antigravity files"]
Loading

Recommendation

Fold complete YAML serialization and package-atomic instruction preflight before shipping, then lock both failures down with install-path tests. Preserve one actionable diagnostic as a follow-up if it cannot be folded without broad discovery changes.


Full per-persona findings

Python Architect

  • [nit] No additional abstraction is warranted. The yaml_io.loads_frontmatter facade and static ownership guard remain the simplest correct shape.

CLI Logging Expert

  • [recommended] Render rejected frontmatter once through the structured install error path at src/apm_cli/primitives/discovery.py:155. The plain warning exposes an internal materialization path and duplicates the final failure.

DevX UX Expert

  • [recommended] Give rejected frontmatter a concrete recovery action and assert it at the install boundary.

Supply Chain Security Expert

  • [blocking] Valid YAML control escapes produce malformed Cursor frontmatter at src/apm_cli/utils/patterns.py:80. Exact-head E2E evidence: install returned zero, wrote a raw NUL, and the deployed YAML parser raised ReaderError.
  • [blocking] Parser rejection leaves earlier package instructions deployed at src/apm_cli/integration/instruction_integrator.py:225. Exact-head E2E evidence: install returned nonzero, the good rule existed, and the later bomb did not.

OSS Growth Hacker

No findings.

Auth Expert -- inactive

The parser and converter diff does not change authentication behavior.

Doc Writer

No findings.

Test Coverage Expert

  • [recommended] Cursor multiline-description quoting lacks integration-with-fixtures coverage. Add a real Cursor install and parse the emitted metadata.

Performance Expert

No findings.

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

@danielmeppiel

Copy link
Copy Markdown
Collaborator

APM Review Panel: ship_with_followups

PR #2666 restores lossless installation of unfenced Markdown with horizontal rules through bounded frontmatter parsing, with all runtime and end-to-end checks passing.

cc Manideep Malyala (@manideep-malyala) Daniel Meppiel (@danielmeppiel) Sergio Sisternes (@sergio-sisternes-epam) -- a fresh advisory pass is ready for your review.

The panel converges at exact SHA f2691fe59: architecture, CLI logging, DevX, security, documentation, performance, and growth report no substantive concerns, and all six CI workflows are green. Eight install cases exercise the four user-facing promises end to end, including the #2663 regression path.

The bounded parser, complete serializer, and atomic preflight preserve install UX, portability, and predictable behavior without new abstraction or material performance cost. The sole recommendation concerns PR evidence metadata, not implementation correctness: normalize the Scenario Evidence table and explicitly identify the regression trap.

Aligned with: portable-by-manifest lossless installation; secure-by-default bounded parsing and atomic writes; governed canonical ownership; npm-like behavior without flags or workarounds.

Panel summary

Persona B R N Takeaway
Python Architect 0 0 1 No architecture concerns remain; canonical owners and guards are clear.
CLI Logging Expert 0 0 0 The final error names the invalid file and gives an actionable fix.
DevX UX Expert 0 0 0 No DevX regression; all 8 user-promise install cases pass.
Supply Chain Security Expert 0 0 0 No demonstrated bypass; bounded parsing, atomic preflight, and focused tests pass.
OSS Growth Hacker 0 0 0 Clear changelog, author guidance, scenario evidence, and green CI.
Doc Writer 0 0 0 Docs match the line-1 grammar and atomic converted-target behavior.
Test Coverage Expert 0 1 0 Behavioral surfaces pass E2E; normalize the Scenario Evidence table.
Performance Expert 0 0 0 Atomic preflight remains linear with no network round trips.

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

Top 1 follow-up

  1. [Test Coverage Expert] Replace the three-column Scenario Evidence table with the required five columns and label the install test as the regression trap for [BUG] load_frontmatter misinterprets middle horizontal rules (---) as YAML frontmatter #2663 -- behavior is proven, but normalized metadata keeps the evidence auditable.

Architecture

flowchart TD
    A["Instruction package"] --> B["canonical bounded parser"]
    B --> C["complete target scalar serialization"]
    C --> D["pre-render every converted rule"]
    D --> E{"all valid?"}
    E -->|No| F["actionable error; no target writes"]
    E -->|Yes| G["write target-native rules"]
Loading

Recommendation

Ship exact head f2691fe59. Correct the PR body evidence table as a non-code follow-up; no runtime, security, documentation, performance, or test failure warrants rework.


Full per-persona findings

Python Architect

  • [nit] Design-pattern inventory; no action requested. The facade, serializer, and validate-before-mutate flow are the simplest correct design at this scope.

CLI Logging Expert

No findings.

DevX UX Expert

No findings.

Supply Chain Security Expert

No findings.

OSS Growth Hacker

No findings.

Auth Expert -- inactive

The parser and converter diff does not change authentication behavior.

Doc Writer

No findings.

Test Coverage Expert

Performance Expert

No findings.

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

@danielmeppiel

Copy link
Copy Markdown
Collaborator

APM review panel advisory

Recommendation: changes requested before landing.

The panel re-reviewed exact head ee5c5078a. Parser ownership, converter consistency, fail-closed behavior, cross-target instruction validation, control-character serialization, documentation, and focused regression coverage are sound. One in-scope atomicity gap remains: instruction preflight runs only when the instructions primitive is reached, so an earlier prompt, agent, or command can be deployed before invalid instruction frontmatter aborts installation.

Fold now

  • Move instruction preflight before the outer primitive deployment loop.
  • Extend the end-to-end rejection scenario with an earlier valid prompt and assert that neither it nor either valid/rejected instruction target is deployed.
  • Clarify that this package-wide guarantee applies during apm install.

Validation required

  • Mutation-break the package-wide preflight by removing it and confirm the earlier prompt remains deployed.
  • Re-run focused install orchestration and binary scenarios, canonical lint, architecture checks, and exact-head CI.

No auth-specific review was activated. No out-of-scope follow-up is required.

@danielmeppiel

Daniel Meppiel (danielmeppiel) commented Aug 23, 2026

Copy link
Copy Markdown
Collaborator

APM review panel advisory

CEO recommendation: ship now.

The converged panel found no surviving blocker. Package-wide instruction preflight now runs before any primitive write, and the mutation test is decisive: removing that preflight leaves the earlier prompt deployed and fails the end-to-end scenario.

Folded in this run

  • Package-wide primitive atomicity in 37cfe4023.
  • Clear absent-fence versus invalid-YAML documentation in d27ed4ed9, with producer-guide and bundled-guide parity.

Copilot signals reviewed

  • LEGIT 3837238239: line-1 fence detection now rejects leading whitespace; resolved in dfb7207a1.
  • LEGIT 3837238250: unused test imports were removed; resolved in dfb7207a1.

Deferred

None.

Validation

  • 233 exact-head owner functional tests passed.
  • 8 exact-head binary install scenarios passed.
  • 170 focused install-service tests passed before the final docs-only commit.
  • Package-wide preflight mutation failed the intended prompt-absence assertion.
  • Exact-head architecture boundary lint passed.
  • All exact-head GitHub Actions checks are green, including Lint, both Linux test shards, Windows, lifecycle, coverage, CodeQL, spec conformance, and Merge Gate.

Mergeability

PR Head CEO stance Iterations Folds Deferrals Copilot rounds CI Mergeable Merge state Notes
#2666 d27ed4e ship_now 4 10 0 2 green MERGEABLE BLOCKED pending required review

No auth-specific review was activated. No in-scope follow-up remains.

Route the line-1 gate through the bounded handler's canonical delimiter grammar, preserve supported fences, and add unit plus install-path regression evidence. Document the authoring contract and release impact. Addresses panel follow-ups from python-architect, test-coverage-expert, doc-writer, and oss-growth-hacker.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The loader no longer calls frontmatter.load directly, so tests must patch the imported consumer symbols. This restores dependency aggregation and validation exception coverage after the loader fix. Addresses CI run 32640706044.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Enforce bounded-handler fence detection with an AST boundary check and mutation-backed architecture tests so local delimiter grammars or direct parser bypasses cannot reintroduce the bug. Addresses the final python-architect panel follow-up.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
apm-spec-waiver: Internal parser consolidation restores existing accepted frontmatter grammar across target converters.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Quote Cursor descriptions and add multi-target install regressions for canonical frontmatter handling.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Preflight converted rules before writes and serialize all YAML control characters safely. Add end-to-end rollback, recovery, and Cursor round-trip coverage.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Validate every converted target before identity targets can write package rules. This keeps multi-target install failures atomic.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Current main raises the existing install integration function to the complexity threshold once package-wide instruction preflight is replayed. Keep the established complex-function annotation explicit.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@danielmeppiel
Daniel Meppiel (danielmeppiel) force-pushed the fix/frontmatter-horizontal-rule-parsing branch from d27ed4e to 60a3524 Compare August 25, 2026 19:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG] load_frontmatter misinterprets middle horizontal rules (---) as YAML frontmatter

3 participants