Skip to content

Add PPTX Merger skill - #267

Open
Angara V L Gopala Sandeep (hisandeepangara) wants to merge 23 commits into
microsoft:mainfrom
hisandeepangara:add-pptx-merger-skill
Open

Add PPTX Merger skill#267
Angara V L Gopala Sandeep (hisandeepangara) wants to merge 23 commits into
microsoft:mainfrom
hisandeepangara:add-pptx-merger-skill

Conversation

@hisandeepangara

Copy link
Copy Markdown
Contributor

This skill merges two or more pptx into a single pptx file. Tested in Copilot Studio and also validated locally before pushing to the repo.

Copilot AI lite review requested due to automatic review settings August 7, 2026 07:38

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

Adds a new pptx-merger submission to the CAT Agent Skills gallery, providing an agent workflow plus Python helper scripts to safely merge multiple base64-delivered PPTX decks into a single validated output for Copilot Studio scenarios.

Changes:

  • Added a new pptx-merger skill (SKILL.md) describing a 4-step ingest → merge → validate → export pipeline.
  • Introduced Python scripts to decode/encode base64, merge PPTX packages, and validate via structural checks + optional LibreOffice render.
  • Added submission catalog metadata (metadata.json) for gallery publishing.

Reviewed changes

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

Show a summary per file
File Description
submissions/pptx-merger/SKILL.md Skill instructions and operational guidance for running the merge pipeline.
submissions/pptx-merger/scripts/b64_to_pptx.py Base64 → verified PPTX ingest script.
submissions/pptx-merger/scripts/pptx_merge.py Core PPTX merge implementation (masters/layouts/media/charts/notes/content types).
submissions/pptx-merger/scripts/verify_pptx.py Validation gate (structural checks + optional LibreOffice render).
submissions/pptx-merger/scripts/pptx_to_b64.py Verified PPTX → base64 export script with round-trip guard.
submissions/pptx-merger/metadata.json Gallery metadata for the new submission.

Comment thread submissions/pptx-merger/scripts/verify_pptx.py Outdated
Comment thread submissions/pptx-merger/scripts/verify_pptx.py
Comment thread submissions/pptx-merger/scripts/pptx_merge.py
Comment thread submissions/pptx-merger/SKILL.md Outdated
Copilot AI review requested due to automatic review settings August 7, 2026 07:42
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

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

Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.

Suppressed comments (1)

submissions/pptx-merger/SKILL.md:18

  • This SKILL.md includes substantial human-facing onboarding/integration guidance (overview + Copilot Studio delivery/requirements). Per repo rules, that content should live in a README.md sidecar, while SKILL.md should be the lean agent runtime SOP (activation/procedure/output).
# PPTX Merger — corruption-safe deck merge for agent sandboxes

Combine PowerPoint decks into a single file that opens cleanly in PowerPoint.
The skill is built for the case where decks arrive as **base64** (e.g. from the
SharePoint *Get file content using path* connector) and must be merged inside a

Comment thread submissions/pptx-merger/scripts/verify_pptx.py Outdated
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

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

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

Suppressed comments (3)

submissions/pptx-merger/scripts/pptx_merge.py:136

  • zipfile.ZipFile(...).extractall() will write paths from the archive verbatim. A crafted PPTX (ZIP) can include ../ or absolute paths and overwrite files outside the intended temp directory (Zip Slip). Please validate member paths before extraction and reject unsafe entries.
        for i, inp in enumerate(inputs):
            d = self.tmp / f"s{i}"
            with zipfile.ZipFile(inp) as z:
                z.extractall(d)
            srcs.append(d)

submissions/pptx-merger/scripts/verify_pptx.py:64

  • The comment says “Every part referenced by a .rels must exist”, but the code currently only checks for absolute internal targets and never verifies that relationship targets resolve to an existing ZIP part. This can let structurally broken packages pass when render validation is skipped.
                part_folder = "/".join(n.split("/")[:-2])  # strip _rels/<file>.rels
                for rel in root:
                    tgt = rel.get("Target", "")
                    mode = rel.get("TargetMode", "")
                    if mode == "External":

submissions/pptx-merger/SKILL.md:112

  • SKILL.md currently contains substantial human-facing adoption/setup guidance (e.g., the Power Automate upload flow instructions under “Delivering the result (Copilot Studio)”, plus install guidance). In this repo, that content should live in a README.md sidecar, keeping SKILL.md focused on agent runtime instructions.
