fix(create-chapter): rebrand file names + xlsx inline strings; render via Slides API not LibreOffice

[rparundekar]

Summary

Fixes two gaps in the chapter-cloning rebrand engine (file/folder names and xlsx inline-string cells were never rewritten) and replaces LibreOffice-based pptx→PNG exports with a Slides-API-based renderer, since LibreOffice silently substitutes local system fonts for the deck's brand fonts.

Changesets

1. fix(create-chapter): rebrand file names + xlsx inline strings; render via Slides API not LibreOffice

Files: lib/aaif_events/slides_export.py (new), skills/aaif-create-chapter/scripts/create_chapter.py, skills/aaif-create-chapter/scripts/test_create_chapter.py, skills/aaif-create-chapter/SKILL.md, skills/aaif-create-event/SKILL.md

• rebrand_part() only rewrote OOXML part content; cloned chapter files kept the source city in their own names (e.g. a cloned chapter's CRM stayed "San Francisco CRM.xlsx"). clone_and_rebrand() now runs each file/folder name through the same transform_text() used for content.
• xlsx cells stored as inline strings (<is><t>...</t></is>, used by e.g. the CRM's "Guide" sheet title) live in xl/worksheets/sheetN.xml, not xl/sharedStrings.xml — the only xlsx part type the engine previously handled. Added a matching branch reusing the existing _process_paragraphs helper.
• Both fixes were verified with a real throwaway chapter clone (created, inspected, confirmed correct, then trashed) before landing.
• New lib/aaif_events/slides_export.py: renders a Drive pptx slide to PNG via a throwaway Google Slides copy + the Slides API thumbnail endpoint, instead of a local soffice --headless --convert-to png/pdf call. LibreOffice substitutes local system fonts for the deck's actual brand fonts, producing exports that look wrong in ways the live file isn't. Follows the same gws CLI wrapper conventions (retry-on-transient-error, empty/non-JSON-output guards) already used elsewhere in the toolkit.
• Updated both call sites' docs (aaif-create-chapter's map-dot recalibration step, aaif-create-event's Luma cover export step) to use the new helper instead of soffice.
• Added regression tests for the new xlsx inline-string rebrand branch.

2. fix: address self-review findings

Files: lib/aaif_events/slides_export.py, lib/aaif_events/tests/test_slides_export.py (new), skills/aaif-create-chapter/scripts/test_create_chapter.py

• Fixed a misleading test that passed for the wrong reason (routed through the .rels branch rather than the claimed untouched fallback); added a genuine falls-through-untouched test.
• slides_export.py: cleanup failures are now logged instead of silently swallowed; the copy call is inside the try/finally so a failure right after it still attempts cleanup; the exhausted-retries error now includes which gws subcommand failed; corrected a comment that understated the real risk of a failed cleanup (a stray copy can land next to the source file and get propagated if this is ever run against TemplateCity).
• Added lib/aaif_events/tests/test_slides_export.py (11 tests covering retry/backoff, output-parsing guards, and the cleanup-on-error contract) and TestTransformText in test_create_chapter.py (4 tests covering transform_text() on filename-shaped strings, which previously had no direct coverage).

Test Plan

• test_create_chapter.py — 34/34 passing (30 from changeset 1 + 4 new TestTransformText)
• lib/aaif_events/tests/ — 66/66 passing (55 pre-existing + 11 new in test_slides_export.py)
• ruff check, pre-commit run --all-files — clean
• Real integration test: created a throwaway chapter via create_chapter.py, confirmed the CRM file name and Guide-sheet title were both correctly rebranded (previously showed the source city), then trashed the test chapter
• render_slide_png() spot-checked against several real Drive files, including after the self-review refactor — correct fonts, no LibreOffice font-substitution artifacts
• Self-review completed via hero-skills:review-pr — 1 critical + 5 important + 4 suggestions found, 9 fixed, 3 explicitly deferred/skipped (see PR comments)

Generated using hero-skills.

[rparundekar]

Self-Review

Critical (1)

• [code-reviewer + comment-analyzer] test_create_chapter.py:346-353 — test_unmatched_worksheet_number_pattern_is_untouched doesn't test what its name/comment claim. "xl/worksheets/_rels/sheet2.xml.rels" does NOT fall through to rebrand_part's final else: return data — it matches the .rels elif branch (which actively rewrites Luma slugs). The test only passes because the fixture content happens to contain no aaif-sf/aaif-sanfrancisco pattern for that branch to touch, not because the path is inert. A wrong regression test is a false safety net.

Important (5)

• [silent-failure-hunter] lib/aaif_events/slides_export.py:78-79 — bare except Exception: pass around the temp-copy trash call silently swallows every failure with zero visibility (no log/print). Masks leaked Drive files and any unrelated future bug on that line.
• [silent-failure-hunter] lib/aaif_events/slides_export.py:33 (_gws's retry-exhausted RuntimeError) — omits which gws subcommand/args failed, unlike _gws_json's own error paths which do include them. Makes debugging a batch run harder.
• [comment-analyzer] lib/aaif_events/slides_export.py:78-79 — the "harmless clutter" comment understates the real risk: the throwaway copy has no parents set, so it lands in the same folder as the source file. Since aaif-create-chapter/SKILL.md's own recalibration step recommends running this against TemplateCity's Slides.pptx, a repeatedly-failed cleanup there would leave a stray file that clone_and_rebrand()'s folder walk would propagate into every subsequently cloned chapter.
• [pr-test-analyzer] lib/aaif_events/slides_export.py has no test coverage for _gws's retry/backoff, _gws_json's empty/non-JSON output guards, or render_slide_png's cleanup-fires-even-on-error contract — all mockable via the same monkey-patch pattern already used by TestGeocodeCity in test_create_chapter.py.
• [pr-test-analyzer] transform_text() (now also applied to filenames via clone_and_rebrand) has never had a direct unit test — only indirect coverage through prose-text XML fixtures, which don't exercise the same ±30-char lookback the bare-"SF" case heuristic uses on short filename-shaped strings.

Suggestions (4)

• [silent-failure-hunter] slides_export.py — presentation_id = copy["id"] extraction happens just before the try block; moving the try up one line to also wrap the extraction is cheap defensive hygiene (though nothing between the two currently can fail).
• [type-design-analyzer / simplify-pass reuse agent] _gws/_gws_json in slides_export.py duplicates create_chapter.py's own _gws/gws_json (already noted in the PR description as an intentional mirror). A follow-up could consolidate both into one shared lib/aaif_events helper — deferred as a separate refactor rather than folded into this fix.
• [type-design-analyzer] No custom exception type (e.g. SlidesExportError) — every failure mode raises a bare RuntimeError. Reasonable given the module's current scope (called from documented one-off SKILL.md snippets, not a general library API); not fixing to avoid over-engineering.
• [comment-analyzer] Minor doc polish: the "LibreOffice silently substitutes fonts" claim could note it's conditional on the render machine lacking the brand font (true in practice everywhere, but stated as unconditional fact); the "CRM Guide sheet" example is repeated verbatim in 3 places with nothing to catch drift if it's ever renamed.

Strengths

• Security review: NO FINDINGS (no injection, path traversal, or credential exposure — all Drive/Slides calls stay in argv lists, never a shell string).
• Existing test suite fully green (29/29), ruff/pre-commit/codespell clean.
• The xlsx inline-string fix correctly reuses the existing _process_paragraphs machinery; silent-failure-hunter independently verified rich-text <is><r><t> runs are handled correctly, and that residual_tokens() + a hard sys.exit backstops the whole rebrand pipeline even in an unanticipated edge case.
• Comments throughout are accurate, explain real non-obvious constraints (Slides API needing a native-Slides copy, _TRANSIENT retry rationale), and aren't stale restatements of the code.

---

Generated using hero-skills.

[rparundekar]

Self-Review — Improvements

Critical (1 / 1 fixed):

• test_create_chapter.py:345-350 — misleading test (passed for the wrong reason, routed through the .rels branch rather than the claimed untouched fallback) — renamed/re-commented to describe what it actually verifies, and added a genuine falls-through-untouched test (test_unrelated_part_type_is_left_untouched) against a real inert part type.

Important (5 / 5 fixed):

• lib/aaif_events/slides_export.py:78-79 — silent except Exception: pass on cleanup — now logs a WARNING: to stderr with the presentation ID and error instead of swallowing silently.
• lib/aaif_events/slides_export.py:33 — _gws's exhausted-retries error omitted which gws subcommand failed — now includes the command.
• lib/aaif_events/slides_export.py:78-86 — "harmless clutter" comment corrected to state the real risk (throwaway copy has no parents, lands next to the source file — matters if this is ever run against TemplateCity, since clone_and_rebrand() would then propagate a stray copy into every future chapter).
• No test coverage for _gws/_gws_json retry/parsing or render_slide_png's cleanup-on-error contract — added lib/aaif_events/tests/test_slides_export.py (11 tests: retry-then-succeed, non-transient fails fast, empty/non-JSON output guards, cleanup fires even when the body raises, cleanup failure doesn't mask the original exception, param/body serialization).
• transform_text() had no direct test, only indirect coverage via prose fixtures — added TestTransformText (4 tests) exercising it on filename-shaped strings, including the bare-"SF" case heuristic.

Suggestions (4 / 3 fixed):

• slides_export.py — moved the try to wrap the copy call + presentation_id extraction (defensive hygiene) — fixed.
• Doc polish: qualified the "LibreOffice silently substitutes fonts" claim as conditional on the render machine lacking the brand font — fixed.
• Consolidating the duplicated _gws/_gws_json helper (also present in create_chapter.py) into one shared module — skipped, deferred as a separate follow-up refactor rather than folded into this fix (would touch create_chapter.py's core Drive-call path again, broader scope than this PR).
• Custom exception type (e.g. SlidesExportError) instead of bare RuntimeError — skipped, reasonable given the module's current scope (one-off SKILL.md snippets, not a general library API); avoiding over-engineering.
• The "CRM Guide sheet title" example repeated verbatim in 3 places — skipped, all three copies are accurate and clear; not worth thrashing for variety alone.

Commits: 7f2bb7c, 149c9b3

---

Generated using hero-skills.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.