Fix : replace binary PPTX fixture with programmatic generator#2081
Fix : replace binary PPTX fixture with programmatic generator#2081PratikWayase wants to merge 1 commit into
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2081 +/- ##
==========================================
+ Coverage 80.82% 85.95% +5.13%
==========================================
Files 117 84 -33
Lines 19095 11686 -7409
==========================================
- Hits 15433 10045 -5388
+ Misses 3662 1641 -2021
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
katriendg
left a comment
katriendg
left a comment
There was a problem hiding this comment.
Thanks for tackling this — replacing the opaque binary .pptx with a transparent, code-reviewable generator is a real security and auditability win, and the roundtrip validate_deck test is a nice closed-loop guarantee.
Note I reviewed the current version of the branch, though due to a merge conflict some of the comments may be stale. Please rebase and re-run the CI checks before merge.
Before merge, a few items: the two modified Python files currently fail the repo's ruff lint gate (npm run lint:py) with 13 errors — 9 trailing-whitespace blank lines, 2 lines over 88 chars, and 2 unsorted import blocks. Most are auto-fixable with ruff check --fix / ruff format. Beyond lint, please add type hints to the new helper functions, mark the new roundtrip test with @pytest.mark.integration (matching its sibling tests) and give it AAA structure, and reconsider the direct lxml import (it's only a transitive dep of python-pptx) and the author = "ChatGPT" fixture value. Details are in the inline review.
katriendg
left a comment
katriendg
left a comment
There was a problem hiding this comment.
Inline findings from the code review (the overall verdict remains Request changes from my earlier review). Each comment maps to a finding in the review summary.
| """Shared fixtures for PowerPoint skill tests.""" | ||
|
|
||
| import io | ||
| from lxml import etree |
There was a problem hiding this comment.
[Standards] Undeclared direct dependency. lxml is imported directly but is not listed in pyproject.toml dependencies — it only works because python-pptx pulls it in transitively. Either add lxml to dependencies, or manipulate the theme XML through python-pptx's own element API (theme_part.element) instead of re-parsing theme_part.blob with lxml.etree.
| + _chunk(b"IEND", b"") | ||
| ) | ||
|
|
||
| def _set_theme_colors(prs): |
There was a problem hiding this comment.
[Standards] Missing type hints. Repo Python conventions require annotations, and the sibling helpers in this file are annotated. Suggest def _set_theme_colors(prs: Presentation) -> None:, and annotate the nested set_color(color_name: str, hex_val: str) -> None:.
| ) | ||
|
|
||
| def _set_theme_colors(prs): | ||
| """Sets specific theme colors by modifying the theme part's blob via its public setter.""" |
There was a problem hiding this comment.
[Standards] E501 line too long (94 > 88). Wrap this docstring to <=88 chars to satisfy the skill's ruff line-length = 88.
| set_color('dk1', '000000') | ||
| set_color('accent1', '4F81BD') | ||
|
|
||
| theme_part.blob = etree.tostring(theme_element, xml_declaration=True, encoding='UTF-8', standalone=True) |
There was a problem hiding this comment.
[Standards] E501 line too long (108 > 88). Break this etree.tostring(...) call across multiple lines to satisfy line-length = 88.
|
|
||
| theme_part.blob = etree.tostring(theme_element, xml_declaration=True, encoding='UTF-8', standalone=True) | ||
|
|
||
| def generate_minimal_fixture(output_path: Path): |
There was a problem hiding this comment.
[Standards] Missing return annotation. generate_minimal_fixture returns nothing; annotate as def generate_minimal_fixture(output_path: Path) -> None:.
| prs = make_blank_presentation() | ||
|
|
||
| prs.core_properties.title = "Minimal Test Fixture" | ||
| prs.core_properties.author = "ChatGPT" |
There was a problem hiding this comment.
[Standards] Fixture author hardcoded to "ChatGPT". Setting an external AI product name as the fixture author is inappropriate for a repo fixture. Use a neutral value (e.g. "HVE Core Test Fixture") and update the matching EXPECTED_FIXTURE["metadata"]["author"] assertion in the integration test.
| slide2 = prs.slides.add_slide(slide_layout_2) | ||
| slide2.placeholders[0].text = "Slide with Image" | ||
| slide2.placeholders[1].text = "Below is an embedded image." | ||
| slide2.notes_slide |
There was a problem hiding this comment.
[Standards] Bare expression reads as dead code. slide2.notes_slide on its own line only has the side effect of lazily creating an empty notes slide; it looks like a leftover and would trip ruff B018 if B rules were on. Either remove it, or make the intent explicit: _ = slide2.notes_slide # ensure notes part exists.
| import yaml | ||
| from pathlib import Path | ||
| from extract_content import main | ||
| from validate_deck import validate_deck, max_severity |
There was a problem hiding this comment.
[Standards] Import block I001 (unsorted). from pathlib import Path (stdlib) is placed after the third-party pytest/yaml imports. Group stdlib first, then third-party, then local (extract_content, validate_deck). ruff check --fix resolves this.
| slide_1["elements"][0]["font_color"] == EXPECTED_FIXTURE["slide_1_font_color"] | ||
| ) | ||
|
|
||
| def test_generated_fixture_passes_validate_deck(minimal_test_fixture_path: Path): |
There was a problem hiding this comment.
[Functional] Missing @pytest.mark.integration + AAA structure. This builds a full deck and runs structural validation (a roundtrip integration test) but, unlike the two sibling tests in this module, is not decorated with @pytest.mark.integration (a marker registered in pyproject.toml). Add the marker and structure the body with explicit Arrange/Act/Assert sections per the repo test conventions.
PratikWayase
force-pushed
the
fix/pptx-fixture-generator
branch
from
b9ab10b to
e23cca5
Compare
PratikWayase
force-pushed
the
fix/pptx-fixture-generator
branch
from
e23cca5 to
2fad6cf
Compare
4 participants
Pull Request
Description
This PR resolves the security and auditability concerns raised in #1135 regarding the committed binary
minimal_test_fixture.pptx.Changes Made:
tests/fixtures/minimal_test_fixture.pptxto eliminate the risk of hidden macros, OLE objects, or malicious XML embedded in an unreviewable binary blob.conftest.pyto dynamically generate the minimal PPTX fixture at test runtime usingpython-pptx. Usedlxmlto manipulate the theme XML directly to ensure deterministic theme color resolution (dk1,accent1).test_generated_fixture_passes_validate_deckintest_extract_content_integration.py. This ensures the programmatically generated PPTX passes structural validation viavalidate_deck.pybefore extraction tests run, creating a closed-loop guarantee.Benefits:
.pptxfile.Related Issue(s)
Fixes #1135
Type of Change
Select all that apply:
Code & Documentation:
Infrastructure & Configuration:
AI Artifacts:
prompt-builderagent and addressed all feedback.github/instructions/*.instructions.md).github/prompts/*.prompt.md).github/agents/*.agent.md).github/skills/*/SKILL.md)Other:
.ps1,.sh,.py)Testing
npm run test:py.test_generated_fixture_passes_validate_deckroundtrip validation test.tts-voiceovertest failures observed in the CI logs are pre-existingargparseissues in a completely separate skill and are unrelated to this PR).Checklist
Required Checks
Required Automated Checks
The following validation commands must pass before merging:
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
python-pptxandlxml)