## Delivering the result (Copilot Studio)

Keep the out-of-the-box SharePoint tools as-is. The base64 from step 4 is what you
hand to your upload path. **Never read, print, chunk, or reconstruct the base64 in
the model** — pass it as a variable reference from export straight into the upload

Comment thread submissions/pptx-merger/scripts/pptx_merge.py
Comment thread submissions/pptx-merger/scripts/verify_pptx.py Outdated
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

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

Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.

Suppressed comments (3)

submissions/pptx-merger/SKILL.md:112

  • This SKILL.md includes human-facing setup/adoption guidance (e.g. how to upload via Power Automate / SharePoint and install dependencies). In this repo, that content belongs in a submission README.md (see submissions/_template/README.md:5-16), while SKILL.md should stay focused on agent runtime instructions.
## Delivering the result (Copilot Studio)

Keep the out-of-the-box SharePoint tools as-is. The base64 from step 4 is what you
hand to your upload path. **Never read, print, chunk, or reconstruct the base64 in
the model** — pass it as a variable reference from export straight into the upload

submissions/pptx-merger/scripts/verify_pptx.py:38

  • structural_checks() will raise zipfile.BadZipFile (and crash with a traceback) when the input is not a valid ZIP/PPTX. Since this script is meant to validate and return actionable errors (especially with --json), it should catch this and report it as a structural error instead of throwing.
def structural_checks(path: Path):
    errors, warnings = [], []
    with zipfile.ZipFile(path) as z:
        bad = z.testzip()

submissions/pptx-merger/scripts/verify_pptx.py:100

  • --render is documented as forcing the render check, but if LibreOffice/soffice is missing render_check() returns (None, "..."). That makes a forced render behave like a skip and can still yield ok: true, which contradicts the flag’s intent.
def render_check(path: Path):
    soffice = shutil.which("soffice") or shutil.which("libreoffice")
    if not soffice:
        return None, "soffice not available; render check skipped"

Comment thread submissions/pptx-merger/scripts/pptx_merge.py
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

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

Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.

Suppressed comments (4)

submissions/pptx-merger/SKILL.md:112

  • The section "Delivering the result (Copilot Studio)" is user/adoption guidance (Power Automate flow wiring, SharePoint tool usage). Per repo submission rules, this kind of human-facing setup content should live in a README.md sidecar, while SKILL.md should stay agent-facing runtime instructions only. Also, the pip example uses --break-system-packages, which is distro/pip-specific and may not work in the target sandbox; move installation notes to README and use a portable python -m pip install lxml form (or describe when the flag is needed).
## Delivering the result (Copilot Studio)

Keep the out-of-the-box SharePoint tools as-is. The base64 from step 4 is what you
hand to your upload path. **Never read, print, chunk, or reconstruct the base64 in
the model** — pass it as a variable reference from export straight into the upload

submissions/pptx-merger/scripts/b64_to_pptx.py:53

  • _has_replacement_chars() returns True on a UnicodeDecodeError, which makes the script report "contains U+FFFD" even when the real problem is that the input file is not valid UTF-8 text. Because this check runs before _load_b64_text(), the clearer "not valid UTF-8 text" error path is never reached.
def _has_replacement_chars(arg: str) -> bool:
    # If the caller passed a file, a genuine base64 string can't contain U+FFFD.
    # Its presence means corruption already happened upstream of this script.
    p = Path(arg)
    try:
        txt = p.read_text(encoding="utf-8", errors="strict") if (p.exists() and p.is_file()) else arg
    except UnicodeDecodeError:
        return True
    return "\ufffd" in txt

submissions/pptx-merger/scripts/verify_pptx.py:69

  • structural_checks() claims "Every part referenced by a .rels must exist", but the implementation currently only checks for absolute internal targets and never verifies that internal relationship targets resolve to an existing ZIP entry. This can incorrectly PASS packages with broken/missing internal relationships (and part_folder is computed but unused).
        # Every part referenced by a .rels must exist; no absolute internal targets.
        for n in names:
            if n.endswith(".rels"):
                root = etree.fromstring(z.read(n))
                part_folder = "/".join(n.split("/")[:-2])  # strip _rels/<file>.rels
                for rel in root:
                    tgt = rel.get("Target", "")
                    mode = rel.get("TargetMode", "")
                    if mode == "External":
                        continue
                    if tgt.startswith("/"):
                        errors.append(f"{n}: absolute internal Target '{tgt}' "
                                     "(needs relative path or TargetMode=External)")

