Skip to content

fix: harden against confirmed review findings - #2

Merged
conorbronsdon merged 3 commits into
mainfrom
fix/hardening-review
Jul 6, 2026
Merged

fix: harden against confirmed review findings#2
conorbronsdon merged 3 commits into
mainfrom
fix/hardening-review

Conversation

@conorbronsdon

Copy link
Copy Markdown
Owner

Hardens the parser against one confirmed review finding. From an automated multi-agent review (personal-context#62); implemented + verified by Claude Code.

Fix 1 — --> in cue text destroys the block (correctness)

_parse_block treated any line containing the substring --> as a timing line. A legal cue whose text contains --> (prose, code, an arrow gloss) was therefore read as a cue boundary, failed to parse as timestamp --> timestamp, and the raised error discarded the entire block.

Fix: added _is_timing_line, which only treats a line as a cue boundary when both sides parse as timestamps, and used it for both the first-timing-line scan and the glued-cue boundary scan. Also isolated each segment's _parse_timing in its own try so a malformed timing line skips only that segment instead of discarding cues already gathered from the block.

Before/after evidence (input "1\n00:00:01,000 --> 00:00:02,000\nUse map --> filter here.\n"), run against the known-good Mojo nightly:

# before
cue count: 0
# after
cue count: 1
  text=[ Use map --> filter here. ]
srt roundtrip cue count: 1
vtt roundtrip cue count: 1

Regression tests added in test/test_captions.mojo:

  • test_arrow_in_cue_text_not_a_boundary — single-line --> text, plus to_srt/to_vtt round-trips
  • test_arrow_in_multiline_text_preserved--> on a later text line doesn't truncate the cue
  • test_arrow_text_still_splits_glued_real_cue — a genuinely glued second cue is still split even when the first cue's text holds a -->

Bundled build prerequisite — pixi pin

The repo's pixi.toml pinned mojo = ">=1.0.0b3,<2", which sorts below dev nightlies, so pixi install failed to solve (No candidates were found for mojo >=1.0.0b3,<2). Fixed to >=1.0.0b3.dev0,<2 (same fix already confirmed in mojo-redis) so the fix could be built and tested.

Build + tests

  • pixi install now solves.
  • pixi run test: 32 tests run, 32 passed, 0 failed, 0 skipped (29 pre-existing + 3 new). No live external servers involved.
  • test/fuzz_runner.mojo and examples/clip_transcript.mojo both build clean against the changed module.

Notes

  • Did not touch README.md or CHANGELOG.md (a separate agent is editing those in parallel PRs). No overlap required for this fix. The README's "parses liberally" behavior is unchanged and now actually holds for -->-bearing text.

conorbronsdon and others added 3 commits July 5, 2026 23:05
Adds a "Coming from Python" on-ramp table (verified against the repo's own
examples/ and tests) and corrects suite/test-count accuracy issues.

Co-Authored-By: Claude <noreply@anthropic.com>
A cue whose text contains `-->` (e.g. "Use map --> filter here.")
parsed as zero cues: `_parse_block` treated any line containing the
substring `-->` as a timing line, so the text line was taken as a new
cue boundary, then failed to parse as `timestamp --> timestamp` and
threw away the whole block.

Add `_is_timing_line`, which only treats a line as a cue boundary when
both sides parse as timestamps, and use it for both the first-timing-
line scan and the glued-cue boundary scan. Also isolate each segment's
`_parse_timing` in its own try so a malformed timing line skips only
that segment instead of discarding cues already gathered from the block.

Also fix the pixi mojo pin (`>=1.0.0b3` sorts below dev nightlies, so
`pixi install` failed to solve) to `>=1.0.0b3.dev0,<2` as a build
prerequisite for verifying the fix.

Adds regression tests for `-->` in single-line and multi-line cue text
and for a genuinely glued second cue whose predecessor's text holds a
`-->`. Full suite: 32 tests, all passing.

Co-Authored-By: Claude <noreply@anthropic.com>
… parallel docs branch

The Coming-from-Python README section belongs to the docs PR, not this
security/robustness fix branch.

Co-Authored-By: Claude <noreply@anthropic.com>
@conorbronsdon

Copy link
Copy Markdown
Owner Author

🤖 Independent Claude review: Ready to mark for review.

Adversarial second look — no blocking correctness/security issue found. I traced every changed path by hand against the source (no mojo toolchain in this sandbox, so I did not recompile; relying on the author’s green run for compilation).

Verified:

  • _is_timing_line returns True iff _parse_timing succeeds, so boundary detection is now exactly "line parses as ts --> ts". Both the first-timing scan and the glued-cue scan use it consistently.
  • Reproduced the before/after by trace: on the bug input the old code set t on the prose --> line on its second loop iteration, _parse_timing raised with no guard, and the whole block was discarded (0 cues). New code never selects a non-timing line, yielding 1 cue. Matches the PR’s claimed 0→1.
  • Tests are non-vacuous: all three would FAIL on the unfixed code (test_arrow_in_cue_text → 0 cues vs expected 1; multiline → truncated/0; still_splits_glued → 0 vs expected 2). to_srt/to_vtt round-trips check out.
  • New tests auto-register via TestSuite.discover_tests[__functions_in_module()] — no wiring gap.
  • No over-reach: only a private helper added; no public/Python-mirror signature change; the fix strictly recovers cues the old code discarded and can never drop a cue the old code emitted.
  • pixi pin >=1.0.0b3.dev0,<2 only widens the floor downward (dev sorts below b3), so it cannot break an environment that already solves; consistent with the mojo-redis precedent. (Could not reproduce the solve here — no toolchain.)

Non-blocking nits (optional):

  • captions.mojo:324 — the try/except around _parse_timing is effectively dead: t is only ever a line already validated by _is_timing_line. Fine as documented defense-in-depth.
  • A prose line that itself parses as ts --> ts (e.g. "00:01 --> 00:02") is still treated as a boundary — pre-existing inherent ambiguity, not introduced by this PR; a one-line doc note could acknowledge it.

@conorbronsdon
conorbronsdon marked this pull request as ready for review July 6, 2026 07:58
@conorbronsdon

Copy link
Copy Markdown
Owner Author

Code review (opus, static — verify CI): SHIP

The -->-in-cue-text fix is correct and the higher-value kind (includes real round-trip coverage). Root cause was find("-->") != -1 treating any -->-bearing line as a cue boundary; _is_timing_line now requires both sides to parse as timestamps, applied to both the first-timing scan (:306) and the glued-cue scan (:337). Traced parse→serialize→re-parse for "Use map --> filter here." — round-trips identically through to_srt and to_vtt; genuine glued cues still split. _is_timing_line correctly non-raises.

Nits (informational): the try at :324 is effectively dead (line pre-validated); _is_timing_line allocates per candidate so a pathological all---> block is O(n²) — pre-existing structure, fine for real subtitles. Ready pending CI-green.

@conorbronsdon
conorbronsdon merged commit 77cf74c into main Jul 6, 2026
1 check passed
@conorbronsdon
conorbronsdon deleted the fix/hardening-review branch July 6, 2026 08:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant