Repository navigation
fix: composition contract repairs found by an end-to-end validation run - #518
Open
farhadganji371 wants to merge 4 commits into
Open
farhadganji371 wants to merge 4 commits into
farhadganji371 wants to merge 4 commits into
Conversation
Post-validation Revision 1. An end-to-end zero-cost production surfaced several component and validation defects; each is fixed conservatively so existing dark-theme output is byte-identical. - StatCard: Explainer never forwarded a text colour, so the subtitle fell back to #FFFFFF and was unreadable on light playbooks (E1). - KPIGrid: add optional `decimals` / `abbreviate`; defaults reproduce the previous rounded/abbreviated output exactly (E2). - HeroTitle / SectionTitle / StatReveal: accept optional theme colours, defaulting to today's constants. Explainer forwards theme-derived values only when the resolved surface is light, so dark themes are untouched (E3). - ComparisonCard: forward cut.leftColor / cut.rightColor when supplied; component defaults are unchanged when they are not (E4). - composition_validator: reject overlay timing errors before Remotion renders, naming in_seconds/out_seconds instead of failing mid-render with a NaN Sequence error (E7). Bumped to 0.2.0. - edit_decisions: add the optional `templated` provenance block so a templated run records which composition props it rendered, without duplicating Remotion scene props into cuts (E8). Tests render real frames via the Remotion CLI and measure them with Pillow; no JS test runner or new production dependency is introduced. Dark-theme goldens captured before the fix are committed as compatibility evidence. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…tical margin Post-validation Revisions 2A and 2C. Canonical subtitle settings on edit_decisions.subtitles were largely inert: the renderer spoke a different vocabulary, so most declarations were silently dropped. Fixed: - `subtitles.style` is schema-typed as a display-style STRING, but the resolver called .items() on it, raising AttributeError deep inside compose for any schema-valid artifact. Strings are now recorded as a display style; an unusable type fails with a clear message instead of crashing. - Canonical `color`, `outline_color`, `background` and `position` now reach the renderer as PrimaryColour / OutlineColour / BackColour / Alignment. - CSS colours are converted to ASS &HAABBGGRR (blue-green-red order, with alpha inverted from opacity to transparency); ASS values pass through. - Font size is converted against the real libass canvas. ffmpeg renders SRT on a 384x288 script canvas (verified from its own generated header), so a raw pass-through made a requested 42px render ~3.75x too large. Sizes are now expressed in video pixels and scaled by the probed output height. Added: - Optional `subtitles.vertical_margin` (integer, video-space pixels, the same contract as video_stitch.pip_margin), converted to ASS MarginV. This removes the last reason to reach for a raw ASS override: the governed contract alone now reproduces the intended caption placement. Precedence is defaults -> playbook -> canonical fields -> explicit native ASS override, so a caller that already speaks ASS is never overwritten by a higher-level field. Omitting every canonical field reproduces the previous force_style byte for byte. Revisions 2A and 2C are committed together because their changes interleave line-by-line inside _build_subtitle_style and the resolver defaults; the per-revision history is preserved in engineering/post-validation-2026-08-19/. Behavioural note: projects that declared canonical subtitle fields, or that relied on a style playbook while burning subtitles, will see captions change because those declarations finally take effect. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Post-validation Revision 2B. The slideshow-risk gate had never actually run
in production.
_pre_compose_validation did `scenes = scene_plan or []`, but its only caller
passes the scene_plan ARTIFACT ({version, style_playbook, scenes[], ...}).
Iterating that dict yields its string keys, so every scoring dimension raised
"'str' object has no attribute 'get'" and a bare `except` swallowed the
failure as a one-line log warning.
- Normalise the input at the boundary: the artifact dict, a bare scenes list
and None are all accepted; anything else is reported rather than iterated.
- Make failures visible: the exception type, message and the scene
representation used are surfaced as a structured warning and recorded on
VideoCompose.last_slideshow_risk. No more silent skips.
- Run REPORT-ONLY by default. A `fail` verdict warns and does not stop
compose. The original blocking branch is preserved and re-enabled only by
OPENMONTAGE_SLIDESHOW_RISK_ENFORCEMENT=block, following the existing
OPENMONTAGE_* convention. Blocking is NOT enabled by this change.
Analysis recorded in engineering/post-validation-2026-08-19/E6_ANALYSIS.json:
the same production scores 0.67/strong from its scene_plan but 3.42/revise
from cuts, because edit_decisions.cuts cannot carry shot_intent, roles,
hero_moment or shot_language (additionalProperties: false). scene_plan.scenes
is therefore the canonical representation; the cuts fallback is diagnostic
only and must not drive enforcement.
Tests read committed fixtures under tests/fixtures/slideshow_risk/ rather
than the gitignored validation project, so they pass in a clean clone.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Post-validation Revision 2D plus consolidation of Revisions 1-2D.
Theme precedence (E10): resolveTheme returned a built-in preset as soon as
theme/playbook matched one, discarding an explicit themeConfig. That
contradicts how video_compose builds a themeConfig from the playbook's own
palette ("not picked from a preset menu"), so a preset-named playbook could
never apply its YAML colours.
- Add the optional composition prop `themeConfigWins`. When set, resolution
becomes DEFAULT_THEME -> named preset -> themeConfig, merged field by
field so a partial config refines a preset instead of blanking it.
- Legacy precedence remains the DEFAULT: omit the flag and a recognised
preset still wins. Verified byte-identical across all four built-in
presets (16/16 golden frames), and no shipped demo would change.
Consolidation:
- video_compose 0.1.0 -> 0.2.0, remotion-composer 1.0.0 -> 1.1.0. Artifact
schema versions are unchanged; every schema addition is optional.
- Add CHANGELOG.md and docs/COMPOSITION_CONTRACTS.md covering the subtitle
contract, theme precedence and slideshow-risk behaviour.
- Correct stale guidance in the talking-head compose director that treated
ASS MarginV as frame pixels; on a 1920-high frame MarginV=160 is roughly
1067px from the edge, i.e. mid-frame.
- Retain the small text/JSON engineering evidence (freeze records, analyses,
checksums, provenance) plus FOLLOW_UPS.md. Generated binary evidence and
the local validation composition props stay untracked; their SHA-256s and
provenance are recorded in BASELINE.json.
Not enabled by default: slideshow-risk blocking, and themeConfig precedence.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Independent reproduction on a different machine — confirms the diagnosis in this PR.
Confirming the need for "Restore slideshow-risk analysis against the canonical Passing the full
Passing Worth considering alongside this PR: surfacing the swallowed exception into |
This was referenced Nov 9, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR hardens OpenMontage's Remotion composition and video rendering contracts based on defects found during a full end-to-end production validation run.
The changes focus on compatibility, validation, subtitle rendering, theme handling, and slideshow-risk analysis while preserving existing default behaviour where possible.
Changes
composition_props_path.subtitles.vertical_marginwith video-space pixel conversion to ASSMarginV.scene_plan.scenesrepresentation.themeConfigWinsprecedence without changing legacy preset behaviour.Compatibility notes
report_onlyby default. Automatic blocking is not enabled.themeConfigWinsremains opt-in. Existing preset precedence remains the default.Testing
Validated locally after the complete change set:
git diff --check: cleanCommitted component/tool versions:
video_compose: 0.2.0composition_validator: 0.2.0remotion-composer: 1.1.0Checklist