submissions/pptx-merger/scripts/verify_pptx.py:101

  • --render is documented as forcing the render check on, but if LibreOffice is missing render_check() currently returns (None, "...skipped"), which makes a forced render effectively a no-op and can still PASS. For a forced render, missing soffice should fail validation so callers can rely on the flag's semantics.
def render_check(path: Path):
    soffice = shutil.which("soffice") or shutil.which("libreoffice")
    if not soffice:
        return None, "soffice not available; render check skipped"
    with tempfile.TemporaryDirectory() as td:

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

Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.

Suppressed comments (3)

submissions/pptx-merger/SKILL.md:112

  • The SKILL.md includes adoption/setup guidance specific to Copilot Studio/Power Automate (e.g., recommended upload flow and connector configuration). Per repo guidance, human-facing setup/adoption instructions should live in a README.md sidecar, while SKILL.md should stay focused on runtime agent procedure.
## Delivering the result (Copilot Studio)

Keep the out-of-the-box SharePoint tools as-is. The base64 from step 4 is what you
hand to your upload path. **Never read, print, chunk, or reconstruct the base64 in
the model** — pass it as a variable reference from export straight into the upload

submissions/pptx-merger/scripts/verify_pptx.py:69

  • The .rels loop comment says targets must exist, but the code currently only checks for absolute internal Targets and never verifies that each internal relationship Target resolves to a part present in the ZIP. This can allow a broken PPTX to pass validation.
        # Every part referenced by a .rels must exist; no absolute internal targets.
        for n in names:
            if n.endswith(".rels"):
                root = etree.fromstring(z.read(n))
                part_folder = "/".join(n.split("/")[:-2])  # strip _rels/<file>.rels

submissions/pptx-merger/scripts/verify_pptx.py:16

  • The module docstring usage/help text is out of sync with the actual CLI: the script supports --no-render but the docstring doesn’t mention it, which can mislead users following the built-in usage guidance.
Usage:
    python verify_pptx.py <file.pptx> [--render] [--json]

--render forces the render check on (it is on by default when soffice exists).
Exit code 0 only if every enabled check passes.

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

Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.

Suppressed comments (7)

submissions/pptx-merger/scripts/b64_to_pptx.py:52

  • Same issue as _load_b64_text(): Path(arg).exists()/is_file() can raise OSError for large literal base64 strings (e.g., filename too long), causing a crash before validation.
    p = Path(arg)
    try:
        txt = p.read_text(encoding="utf-8", errors="strict") if (p.exists() and p.is_file()) else arg
    except UnicodeDecodeError:
        return True

submissions/pptx-merger/SKILL.md:18

  • SKILL.md contains substantial human-facing overview/setup/adoption guidance (e.g., the long narrative introduction, pipeline explanation, and Copilot Studio delivery/upload walkthrough). Per submissions/README.md, this content should live in an optional submissions//README.md for humans, leaving SKILL.md as lean agent runtime instructions.
# PPTX Merger — corruption-safe deck merge for agent sandboxes

Combine PowerPoint decks into a single file that opens cleanly in PowerPoint.
The skill is built for the case where decks arrive as **base64** (e.g. from the
SharePoint *Get file content using path* connector) and must be merged inside a

submissions/pptx-merger/scripts/b64_to_pptx.py:37

  • When <base64_input> is a literal base64 string, calling Path(arg).exists()/is_file() can raise OSError (e.g., filename too long). That would make ingest fail on large inputs even though literal base64 is a supported mode.

This issue also appears on line 48 of the same file.

    p = Path(arg)
    if p.exists() and p.is_file():
        raw = p.read_text(encoding="utf-8", errors="strict")
    else:
        raw = arg

submissions/pptx-merger/scripts/verify_pptx.py:28

  • This script depends on lxml but will currently crash with a raw traceback if lxml isn't installed. Also, etree.fromstring() uses the default parser, which can resolve entities / load DTDs unless explicitly disabled. For a script that parses untrusted PPTX parts, set a safe default parser and fail fast with a clear message if lxml is missing.
from lxml import etree

