From bcd8eb6e5301de03f50c3e1d26a7b5d92d76a973 Mon Sep 17 00:00:00 2001 From: 0xDevNinja Date: Tue, 7 Jul 2026 15:29:03 +0530 Subject: [PATCH 1/2] fix(subtitle_gen): stop millisecond rounding from overflowing timestamps MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit _ts_srt/_ts_vtt computed the seconds and millisecond fields independently: `ms = int(round((seconds % 1) * 1000))`. When the fractional part is >= 0.9995 that rounds to 1000, emitting a malformed 4-digit `…,1000` value with no carry into the seconds field (and, at 59.9999/3599.9999, no carry into minutes/hours). For example 0.9999s became `00:00:00,1000` instead of `00:00:01,000`. ASR word and segment end-times routinely land on such fractional boundaries, and the resulting cue is rejected or mistimed by strict SRT/VTT parsers (ffmpeg subtitles filter, VLC, browser WebVTT). Decompose from a single rounded total-milliseconds value so the carry propagates across all fields. Both formatters now share one `_hmsms` helper. --- diagram.png | Bin 5915 -> 5336 bytes tests/tools/test_subtitle_timestamps.py | 49 ++++++++++++++++++++++++ tools/subtitle/subtitle_gen.py | 30 +++++++++------ 3 files changed, 68 insertions(+), 11 deletions(-) create mode 100644 tests/tools/test_subtitle_timestamps.py diff --git a/diagram.png b/diagram.png index 2f38f343b453f8ac19c94ac747715acc2409d929..408bf71c59e37ffc6788973fee57e90425aa6a4f 100644 GIT binary patch delta 1015 zcmVZgM9l-J5w!10xKYDyOW+cn~ky=lBP-3Hu z{uzWsPZEjT|WQ-0MN5^u?GhL003Zd2(u9p@&&UX z0b2rpHM7_Q0I(1?-?q<%=4@@fZlj9^Fm6j`XV)+G3;EYySmqvS_r0LDoTa=uLer{LaWfKd{J0000{ zf)HSYY!7mN*(3xQD?tbV01zbz0RRA^1R(&E?*U?enpx}t0O0)l#?4!gKmFu%{m9Wn zd+xpGte$?w`>wk8H#@yC0F0g>1Ym^b=jXP4_m*>h`W2f#C(!t5)C2eL zzGLgH@4fN^|2j1D%<-pg-*WTb2lssFW7piX^XD(U^!M-I{_R8i_g=Q{@?jV*U9;}! z;R9+AfbkK80F2PGWiy|<{wveJ*;T8DVVF#RCeI#!>cx{U%+Jq1bNtxJlP8w1STQ|$ zG8z7G_`om>M-J{=dHUq3gZm#^vv$KU4419laA^PD#d|dXaG|(m6oABc2q%dgt&n@mnkCjUHi`smy%kN^3_7bcU*>)-IkH@*2Se|q-#Ld^~U3&##20Ch`$ zmoB?>&AMS2)?L2w@jpH~48xMshG95-;Gqv+ea)kf99irc0pNnLLkPfVZyBAgpFZ}) z&R_iWznnZb_ww2e?|9FZ@BiCh|8n&OkAL!4zx>%k z4GsVcN)Q4tLN|VO^Q#`Y>1$v7Pm5tn81A}r`_#9EM@j4LcTl5JqD9S(<*X02mWN2mk;OB?tk=$tEG^i|s*x5nD1lyMD1}0N_H| zeA~Y1V%L^yU1iSJD|a2cSrlNb1R+4B3E|wRL4Xkxga8S$kaHjf0Y*;{0ssI+`B53O lF%Z%QvmgNp0T@HmKL9bB7IUa)Y{LKm002ovPDHLkV1l8R^k@J8 delta 1631 zcmV-l2B7)aDVr{kBYzg+NklT{I@fpeCDP&c#Hd{-GrMN9JOqRzL<}=wvJlB!UWJ%NV=F6?>x~P;RA^())!= zOJ%*7dQ8_8vaDU_oA-P_pTC~>Ip;j*Ilr8LzVGvUerIg@^bc7B0001bCXM$A0000m zKC>YWp9hmr0k400+jyS<0RJC8al>K*&EdNKnmGmyV6=@*pFV56&jElRASuZANfrW( zFhK~AsKJHvuO9DnVd9zl$=ik-nh;=A2||DouR$ruQOQD02r!Z?Dg!W@)F4O8@P7*C z9s`UdK?nc<5G4o!Mv%*c99=FF0*op_2mk;OB?tik0HTvz16qIHHr^)y0Eg`(zx-?G zU-xT=&w8zj)QJzU#~x-+tqbN0u$V_ntfV?Abkh@=I&i zy!gWN7hn2;6Hb`C?xi&g?z`vdm5&bwgWve=YtEW+_Fyph^mX%>Em`#P_HFOG?DBb^ zxpw3Fb;CCr8yowRYd^Dk)w8$%{Y~Tjp#k9VSX2gpku`r~);SkmGWUV||K-cK-+JtE z$N&1(zw`0m{ezu5cHHp~H$S#w*##G0`reDab0gvzXU@3%!&iOtp$ESF#as97-S?Xx z`@Pf8c=up1xc}a}mo5I*U@-Xn=ltHZg_1x1NH>}&RZtePY zYYz^>SwC~$u3fLMeB#k@`jh~0^1p_E?%TKTKDZ_&CE3oI3U7w+OEy&rA3y37G%llHgb-hKNs@(|m%ZC$FM#r=4=}KukON)WKk|W%GZgw;Y}2OBcQ4ou{2Nb=p}oX0KTGpToU zb3b(ASs*-+T|@U@&-a!F`9!|NI{P*2h1!;NR}qwQJW!@0)*n z{13nV3%A}l-2d8ZuMP%-pMCGePd@SJ@aG42?%474%E!<9xr?@J-n3@*s_*-w0KoVV zgaAxb7hHVVTkikbga3X6LHNS+&px(d+0S2o<*~;dw|dpHH_iWx7uT#FKJ>`ahi9EV z=Yv;VHDmTUH~iIg!=1~PEIQ+?nU8Hy#B=vucier~9pC*qfBo4{AN=Pt zPd)L+zyGm=_uO*hyhC2{^{+1Y`d1gc`A|D|?zri1KYPf|;XG3FMFE)5+(QTeN8;S~ zoWExEs*M}=&maYW3Ca~h05}YjCQbTLjn@6zyl1;8i~ga7~lQGyU)G`UE~(c= 0.9995, +that rounds to 1000, emitting a malformed 4-digit `…,1000` with no carry into +the seconds (and, at boundaries, minutes/hours) field — e.g. 0.9999s -> +`00:00:00,1000` instead of `00:00:01,000`. Such timestamps are rejected by +strict SRT/VTT parsers (ffmpeg subtitles filter, VLC, browser WebVTT). +""" + +import re +import sys +from pathlib import Path + +PROJECT_ROOT = Path(__file__).resolve().parent.parent.parent +sys.path.insert(0, str(PROJECT_ROOT)) + +from tools.subtitle.subtitle_gen import SubtitleGen # noqa: E402 + +_SRT_RE = re.compile(r"^\d{2}:\d{2}:\d{2},\d{3}$") +_VTT_RE = re.compile(r"^\d{2}:\d{2}:\d{2}\.\d{3}$") + + +def test_millisecond_carry_does_not_overflow(): + # Every fractional part >= 0.9995 must carry into the next second, never + # emit a 4-digit millisecond field. + assert SubtitleGen._ts_srt(0.9999) == "00:00:01,000" + assert SubtitleGen._ts_vtt(0.9999) == "00:00:01.000" + assert SubtitleGen._ts_srt(1.9996) == "00:00:02,000" + + +def test_carry_propagates_across_minute_and_hour_boundaries(): + assert SubtitleGen._ts_srt(59.9999) == "00:01:00,000" + assert SubtitleGen._ts_srt(3599.9999) == "01:00:00,000" + assert SubtitleGen._ts_srt(7261.9999) == "02:01:02,000" + + +def test_normal_values_unchanged(): + assert SubtitleGen._ts_srt(0.0) == "00:00:00,000" + assert SubtitleGen._ts_srt(1.5) == "00:00:01,500" + assert SubtitleGen._ts_srt(0.4994) == "00:00:00,499" + assert SubtitleGen._ts_vtt(83.25) == "00:01:23.250" + + +def test_all_outputs_are_well_formed(): + # Sweep values that land on and around the dangerous boundary. + for t in (0.0, 0.4995, 0.9995, 0.9999, 1.0, 59.9995, 3599.9999, 12345.9999): + assert _SRT_RE.match(SubtitleGen._ts_srt(t)), SubtitleGen._ts_srt(t) + assert _VTT_RE.match(SubtitleGen._ts_vtt(t)), SubtitleGen._ts_vtt(t) diff --git a/tools/subtitle/subtitle_gen.py b/tools/subtitle/subtitle_gen.py index ac8472b6..a22f67d8 100644 --- a/tools/subtitle/subtitle_gen.py +++ b/tools/subtitle/subtitle_gen.py @@ -309,19 +309,27 @@ class SubtitleGen(BaseTool): return "\n".join(lines) @staticmethod - def _ts_srt(seconds: float) -> str: + def _hmsms(seconds: float) -> tuple[int, int, int, int]: + """Decompose seconds into (h, m, s, ms), rounding to whole ms first. + + Rounding to total milliseconds before splitting the fields lets the + carry propagate: 0.9995s+ must become the next second (…,000), not a + malformed 4-digit …,1000 with the seconds field left unincremented. + """ + total_ms = int(round(max(0.0, seconds) * 1000)) + h, rem = divmod(total_ms, 3_600_000) + m, rem = divmod(rem, 60_000) + s, ms = divmod(rem, 1_000) + return h, m, s, ms + + @classmethod + def _ts_srt(cls, seconds: float) -> str: """Format seconds as SRT timestamp: HH:MM:SS,mmm""" - h = int(seconds // 3600) - m = int((seconds % 3600) // 60) - s = int(seconds % 60) - ms = int(round((seconds % 1) * 1000)) + h, m, s, ms = cls._hmsms(seconds) return f"{h:02d}:{m:02d}:{s:02d},{ms:03d}" - @staticmethod - def _ts_vtt(seconds: float) -> str: + @classmethod + def _ts_vtt(cls, seconds: float) -> str: """Format seconds as VTT timestamp: HH:MM:SS.mmm""" - h = int(seconds // 3600) - m = int((seconds % 3600) // 60) - s = int(seconds % 60) - ms = int(round((seconds % 1) * 1000)) + h, m, s, ms = cls._hmsms(seconds) return f"{h:02d}:{m:02d}:{s:02d}.{ms:03d}" From 5dcd026ef77fc402e36aa630dd5f1357811c5039 Mon Sep 17 00:00:00 2001 From: 0xDevNinja Date: Tue, 7 Jul 2026 15:29:03 +0530 Subject: [PATCH 2/2] fix(checkpoint): don't KeyError on manifest stages without a canonical artifact MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit _validate_artifacts_for_stage looked up CANONICAL_STAGE_ARTIFACTS[stage] unconditionally, but the valid stage list comes from the pipeline manifest via get_pipeline_stages(), which declares stages beyond the 9 canonical ones — e.g. character-animation adds `character_design`/`rig_plan`. Such a stage passes the `stage in valid_stages` guard, then raised an unhandled KeyError on the canonical lookup, so those stages could never be checkpointed (the crash hits write/read_checkpoint and friends, even for in_progress checkpoints). Look the canonical artifact up defensively with `.get()` and skip the required-artifact check when there is none. Canonical stages still require their artifact when completed. --- lib/checkpoint.py | 13 +++- .../lib/test_checkpoint_noncanonical_stage.py | 62 +++++++++++++++++++ 2 files changed, 73 insertions(+), 2 deletions(-) create mode 100644 tests/lib/test_checkpoint_noncanonical_stage.py diff --git a/lib/checkpoint.py b/lib/checkpoint.py index b2b597a0..e35e6164 100644 --- a/lib/checkpoint.py +++ b/lib/checkpoint.py @@ -106,8 +106,17 @@ def _validate_artifacts_for_stage( status: str, artifacts: dict[str, Any], ) -> None: - required_artifact = CANONICAL_STAGE_ARTIFACTS[stage] - if status in {"completed", "awaiting_human"} and required_artifact not in artifacts: + # Valid stages come from the pipeline manifest (get_pipeline_stages), which + # can declare stages beyond the 9 canonical ones (e.g. character-animation's + # `character_design`/`rig_plan`, screen-demo's `real_capture`). Those have no + # canonical artifact, so look it up defensively — a missing entry means the + # stage simply has no required artifact, not a crash. + required_artifact = CANONICAL_STAGE_ARTIFACTS.get(stage) + if ( + required_artifact is not None + and status in {"completed", "awaiting_human"} + and required_artifact not in artifacts + ): raise CheckpointValidationError( f"Stage {stage!r} with status {status!r} must include " f"canonical artifact {required_artifact!r}" diff --git a/tests/lib/test_checkpoint_noncanonical_stage.py b/tests/lib/test_checkpoint_noncanonical_stage.py new file mode 100644 index 00000000..15225bc9 --- /dev/null +++ b/tests/lib/test_checkpoint_noncanonical_stage.py @@ -0,0 +1,62 @@ +"""Regression test: checkpoint validation must not crash on manifest-only stages. + +`_validate_artifacts_for_stage` looked up `CANONICAL_STAGE_ARTIFACTS[stage]` +unconditionally. Valid stages, however, come from the pipeline manifest +(`get_pipeline_stages`), which declares stages beyond the 9 canonical ones — +e.g. `character-animation` adds `character_design` / `rig_plan`. Those pass the +`stage in valid_stages` guard, then raised an unhandled `KeyError` on the +canonical lookup, so those stages could never be checkpointed. The lookup is now +defensive (`.get`), treating a missing entry as "no required artifact". +""" + +import sys +from pathlib import Path + +import pytest + +PROJECT_ROOT = Path(__file__).resolve().parent.parent.parent +sys.path.insert(0, str(PROJECT_ROOT)) + +from lib.checkpoint import ( # noqa: E402 + CheckpointValidationError, + get_pipeline_stages, + validate_checkpoint, +) + + +def _checkpoint(stage, status, artifacts, pipeline_type): + return { + "version": "1.0", + "project_id": "proj", + "pipeline_type": pipeline_type, + "stage": stage, + "status": status, + "timestamp": "2026-01-01T00:00:00Z", + "artifacts": artifacts, + } + + +def test_manifest_declares_noncanonical_stage(): + # Guard the premise: the manifest really does add stages the canonical map + # doesn't know about. + stages = get_pipeline_stages("character-animation") + assert "character_design" in stages + + +def test_noncanonical_stage_does_not_raise_keyerror(): + # character_design has no canonical artifact; completing it with no + # artifacts must validate cleanly rather than crash. + validate_checkpoint( + _checkpoint("character_design", "completed", {}, "character-animation") + ) + validate_checkpoint( + _checkpoint("rig_plan", "in_progress", {}, "character-animation") + ) + + +def test_canonical_stage_still_requires_its_artifact(): + # The fix must not weaken enforcement for canonical stages. + with pytest.raises(CheckpointValidationError): + validate_checkpoint( + _checkpoint("compose", "completed", {}, "character-animation") + )