feat(skills): add 6 PowerPoint skill enhancements#1481
feat(skills): add 6 PowerPoint skill enhancements#1481
Conversation
- Add validate_geometry.py for structural margin, gap, and overflow checks - Add style["colors"] population from themes for content-extra.py portability - Add generate_themes.py for YAML-driven theme variant generation - Add --dry-run pre-flight check to build_deck.py - Add embed_audio.py for WAV embedding into PPTX slides - Add export_svg.py for PPTX-to-SVG export via LibreOffice and PyMuPDF - Wire geometric validation into Invoke-PptxPipeline.ps1 Validate action - Update SKILL.md with new script docs and architecture table entries - Update pptx.instructions.md with geometric validation and color palette docs
- Fix E501 line-length violations in build_deck.py, embed_audio.py, and validate_geometry.py - Apply ruff format to all 5 new/modified scripts
- test_validate_geometry.py: 35 tests covering geometry checks, parser, report generation, and main entry point - test_generate_themes.py: 18 tests covering hex/RGB remap, theme loading, directory processing, and full theme generation - test_embed_audio.py: 18 tests covering audio discovery, embedding, poster frame creation, and run function - test_export_svg.py: 11 tests covering parser, LibreOffice detection, slide number parsing, and error paths Coverage: 89% (above 85% threshold)
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Review: feat(skills): add 6 PowerPoint skill enhancements
This PR adds six meaningful enhancements to the PowerPoint skill (geometry validation, audio embedding, SVG export, theme generation, dry-run mode, and RGB remap). The work is substantive and the new test coverage is generally solid. However, there are template compliance issues that must be resolved before this is ready for merge, plus several code-quality violations in the new scripts.
❌ Issue Alignment
Closes #1482is present at the top of the PR body. The issue could not be read directly, but the PR description itself is detailed and maps clearly to the stated changes. No blocking finding here.
❌ PR Template Compliance
The PR body does not follow .github/PULL_REQUEST_TEMPLATE.md. The template structure has been replaced with a custom ## Summary / ## Changes / ## Testing / ## Motivation format. The following required sections are missing:
| Required Section | Status |
|---|---|
## Type of Change checkboxes |
❌ Not present — no checkboxes checked |
## Checklist > Required Checks |
❌ Not present |
## Checklist > AI Artifact Contributions |
❌ Not present — required because SKILL.md and .github/instructions/experimental/pptx.instructions.md were modified |
## Checklist > Required Automated Checks |
❌ Not present — no evidence that npm run validate:skills, npm run lint:md, or npm run lint:py were run |
## Security Considerations |
❌ Not present |
Action required: Replace the PR description with content that follows the template structure. At minimum:
- Check
Copilot skill(or the appropriate AI artifact type) under Type of Change - Complete the AI Artifact Contributions checklist items
- Complete or acknowledge the Required Automated Checks items
- Fill in the Security Considerations section (the new scripts call
subprocess.runwith external process paths — worth documenting)
⚠️ Coding Standards
Python instructions (python-script.instructions.md) and the established shared-utilities pattern in this skill apply to all four new scripts.
1. export_svg.py and generate_themes.py — Duplicate shared utilities (see inline comments)
Both scripts re-define EXIT_SUCCESS/FAILURE/ERROR and configure_logging. validate_geometry.py and embed_audio.py — added in the same PR — correctly import these from pptx_utils. This inconsistency should be resolved before merge.
2. export_svg.py — sys.exit() inside internal helpers (see inline comment)
convert_pptx_to_pdf() and export_pdf_to_svg() call sys.exit() directly rather than raising exceptions. This violates the single-entry-point exit pattern required by the Python instructions, prevents these functions from being composed or reused, and forces unit tests to use pytest.raises(SystemExit) instead of asserting on return values. test_export_svg.py::TestRun::test_missing_libreoffice already demonstrates this friction.
3. generate_themes.py — Fragile YAML patching (see inline comment)
update_style_metadata() uses regex substitution to update YAML fields. This will corrupt or silently skip updates for any YAML file using single-quoted strings, multi-line scalars, or special characters in values. A yaml.safe_load / modify / yaml.dump round-trip is the correct approach.
4. build_deck.py — print() mixed with logger in dry-run block (see inline comment)
Three print() calls in the new dry-run section bypass the logging infrastructure used everywhere else in the file. These should use logger.error() / logger.info() and return exit codes rather than calling sys.exit() directly inside the function.
💡 Code Quality Notes (non-blocking)
generate_themes.py :: remap_rgb_in_python: The hex string argument passed asold_hexis not validated for length or format before slicing withold_bare[0:2],[2:4],[4:6]. A string shorter than 6 characters will raise anIndexErrorat runtime. A guard likeif len(old_bare) != 6: continuewould add robustness.validate_geometry.py :: check_adjacent_gaps: The gap-check logic currently checks vertical proximity only. Horizontally crowded shapes will not be flagged. This may be intentional (left for a follow-up), but worth a comment in the code noting the current scope.- New standalone scripts not in
Invoke-PptxPipeline.ps1:embed_audio.py,export_svg.py, andgenerate_themes.pyare documented as CLI scripts inSKILL.mdbut are not integrated into the PowerShell pipeline orchestrator. This is fine if they are intended to be invoked directly, but the SKILL.md should make that distinction explicit so users understand when to use the pipeline vs. the standalone scripts.
📋 Summary
Must fix before merge:
- Restore the PR description to follow
.github/PULL_REQUEST_TEMPLATE.md— fill in Type of Change, AI Artifact Contributions, Required Automated Checks, and Security Considerations export_svg.py: importEXIT_*/configure_loggingfrompptx_utils; replacesys.exit()inside helper functions with exceptions propagated torun()generate_themes.py: importEXIT_*/configure_loggingfrompptx_utils; use YAML round-trip inupdate_style_metadatabuild_deck.py: replaceprint()withloggercalls in the dry-run block
Suggested follow-ups (non-blocking):
- Hex-length guard in
remap_rgb_in_python - Document horizontal-gap limitation in
check_adjacent_gaps - Clarify standalone vs. pipeline usage scope in
SKILL.md
Note
🔒 Integrity filter blocked 1 item
The following item were blocked because they don't meet the GitHub integrity level.
- #1482
issue_read: has lower integrity than agent requires. The agent cannot read data with integrity below "approved".
To allow these resources, lower min-integrity in your GitHub frontmatter:
tools:
github:
min-integrity: approved # merged | approved | unapproved | none
katriendg
left a comment
katriendg
left a comment
There was a problem hiding this comment.
Thanks for this contribution, @auyidi1!
Valuable addition to the PowerPoint skill, especially based on your usage of the skill.
Left a minor comment, approving as long as you can address the PR comment feedback from copilot. Thanks.
- export_svg.py: replace sys.exit() with LibreOfficeError exception, import constants from pptx_utils, remove duplicate configure_logging - generate_themes.py: single-pass remap_hex_in_text to prevent chain remapping, yaml.safe_load round-trip in update_style_metadata, import constants from pptx_utils, remove duplicate configure_logging - build_deck.py: replace print() with logger in dry-run, replace sys.exit() with return EXIT_*, import EXIT_*/configure_logging/logger - test_export_svg.py: assert return code instead of SystemExit - test_generate_themes.py: add chain remapping and empty map tests
There was a problem hiding this comment.
PR Review Summary
Three issues require changes before this PR is ready to merge, plus one suggestion worth considering. The overall shape of the additions is solid — the four new scripts are well-structured, include tests, follow copyright conventions, and integrate cleanly into the existing pipeline. The concerns below are targeted and actionable.
Issue Alignment
Issue #1482 was not readable by this reviewer (integrity policy). The PR description, stated motivation, and change set are internally consistent and self-explanatory, so the review proceeds on the basis of the PR content.
PR Template Compliance ⚠️
The PR body does not follow the repository's PR template at .github/PULL_REQUEST_TEMPLATE.md. The following required sections are entirely absent:
| Missing Section | Why Required |
|---|---|
| Type of Change checkboxes | At minimum "New feature", "Script/automation", "Copilot skill", and "Copilot instructions" should be checked |
| AI Artifact Contributions checklist | SKILL.md and pptx.instructions.md are modified; the three AI artifact checklist items must be evaluated |
| Required Automated Checks checklist | npm run lint:md, npm run spell-check, npm run validate:skills, npm run lint:md-links, npm run lint:ps, npm run plugin:generate, npm run docs:test must all be run and checked off |
| Security Considerations | The three security checkboxes must be completed |
The Summary / Changes / Testing / Motivation structure used here is informative but does not replace the template. Please re-submit the PR description using the template as a base and check the appropriate boxes.
Code Quality Findings 🔍
Three code-level issues are flagged as inline comments. Two are blocking; one is a suggestion.
🔴 Blocking — generate_themes.py: chain remapping in remap_rgb_in_python
remap_rgb_in_python iterates over the color map and applies re.sub sequentially. When a target value in the map is also a source value, a previous substitution's output gets remapped again in a later iteration. remap_hex_in_text in the same file explicitly avoids this with a single-pass regex; remap_rgb_in_python must use the same approach.
See inline comment on generate_themes.py lines 97–130.
🔴 Blocking — build_deck.py: dry-run output is silently discarded
The --dry-run block reports per-slide status via logger.info(), but build_deck.py never calls configure_logging(). Without a logging handler, INFO messages are silently discarded at runtime — the feature as documented in SKILL.md ("Reports per-slide status") does not work.
See inline comment on build_deck.py lines 1116–1181.
🟡 Non-blocking — build_deck.py: themes[0] used unconditionally for style["colors"]
build_slide() always populates style["colors"] from themes[0] regardless of which theme applies to the current slide. For multi-theme decks (the primary motivation of this PR), slides assigned to themes other than the first one receive the wrong color palette. Additionally, mutating the shared style dict has a cumulative side effect across slides. A per-slide theme lookup using a local copy is the safer approach.
See inline comment on build_deck.py lines 967–981.
💡 Suggestion — validate_geometry.py: left/top boundary overflow not detected
check_boundary_overflow only checks right-edge and bottom-edge overflow; shapes with left < 0 or top < 0 are not reported as overflows. These are caught as margin warnings by check_edge_margins when margin > 0, but they are architecturally out-of-scope for the overflow checker. Consider adding the left/top checks for completeness.
See inline comment on validate_geometry.py lines 87–115.
Coding Standards
- Python scripts follow the copyright header, Google docstring,
argparse/pathlib, and exit-code conventions frompython-script.instructions.md. ✅ - All new scripts import and call
configure_logging— exceptbuild_deck.py(blocking issue above). ✅ /⚠️ embed_audio.pycorrectly wrapscreate_poster_frame()intry/finallyfor cleanup. ✅Invoke-PptxPipeline.ps1patch follows PowerShell conventions (Write-Host,$LASTEXITCODEchecks, no aliases). ✅SKILL.mdandpptx.instructions.mdadditions are clear and well-structured. ✅fuzz_harness.pyexists and thepyproject.tomlhas the requiredfuzzdependency group andpython_filesdiscovery setting. ✅
Action Items
- Re-submit PR description using the repository PR template — fill in all required checkbox sections.
- Fix chain remapping in
remap_rgb_in_python(see inline comment). - Add
configure_loggingcall inbuild_deck.py'smain()to make dry-run output visible (see inline comment). - (Recommended) Fix per-slide theme color resolution in
build_slide()to avoid always usingthemes[0](see inline comment).
Note
🔒 Integrity filter blocked 1 item
The following item were blocked because they don't meet the GitHub integrity level.
- #1482
issue_read: has lower integrity than agent requires. The agent cannot read data with integrity below "approved".
To allow these resources, lower min-integrity in your GitHub frontmatter:
tools:
github:
min-integrity: approved # merged | approved | unapproved | none
There was a problem hiding this comment.
PR Review: feat(skills): add 6 PowerPoint skill enhancements
The implementation itself is well-structured — four focused scripts with clear responsibilities, proper copyright headers, pptx_utils reuse, consistent exit-code semantics, and good integration into the existing PowerShell pipeline. The approach to geometric validation and theme generation addresses real pain points. However, several required process and coding-standards items must be resolved before merge.
❌ Issue Alignment
Issue #1482 could not be read due to repository access constraints. The PR description is detailed and the scope appears internally consistent, but maintainers should verify the linked issue is fully addressed.
❌ PR Template Compliance
The PR body replaces the entire template with custom sections (Summary, Changes, Testing, Motivation). While the content is informative, the required template structure is absent:
-
Type of Change — no checkboxes selected. The template requires at least one checkbox from the Type of Change section. Based on the diff, the following should be checked:
New featureScript/automation (.ps1, .sh, .py)Copilot instructions (.github/instructions/*.instructions.md)Copilot skill (.github/skills/*/SKILL.md)
-
Sample Prompts section missing. The template states this section is required when any AI Artifact checkbox (Copilot skill or instructions) is checked. It must include User Request, Execution Flow, Output Artifacts, and Success Indicators for the skill enhancements.
-
AI Artifact Contributions checklist absent. The checklist requires attestation that:
/prompt-analyzewas used to review the contribution- All feedback from
prompt-builderreview was addressed - The contribution follows common standards and type-specific requirements
-
Security Considerations section missing. Required checkboxes — sensitive data attestation and dependency security review — are not present.
-
Checklist > Required Checks missing. The automated check items (
npm run lint:md,npm run spell-check,npm run validate:skills,npm run lint:ps,npm run plugin:generate, etc.) must be present and checked to confirm CI gates passed.
⚠️ Coding Standards
Python Test Instructions — test method naming (all four new test files)
Per .github/instructions/**/*.py, test methods must follow the BDD format test_given_context_when_action_then_expected. All test methods across test_embed_audio.py, test_export_svg.py, test_generate_themes.py, and test_validate_geometry.py use short descriptive names instead. See the inline comment on test_embed_audio.py line 76 for a rename example.
Python Script Instructions — import placement (test_embed_audio.py line 21)
import struct appears inside the _make_wav_bytes helper function. Standard library imports must appear at module level. See the inline comment on line 21.
⚠️ Code Quality
Coverage gaps (Codecov report)
export_svg.py: 53.65% patch coverage (57 lines missing). The LibreOffice conversion path is not tested with mocks for the failure cases —find_libreofficereturningNone, non-zero subprocess exit code, and missing PDF output. See the inline comment ontest_export_svg.pyline 85.build_deck.pydry-run additions: 16.66% patch coverage (35 lines missing). The--dry-runpath including YAML parse errors,ContentExtraErrorhandling, and the success/failure return values needs dedicated test coverage.
The project's pyproject.toml sets fail_under = 85; these gaps risk failing the coverage gate.
✅ Positive Observations
- All new Python scripts follow the correct file structure: copyright header → module docstring → standard library imports → third-party imports →
logger = logging.getLogger(__name__)→create_parser()→ main logic →if __name__ == "__main__"guard. - Exit-code semantics (
EXIT_SUCCESS = 0,EXIT_FAILURE = 1for warnings,EXIT_ERROR = 2for runtime errors) are consistent across scripts and correctly handled inInvoke-PptxPipeline.ps1. conftest.py-resident shared fixtures (blank_slide,blank_presentation) are properly reused rather than duplicated.generate_themes.pyandvalidate_geometry.pyhave strong test coverage (85–97%).
Action Items
- Rewrite the PR description using the repository PR template. Fill in all required sections including Type of Change, Sample Prompts, AI Artifact Contributions checklist, Security Considerations, and Required Automated Checks.
- Rename all test methods across the four new test files to follow the
test_given_context_when_action_then_expectedBDD convention. - Move
import structto the module-level import block intest_embed_audio.py. - Add mocked tests for the LibreOffice error paths in
test_export_svg.pyto raise coverage above 85%. - Add tests for the
--dry-runerror branches inbuild_deck.py(YAML parse failure,ContentExtraError, empty slide list).
Note
🔒 Integrity filter blocked 1 item
The following item were blocked because they don't meet the GitHub integrity level.
- #1482
issue_read: has lower integrity than agent requires. The agent cannot read data with integrity below "approved".
To allow these resources, lower min-integrity in your GitHub frontmatter:
tools:
github:
min-integrity: approved # merged | approved | unapproved | none- generate_themes.py: single-pass remap_rgb_in_python to prevent chain remapping (matches remap_hex_in_text approach) - build_deck.py: add configure_logging() call and --verbose flag so dry-run output is visible; per-slide theme color lookup using themes[].slides instead of always using themes[0]; use shallow copy to avoid mutating shared style dict - validate_geometry.py: add left/top boundary overflow detection for shapes extending off-screen with negative positions - test_generate_themes.py: add chain remap and empty map tests for remap_rgb_in_python - test_validate_geometry.py: add left/top overflow detection tests
There was a problem hiding this comment.
Review Summary
This PR adds 6 well-motivated PowerPoint skill enhancements with solid implementation quality overall. The scripts are clearly structured, follow copyright conventions, include docstrings, and ship with comprehensive test suites. Two blocking issues must be addressed before merge.
Issue Alignment
Linked issue #1482 is referenced as Closes #1482. ✅
PR Template Compliance ❌
The PR description does not follow the repository's pull request template (.github/PULL_REQUEST_TEMPLATE.md). Several required sections are absent or unfilled:
Type of Change — no checkboxes checked
None of the Type of Change checkboxes are checked. Based on the diff, the following should be selected at minimum:
[x]Script/automation (.ps1,.sh,.py) — four new Python scripts and one modified PS1[x]Copilot skill (SKILL.md) —SKILL.mdis modified[x]Copilot instructions (.github/instructions/*.instructions.md) —pptx.instructions.mdis modified
Checklist section missing
The Required Checks, AI Artifact Contributions, and Required Automated Checks sub-sections from the template are absent. Required automated checks that must pass before merge include:
npm run lint:md
npm run spell-check
npm run lint:frontmatter
npm run validate:skills
npm run lint:md-links
npm run lint:ps
npm run plugin:generate
Security Considerations section missing
The template requires confirming that the PR does not contain sensitive information and that any new dependencies have been reviewed for security. This section should be added.
Action required: Update the PR description to fill in all required template sections.
Coding Standards ❌
export_svg.py — duplicates parse_slide_filter from pptx_utils
export_svg.py defines a local parse_slide_numbers function (line 144) that duplicates functionality already provided by parse_slide_filter in pptx_utils, which is used by both embed_audio.py and validate_geometry.py. See the inline comment for the suggested fix.
test_export_svg.py — @patch decorator instead of mocker fixture
New test code must use pytest-mock's mocker.patch() per the Python test instructions. @patch from unittest.mock is reserved for migrating existing tests only. pytest-mock>=3.14 is already present in pyproject.toml. See the inline comment for the migration pattern.
Code Quality ✅
No security vulnerabilities, logic errors, resource leaks, or breaking changes were identified. Specific observations:
validate_geometry.py: Clean separation of check functions, proper use ofEXIT_ERROR/EXIT_FAILUREexit codes, accent bar exemption is well-documented.build_deck.pydry-run:style = {**style, "colors": style_colors}creates a new dict per slide rather than mutating the sharedstyle; this is correct and intentional.embed_audio.py:create_poster_framecorrectly wraps the temp file lifecycle in atry/finallyblock.generate_themes.py: Single-pass regex inremap_hex_in_textandremap_rgb_in_pythoncorrectly prevents chain-remapping bugs and is well-tested.Invoke-PptxPipeline.ps1: Step 2b integration mirrors the existing Step 2a pattern correctly; exit code semantics (1 = warnings, 2 = error) align withvalidate_geometry.py.
Action Items
- Update PR description to check the correct Type of Change boxes, add the Checklist section, and add the Security Considerations section.
export_svg.py: Replaceparse_slide_numberswithparse_slide_filterfrompptx_utils(see inline comment).test_export_svg.py: Migrate@patchdecorator usages tomocker.patch()(see inline comment).
Note
🔒 Integrity filter blocked 1 item
The following item were blocked because they don't meet the GitHub integrity level.
- #1482
issue_read: has lower integrity than agent requires. The agent cannot read data with integrity below "approved".
To allow these resources, lower min-integrity in your GitHub frontmatter:
tools:
github:
min-integrity: approved # merged | approved | unapproved | none- test_embed_audio.py: move import struct to module top per Python coding standards (thread #11) - test_export_svg.py: replace @patch decorator with mocker.patch() per pytest-mock convention (thread #14) - export_svg.py: replace local parse_slide_numbers with shared parse_slide_filter from pptx_utils (thread #15) - test_export_svg.py: remove parse_slide_numbers tests (function removed), drop unused import
There was a problem hiding this comment.
PR Review
Overview
This PR adds 6 well-scoped enhancements to the PowerPoint skill, with solid test coverage (751 tests, 89 % coverage) and clean code style across the new scripts. However, there are three required template compliance items missing and two medium-severity code bugs that need to be resolved before merge.
❌ Issue Alignment
Issue #1482 could not be read due to an access control filter on this review system. The PR description is thorough and the change scope looks internally consistent, so no misalignment is apparent from the available information. If a reviewer with issue access can confirm alignment, this item can be cleared.
❌ PR Template Compliance — Blocking
The following required template sections are absent from the PR description. These are mandatory when any AI Artifact checkbox is checked.
1. Reviewed contribution with prompt-builder agent checkbox not checked
The template under AI Artifacts includes:
* [ ] Reviewed contribution with `prompt-builder` agent and addressed all feedback
```
This checkbox is absent from the PR body. Per contributing guidelines, all instruction and skill contributions must be reviewed with the prompt-builder agent.
**2. Missing `Sample Prompts` section**
The template requires a **Sample Prompts** section when AI artifact checkboxes are checked. The PR body omits this section entirely. At minimum the section should describe how to invoke the modified `.github/instructions/experimental/pptx.instructions.md` and `SKILL.md`.
**3. Missing `AI Artifact Contributions` checklist**
Under the **Checklist** section the template includes:
```
### AI Artifact Contributions
* [ ] Used `/prompt-analyze` to review contribution
* [ ] Addressed all feedback from `prompt-builder` review
* [ ] Verified contribution follows common standards and type-specific requirements
This subsection is not present in the PR checklist.
⚠️ Coding Standards
validate_geometry.py — two findings (see inline comments)
-
check_adjacent_gapsfalse positives in multi-column layouts (line ~196–225): shapes are sorted by top position and consecutive pairs are checked for vertical clearance without verifying horizontal overlap. A left-column shape whose bottom edge is close to the top of a right-column shape will generate a spuriousadjacent_gapwarning. Multi-column or grid layouts will produce many false positives that degrade trust in the validator. A horizontal-overlap guard (h_overlap = min(right_a, right_b) - max(left_a, left_b) > 0.01) is needed before flagging. -
Exit-code mismatch demotes boundary errors to warnings (line ~547):
max_severity == "error"(boundary overflow) still returnsEXIT_FAILURE(1), whichInvoke-PptxPipeline.ps1treats as a non-blocking "warnings found" message. Only exit code 2 stops the pipeline. Boundary overflow should returnEXIT_ERROR(2); margin/gap warnings should returnEXIT_FAILURE(1).
generate_themes.py — two findings (see inline comments)
-
Misleading docstring (line ~76):
remap_hex_in_textandremap_rgb_in_pythonsay "Keys and values must include the leading#" but both functions accept bare hex strings vialstrip("#"). The YAML example in SKILL.md uses bare strings. The docstring constraint is inaccurate. -
Missing
help=string on--verbose(line ~56): every other new script includes a help string for this flag; this one does not, leaving a blank entry in--helpoutput.
✅ Code Quality
- New scripts consistently follow the established
pptx_utilsconventions (configure_logging,EXIT_*constants,parse_slide_filter). embed_audio.pycorrectly wraps the temp-file poster frame in atry/finallyblock.generate_themes.pysingle-pass regex approach correctly prevents chain remapping.export_svg.pyLibreOffice discovery covers macOS, Linux, and Windows with sensible fallback candidates.- Test coverage for all four new scripts; parametrize-heavy test style is consistent with the repo's Python test conventions.
Action Items for the Author
- ☐ Check the
Reviewed contribution with prompt-builder agentcheckbox (or complete the review and then check it). - ☐ Add the Sample Prompts section to the PR description.
- ☐ Add the AI Artifact Contributions checklist items to the Checklist section.
- ☐ Fix
check_adjacent_gapsto guard on horizontal overlap before reporting a gap violation. - ☐ Fix
validate_geometry.pymain()to returnEXIT_ERRORfor "error" severity andEXIT_FAILUREfor "warning" severity. - ☐ (Optional / low priority) Correct the misleading docstring in
remap_hex_in_text/remap_rgb_in_python. - ☐ (Optional / low priority) Add
help=string to--verboseingenerate_themes.py.
Note
🔒 Integrity filter blocked 1 item
The following item were blocked because they don't meet the GitHub integrity level.
- #1482
issue_read: has lower integrity than agent requires. The agent cannot read data with integrity below "approved".
To allow these resources, lower min-integrity in your GitHub frontmatter:
tools:
github:
min-integrity: approved # merged | approved | unapproved | none
There was a problem hiding this comment.
Automated Review — PR #1481: feat(skills): add 6 PowerPoint skill enhancements
This review covers issue alignment, PR template compliance, coding standards, and code quality across all 12 changed files.
📋 Issue Alignment
The PR references issue #1482 in its description. The linked issue could not be fully read during this review. Based on the PR description and diff, the six enhancements (audio embedding, SVG export, theme generation, geometry validation, updated pipeline, and updated instructions) appear to be cohesive skill additions, but the exact issue requirements could not be verified. If issue #1482 captures the full scope of these enhancements, no misalignment is visible in the diff itself.
⚠️ PR Template Compliance — Action Required
Three required template elements are absent from the PR description:
-
Missing
prompt-builderattestation checkbox — The AI Artifacts section in the template includes[ ] Reviewed contribution with \prompt-builder` agent and addressed all feedback`. This item is not present in the submitted PR description. It is required when AI artifacts (instructions or skills) are contributed. -
Missing
## Sample Promptssection — The PR checks bothCopilot instructionsandCopilot skillunder AI Artifacts. The template requires a## Sample Promptssection showing how to invoke the contributed artifacts. This section is entirely absent. Please add at least one sample prompt demonstrating usage of the new skill capabilities (e.g., embedding audio, exporting SVG, or running theme generation). -
Missing
### AI Artifact Contributionschecklist — The template includes three checklist items under this heading:[ ] Used /prompt-analyze to review contribution[ ] Addressed all feedback from \prompt-builder` review`[ ] Verified contribution follows common standards and type-specific requirements
None of these appear in the PR description.
🔍 Coding Standards
Python script shebang (python-script.instructions.md)
All four new Python scripts (embed_audio.py, export_svg.py, generate_themes.py, validate_geometry.py) are missing #!/usr/bin/env python3 as the first line. The instructions require the shebang to precede the copyright header on every .py script. Inline comments have been added to each file.
Undocumented dependency (prompt-builder.instructions.md — Skill Authoring)
embed_audio.py introduces a Pillow (PIL) dependency for poster frame generation. The SKILL.md ## Embed Audio section does not document this requirement, while the analogous ## Export SVG section explicitly calls out LibreOffice and pymupdf. This creates an inconsistency that will cause import failures for users who don't have Pillow installed. An inline comment has been added to embed_audio.py with the suggested SKILL.md addition.
🐛 Code Quality
Negation style (export_svg.py line 201)
not pptx_path.suffix.lower() == ".pptx" should be pptx_path.suffix.lower() != ".pptx". The current form can be flagged by Ruff SIM201/E714 and is less readable. An inline comment with the fix has been added.
Strict float equality on EMU-converted value (validate_geometry.py line 50)
_is_accent_bar uses top_in == 0.0 to detect top-edge alignment, while every other comparison in that function uses a tolerance (e.g., left_in <= 0.01, abs(width_in - slide_width_in) < 0.01). EMU-to-inch division by 914400 can introduce sub-millimeter floating-point residuals. Using abs(top_in) < 0.001 makes the check consistent with the rest of the function. An inline comment has been added.
✅ What Looks Good
generate_themes.pysingle-pass substitution design (remap_hex_in_text,remap_rgb_in_python) correctly prevents chain remapping — the approach is sound.export_svg.pylazy import offitzwith# noqa: PLC0415is an acceptable pattern for an optional heavy dependency.- All four new scripts include copyright headers,
set -euo pipefailequivalents (from __future__ import annotations), typed parameters, and docstrings. - Test files cover the new scripts with appropriate Arrange/Act/Assert structure.
pptx.instructions.mdadditions are well-structured and follow the existing style.
🔧 Action Items
| # | File | Required? | Description |
|---|---|---|---|
| 1 | PR Description | ✅ Required | Add prompt-builder attestation checkbox |
| 2 | PR Description | ✅ Required | Add ## Sample Prompts section |
| 3 | PR Description | ✅ Required | Add ### AI Artifact Contributions checklist |
| 4 | embed_audio.py:1 |
✅ Required | Add #!/usr/bin/env python3 shebang |
| 5 | export_svg.py:1 |
✅ Required | Add #!/usr/bin/env python3 shebang |
| 6 | generate_themes.py:1 |
✅ Required | Add #!/usr/bin/env python3 shebang |
| 7 | validate_geometry.py:1 |
✅ Required | Add #!/usr/bin/env python3 shebang |
| 8 | SKILL.md + embed_audio.py:27 |
✅ Required | Document pillow as a prerequisite for Embed Audio |
| 9 | export_svg.py:201 |
💡 Recommended | Replace not ... == with != |
| 10 | validate_geometry.py:50 |
💡 Recommended | Replace top_in == 0.0 with abs(top_in) < 0.001 |
Note
🔒 Integrity filter blocked 1 item
The following item were blocked because they don't meet the GitHub integrity level.
- #1482
issue_read: has lower integrity than agent requires. The agent cannot read data with integrity below "approved".
To allow these resources, lower min-integrity in your GitHub frontmatter:
tools:
github:
min-integrity: approved # merged | approved | unapproved | none
There was a problem hiding this comment.
PR Review Summary
Good set of enhancements — the architecture is sound, the single-pass colour remapping logic is well-considered, and test coverage is thorough. One functional bug must be fixed before merging; the remaining items are quality improvements.
Issue Alignment
Issue #1482 could not be read due to access restrictions. Based on the PR description, the changes appear internally consistent with the stated goals (geometry validation, theme generation, audio embedding, SVG export, dry-run mode). No obvious scope creep detected.
PR Template Compliance ✅
All required sections are populated. Type-of-change checkboxes match the actual diff (new Python scripts, PowerShell update, SKILL.md, .instructions.md). The testing section is detailed and credible.
Code Quality — Action Required
🐛 Critical: exit code mismatch breaks pipeline error handling (validate_geometry.py lines 547–549)
main() returns EXIT_FAILURE (1) for both "warning" and "error" severities. Invoke-PptxPipeline.ps1 Step 2b distinguishes them:
if ($LASTEXITCODE -eq 2) { throw "...error..." } # never reached
if ($LASTEXITCODE -eq 1) { Write-Host "...warnings..." }Boundary-overflow errors (severity "error") are silently swallowed as non-fatal warnings; the pipeline never throws. The PR description explicitly states "exit code 1 for warnings, 2 for errors" — but the implementation doesn't match. Fix:
if severity == "error":
return EXIT_ERROR # 2
if severity == "warning":
return EXIT_FAILURE # 1
return EXIT_SUCCESS # 0Code Quality — Non-Blocking Suggestions
embed_audio.py lines 42–43 — AUDIO_TOP = Inches(7.0) places the icon at 7.0–7.3" on a 7.5"-tall slide — within the slide boundary, not off-screen as SKILL.md claims. Use Inches(8.0) (or a negative value) for truly off-screen placement, and update the SKILL.md description to match.
validate_geometry.py line 50 — top_in == 0.0 uses exact float equality. EMU rounding can prevent this from matching shapes flush with the top edge. Use top_in <= 0.01 to match the existing tolerance pattern on left_in.
generate_themes.py line 60 — def load_themes(...) -> dict: uses an unparameterised type. Use -> dict[str, Any]: per the repo's Python 3.11+ typing conventions.
generate_themes.py line 56 — --verbose is the only argument without a help= string. Add help="Enable verbose output" for consistency.
Action Items
- Fix the
validate_geometry.pyexit code logic so errors returnEXIT_ERROR(2) and warnings returnEXIT_FAILURE(1). - Fix the
embed_audio.pyaudio icon coordinates so the shape is genuinely off-screen, and update the corresponding SKILL.md sentence. - (Optional) Address the float equality and typing nits in
validate_geometry.pyandgenerate_themes.py.
Note
🔒 Integrity filter blocked 1 item
The following item were blocked because they don't meet the GitHub integrity level.
- #1482
issue_read: has lower integrity than agent requires. The agent cannot read data with integrity below "approved".
To allow these resources, lower min-integrity in your GitHub frontmatter:
tools:
github:
min-integrity: approved # merged | approved | unapproved | none- add shebangs to embed_audio, export_svg, validate_geometry, generate_themes - fix validate_geometry exit codes to distinguish errors (2) from warnings (1) - add horizontal overlap guard to prevent false positives in multi-column layouts - fix float equality in accent bar detection to use tolerance - move audio icon off-screen and document Pillow dependency in SKILL.md - fix negation style in export_svg, add verbose help and return type in generate_themes - correct docstrings about optional # prefix in color_map keys 🐛 - Generated by Copilot
There was a problem hiding this comment.
Automated Quality Review
Verdict: REQUEST_CHANGES — Two required PR template sections are absent and three code-quality issues need attention before this is ready for human review.
Issue Alignment
The PR references Closes #1482. That issue could not be read in this workflow, so issue-to-code alignment could not be fully verified. The PR description is detailed and internally consistent; no scope creep or obvious gaps are apparent from the description alone.
PR Template Compliance
❌ AI Artifact Contributions checklist section is missing
The PR checks boxes under AI Artifacts (Copilot instructions and Copilot skill), which activates the following required checklist section from the template:
### AI Artifact Contributions
* [ ] Used `/prompt-analyze` to review contribution
* [ ] Addressed all feedback from `prompt-builder` review
* [ ] Verified contribution follows common standards and type-specific requirements
```
This entire subsection is absent from the PR description. Please add it and check the applicable items.
#### ❌ Sample Prompts section is missing
The template requires a **Sample Prompts** section for all AI Artifact contributions:
```
## Sample Prompts (for AI Artifact Contributions)
**User Request:** ...
**Execution Flow:** ...
**Output Artifacts:** ...
**Success Indicators:** ...
This section is completely absent. Please fill it in for the instruction and skill changes (e.g., a natural-language prompt that demonstrates the dry-run validation flow, or the theme generation workflow).
Coding Standards
Three issues found across the new Python scripts; see inline comments for details.
⚠️ embed_audio.py — AUDIO_TOP hardcoded to 8.0" (line 44)
Inches(8.0) is not portable across presentations with non-standard slide heights. Compute this dynamically from prs.slide_height.
⚠️ generate_themes.py — Sequential remap_rgb_in_python → remap_hex_in_text on Python files (line 159)
remap_rgb_in_python already handles quoted "#RRGGBB" strings via its single-pass regex. Calling remap_hex_in_text afterwards on the same output can produce unintended double-substitution when any target color is also a source key in color_map. The per-function "single-pass" guarantee does not cover cross-function composition.
💡 validate_geometry.py — Untyped shape parameters (line 58)
_shape_label, _is_accent_bar, check_boundary_overflow, check_edge_margins, check_adjacent_gaps, and check_title_clearance all omit the BaseShape type annotation. Per the repo's Python instructions, all parameters should carry 3.11+ type hints.
Code Quality
Overall the implementation is well-structured: copyright headers are present, exit codes follow the repo convention, configure_logging/parse_slide_filter are reused from pptx_utils, test coverage is solid (37 + 20 + 18 + 11 = 86 new tests), and the single-pass chain-remapping design is clearly documented and tested.
No security vulnerabilities found. subprocess.run in export_svg.py uses list form (no shell injection risk). yaml.safe_load is used throughout. No secrets exposure.
Required Actions
- Add the AI Artifact Contributions checklist subsection and check the applicable items.
- Add the Sample Prompts section with at least one example showing the new skill capabilities.
- Fix
AUDIO_TOPto be derived fromprs.slide_heightrather than a hardcoded constant. - Resolve the double-processing concern in
process_filefor Python files. - (Non-blocking) Add
BaseShapetype annotations to the six helper functions invalidate_geometry.py.
Note
🔒 Integrity filter blocked 1 item
The following item were blocked because they don't meet the GitHub integrity level.
- #1482
issue_read: has lower integrity than agent requires. The agent cannot read data with integrity below "approved".
To allow these resources, lower min-integrity in your GitHub frontmatter:
tools:
github:
min-integrity: approved # merged | approved | unapproved | none- compute audio_top dynamically from slide height for off-screen placement - add BaseShape type annotations to validate_geometry helper functions - remove chain-remap risk by dropping remap_hex_in_text call on .py files 🐛 - Generated by Copilot
There was a problem hiding this comment.
Automated Quality Review — PR #1481
feat(skills): add 6 PowerPoint skill enhancements
This PR adds validate_geometry.py, embed_audio.py, export_svg.py, generate_themes.py, corresponding test files, and documentation updates to the PowerPoint skill. The core Python implementations are solid and follow project conventions. However, there are two blocking issues — a functional gap and PR template non-compliance — that must be resolved before this is ready for human review.
1. Issue Alignment ✅
The PR links Closes #1482. The six enhancement descriptions in the PR description are coherent with the changed files (4 new scripts, updated pipeline, updated docs). No scope creep identified.
2. PR Template Compliance ❌
Three required items are missing from the PR description:
a) "Sample Prompts" section absent
The PR checks boxes under AI Artifacts (Copilot instructions, Copilot skill). The template requires a Sample Prompts section whenever AI Artifact boxes are checked — it must show how to invoke the modified artifacts. This section is completely absent from the current description.
b) AI Artifact Contributions checklist not completed
Under Checklist → AI Artifact Contributions, all three required items are unchecked:
- Used
/prompt-analyzeto review this contribution - Addressed all feedback from prompt-builder agent review
- Verified this contribution follows common standards and type-specific requirements
c) "Reviewed contribution with prompt-builder agent" checkbox unchecked
Under Type of Change → AI Artifacts, the attestation checkbox for prompt-builder review is unchecked. This is listed as a required step for AI artifact contributions.
Required action: Fill in the Sample Prompts section and complete the AI Artifact Contributions checklist. If the prompt-builder review has not been performed, run it before completing this PR.
3. Coding Standards ⚠️
The new Python scripts (validate_geometry.py, embed_audio.py, export_svg.py, generate_themes.py) follow python-script.instructions.md conventions: correct main() entry-point pattern, typed parameters, configure_logging, parse_slide_filter from pptx_utils, and proper exit code usage. No violations found in the four new scripts.
One non-blocking issue is flagged inline: update_style_metadata in generate_themes.py uses a yaml.safe_load + yaml.dump round-trip that silently destroys comments and reorders keys in style.yaml. See the inline comment for the recommended ruamel.yaml fix.
Invoke-PptxPipeline.ps1 additions follow PowerShell conventions from powershell.instructions.md — copyright header present, [CmdletBinding()] declared, error handling consistent with the existing pattern.
4. Code Quality and Cross-Platform Parity 🔴
Bash script not updated (blocking)
invoke-pptx-pipeline.sh was not modified in this PR, but Invoke-PptxPipeline.ps1 adds Step 2b (geometric validation via validate_geometry.py). This means the validate action silently skips geometric validation for macOS/Linux users — a functional regression for that platform.
Per the skill's own contributing guidance, bash and PowerShell pipeline scripts must be functionally equivalent. An inline comment on the PS1 file shows the equivalent bash block that must be added to invoke_validate_deck() in invoke-pptx-pipeline.sh.
Test file coverage
Four new test files (test_embed_audio.py, test_export_svg.py, test_generate_themes.py, test_validate_geometry.py) are listed as changed files but were not included in the diff payload reviewed. Their presence is confirmed; their content was not inspected. The author should verify test coverage is adequate before merging.
Action Items
| # | Blocking | File | Action |
|---|---|---|---|
| 1 | ✅ Yes | PR description | Add Sample Prompts section and complete AI Artifact Contributions checklist |
| 2 | ✅ Yes | invoke-pptx-pipeline.sh |
Add Step 2b geometric validation to invoke_validate_deck() — see inline comment on PS1 for the equivalent bash block |
| 3 | generate_themes.py line ~197 |
Replace yaml.dump round-trip with ruamel.yaml to preserve comments/ordering — see inline comment |
Note
🔒 Integrity filter blocked 1 item
The following item were blocked because they don't meet the GitHub integrity level.
- #1482
issue_read: has lower integrity than agent requires. The agent cannot read data with integrity below "approved".
To allow these resources, lower min-integrity in your GitHub frontmatter:
tools:
github:
min-integrity: approved # merged | approved | unapproved | none
- switch update_style_metadata to ruamel.yaml for round-trip fidelity - add geometric validation Step 2b to invoke-pptx-pipeline.sh 🐛 - Generated by Copilot
Dependency ReviewThe following issues were found:
License Issues.github/skills/experimental/powerpoint/pyproject.toml
.github/skills/experimental/powerpoint/uv.lock
plugins/hve-core-all/skills/experimental/powerpoint/uv.lock
OpenSSF Scorecard
Scanned Files
|
There was a problem hiding this comment.
Review Summary
This PR adds six well-scoped PowerPoint skill enhancements with solid core implementations. The code quality of the new scripts is generally good — the single-pass regex approach in generate_themes.py, the structured exit-code semantics in validate_geometry.py, and the consistent use of pptx_utils helpers are all positive patterns. Several issues require fixes before this is ready to merge.
Issue Alignment
Issue #1482 could not be read (integrity policy), so alignment is assessed from the PR description alone. The PR description is clear and the scope appears consistent with the stated enhancements — no obvious misalignment. If the issue is accessible to maintainers, please verify coverage of all acceptance criteria.
PR Template Compliance
❌ Missing: AI Artifact Contributions checklist
The PR checks AI Artifact boxes (Copilot instructions, Copilot skill) but the AI Artifact Contributions checklist section is absent from the PR description:
### AI Artifact Contributions
* [ ] Used `/prompt-analyze` to review contribution
* [ ] Addressed all feedback from `prompt-builder` review
* [ ] Verified contribution follows common standards and type-specific requirements
This section is required when any AI Artifact checkbox is checked. Please add it and complete the items.
❌ Missing: Sample Prompts section
The PR template requires a Sample Prompts section (User Request, Execution Flow, Output Artifacts, Success Indicators) when AI Artifact checkboxes are checked. This section is entirely absent.
Coding Standards
See inline comments for specific violations. Summary:
export_svg.pyline 87:os.path.isfileused instead ofPath.is_file()—python-script.instructions.mdrequires exclusive use ofpathlib.Path, noos.path.pptx.instructions.mdline 122: Em-dash (—) used in prose —writing-style.instructions.mdprohibits em dashes; rewrite as two sentences or use a colon.generate_themes.pyline 30: Dual YAML library imports (yaml+ruamel.yaml);ruamel.yamlhas an unresolved license flag from the dependency review bot.
Code Quality
❌ Test coverage below threshold
Codecov patch coverage is 80.00% overall; the 85% threshold is not met:
| File | Patch Coverage | Missing Lines |
|---|---|---|
export_svg.py |
50.42% | 58 |
build_deck.py |
21.27% | 37 |
generate_themes.py |
87.01% | 20 |
embed_audio.py |
87.73% | 13 |
The --dry-run path in build_deck.py and most of export_svg.py's execution paths are untested. See inline comments on both files.
⚠️ ruamel.yaml unknown license
The dependency review bot flagged ruamel.yaml 0.19.1 with a Null license in both pyproject.toml and uv.lock. Although ruamel.yaml is in practice MIT-licensed, the scanner couldn't resolve it. This needs to be resolved — either confirm the license is compatible and address the scanner's gap, or consider removing the dependency (see inline comment on generate_themes.py).
⚠️ Temp file resource management in embed_audio.py
create_poster_frame() returns a Path to a delete=False temp file. Callers (correctly) clean it up with poster.unlink(missing_ok=True). Please verify run() wraps the call in a try/finally so the file is deleted even on early-exit paths (e.g., Presentation load failure, early EXIT_FAILURE returns).
Action Items
- Add the AI Artifact Contributions checklist and Sample Prompts sections to the PR description.
- Add tests for the
--dry-runpath inbuild_deck.pyand the execution paths inexport_svg.pyto bring coverage to ≥ 85%. - Resolve the
ruamel.yamllicense flag (confirm it is allowed, or remove the dependency). - Fix
os.path.isfile→Path.is_file()inexport_svg.py. - Remove the em-dash from
pptx.instructions.md. - Verify
run()inembed_audio.pyhastry/finallycleanup for the poster frame temp file.
Note
🔒 Integrity filter blocked 1 item
The following item were blocked because they don't meet the GitHub integrity level.
- #1482
issue_read: has lower integrity than agent requires. The agent cannot read data with integrity below "approved".
To allow these resources, lower min-integrity in your GitHub frontmatter:
tools:
github:
min-integrity: approved # merged | approved | unapproved | none- replace os.path.isfile with Path.is_file in export_svg.py - consolidate generate_themes.py to use only ruamel.yaml, drop pyyaml import - add dry-run tests for build_deck.py (success, no-slides, yaml-error) - add export_svg.py test coverage (non-pptx, slide filter, pdf-to-svg mock, pathlib) - fix em-dash in pptx.instructions.md 🐛 - Generated by Copilot
There was a problem hiding this comment.
Automated Quality Review
This PR adds six well-scoped PowerPoint skill enhancements with good overall structure, solid test coverage (89.26% / 751 tests), and consistent Python conventions. Three items require changes before merge.
🔗 Issue Alignment
Issue #1482 is linked via Closes #1482. The PR description clearly maps each of the six enhancements to the new and modified files. No scope creep detected.
Note: The linked issue could not be independently verified due to repository access policy. The PR description is taken as the source of truth for requirements.
📋 PR Template Compliance
❌ Required — AI Artifact Contributions checklist missing
The PR checks two AI Artifact type-of-change boxes (Copilot instructions, Copilot skill) but the corresponding AI Artifact Contributions checklist section is entirely absent from the PR body. The template requires:
### AI Artifact Contributions
* [ ] Used `/prompt-analyze` to review contribution
* [ ] Addressed all feedback from `prompt-builder` review
* [ ] Verified contribution follows common standards and type-specific requirements
None of these items appear in the PR, and the first-listed AI Artifact checkbox "Reviewed contribution with prompt-builder agent and addressed all feedback" is also unchecked in the Type of Change section.
❌ Required — Sample Prompts section missing
The template requires a Sample Prompts section whenever AI Artifact boxes are checked (instructions, skill). The section must include User Request, Execution Flow, Output Artifacts, and Success Indicators. It is absent from this PR.
🛠️ Coding Standards
All changed Python files follow the codebase conventions: EXIT_SUCCESS/EXIT_FAILURE/EXIT_ERROR constants, configure_logging(), argparse with create_parser(), module-level logger, pathlib.Path, and if __name__ == "__main__" guards. Copyright headers are present on all new files. ✅
❌ Required — Missing bash and PowerShell wrapper scripts (skill contribution requirement)
The PR template notes: "Skills: Must include both bash and PowerShell scripts." validate_geometry.py is already wired into both Invoke-PptxPipeline.ps1 and invoke-pptx-pipeline.sh and is therefore covered. However, the three remaining standalone utilities (embed_audio.py, export_svg.py, generate_themes.py) are documented with Python-only invocations and have no .ps1 or .sh counterparts. See the inline comment on SKILL.md line 436.
PowerShell analysis rules apply to any new .ps1 file: [CmdletBinding()] param block, $ErrorActionPreference = 'Stop', copyright header, and Write-Host for status output.
🔍 Code Quality
generate_themes.py — No hex-length validation in color maps
remap_hex_in_text and remap_rgb_in_python do not validate that color-map keys and values are exactly 6 hex characters. A malformed entry like "#abc": "#de" silently produces an invalid replacement (#de). See inline comment at line 90.
i️ Step label formatting inconsistency (minor)
Invoke-PptxPipeline.ps1 emits "Step 2b:" while invoke-pptx-pipeline.sh emits "Step 2b/${total_steps}:". The bash format is more informative. See inline comment at line 434.
✅ build_deck.py theme color lookup
The per-slide style = {**style, "colors": style_colors} shallow-copy correctly avoids mutating the shared style dict across slides.
✅ embed_audio.py temp file lifecycle
create_poster_frame() returning a delete=False temp path is correctly paired with a try/finally block in run() that calls poster_frame.unlink(missing_ok=True).
✅ generate_themes.py single-pass color remapping
The regex callback approach in both remap_hex_in_text and remap_rgb_in_python correctly prevents chain-remapping (A→B then B→C producing C).
✅ ruamel.yaml dependency justification
ruamel.yaml is used only in generate_themes.py for round-trip YAML fidelity (preserving comments and key ordering in update_style_metadata). The choice is justified and the package is pinned in uv.lock at version 0.19.1.
✅ Action Items
- Add the AI Artifact Contributions checklist with all three items to the PR body.
- Add the Sample Prompts section (User Request, Execution Flow, Output Artifacts, Success Indicators).
- Check "Reviewed contribution with
prompt-builderagent" or describe why it was not used. - Add cross-platform wrapper scripts for
embed_audio.py,export_svg.py, andgenerate_themes.py. - Add hex-length validation in
remap_hex_in_text(and correspondinglyremap_rgb_in_python).
Note
🔒 Integrity filter blocked 1 item
The following item were blocked because they don't meet the GitHub integrity level.
- #1482
issue_read: has lower integrity than agent requires. The agent cannot read data with integrity below "approved".
To allow these resources, lower min-integrity in your GitHub frontmatter:
tools:
github:
min-integrity: approved # merged | approved | unapproved | none- add hex-value length validation to remap_hex_in_text and remap_rgb_in_python - add cross-platform wrapper scripts for embed_audio, export_svg, generate_themes - fix PS1 step 2b label to include total steps suffix 🐛 - Generated by Copilot
There was a problem hiding this comment.
PR Review: feat(skills) — PowerPoint skill enhancements
This PR adds solid functionality and the Python code is well-structured with thorough test coverage. There are three categories of required fixes before this can merge.
🔍 Issue Alignment
Issue #1482 could not be read (integrity policy), so alignment cannot be automatically verified. A human reviewer should confirm the linked issue's requirements are fully addressed.
⚠️ PR Template Compliance
Three required template items are missing or incomplete:
-
Sample Prompts section is absent. The template requires this section when any AI Artifact checkbox is checked (Copilot instructions and Copilot skill are both checked here). It must include: User Request, Execution Flow, Output Artifacts, and Success Indicators sub-sections showing how to invoke the new skill features.
-
AI Artifact Contributions checklist is unchecked. All three items under
### AI Artifact Contributionsare unchecked:[ ] Used /prompt-analyze to review contribution[ ] Addressed all feedback from prompt-builder review[ ] Verified contribution follows common standards and type-specific requirements
-
prompt-builderreview checkbox is unchecked. Under**AI Artifacts:**, the first item — "Reviewed contribution withprompt-builderagent and addressed all feedback" — is not checked.
❌ Coding Standards — PowerShell
All three new PowerShell wrapper scripts (Invoke-EmbedAudio.ps1, Invoke-ExportSvg.ps1, Invoke-GenerateThemes.ps1) violate the [CmdletBinding()] requirement from powershell.instructions.md:
[CmdletBinding()]with a typedparam()block is required on all scripts.
See inline comments on each file for the exact fix. Also note the convention calls for [Parameter(Mandatory = $true)] (explicit assignment) rather than the bare [Parameter(Mandatory)] shorthand.
🐛 Code Quality — Exit Code Propagation
All three new PowerShell wrapper scripts end with & $python @Args_ and do not propagate the Python process's exit code. $ErrorActionPreference = 'Stop' does not intercept non-zero exit codes from external processes — only PowerShell cmdlet errors. A validate_geometry.py warning (exit 1) or embed_audio.py failure (exit 2) will silently appear successful to any calling script or CI pipeline.
Fix: add exit $LASTEXITCODE as the final line in each wrapper. See inline comments.
💡 Suggestion — Unnecessary Dependency
ruamel.yaml was added to pyproject.toml but is only used for a read-only YAML load in generate_themes.py. pyyaml (already a dependency) is sufficient. See the inline comment on pyproject.toml for the one-line replacement.
✅ What Looks Good
- Python scripts follow
python-script.instructions.mdconventions: copyright headers,create_parser()extraction,main()entry point, Google-style docstrings,EXIT_SUCCESS/EXIT_FAILURE/EXIT_ERRORconstants, and type annotations throughout. - Single-pass regex color remapping in
generate_themes.pycorrectly avoids chain-substitution bugs. validate_geometry.pyexit code semantics (0/1/2) are consistent with the existingvalidate_deck.pypattern and are correctly handled in bothInvoke-PptxPipeline.ps1andinvoke-pptx-pipeline.sh.- Bash wrapper scripts follow
bash.instructions.md:set -euo pipefail, copyright header,err()helper,usage()function, andmain "$@"pattern. - Test coverage (89.26%) exceeds the 85% threshold with 86 new tests across 4 files.
📋 Required Actions
- Add the Sample Prompts section to the PR description.
- Check the three AI Artifact Contributions items and the
prompt-builderreview checkbox. - Add
[CmdletBinding()](withMandatory = $true) to all three new PS1 scripts. - Add
exit $LASTEXITCODEto all three new PS1 scripts. - (Suggested) Remove
ruamel.yamland useyaml.safe_load()instead.
Note
🔒 Integrity filter blocked 1 item
The following item were blocked because they don't meet the GitHub integrity level.
- #1482
issue_read: has lower integrity than agent requires. The agent cannot read data with integrity below "approved".
To allow these resources, lower min-integrity in your GitHub frontmatter:
tools:
github:
min-integrity: approved # merged | approved | unapproved | none🐛 - Generated by Copilot
There was a problem hiding this comment.
PR Review: feat(skills): add 6 PowerPoint skill enhancements
This PR adds six well-structured enhancements to the experimental PowerPoint skill — geometric validation, dry-run mode, theme generation, audio embedding, SVG export, and per-slide theme lookup. The test coverage (89.26%, 751 tests) and the depth of the implementation are commendable. However, there are required template items missing and a few code-level issues that must be addressed before this is merge-ready.
1. Issue Alignment
Issue #1482 could not be read directly. Based on the PR description ("Closes #1482") and the scope of the changes, the implementation appears aligned with adding skill enhancements. No scope creep detected — all changed files are within .github/skills/experimental/powerpoint/ and .github/instructions/experimental/. No assessment of missing issue requirements is possible without reading the issue.
2. PR Template Compliance ❌
Three required template sections are absent or incomplete:
a) Missing: "Reviewed contribution with prompt-builder agent" checkbox (Type of Change → AI Artifacts)
The first checkbox under AI Artifacts reads:
[ ] Reviewed contribution with prompt-builder agent and addressed all feedback
This is not checked in the PR body. Since Copilot instructions and Copilot skill are both checked, the prompt-builder review checkbox is required per the template.
b) Missing: AI Artifact Contributions checklist (Checklist section)
The template's ### AI Artifact Contributions sub-section, which is required when AI artifact checkboxes are checked, is entirely absent from the PR body. The three required items are:
* [ ] Used /prompt-analyze to review contribution
* [ ] Addressed all feedback from prompt-builder review
* [ ] Verified contribution follows common standards and type-specific requirements
c) Missing: Sample Prompts section
The template requires a ## Sample Prompts section for AI artifact contributions, with sub-sections for User Request, Execution Flow, Output Artifacts, and Success Indicators. This section is absent from the PR description.
Please add all three missing items to the PR description before this can be merged.
3. Coding Standards Review ⚠️
Applicable instruction files reviewed: pptx.instructions.md, python-script.instructions.md, python-test.instructions.md, powershell.instructions.md, bash.instructions.md.
All PowerShell scripts include [CmdletBinding()], #Requires -Version 7.0, copyright headers, and comment-based help. Python files follow Google-style docstrings and sys.exit(main()). No material convention violations found beyond the inline comments posted above.
Inline findings (see individual comments):
validate_geometry.py~line 398: Unescaped|in thedescfield may corrupt the Markdown report table — functional bug in output formatting.invoke-pptx-pipeline.sh~line 244: Step numbering is1, 2, 2b, 3withtotal_steps=3— should be1, 2, 3, 4withtotal_steps=4.invoke-pptx-pipeline.sh~line 293 andInvoke-PptxPipeline.ps1~line 457: Verbose flag is not forwarded tovalidate_geometry.pyin Step 2b, inconsistent with other steps.build_deck.py~line 1137:getattr(args, "verbose", False)is unnecessarily defensive — useargs.verbosedirectly (minor).
4. Code Quality and Security ✅
No security vulnerabilities found. Subprocess calls pass arguments as lists (no shell injection). New dependency ruamel-yaml 0.19.1 is hash-pinned in uv.lock. No secrets or credentials exposed.
One informational caveat: embed_audio.py uses python-pptx's add_movie API with mime_type="audio/wav". This API was designed primarily for video and WAV audio compatibility across PPTX viewers (particularly PowerPoint Online and LibreOffice Impress) is limited. This is not a blocking issue but is worth a note in SKILL.md or the docstring so users understand the viewer compatibility constraints.
Required Action Items
- [Required] Add the missing
### AI Artifact Contributionschecklist to the PR description and check all three items. - [Required] Add the
## Sample Promptssection with the four required sub-sections. - [Required] Check or add the "Reviewed contribution with
prompt-builderagent" checkbox under Type of Change → AI Artifacts. - [Required] Fix unescaped
|characters ingenerate_reportdescription/location fields (validate_geometry.py). - [Required] Fix step numbering (
total_steps=4, sequential step labels 1–4) in bothinvoke-pptx-pipeline.shandInvoke-PptxPipeline.ps1. - [Suggested] Propagate verbose flag to
validate_geometry.pyin Step 2b of both pipeline scripts. - [Suggested] Replace
getattr(args, "verbose", False)withargs.verboseinbuild_deck.py. - [Informational] Consider documenting WAV viewer compatibility caveat in
SKILL.md.
Note
🔒 Integrity filter blocked 1 item
The following item were blocked because they don't meet the GitHub integrity level.
- #1482
issue_read: has lower integrity than agent requires. The agent cannot read data with integrity below "approved".
To allow these resources, lower min-integrity in your GitHub frontmatter:
tools:
github:
min-integrity: approved # merged | approved | unapproved | none- renumber pipeline steps sequentially (1,2,3,4) in both bash and PS1 - propagate verbose flag to validate_geometry in both pipelines - escape pipe characters in markdown report table to prevent corruption - replace defensive getattr with direct args.verbose in build_deck and validate_geometry - switch generate_seeds.py from os.path to pathlib 🐛 - Generated by Copilot
There was a problem hiding this comment.
PR Review Summary
This PR adds six well-motivated enhancements to the PowerPoint skill. The Python implementations are clean, the test suite is thorough (751 tests, 89% coverage), and the single-pass color remapping design in generate_themes.py is a thoughtful choice. That said, there are template compliance gaps and a recurring bug across all three new PowerShell wrappers that must be addressed before merging.
Issue Alignment ✅
Closes #1482 is referenced. The scope of changes — four new scripts, four modified files — is consistent with a multi-enhancement issue.
PR Template Compliance ⚠️ — Action Required
Missing: AI Artifact Contributions checklist
The PR checks the Copilot instructions and Copilot skill AI Artifact boxes but omits the required "AI Artifact Contributions" checklist section entirely from the PR description. When AI artifact types are checked, the following items are required:
### AI Artifact Contributions
* [ ] Used `/prompt-analyze` to review contribution
* [ ] Addressed all feedback from `prompt-builder` review
* [ ] Verified contribution follows common standards and type-specific requirements
Additionally, the "Reviewed contribution with prompt-builder agent and addressed all feedback" checkbox — the first item in the AI Artifacts section of the template — is absent from the PR body.
Missing: Sample Prompts section
The template requires a Sample Prompts section (User Request, Execution Flow, Output Artifacts, Success Indicators) whenever AI artifact checkboxes are checked. pptx.instructions.md and SKILL.md were both modified, so this section applies.
Coding Standards ⚠️ — Inline Comments Filed
| File | Issue | Severity |
|---|---|---|
Invoke-EmbedAudio.ps1 line 71 |
Python exit code silently discarded | 🔴 Bug |
Invoke-GenerateThemes.ps1 line 66 |
Python exit code silently discarded | 🔴 Bug |
Invoke-ExportSvg.ps1 line 67 |
Python exit code silently discarded | 🔴 Bug |
export_svg.py lines 115–128 |
No timeout on LibreOffice subprocess.run |
|
generate_themes.py lines 74–76 |
Color map values not validated as strings in load_themes |
Root cause of the PS1 bug: $ErrorActionPreference = 'Stop' does not automatically throw for non-zero exits from external programs — it only applies to PowerShell cmdlets. All three new wrappers end with & $python @Args_ without checking or propagating $LASTEXITCODE, so callers always see success even when the wrapped Python script fails. The fix is exit $LASTEXITCODE (or the if ($LASTEXITCODE -ne 0) { throw ... } pattern used in Invoke-PptxPipeline.ps1).
Code Quality ✅
- Copyright headers,
configure_logging,EXIT_*constants, andparse_slide_filterreuse are consistent throughout. embed_audio.pycorrectly cleans up the temporary poster-frame PNG viatry/finally.- The
--dry-runimplementation inbuild_deck.pyis clean and the per-slide reporting is useful. - The single-pass regex approach in
remap_hex_in_textandremap_rgb_in_pythoncorrectly avoids chain-substitution bugs.
Required Actions
- Add the missing AI Artifact Contributions checklist to the PR description and complete the Sample Prompts section.
- Add
exit $LASTEXITCODEafter& $python@Args_in all three newInvoke-*.ps1wrapper scripts. - Add a
timeout(e.g.timeout=300) to the LibreOfficesubprocess.run()call inexport_svg.pyand handlesubprocess.TimeoutExpired. - Add string type validation for color map values in
load_themes()ingenerate_themes.py.
Note
🔒 Integrity filter blocked 1 item
The following item were blocked because they don't meet the GitHub integrity level.
- #1482
issue_read: has lower integrity than agent requires. The agent cannot read data with integrity below "approved".
To allow these resources, lower min-integrity in your GitHub frontmatter:
tools:
github:
min-integrity: approved # merged | approved | unapproved | none- propagate Python exit code via exit $LASTEXITCODE in all 3 PS1 wrappers - add 300s timeout to LibreOffice subprocess.run with TimeoutExpired handling - validate color map keys and values are strings in load_themes 🐛 - Generated by Copilot
There was a problem hiding this comment.
Advisory review — this PR is from a maintainer. Findings are informational only.
Review Summary
Strong work overall — 751 tests passing at 89.26% coverage, ruff clean, and four well-structured new Python scripts. Three items are worth addressing before merge.
🐛 Bug: Step Counter Off by One (both pipeline scripts)
Both invoke-pptx-pipeline.sh and Invoke-PptxPipeline.ps1 set total_steps / $totalSteps to 4 (no vision) and 5 (with vision), but the invoke_validate_deck / Invoke-ValidateDeck function only executes 3 numbered steps in a non-vision run (export → PPTX property checks → geometric validation). The step count was incremented by 2 when only 1 new step was added.
Effect: a non-vision validate run finishes on "Step 3/4", making it look as though a step was silently skipped. Inline comments on both files have the one-line fix (3/4 instead of 4/5).
⚠️ Coding Standard: validate_geometry.py — Missing Error-Handling Wrapper
validate_geometry.py's main() does not use the run() + try/except KeyboardInterrupt / BrokenPipeError / Exception pattern required by python-script.instructions.md. All three other new scripts (embed_audio.py, export_svg.py, generate_themes.py) implement this correctly. The inline comment on line 495 has the suggested refactor.
💡 Minor Convention: New PS1 Wrapper Scripts — Missing Invocation Guard
Invoke-EmbedAudio.ps1, Invoke-ExportSvg.ps1, and Invoke-GenerateThemes.ps1 run their execution block at module scope without wrapping it in:
if ($MyInvocation.InvocationName -ne '.') {
# execution block
}Per powershell.instructions.md, this guard enables dot-sourcing for Pester tests. The practical impact is low for these thin wrappers (no function definitions to surface), but it is worth applying for consistency with the rest of the skill's PowerShell scripts and to avoid surprising test harness behavior when dot-sourced in the future.
✅ Everything Else Looks Good
- PR template is fully filled; issue reference, type-of-change checkboxes, and AI artifact checkboxes all match the actual diff.
ruamel.yamldependency is correctly reflected in bothpyproject.tomlanduv.lock;YAML(typ="safe")for load andYAML()(round-trip) for write is the correct split.- Single-pass regex callbacks in
remap_hex_in_text/remap_rgb_in_pythonare a deliberate and correct design choice — the inline comments document the intent well. NamedTemporaryFile(delete=False)+finally: poster_frame.unlink(missing_ok=True)cleanup inembed_audio.pyis correct.- Copyright headers, SPDX identifiers,
#Requires -Version 7.0, and$ErrorActionPreference = 'Stop'are present and correctly placed in all new scripts. EXIT_SUCCESS / EXIT_FAILURE / EXIT_ERRORexit-code conventions are consistent across all new Python scripts.
Note
🔒 Integrity filter blocked 1 item
The following item were blocked because they don't meet the GitHub integrity level.
- #1482
issue_read: has lower integrity than agent requires. The agent cannot read data with integrity below "approved".
To allow these resources, lower min-integrity in your GitHub frontmatter:
tools:
github:
min-integrity: approved # merged | approved | unapproved | none- fix total_steps to 3 (no vision) / 4 (with vision) in both pipelines - extract run() + try/except guard in validate_geometry.py main() 🐛 - Generated by Copilot
There was a problem hiding this comment.
PR Review — feat(skills): add 6 PowerPoint skill enhancements
This PR adds meaningful, well-scoped enhancements to the PowerPoint skill: structural geometry validation, theme variant generation, audio embedding, SVG export, a --dry-run pre-flight mode, and per-slide theme color lookup. The implementation quality is generally strong — copyright headers, strict error handling, proper exit codes, single-pass color remapping, and 86 new tests are all in order. However, several required PR template items are absent or incomplete, and there is a functional gap in generate_themes.py that needs to be addressed before merge.
❌ Issue Alignment
Linked issue: Closes #1482
⚠️ The linked issue could not be read due to a repository integrity policy restriction. Issue alignment could not be verified. Please confirm that #1482 explicitly calls for all six enhancements in scope — or split enhancements without a linked requirement into a follow-up.
❌ PR Template Compliance
1. Missing Reviewed contribution with 'prompt-builder' agent attestation
The PR template's AI Artifacts section includes a required first checkbox:
[ ] Reviewed contribution with 'prompt-builder' agent and addressed all feedback
This checkbox is absent from the PR body. Since Copilot instructions and Copilot skill boxes are both checked, the attestation is required.
2. Missing AI Artifact Contributions checklist section
The Checklist section in the PR template includes a dedicated sub-section for AI artifact contributions:
### AI Artifact Contributions
* [ ] Used `/prompt-analyze` to review contribution
* [ ] Addressed all feedback from `prompt-builder` review
* [ ] Verified contribution follows common standards and type-specific requirements
This entire section is absent from the PR body, even though AI artifact checkboxes are checked.
3. Missing Sample Prompts section
The template instructs contributors to provide a ## Sample Prompts section when any AI Artifact box is checked. The section was deleted from the PR body. Since Copilot skill and Copilot instructions are checked, sample prompts for each new script action (dry-run, theme generation, audio embedding, SVG export) should be provided.
⚠️ Coding Standards Review
All Python files follow python-script.instructions.md conventions: copyright headers, #!/usr/bin/env python3, create_parser() + main() pattern, module-level loggers, and configure_logging() from pptx_utils. PowerShell wrappers and bash scripts follow powershell-script.instructions.md and bash-script.instructions.md requirements. fuzz_harness.py is present and pyproject.toml is correctly configured.
One coding standard issue is flagged as an inline comment on generate_themes.py:
remap_rgb_in_pythononly matches double-quoted hex strings ("#RRGGBB"). Single-quoted hex literals ('#RRGGBB') incontent-extra.pywill be silently skipped. This is a functional correctness issue for any color specified asfill_color = '#XXXXXX'.
⚠️ Documentation
One documentation inaccuracy is flagged as an inline comment on pptx.instructions.md:
- The new "Theme Colors in content-extra.py" section describes
style["colors"]as always containing the first theme's color map. The actualbuild_deck.pyimplementation performs a per-slide lookup viathemes[].slidesand only falls back tothemes[0]. The instruction as written would mislead authors creating multi-theme decks.
✅ Code Quality — No Blocking Issues
validate_geometry.py: boundary overflow, edge margin, adjacent gap, and title clearance checks are correctly scoped. Accent bar exemption logic is sound.embed_audio.py: poster frame temp file is correctly cleaned up in afinallyblock; off-screen placement viaprs.slide_height + AUDIO_OFFSCREEN_OFFSETis correct (both are EMU values).export_svg.py:tempfile.TemporaryDirectory()context manager ensures PDF temp files are always cleaned; deferredimport fitzwithLibreOfficeErrorpropagation is acceptable.build_deck.pydry-run: per-slide status reporting is clear;EXIT_ERRORvsEXIT_FAILUREexit code distinction is correctly applied.generate_themes.py: single-pass regex replacement forremap_hex_in_textandremap_rgb_in_pythoncorrectly avoids chain remapping (A→B→C problem).
🔧 Required Actions
- Add the
Reviewed contribution with 'prompt-builder' agentcheckbox to the Type of Change > AI Artifacts section and check it (or explain why this step was skipped). - Add the
### AI Artifact Contributionschecklist under Checklist and check or address each item. - Add a
## Sample Promptssection showing how to invoke each new script action via the skill. - Fix
remap_rgb_in_pythoningenerate_themes.pyto also match single-quoted hex strings — or add an explicit docstring/SKILL.md limitation note with a recommendation to useRGBColor(...)or double-quoted hex incontent-extra.py. - Correct the
pptx.instructions.mddescription to accurately reflect per-slide theme color lookup rather than always using the first theme's map.
Note
🔒 Integrity filter blocked 1 item
The following item were blocked because they don't meet the GitHub integrity level.
- #1482
issue_read: has lower integrity than agent requires. The agent cannot read data with integrity below "approved".
To allow these resources, lower min-integrity in your GitHub frontmatter:
tools:
github:
min-integrity: approved # merged | approved | unapproved | none- extend remap_rgb_in_python to match '#RRGGBB' single-quoted literals - correct pptx.instructions.md to describe per-slide theme color lookup 🐛 - Generated by Copilot
There was a problem hiding this comment.
PR Review: feat(skills): add 6 PowerPoint skill enhancements
This PR has made substantial progress since the first review cycle. The author has addressed the majority of the prior feedback: shebangs are present on all Python scripts, [CmdletBinding()] and exit $LASTEXITCODE are in all three PowerShell wrappers, ruamel.yaml round-trip YAML is used correctly in update_style_metadata, shared pptx_utils constants are imported rather than duplicated, and check_adjacent_gaps now validates horizontal overlap before flagging pairs. That is a significant body of work and it shows.
Two items require resolution before this is ready to merge.
❌ Issue Alignment
Issue #1482 could not be read due to an integrity policy restriction. The PR description is detailed and the changes map clearly to the stated enhancements; no blocking finding here, but manual verification of alignment with the linked issue is recommended.
❌ PR Template Compliance
The PR description is missing required sections that the template mandates when AI Artifact boxes are checked.
Missing item under Type of Change → AI Artifacts:
The first checkbox in the AI Artifacts group is absent from the PR body:
* [ ] Reviewed contribution with `prompt-builder` agent and addressed all feedback
Missing section under Checklist:
The entire ### AI Artifact Contributions section is absent. Because Copilot instructions and Copilot skill are checked, the template requires:
### AI Artifact Contributions
* [ ] Used `/prompt-analyze` to review contribution
* [ ] Addressed all feedback from `prompt-builder` review
* [ ] Verified contribution follows common standards and type-specific requirementsAction required: Add the missing prompt-builder checkbox under Type of Change, and add the ### AI Artifact Contributions checklist section with its three items checked (or unchecked if not yet performed) under Checklist.
⚠️ Coding Standards
Python — validate_geometry.py (see inline comment at line 569):
main() is the only one of the four new scripts that does not include an except Exception as e: fallback. Adding it aligns the error-handling pattern with embed_audio.py, export_svg.py, and generate_themes.py and ensures corrupt-PPTX errors surface as clean log messages rather than unhandled tracebacks.
PowerShell — $args_ variable naming (see inline comment at line 67):
$args_ does not follow the PascalCase convention required by powershell.instructions.md. Use $ScriptArgs (or similar) in all three wrapper scripts.
✅ Code Quality
No new security vulnerabilities, logic errors, or resource leaks were found. The geometry-validation exit-code mapping (EXIT_ERROR for errors, EXIT_FAILURE for warnings) is correct. The YAML round-trip in update_style_metadata preserves comments and key order as intended. The single-pass hex and RGBColor remapping in remap_hex_in_text / remap_rgb_in_python correctly avoids chain substitution.
📋 Required Actions
- PR description — add the missing
prompt-buildercheckbox under AI Artifacts and restore the### AI Artifact Contributionschecklist section. validate_geometry.pyline 569 — addexcept Exception as e: logger.error("Unexpected error: %s", e); return EXIT_FAILUREafter theBrokenPipeErrorhandler.Invoke-EmbedAudio.ps1,Invoke-ExportSvg.ps1,Invoke-GenerateThemes.ps1— rename$args_to$ScriptArgs(or another PascalCase name) in all three files.
Note
🔒 Integrity filter blocked 1 item
The following item were blocked because they don't meet the GitHub integrity level.
- #1482
issue_read: has lower integrity than agent requires. The agent cannot read data with integrity below "approved".
To allow these resources, lower min-integrity in your GitHub frontmatter:
tools:
github:
min-integrity: approved # merged | approved | unapproved | none…lCase ScriptArgs - Add catch-all except Exception handler in validate_geometry.py main() to match sibling scripts (embed_audio, export_svg, generate_themes) so corrupt PPTX errors surface as clean log messages instead of unhandled tracebacks. - Rename $args_ to $ScriptArgs in Invoke-EmbedAudio.ps1, Invoke-ExportSvg.ps1, and Invoke-GenerateThemes.ps1 to comply with PowerShell PascalCase convention. Generated by Copilot
auyidi1
force-pushed
the
users/auyidi/pptx-skill-enhancements
branch
from
b4f44ff to
2daa49c
Compare
There was a problem hiding this comment.
PR Review: feat(skills): add 6 PowerPoint skill enhancements
Significant progress since the previous review passes — the blocking code issues have all been resolved: configure_logging is now wired in build_deck.py, remap_rgb_in_python uses single-pass substitution, per-slide theme color lookup is correctly scoped, left/top overflow detection is present in validate_geometry.py, and export_svg.py no longer calls sys.exit() inside helper functions. The implementation is in good shape overall.
Three process items and one code-quality item remain outstanding.
❌ PR Template Compliance
The PR description is informative but three sections required by .github/PULL_REQUEST_TEMPLATE.md are still absent.
1. "Reviewed contribution with prompt-builder agent" checkbox unchecked
Under Type of Change → AI Artifacts, the template requires:
* [ ] Reviewed contribution with `prompt-builder` agent and addressed all feedback
```
This item is absent from the PR description. It must be evaluated and checked (or explicitly left unchecked with a comment explaining why it was skipped).
**2. Sample Prompts section missing**
The template requires a **Sample Prompts** section whenever Copilot instructions or Copilot skill boxes are checked. This section must include sub-sections for:
- User Request
- Execution Flow
- Output Artifacts
- Success Indicators
**3. Checklist → AI Artifact Contributions subsection missing**
The **Checklist** section is missing the **AI Artifact Contributions** block:
```
### AI Artifact Contributions
* [ ] Used /prompt-analyze to review contribution
* [ ] Addressed all feedback from prompt-builder review
* [ ] Verified contribution follows common standards and type-specific requirements
These three items must be included and evaluated since SKILL.md and pptx.instructions.md are modified in this PR.
⚠️ Coding Standards
PowerShell: Three new wrapper scripts missing required structural elements (inline comments on each file)
Invoke-EmbedAudio.ps1, Invoke-ExportSvg.ps1, and Invoke-GenerateThemes.ps1 each violate three requirements from powershell.instructions.md:
| Missing Element | Requirement |
|---|---|
.NOTES section in help block |
Required in all scripts (e.g. "Runs via: ...") |
#region/#endregion blocks |
Required to group logical sections |
| Main execution guard | if ($MyInvocation.InvocationName -ne '.') wrapping the execution body enables safe dot-sourcing by Pester |
The existing Invoke-PptxPipeline.ps1 demonstrates the correct pattern. See inline comments for the recommended refactor.
Python: validate_geometry.py missing .pptx extension check (inline comment)
run() validates file existence but not the file type. export_svg.py (added in this same PR) correctly rejects non-.pptx input before calling Presentation(). The same guard should be added to validate_geometry.py.
✅ Issue Alignment
Issue #1482 is linked via Closes #1482. The PR description is detailed and self-consistent; the changes map clearly to the stated scope.
Action Items
- Update PR description to add:
prompt-buildercheckbox (under AI Artifacts), Sample Prompts section, and AI Artifact Contributions checklist. - Refactor all three
Invoke-*.ps1scripts to add.NOTES,#region/#endregion, and main execution guard (see inline comments). - Add
.pptxextension check invalidate_geometry.py'srun()function after the existence guard (see inline comment).
Note
This review is automated. Findings are based on diff analysis against .github/instructions/ files and .github/PULL_REQUEST_TEMPLATE.md. A human maintainer retains final merge authority.
Note
🔒 Integrity filter blocked 1 item
The following item were blocked because they don't meet the GitHub integrity level.
- #1482
issue_read: has lower integrity than agent requires. The agent cannot read data with integrity below "approved".
To allow these resources, lower min-integrity in your GitHub frontmatter:
tools:
github:
min-integrity: approved # merged | approved | unapproved | none* validate_geometry.py: validate .pptx extension before Presentation() * Invoke-EmbedAudio.ps1: add .NOTES, #region blocks, main execution guard * Invoke-ExportSvg.ps1: add .NOTES, #region blocks, main execution guard * Invoke-GenerateThemes.ps1: add .NOTES, #region blocks, main execution guard 🔧 - Generated by Copilot
There was a problem hiding this comment.
PR Review
This PR adds six well-structured PowerPoint skill enhancements with solid test coverage (751 tests, 89.26% coverage) and clean code patterns. However, there are template compliance gaps and a coding standard violation that require resolution before merge.
📋 Issue Alignment
📝 PR Template Compliance
❌ Missing: Sample Prompts section
The PR checks AI Artifact boxes (Copilot instructions, Copilot skill) but the Sample Prompts section is absent from the PR description. Per the PR template, this section is required whenever any AI Artifact checkbox is checked. Please add:
- User Request: A natural language prompt that triggers these features
- Execution Flow: Step-by-step description including tool usage and decision points
- Output Artifacts: What files or content are created (with first 10–20 lines as preview)
- Success Indicators: How a user verifies correct operation
❌ Missing: AI Artifact Contributions checklist
The ### AI Artifact Contributions sub-section is absent from the Checklist. Since Copilot instructions and skill files are included in this PR, the following items must appear and be addressed:
- Used
/prompt-analyzeto review contribution - Addressed all feedback from
prompt-builderreview - Verified contribution follows common standards and type-specific requirements
🔍 Coding Standards
❌ Missing from __future__ import annotations in all four new Python scripts
Per python-script.instructions.md (Script Organization, item 5), this import is required in every script before standard library imports. Inline comments identify the exact insertion point in each file:
embed_audio.py(line 19)export_svg.py(line 15)generate_themes.py(line 17)validate_geometry.py(line 19)
💡 Code Quality
ruamel.yaml dependency
generate_themes.py adds ruamel.yaml solely to load a YAML file with YAML(typ="safe"). The codebase already has pyyaml as a dependency, with pptx_utils.load_yaml() as a reusable wrapper. Introducing a second YAML library increases the dependency surface without clear benefit for this use case. See the inline comment on pyproject.toml for the recommended approach.
✅ What Looks Good
- All four new Python scripts follow the standard entry-point pattern (
main()+sys.exit(main())), error handling hierarchy, and Google-style docstrings. - Exit code semantics (
EXIT_SUCCESS/EXIT_FAILURE/EXIT_ERROR) are applied correctly and consistently with the rest of the skill. - PowerShell wrapper scripts include
[CmdletBinding()],$ErrorActionPreference = 'Stop', main execution guard, and comment-based help. - Bash wrappers follow project conventions with
set -euo pipefail, copyright headers, and pass-through argument pattern. - Single-pass color remapping (
remap_hex_in_text,remap_rgb_in_python) correctly avoids chain-remapping bugs. - The
--dry-runimplementation returns early without producing a PPTX, keeping the validation path clean. - The
ruamel.yamldependency is pinned inuv.lock— good security practice. generate_seeds.pymodernization (pathlib, type hints) is a clean improvement.
🔧 Action Items Required Before Merge
- Add the Sample Prompts section to the PR description
- Add and complete the AI Artifact Contributions checklist
- Add
from __future__ import annotationsto all four new Python scripts - Replace
ruamel.yamlwith the existingpyyaml/pptx_utils.load_yaml()ingenerate_themes.py(or add a comment explaining whyruamel.yamlis needed)
Note
🔒 Integrity filter blocked 1 item
The following item were blocked because they don't meet the GitHub integrity level.
- #1482
issue_read: has lower integrity than agent requires. The agent cannot read data with integrity below "approved".
To allow these resources, lower min-integrity in your GitHub frontmatter:
tools:
github:
min-integrity: approved # merged | approved | unapproved | none…ruamel.yaml rationale * embed_audio.py, export_svg.py, generate_themes.py, validate_geometry.py: add from __future__ import annotations * generate_themes.py: add comment explaining ruamel.yaml round-trip fidelity requirement 🔧 - Generated by Copilot
There was a problem hiding this comment.
Dependency Review
This PR adds one new Python dependency to the PowerPoint skill: ruamel.yaml 0.19.1.
New Dependency: ruamel.yaml 0.19.1
| Dimension | Status | Notes |
|---|---|---|
| License | ✅ MIT | Compatible with project's MIT license |
| Necessity | ✅ Justified | Round-trip YAML parsing with comment/format preservation — pyyaml cannot preserve these properties. Code documents this explicitly. |
| Version pinning | ✅ Locked | uv.lock pins to 0.19.1 with SHA hashes for both sdist and wheel |
| Maintenance | ✅ Active | Released 2026-01-02, actively maintained |
| Duplication | ✅ No overlap | pyyaml remains appropriate for standard read/write; ruamel.yaml is used only where round-trip fidelity is required |
| Known vulnerabilities | ✅ None | No known vulnerabilities for this version |
Other Dimensions
- GitHub Actions: No workflow file changes — SHA pinning compliance is unaffected.
- Devcontainer / setup-steps: No changes to either environment — synchronization is not impacted.
All dependency review checks pass. No issues to flag.
Warning
⚠️ Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
pypi.org
To allow these domains, add them to the network.allowed list in your workflow frontmatter:
network:
allowed:
- defaults
- "pypi.org"See Network Configuration for more information.
Generated by Dependabot PR Review for issue #1481 · ● 1.1M
There was a problem hiding this comment.
PR Review
This PR adds six well-structured enhancements to the PowerPoint skill with solid test coverage (751 tests, 89 % coverage) and comprehensive documentation. The implementation quality is generally high, but there are a few required fixes before it is ready to merge.
Issue Alignment
Issue #1482 is referenced as "Closes #1482". The PR changes directly address the stated enhancements (geometric validation, theme generation, audio embedding, SVG export, --dry-run, and per-slide theme color lookup). ✅
PR Template Compliance ⚠️
Several required AI Artifact sections are absent from the PR description:
-
Missing
Reviewed contribution with prompt-builder agentcheckbox — The template requires this checkbox as the first item under AI Artifacts. The PR jumps straight to the artifact-type checkboxes without including it. -
Missing "AI Artifact Contributions" checklist block — The template includes a dedicated checklist under Checklist with three required items:
[ ] Used /prompt-analyze to review contribution[ ] Addressed all feedback from prompt-builder review[ ] Verified contribution follows common standards and type-specific requirements
None of these appear in the PR body.
-
Missing "Sample Prompts" section — Because Copilot instructions and a Copilot skill are checked under AI Artifacts, the template requires the Sample Prompts section to be filled in (User Request, Execution Flow, Output Artifacts, Success Indicators). The section is absent from the description.
Please update the PR description to include these items before merging.
Coding Standards
All changed Python files follow the project conventions (python-script.instructions.md): copyright headers, Google-style docstrings, configure_logging, EXIT_* constants from pptx_utils, and proper argparse patterns. ✅
PowerShell wrapper scripts follow the project conventions (powershell.instructions.md): copyright headers, #Requires -Version 7.0, comment-based help, [CmdletBinding()], $ErrorActionPreference = 'Stop', region blocks, and the main execution guard. ✅
Bash scripts follow the project conventions (bash.instructions.md): copyright headers, set -euo pipefail, and proper error functions. ✅
Code Quality
Required fix (resource leak):
export_svg.py — fitz.open() returns a Document object that is never explicitly closed. See the inline comment for the context-manager fix.
Suggestion:
SKILL.md — The --dry-run example uses --output /dev/null which fails silently on Windows (NUL is the equivalent). See the inline comment. Consider also making --output optional when --dry-run is active, since writing an output path to a null device is unintuitive.
Action Items
- Add the missing AI Artifact template sections to the PR description (prompt-builder review checkbox, AI Artifact Contributions checklist, Sample Prompts).
- Fix the
fitz.open()resource leak inexport_svg.pyusing the context-manager form. - Document the Windows (
NUL) equivalent for--outputin the dry-run example, or make--outputoptional in dry-run mode.
Note
🔒 Integrity filter blocked 1 item
The following item were blocked because they don't meet the GitHub integrity level.
- #1482
issue_read: has lower integrity than agent requires. The agent cannot read data with integrity below "approved".
To allow these resources, lower min-integrity in your GitHub frontmatter:
tools:
github:
min-integrity: approved # merged | approved | unapproved | none…t optional in dry-run * export_svg.py: use 'with fitz.open() as doc' to close handle deterministically * build_deck.py: make --output optional; validate presence when not in --dry-run mode * SKILL.md: remove /dev/null from dry-run example since --output is no longer needed * test_export_svg.py: add __enter__/__exit__ to mock_doc for context manager compatibility 🔧 - Generated by Copilot
There was a problem hiding this comment.
Code Review Summary
This PR adds six well-structured enhancements to the PowerPoint skill. The implementation is clean, consistently follows project conventions, has excellent test coverage (89 %), and includes complete documentation. Four observations are surfaced below — three are actionable fixes and one is a maintainability suggestion.
Issue Alignment
The PR closes #1482 and the changes match the stated intent: geometry validation, theme generation, audio embedding, SVG export, dry-run mode, and per-slide theme color lookup are all delivered.
PR Template Compliance
✅ All sections are filled in, checkboxes match the actual changes, and the testing section is detailed.
Coding Standards
No material violations. The Python scripts follow the python-script.instructions.md conventions (entry-point guard, configure_logging, EXIT_* constants, Google-style docstrings). PowerShell wrappers follow powershell.instructions.md (copyright header, CmdletBinding, $ErrorActionPreference = 'Stop', invocation guard).
Code Quality Findings
-
Wrong exception class for missing PyMuPDF (
export_svg.py~line 173) — inline comment.LibreOfficeErroris raised whenimport fitzfails, coupling an unrelated dependency failure to a LibreOffice error type.⚠️ -
Spurious blank line in module docstring usage block (
generate_themes.pyline 13) — inline comment. The blank line after the shell continuation\produces a broken command that users cannot copy-paste.⚠️ -
add_movieAPI for WAV audio (embed_audio.py~line 151) — inline comment.python-pptx'sadd_moviecreates avideorelationship, not theaudiorelationship type required by OOXML for proper audio shapes. This may cause display and auto-play issues in non-PowerPoint viewers. 💡 -
Fragile positional group-index arithmetic in
_replace(generate_themes.py~line 168) — inline comment. The zero-internal-groups invariant assumed by the offset math is not enforced; named capture groups would be safer. 💡
Action Items
- Fix
LibreOfficeErrorbeing raised for a missing PyMuPDF import (item 1 — clear bug in exception semantics). - Remove the blank line in the module docstring of
generate_themes.py(item 2 — documentation correctness). - Investigate and document the
add_movie/audio relationship type behavior, or add a comment explaining the intentional design choice and any viewer limitations (item 3).
Note
🔒 Integrity filter blocked 1 item
The following item were blocked because they don't meet the GitHub integrity level.
- #1482
issue_read: has lower integrity than agent requires. The agent cannot read data with integrity below "approved".
To allow these resources, lower min-integrity in your GitHub frontmatter:
tools:
github:
min-integrity: approved # merged | approved | unapproved | none…e rationale * export_svg.py: introduce PyMuPDFError for PyMuPDF import failures; catch both error types * generate_themes.py: remove spurious blank line in docstring usage block * embed_audio.py: document why add_movie is used for WAV audio embedding 🔧 - Generated by Copilot
There was a problem hiding this comment.
PR Review — feat(skills): add 6 PowerPoint skill enhancements
This PR adds substantial, well-structured new functionality to the PowerPoint skill. The code is generally clean, well-documented, and includes thorough test coverage. Two template compliance issues require resolution before merge.
🔴 Issue Alignment
Issue #1482 is referenced as Closes #1482. The six enhancements described in the PR description are consistent with the stated goals (geometric validation, theme color lookup, theme generation, dry-run, audio embedding, SVG export). No significant scope gaps or scope creep detected. ✅
🔴 PR Template Compliance — Required Fixes
1. AI Artifact Contributions checklist not completed
The PR checks Copilot instructions and Copilot skill under the AI Artifacts section of Type of Change, triggering the requirement to complete the AI Artifact Contributions checklist (under "Checklist > Required Checks"). None of the three items are checked:
[ ] Used /prompt-analyze to review contribution[ ] Addressed all feedback from prompt-builder review[ ] Verified contribution follows common standards and type-specific requirements
Per the template, these are required for AI artifact contributions.
2. "Reviewed contribution with prompt-builder agent" unchecked
The first item in the AI Artifacts section of Type of Change — [ ] Reviewed contribution with \prompt-builder` agent and addressed all feedback` — is not checked. This is a required step for any AI artifact contribution.
3. Sample Prompts section not filled in
The template requires the Sample Prompts section to be populated when AI artifact checkboxes are checked. The section still contains placeholder comment text only. At minimum, provide:
- User Request: A sample natural language prompt that would invoke the new skill actions.
- Execution Flow: Key steps (e.g., invoke dry-run, generate themes, embed audio, export SVG).
- Output Artifacts: Brief description of what gets produced.
- Success Indicators: How a user knows it worked.
⚠️ Coding Standards
Checked against .github/instructions/experimental/pptx.instructions.md and python-script.instructions.md. The Python scripts follow conventions well: copyright headers ✅, entry point guard ✅, configure_logging ✅, create_parser factory ✅, type hints ✅, Google-style docstrings ✅.
PowerShell scripts follow powershell.instructions.md: copyright headers ✅, #Requires -Version 7.0 ✅, comment-based help ✅, [CmdletBinding()] ✅, $ErrorActionPreference = 'Stop' ✅, invocation guard ✅.
Bash scripts follow bash.instructions.md: shebang ✅, copyright ✅, set -euo pipefail ✅, main() pattern ✅, err() helper ✅.
See inline comment on validate_geometry.py for a fragile title-detection pattern.
🔍 Code Quality
Three inline comments are posted for:
-
validate_geometry.py— fragile name-based title detection (line ~253):check_title_clearancedetects title shapes by checking"title" in shape.name. Shape names are user-editable and vary by template. Useplaceholder_format.typefor reliable detection. (Non-blocking suggestion) -
Invoke-PptxPipeline.ps1— silent pass-through on unexpected exit codes (line ~453): Only exit codes1and2fromvalidate_geometry.pyare handled. An unexpected crash (other non-zero code) silently continues the pipeline. Add anelseif ($LASTEXITCODE -ne 0)guard. (Non-blocking suggestion) -
generate_themes.py— hex format not validated at load time (line ~95):load_themesvalidates string types but not hex format. Malformed color values ("blue": "not-a-color") produce a confusing error mid-processing. Add format validation at load time for a better developer experience. (Non-blocking suggestion)
No security vulnerabilities found. No secrets or sensitive data exposure. New dependency ruamel.yaml is used appropriately and narrowly (round-trip YAML fidelity in generate_themes.py). The add_movie workaround for WAV embedding is clearly documented.
✅ Action Items
- Complete the AI Artifact Contributions checklist (all three items under "Checklist > Required Checks").
- Check "Reviewed contribution with
prompt-builderagent" in the Type of Change AI Artifacts section, after running the review. - Fill in the Sample Prompts section with at least a user request example and execution flow for one of the new skill actions (e.g.,
generate-themesordry-run).
The inline code suggestions are non-blocking improvements recommended for this PR but not required for merge.
Note
🔒 Integrity filter blocked 1 item
The following item were blocked because they don't meet the GitHub integrity level.
- #1482
issue_read: has lower integrity than agent requires. The agent cannot read data with integrity below "approved".
To allow these resources, lower min-integrity in your GitHub frontmatter:
tools:
github:
min-integrity: approved # merged | approved | unapproved | none| $geomArgs += $ImageOutputDir | ||
| if ($VerbosePreference -eq 'Continue') { | ||
| $geomArgs += '-v' | ||
| } |
There was a problem hiding this comment.
Reliability — Unexpected exit codes from validate_geometry.py silently succeed
Only exit codes 1 and 2 are explicitly handled. If validate_geometry.py terminates unexpectedly (e.g., Python crash, unhandled exception, OS-level kill), the non-zero exit code falls through both if guards and the pipeline continues as if validation succeeded.
Suggested fix to surface unexpected failures:
& $python `@geomArgs`
if ($LASTEXITCODE -eq 2) {
throw "validate_geometry.py encountered an error (exit code $LASTEXITCODE)."
}
elseif ($LASTEXITCODE -eq 1) {
Write-Host "Geometric validation found warnings — see $geomReportPath"
}
elseif ($LASTEXITCODE -ne 0) {
throw "validate_geometry.py exited with unexpected code $LASTEXITCODE. Check output above."
}Consistent with how other steps in this function surface unexpected exit codes.
| def remap_hex_in_text(text: str, color_map: dict[str, str]) -> str: | ||
| """Replace ``#RRGGBB`` hex color values using *color_map*. | ||
|
|
||
| Uses a single-pass regex callback to avoid chain remapping where |
There was a problem hiding this comment.
Code Quality — Hex format not validated at load time
load_themes validates that color map keys and values are strings, but doesn't verify they are valid 6-character hex strings. The format check (len(k) != 6) only happens later inside remap_hex_in_text and remap_rgb_in_python. A malformed themes file (e.g., "colors": { "blue": "not-a-color" }) will fail with a less helpful error mid-processing after partial output has already been written.
Consider adding a format check directly in load_themes:
import re
_HEX6 = re.compile(r"^[0-9A-Fa-f]{6}$")
for k, v in cfg["colors"].items():
if not isinstance(k, str) or not isinstance(v, str):
raise ValueError(...)
if not _HEX6.match(k.lstrip("#")) or not _HEX6.match(v.lstrip("#")):
raise ValueError(
f"Theme '{theme_id}' color map entry {k!r}: {v!r} "
"must be 6-character hex strings"
)This gives an actionable error before any directory is created.
| top = emu_to_inches(s.top) | ||
| height = emu_to_inches(s.height) | ||
| rects.append((top, top + height, s)) | ||
| rects.sort(key=lambda r: r[0]) |
There was a problem hiding this comment.
Code Quality — Fragile title detection by shape name
The current approach relies on the shape's .name attribute containing "title" (case-insensitive), but PowerPoint shape names are user-editable and vary by template (e.g., "Title 1", "Content Placeholder 2", custom names). A theme could have a title placeholder named anything.
Prefer detecting the placeholder type via python-pptx's placeholder_format:
from pptx.util import PP_PLACEHOLDER # noqa: F401
def _is_title_placeholder(shape: BaseShape) -> bool:
"""Return True when shape is a title (not subtitle) placeholder."""
try:
ph = shape.placeholder_format
return ph is not None and ph.type in (
PP_PLACEHOLDER.TITLE,
PP_PLACEHOLDER.CENTER_TITLE,
)
except AttributeError:
return FalseThen in check_title_clearance, replace the name-based check:
# Instead of:
if "title" not in name_lower or "subtitle" in name_lower:
continue
# Use:
if not _is_title_placeholder(shape):
continueThis is robust across template variants and custom shape names.
4 participants
Pull Request
Description
Adds 6 enhancements to the PowerPoint skill addressing the most common pain points in multi-theme deck production: manual spacing validation, hardcoded theme colors in content-extra.py, per-project theme generation boilerplate, lack of pre-flight checks, no audio pipeline, and missing SVG export.
New Scripts (4)
validate_geometry.pygenerate_themes.pyembed_audio.pyexport_svg.pyModified Files (4)
build_deck.py--dry-runpre-flight validation,--verboseflag,configure_logging(), per-slide theme color lookup viathemes[].slidesInvoke-PptxPipeline.ps1SKILL.mdpptx.instructions.mdRelated Issue(s)
Closes #1482
Type of Change
Code & Documentation:
AI Artifacts:
.github/instructions/*.instructions.md).github/skills/*/SKILL.md)Other:
.ps1,.sh,.py)Testing
test_validate_geometry.py(37 tests),test_generate_themes.py(20 tests),test_embed_audio.py(18 tests),test_export_svg.py(11 tests)--dry-runvalidates YAML parsing, speaker notes, content-extra.py AST, and image refs without buildingremap_hex_in_textandremap_rgb_in_pythonprevents chain remappingthemes[].slideslist withthemes[0]fallback; shallow-copies style dictChecklist
Required Checks
Required Automated Checks
npm run lint:mdnpm run spell-checknpm run lint:frontmatternpm run validate:skillsnpm run lint:md-linksnpm run lint:psnpm run plugin:generatenpm run docs:testSecurity Considerations
Additional Notes
Enhancement priority ranking derived from pain points during multi-theme deck production:
--dry-runpre-flight check