submissions/pptx-merger/scripts/verify_pptx.py:69

  • The comment says "Every part referenced by a .rels must exist", but the implementation only checks for absolute internal Targets and never verifies that the resolved Target part is present in the ZIP. That can let structurally-broken decks pass validation.
        # Every part referenced by a .rels must exist; no absolute internal targets.
        for n in names:
            if n.endswith(".rels"):
                root = etree.fromstring(z.read(n))
                part_folder = "/".join(n.split("/")[:-2])  # strip _rels/<file>.rels

submissions/pptx-merger/scripts/verify_pptx.py:155

  • --render is documented as "force render check", but if soffice isn't available and the caller explicitly passes --render, the script currently skips rendering and can still exit 0 on structural-only checks. If the user asked to force rendering, missing soffice should be treated as a failing validation.
    soffice_present = shutil.which("soffice") or shutil.which("libreoffice")
    do_render = args.render or (soffice_present and not args.no_render)

    if do_render:
        ok, msg = render_check(path)
        result["render"] = {"passed": ok, "detail": msg}
        if ok is False:
            errors = errors + [f"render: {msg}"]
    else:
        if args.no_render:
            result["render"] = {"passed": None, "detail": "render disabled (--no-render)"}
        elif not soffice_present:
            result["render"] = {"passed": None, "detail": "soffice not available; render check skipped"}
        else:
            result["render"] = {"passed": None, "detail": "render check skipped"}

submissions/pptx-merger/scripts/pptx_merge.py:32

  • pptx_merge.py depends on lxml but will currently fail with a raw ImportError traceback if it's missing. For an agent-invoked script, emit a clear, actionable error message and exit non-zero instead.
from lxml import etree

Refactor file existence check and error handling for reading text.

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

Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.

Suppressed comments (2)

submissions/pptx-merger/scripts/verify_pptx.py:167

  • errors = errors + [...] creates a new list, so result["structural_errors"] (which points at the original list) will not include render-related failures. This makes the --json output inconsistent with the actual pass/fail decision and the human-readable error list.
    if args.render and not soffice_present:
        errors = errors + ["render: soffice/libreoffice not found but --render was explicitly requested"]
        result["render"] = {"passed": False, "detail": "soffice not available"}
    elif do_render:
        ok, msg = render_check(path)
        result["render"] = {"passed": ok, "detail": msg}
        if ok is False:
            errors = errors + [f"render: {msg}"]

submissions/pptx-merger/scripts/pptx_merge.py:380

  • Theme relationship parts are copied verbatim, so they can preserve a prefixed namespace and/or absolute internal Target values. This contradicts the script’s stated guarantee that all .rels are written in the default OPC namespace and that absolute internal targets are normalized, and it can reintroduce the “PowerPoint can’t read this file” failure mode from source decks.
            sr = sd / "_rels" / f"{f.name}.rels"
            if sr.exists():
                (od / "_rels").mkdir(exist_ok=True)
                shutil.copy2(sr, od / "_rels" / f"{new}.rels")

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

Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.

Suppressed comments (2)

submissions/pptx-merger/SKILL.md:20

  • SKILL.md’s body starts with a human-facing overview/rationale (what the skill is, why it exists, explanation of corruption modes). Repo convention is that the SKILL.md body should go straight into imperative, agent-facing run instructions; longer overview/adoption context belongs in README.md (see submissions/_template/SKILL.md:9-14). Consider moving this intro/explainer text into README.md and starting SKILL.md with a concise “## Instructions” section and guardrails only.
Combine PowerPoint decks into a single file that opens cleanly in PowerPoint.
The skill is built for the case where decks arrive as **base64** (e.g. from the
SharePoint *Get file content using path* connector) and must be merged inside a
sandbox and handed back out. It exists because the naive path corrupts files in
two places: binary bytes get destroyed when pushed through a text/UTF-8 codec,

submissions/pptx-merger/scripts/verify_pptx.py:175

  • verify_pptx.py currently returns exit code 0 (PASS) when LibreOffice is not available, which contradicts both the script header (“…rather than claiming success it can’t prove”) and SKILL.md/README.md (“skipped and clearly flagged rather than claimed as a pass” / “validation runs structural-only and says so”). If this script is intended to be a hard gate, consider failing when soffice/libreoffice is missing unless the caller explicitly opts into structural-only with --no-render.
    soffice_present = shutil.which("soffice") or shutil.which("libreoffice")
    do_render = args.render or (soffice_present and not args.no_render)

    if args.render and not soffice_present:
        errors = errors + ["render: soffice/libreoffice not found but --render was explicitly requested"]

