Skip to content

Fix crash and misplacement from unclosed style markers #536

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Enkidu93 merged 2 commits into from
Oct 13, 2026
Merged

Fix crash and misplacement from unclosed style markers #536

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
788b02f
Port 'Fix unclosed style marker crash' from machine.py PR #364
github-actions[bot] Oct 7, 2026
3829a43
Port remaining place-markers tests from machine.py PR #364
github-actions[bot] Oct 9, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 9 additions & 17 deletions src/SIL.Machine/Corpora/PlaceMarkersUsfmUpdateBlockHandler.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,17 +54,7 @@ public UsfmUpdateBlock ProcessBlock(UsfmUpdateBlock block)
elements.Count == 0
|| alignmentInfo.Alignment.RowCount == 0
|| alignmentInfo.Alignment.ColumnCount == 0
|| !elements.Any(e =>
(
e.Type == UsfmUpdateBlockElementType.Paragraph
&& alignmentInfo.ParagraphBehavior == UpdateUsfmMarkerBehavior.Preserve
&& e.Tokens.Count == 1
)
|| (
e.Type == UsfmUpdateBlockElementType.Style
&& alignmentInfo.StyleBehavior == UpdateUsfmMarkerBehavior.Preserve
)
)
|| !elements.Any(e => e.IsPlaceable(alignmentInfo.ParagraphBehavior, alignmentInfo.StyleBehavior))
)
{
return block;
Expand Down Expand Up @@ -99,10 +89,8 @@ public UsfmUpdateBlock ProcessBlock(UsfmUpdateBlock block)
else if (
!(
element.Type == UsfmUpdateBlockElementType.Embed
|| (
element.Type == UsfmUpdateBlockElementType.Text
&& element.Tokens[0].ToUsfm().Trim().Length == 0
)
|| element.Type == UsfmUpdateBlockElementType.Other
Comment thread
claude[bot] marked this conversation as resolved.
|| (element.Type == UsfmUpdateBlockElementType.Text && element.GetText().Trim().Length == 0)
)
)
{
Expand All @@ -126,7 +114,7 @@ public UsfmUpdateBlock ProcessBlock(UsfmUpdateBlock block)
{
if (element.MarkedForRemoval)
{
string text = element.Tokens[0].ToUsfm();
string text = element.GetText();

// Track seen tokens
while (
Expand All @@ -146,7 +134,7 @@ public UsfmUpdateBlock ProcessBlock(UsfmUpdateBlock block)
}
else
{
targetSentence += element.Tokens[0].ToUsfm();
targetSentence += element.GetText();

@claude claude Bot Oct 7, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI: F2. Only the last row's alignment applies: targetSentence now joins every row in the block, but AdvanceRows (UpdateUsfmParserHandler.cs:562) keeps only the last matching row's alignment_info. A verse range with a separate alignment per row will place markers against the wrong text. This matches machine.py and the new test shares one alignment, so it is a caller contract to document. Unverified.

}
}

Expand Down Expand Up @@ -238,6 +226,10 @@ public UsfmUpdateBlock ProcessBlock(UsfmUpdateBlock block)
toInsert.Sort((p1, p2) => p1.Index.CompareTo(p2.Index));
toInsert.AddRange(embedElements.Concat(endElements).Select(e => (targetSentence.Length, e)));

// In the case of unclosed markers, toInsert might be empty
if (toInsert.Count == 0)
return block;

// Construct new text tokens to put between markers
// and reincorporate headers and empty end-of-verse paragraph markers
if (toInsert[0].Index > 0)
Expand Down
33 changes: 22 additions & 11 deletions src/SIL.Machine/Corpora/UpdateUsfmParserHandler.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -296,7 +296,13 @@ public override void StartNote(UsfmParserState state, string marker, string call
public override void EndNote(UsfmParserState state, string marker, bool closed)
{
if (closed)
CollectUpdatableTokens(state);
{
// Mirror StartNote: an embed in a duplicate verse is dropped, end marker included.
if (DuplicateVerse)
SkipUpdatableTokens(state);
else
CollectUpdatableTokens(state);
}
base.EndNote(state, marker, closed);
}

Expand Down Expand Up @@ -334,21 +340,26 @@ public override void EndChar(
bool closed
)
{
// strip out char-style markers in verses that are being replaced
if (CurrentTextType == ScriptureTextType.Embed)
{
CollectUpdatableTokens(state);
}
else
// An implicitly closed character style has no end marker of its own, so the token at
// state.Index belongs to whatever closed it. Leave it for the callback that handles it.
if (closed)
{
ReplaceWithNewTokens(state);
if (_styleBehavior == UpdateUsfmMarkerBehavior.Strip)
// strip out char-style markers in verses that are being replaced
if (CurrentTextType == ScriptureTextType.Embed)
{
SkipUpdatableTokens(state);
CollectUpdatableTokens(state);
}
else
{
CollectUpdatableTokens(state);
ReplaceWithNewTokens(state);
if (_styleBehavior == UpdateUsfmMarkerBehavior.Strip)
{
SkipUpdatableTokens(state);
}
else
{
CollectUpdatableTokens(state);
}
}
}

Expand Down
17 changes: 17 additions & 0 deletions src/SIL.Machine/Corpora/UsfmUpdateBlockElement.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
using System.Collections.Generic;
using System.Linq;

namespace SIL.Machine.Corpora
{
Expand Down Expand Up @@ -32,5 +33,21 @@ public List<UsfmToken> GetTokens()
{
return MarkedForRemoval ? new List<UsfmToken>() : new List<UsfmToken>(Tokens);
}

public string GetText()
{
return string.Concat(Tokens.Select(t => t.ToUsfm()));
}

public bool IsPlaceable(UpdateUsfmMarkerBehavior paragraphBehavior, UpdateUsfmMarkerBehavior styleBehavior)

@claude claude Bot Oct 7, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI: F3. This adds public API to a netstandard2.0 package: IsPlaceable and GetText are public on UsfmUpdateBlockElement, and the paragraph single-token rule belongs to PlaceMarkersUsfmUpdateBlockHandler. internal keeps the parity with machine.py without making that rule part of the contract.

{
if (MarkedForRemoval)
return false;
if (Type == UsfmUpdateBlockElementType.Paragraph)
return paragraphBehavior == UpdateUsfmMarkerBehavior.Preserve && Tokens.Count == 1;
if (Type == UsfmUpdateBlockElementType.Style)
return styleBehavior == UpdateUsfmMarkerBehavior.Preserve;
return false;
}
}
}
228 changes: 222 additions & 6 deletions tests/SIL.Machine.Tests/Corpora/PlaceMarkersUsfmUpdateBlockHandlerTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -397,10 +397,10 @@ public void UpdateUsfm_ConsecutiveMarkers()
[Test]
public void UpdateUsfm_VerseRanges()
{
IReadOnlyList<UpdateUsfmRow> rows = Enumerable
.Range(1, 6)
.Select(i => new UpdateUsfmRow(
[ScriptureRef.Parse($"MAT 1:{i}")],
IReadOnlyList<UpdateUsfmRow> rows =
[
new UpdateUsfmRow(
[.. Enumerable.Range(1, 5).Select(i => ScriptureRef.Parse($"MAT 1:{i}"))],
"New verse range text new paragraph 2",
new Dictionary<string, object>
{
Expand All @@ -415,8 +415,8 @@ public void UpdateUsfm_VerseRanges()
)
},
}
))
.ToList();
),
];
string usfm =
@"\id MAT
\c 1
Expand Down Expand Up @@ -911,6 +911,222 @@ public void UpdateUsfm_AdjustmentOfPlacedParagraphMarker()
AssertUsfmEquals(target, result);
}

[Test]
public void UpdateUsfm_UnclosedStyleMarkerInNonVerseParagraph()
{
string source = "(A)";
string pretranslation = "(A translated)";
PlaceMarkersAlignmentInfo alignInfo = new PlaceMarkersAlignmentInfo(
sourceTokens: Tokenizer.Tokenize(source).ToList(),
translationTokens: Tokenizer.Tokenize(pretranslation).ToList(),
alignment: ToWordAlignmentMatrix("0-0 1-1 2-2"),
paragraphBehavior: UpdateUsfmMarkerBehavior.Preserve,
styleBehavior: UpdateUsfmMarkerBehavior.Strip
);
IReadOnlyList<UpdateUsfmRow> rows =
[
new UpdateUsfmRow(
ScrRef("PSA 119:0/1:d"),
pretranslation,
new Dictionary<string, object> { { "alignment_info", alignInfo } }
),
new UpdateUsfmRow(ScrRef("PSA 119:1"), "New verse 1"),
];
string usfm =
@"\id PSA
\c 119
\d \bd (A)
\q1
\v 1 Verse 1
";

string target = UpdateUsfm(rows, usfm, usfmUpdateBlockHandlers: [new PlaceMarkersUsfmUpdateBlockHandler()]);

string result =
@"\id PSA
\c 119
\d (A translated)
\q1
\v 1 New verse 1
";
AssertUsfmEquals(target, result);
}

[Test]
public void UpdateUsfm_UnmatchedEndMarker()
{
string source = "Section header";
string pretranslation = "New section header";
PlaceMarkersAlignmentInfo alignInfo = new PlaceMarkersAlignmentInfo(
sourceTokens: Tokenizer.Tokenize(source).ToList(),
translationTokens: Tokenizer.Tokenize(pretranslation).ToList(),
alignment: ToWordAlignmentMatrix("0-1 1-2"),
paragraphBehavior: UpdateUsfmMarkerBehavior.Preserve,
styleBehavior: UpdateUsfmMarkerBehavior.Preserve
);
IReadOnlyList<UpdateUsfmRow> rows =
[
new UpdateUsfmRow(
ScrRef("MAT 1:0/1:s"),
pretranslation,
new Dictionary<string, object> { { "alignment_info", alignInfo } }
),
new UpdateUsfmRow(ScrRef("MAT 1:1"), "New verse 1"),
];
string usfm =
@"\id MAT
\c 1
\s Section header\it*
\p
\v 1 Verse 1
";

string target = UpdateUsfm(
rows,
usfm,
styleBehavior: UpdateUsfmMarkerBehavior.Preserve,
usfmUpdateBlockHandlers: [new PlaceMarkersUsfmUpdateBlockHandler()]
);

string result =
@"\id MAT
\c 1
\s New section header
\p
\v 1 New verse 1
";
AssertUsfmEquals(target, result);
}

[Test]
public void UpdateUsfm_MultipleTextRowsInVerseRangesAreUpdated()
{
string source = "This is the first part. This is the second part.";
string pretranslation = "Esta es la primera parte. Esta es la segunda parte.";
PlaceMarkersAlignmentInfo alignInfo = new PlaceMarkersAlignmentInfo(
sourceTokens: Tokenizer.Tokenize(source).ToList(),
translationTokens: Tokenizer.Tokenize(pretranslation).ToList(),
alignment: ToWordAlignmentMatrix("0-0 1-1 2-2 3-3 4-4 5-5 6-6 7-7 8-8 9-9 10-10"),
paragraphBehavior: UpdateUsfmMarkerBehavior.Preserve,
styleBehavior: UpdateUsfmMarkerBehavior.Strip
);
IReadOnlyList<UpdateUsfmRow> rows =
[
new UpdateUsfmRow(
ScrRef("MAT 1:1"),
"Esta es la primera parte.",
new Dictionary<string, object> { { "alignment_info", alignInfo } }
),
new UpdateUsfmRow(
ScrRef("MAT 1:2"),
"Esta es la segunda parte.",
new Dictionary<string, object> { { "alignment_info", alignInfo } }
),
];
string usfm =
@"\id MAT
\c 1
\v 1-2 This is the first part.
\p This is the second part.
";

string target = UpdateUsfm(rows, usfm, usfmUpdateBlockHandlers: [new PlaceMarkersUsfmUpdateBlockHandler()]);

string result =
@"\id MAT
\c 1
\v 1-2 Esta es la primera parte.
\p Esta es la segunda parte.
";
AssertUsfmEquals(target, result);
}

[Test]
public void UpdateUsfm_MarkerBehaviorDisagreesWithAlignmentInfo()
{
string source = "Section header";
string pretranslation = "New section header";
PlaceMarkersAlignmentInfo alignInfo = new PlaceMarkersAlignmentInfo(
sourceTokens: Tokenizer.Tokenize(source).ToList(),
translationTokens: Tokenizer.Tokenize(pretranslation).ToList(),
alignment: ToWordAlignmentMatrix("0-1 1-2"),
paragraphBehavior: UpdateUsfmMarkerBehavior.Preserve,
styleBehavior: UpdateUsfmMarkerBehavior.Preserve
);
IReadOnlyList<UpdateUsfmRow> rows =
[
new UpdateUsfmRow(
ScrRef("MAT 1:0/1:s"),
pretranslation,
new Dictionary<string, object> { { "alignment_info", alignInfo } }
),
new UpdateUsfmRow(ScrRef("MAT 1:1"), "New verse 1"),
];
string usfm =
@"\id MAT
\c 1
\s Section \it header\it*
\p
\v 1 Verse 1
";

string target = UpdateUsfm(
rows,
usfm,
styleBehavior: UpdateUsfmMarkerBehavior.Strip,
usfmUpdateBlockHandlers: [new PlaceMarkersUsfmUpdateBlockHandler()]
);

string result =
@"\id MAT
\c 1
\s New section header
\p
\v 1 New verse 1
";
AssertUsfmEquals(target, result);
}

[Test]
public void UpdateUsfm_OtherElementsDoNotAffectEmbedPlacement()
{
string source = "This is the first part. This is the second part.";
string pretranslation = "Esta es la primera parte. Esta es la segunda parte.";
string result =
@"\id MAT
\c 1
\v 1 Esta es la primera parte. Esta es la segunda parte. \f + \ft Footnote\f*
\q1
\q2
";
foreach (string milestone in new[] { "", @" \ts-s\*" })
{
PlaceMarkersAlignmentInfo alignInfo = new PlaceMarkersAlignmentInfo(
sourceTokens: Tokenizer.Tokenize(source).ToList(),
translationTokens: Tokenizer.Tokenize(pretranslation).ToList(),
alignment: ToWordAlignmentMatrix("0-0 1-1 2-2 3-3 4-4 5-5 6-6 7-7 8-8 9-9 10-10"),
paragraphBehavior: UpdateUsfmMarkerBehavior.Preserve,
styleBehavior: UpdateUsfmMarkerBehavior.Strip
);
IReadOnlyList<UpdateUsfmRow> rows =
[
new UpdateUsfmRow(
ScrRef("MAT 1:1"),
pretranslation,
new Dictionary<string, object> { { "alignment_info", alignInfo } }
),
];
string usfm =
"\\id MAT\n\\c 1\n"
+ "\\v 1 This is the first part. This is the second part.\\f + \\ft Footnote\\f*\n"
+ $"\\q1{milestone}\n\\q2\n";

string target = UpdateUsfm(rows, usfm, usfmUpdateBlockHandlers: [new PlaceMarkersUsfmUpdateBlockHandler()]);

AssertUsfmEquals(target, result);
}
}

[Test]
public void UpdateBlock_AnusvaraTokenization()
{
Expand Down
Loading
Loading