@hisandeepangara

Copy link
Copy Markdown
Contributor Author

Hi adilei request for your approval here. Let me know if I need to complete anything else. Thank you

Comment thread submissions/pptx-merger/scripts/pptx_merge.py Outdated
Comment thread submissions/pptx-merger/scripts/pptx_merge.py Outdated
Comment thread submissions/pptx-merger/scripts/verify_pptx.py Outdated

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

Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.

Suppressed comments (2)

submissions/pptx-merger/scripts/pptx_merge.py:441

  • Similarly, you regenerate new relationship IDs for each slide layout .rels but do not update slideLayout*.xml to match. This can leave r:id references pointing to non-existent relationships.
                lrels = read_rels(sl / "_rels" / f"{old_l}.rels")
                nlr, lset = [], set()
                for lr in lrels:
                    rid = next_rid(lset); lset.add(rid)
                    nr = dict(lr); nr["Id"] = rid
                    if lr["Type"] == RT_SLIDE_MASTER:

submissions/pptx-merger/README.md:19

  • The README suggests pip install lxml --break-system-packages for Copilot Studio. In this repo, other Copilot Studio-oriented submissions note that lxml is already available in the sandbox and that pip install may not be supported, so this instruction is likely to fail and/or mislead users. Consider describing lxml as a required dependency that is expected to be present in the Copilot Studio sandbox, and only give pip install guidance for local runs.
- **`lxml`** — merge and validate steps
  (`pip install lxml --break-system-packages` if not already in the sandbox).
- **LibreOffice (`soffice`)** — the render gate in step 3. If unavailable,

Comment thread submissions/pptx-merger/scripts/pptx_merge.py Outdated

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

Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.

Suppressed comments (5)

submissions/pptx-merger/scripts/verify_pptx.py:55

  • [Content_Types].xml is parsed without handling XMLSyntaxError. A malformed or truncated package will raise and crash the validator rather than returning a clean structural error, which defeats the purpose of a "hard gate" script on untrusted inputs.
            ct_bytes = z.read("[Content_Types].xml")
            root = etree.fromstring(ct_bytes, _SAFE_PARSER)
            # The Types element MUST be in the default (unprefixed) namespace.
            if root.prefix is not None:
                errors.append(

submissions/pptx-merger/scripts/verify_pptx.py:90

  • ppt/presentation.xml (and each slideMaster*.xml) is parsed without guarding against XMLSyntaxError. For corrupted inputs this will raise and abort validation instead of reporting the specific structural problem and returning a non-zero exit code.
        # presentation.xml sanity: masters and slides have required ids.
        if "ppt/presentation.xml" in names:
            pr = etree.fromstring(z.read("ppt/presentation.xml"), _SAFE_PARSER)
            gids = []
            ml = pr.find(f".//{{{NS_P}}}sldMasterIdLst")

submissions/pptx-merger/SKILL.md:23

  • SKILL.md contains substantial overview/justification prose (e.g., background on corruption causes) that is human-facing documentation rather than runtime instructions. Per repo guidance, that content should live in README.md, while SKILL.md should open into the agent's activation/procedure/decision rules to conserve runtime context.
# PPTX Merger — corruption-safe deck merge for agent sandboxes

Combine PowerPoint decks into a single file that opens cleanly in PowerPoint.
The skill is built for the case where decks arrive as **base64** (e.g. from the
SharePoint *Get file content using path* connector) and must be merged inside a
sandbox and handed back out. It exists because the naive path corrupts files in
two places: binary bytes get destroyed when pushed through a text/UTF-8 codec,
and careless merging produces packages PowerPoint refuses to open. Each of the
four scripts does one job, verifies its own output, and fails loudly rather than
passing bad data forward.

submissions/pptx-merger/scripts/verify_pptx.py:16

  • The module docstring's usage line omits --no-render, but the CLI supports it and SKILL.md documents it. Keeping the docstring in sync avoids confusing users/agents who rely on __doc__ output.
Usage:
    python verify_pptx.py <file.pptx> [--render] [--json]

--render forces the render check on (it is on by default when soffice exists).
Exit code 0 only if every enabled check passes.

submissions/pptx-merger/README.md:23

  • This README is scoped to Copilot Studio (metadata.json), where lxml is typically preinstalled (see submissions/commenting-content/references/REFERENCE-DOCX.md:60). Suggesting pip install ... --break-system-packages can be misleading and may not be possible/appropriate in the sandbox. Prefer documenting lxml as a required dependency (already present in the sandbox) and reserve pip install for local runs.
## Requirements

- **`lxml`** — merge and validate steps
  (`pip install lxml --break-system-packages` if not already in the sandbox).
- **LibreOffice (`soffice`)** — the render gate in step 3. If unavailable,
  validation runs structural-only and says so.
- Ingest and export use the Python standard library only.

Comment thread submissions/pptx-merger/scripts/pptx_merge.py
Updated the verify_pptx.py script to enhance verification levels and improve error handling. Added support for load checks and refined argument parsing for better usability.

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

Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.

Suppressed comments (5)

submissions/pptx-merger/SKILL.md:18

  • SKILL.md includes human-facing overview/rationale text (e.g., the multi-paragraph explanation of why the skill exists). Repo guidance expects human-facing overview/setup content to live in README.md, with SKILL.md kept as lean, agent-facing runtime instructions.
# PPTX Merger — corruption-safe deck merge for agent sandboxes

Combine PowerPoint decks into a single file that opens cleanly in PowerPoint.
The skill is built for the case where decks arrive as **base64** (e.g. from the
SharePoint *Get file content using path* connector) and must be merged inside a

submissions/pptx-merger/SKILL.md:94

  • This says LibreOffice absence causes the render check to be skipped and still treated as a pass, but verify_pptx.py fails by default when soffice is missing (unless --allow-no-render is provided). The instructions should match the script’s behavior so agents don’t mis-handle validation failures in environments without LibreOffice.
render produces a non-empty PDF. Exit `0` only when every enabled check passes. If
LibreOffice is absent the render check is skipped and clearly flagged rather than
claimed as a pass.

submissions/pptx-merger/SKILL.md:86

  • The verify_pptx.py usage here uses --no-render, but the script implements --allow-no-render / --require-render. Agents following this will pass an unknown flag.

This issue also appears on line 92 of the same file.

python scripts/verify_pptx.py <file.pptx> [--json] [--no-render]

submissions/pptx-merger/README.md:21

  • README claims validation falls back to structural-only when LibreOffice is unavailable, but verify_pptx.py fails by default if soffice is missing (unless --allow-no-render is passed). This mismatch will confuse adopters in Copilot Studio where LibreOffice may not be installed.
- **LibreOffice (`soffice`)** — the render gate in step 3. If unavailable,
  validation runs structural-only and says so.

submissions/pptx-merger/scripts/verify_pptx.py:209

  • structural_checks() parses slide master XML without catching XMLSyntaxError. A malformed slide master will crash the verifier instead of returning a structured error list.
                for n in sorted(names):
                    if n.startswith("ppt/slideMasters/slideMaster") and n.endswith(".xml"):
                        m = etree.fromstring(z.read(n), _SAFE_PARSER)
                        ll = m.find(f".//{{{NS_P}}}sldLayoutIdLst")

Comment thread submissions/pptx-merger/scripts/pptx_merge.py Outdated
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

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

Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.

Suppressed comments (7)

submissions/pptx-merger/SKILL.md:18

  • SKILL.md includes an extended overview/justification section (e.g., the heading and explanatory paragraphs) that reads like human-facing documentation. Per repo submission guidance, SKILL.md should stay lean and agent-facing at runtime, with overview/adoption/context moved to the submission README.md (which you already have).
# PPTX Merger — corruption-safe deck merge for agent sandboxes

Combine PowerPoint decks into a single file that opens cleanly in PowerPoint.
The skill is built for the case where decks arrive as **base64** (e.g. from the
SharePoint *Get file content using path* connector) and must be merged inside a

submissions/pptx-merger/scripts/verify_pptx.py:277

  • --require-render is advertised as an explicit form of the default behavior, but it currently has no effect (and it can be combined with --allow-no-render, producing ambiguous intent). Making these flags mutually exclusive and using args.require_render in the computed render_required field avoids confusing/contradictory CLI states.
    ap = argparse.ArgumentParser(
        description="Verify a PPTX. Render is required unless --allow-no-render.")
    ap.add_argument("pptx")
    ap.add_argument("--allow-no-render", action="store_true",
                    help="Downgrade to structural verification when LibreOffice "
                         "is unavailable, instead of failing.")
    ap.add_argument("--require-render", action="store_true",
                    help="Explicit form of the default behaviour.")
    ap.add_argument("--json", action="store_true")

submissions/pptx-merger/SKILL.md:94

  • The verify_pptx.py usage snippet references --no-render, but the script actually implements --allow-no-render. The surrounding text also says missing LibreOffice will be skipped and still pass, but verify_pptx.py fails by default when LibreOffice is missing unless --allow-no-render is provided. This mismatch will cause agents to invoke validation incorrectly.
```bash
python scripts/verify_pptx.py <file.pptx> [--json] [--no-render]
**submissions/pptx-merger/SKILL.md:116**
* Step 4 calls `verify_pptx.py` without accounting for environments where LibreOffice isn't installed. As implemented, validation fails without `--allow-no-render` when `soffice`/`libreoffice` is missing, so the step order should reflect the supported invocation paths.
  1. verify_pptx.py merged.pptx → must pass, or stop and report the errors.
**submissions/pptx-merger/README.md:21**
* README says validation “runs structural-only” when LibreOffice is unavailable, but `scripts/verify_pptx.py` currently fails by default if `soffice`/`libreoffice` is missing unless `--allow-no-render` is provided. This mismatch can mislead adopters about runtime requirements in Copilot Studio.
  • LibreOffice (soffice) — the render gate in step 3. If unavailable,
    validation runs structural-only and says so.
**submissions/pptx-merger/scripts/verify_pptx.py:233**
* `load_check()` reads `prs.slides._sldIdLst`, which is a private python-pptx attribute and may break across versions. Using the public `len(prs.slides)` keeps the check stable while reporting the same value.
    prs = Presentation(str(path))
    n_slides = len(prs.slides._sldIdLst)
    n_masters = len(prs.slide_masters)
    n_layouts = sum(len(m.slide_layouts) for m in prs.slide_masters)
**submissions/pptx-merger/scripts/b64_to_pptx.py:58**
* `_has_replacement_chars()` treats a `UnicodeDecodeError` as if the input contains U+FFFD and reports the “replacement characters” corruption message. In practice this happens when the argument points to a non-text/binary file, and the script already has a clearer “not valid UTF-8 text” error path later. Returning `False` here lets the later error message fire and avoids a misleading diagnosis.
try:
    txt = p.read_text(encoding="utf-8", errors="strict") if is_file else arg
except UnicodeDecodeError:
    return True
return "\ufffd" in txt
</details>

@adrianatruji

Copy link
Copy Markdown
Collaborator

Thanks for this Angara V L Gopala Sandeep (@hisandeepangara).

Sharing some thoughts after some testing and understanding of the scenario described. The corruption the skill targets right now is a connector limitation, and it persists even with the skill installed. Merging two decks straight from the SharePoint 'Get file content' action, the content comes back through a text/UTF-8 channel instead of clean base64. It gets auto-saved as a .txt, and every byte above 0x80 becomes U+FFFD before any script runs. b64_to_pptx.py correctly detects that and stops, which is the right behavior, but the merge cannot complete either way because the bytes are already lost. We observe this same result with or without the skill, so it's a core gap that we are trying to push to fix directly.

The one path where merging does work is attaching the decks directly as clean binary (uploading the files), and that merges fine on its own without the skill doing the heavy lifting there. Could you help us pin down the exact scenario that the skill is aiming for as a workaround? If there's a genuine gap it fills, let's name it precisely with the caveats. Otherwise, it may be cleaner to hold until the connector issue is sorted. Hope this makes sense.

Thank you!

@hisandeepangara

Copy link
Copy Markdown
Contributor Author

Adriana Trujillo (@adrianatruji) Thanks so much for spending sometime on this.

What does work for me is, I use a Power Automate flow with Get file content using path, and the flow returns clean base64 to the agent. Inputs can be as simple as the SharePoint download URLs for the decks. I've attached a screenshot of the agent I built in Copilot Studio on this pattern — it merges reliably and returns the merged deck with no corruption.

Given that's the scenario that actually works, could you help me shape where this should go from here?

image

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.

4 participants