Skip to content

Fix a dropped morpheme ID when a zero-width rule wraps one ending at the same node - #500

Open
johnml1135 wants to merge 2 commits into
fix-506-directional-boundariesfrom
fix/signature-drops-id-under-identity-rule
Open

johnml1135 wants to merge 2 commits into
fix-506-directional-boundariesfrom
fix/signature-drops-id-under-identity-rule

Conversation

@johnml1135

@johnml1135 johnml1135 commented Sep 14, 2026 •

Copy link
Copy Markdown
Collaborator

Start here: src/SIL.Machine.Morphology.HermitCrab/MorphologicalRules/SynthesisAffixProcessAllomorphRuleSpec.cs, ApplyRhs: one block of code moved below a loop.

Status: stacked on #556 — merge in order #556 → #500 → #480 → #578.

Status: open since 2026-09-14. An automated review (claude) asks for changes: marking the last shape node after the loop moves that node from the wrapped morph to the zero-width rule, so the wrapped morph loses its last segment (F1); the fenos test passes if any one of 100 loads is right (F2); and test comments point at code absent from this repo (F3, F4). No CI check is red (claude-review was skipped; code-review/reviewable is pending).

When a zero-width rule (such as a copy-only CopyFromInput) wraps another rule's affix and both mark the same trailing shape node, the wrapped morpheme ID was dropped. ApplyRhs now marks the zero-width rule's last node after the loop that re-marks wrapped input morphs, so the wrapped ID sticks and the outer rule's ID sorts after it.

Reviewers arrive asking whether the regression test is deterministic. It is not: the fix makes the correct signature reachable in 41 of 100 fresh language loads in an earlier measurement, and 0 of 100 with the fix reverted. The rest trace to a separate, unfixed ordering defect (#506, open; #556 addresses its tie-boundary mechanism and is open). The question worth your time is F1: does the node move cost the wrapped morph its last segment in a case the tests do not cover?

Where to look:

  • ApplyRhs: the outputNewMorph == null marking now runs after the loop. Pinned only by the tests below.
  • IdentityRuleSignatureMorphDropTests: controls Fen_BareRootControl_IdentityWrapsRootDirectly_StableAndCorrect and Feno_NoIdentityRuleApplies_StableAndCorrect are deterministic. Fenos_IdentityWrapsOutermost_IsFlakyPendingASeparateOrderingDefect retries up to 100 fresh loads and passes on the first correct signature.

Not here: the ordering defect behind #506, which keeps the regression from passing on every attempt.

Verification: build, the HermitCrab tests, csharpier and the comment-hygiene script passed when the PR was opened; counts are not recomputed here and nothing was rerun for this description.

Next: author to answer F1 (with a test, or a reason the lost segment is harmless), then re-request review.


Reading this a year from now -- start here

ApplyRhs marks morphs on the output shape in two places: a fallback for rules with no new output morphs (truncation and zero-width rules), and a loop that re-marks each input morph the rule wraps. When a zero-width rule's own affix and an inner wrapped rule's affix end at the same trailing shape node, whichever call runs second keeps that node's morpheme ID. The fallback used to run first, so the outer rule's ID could overwrite or precede the wrapped rule's. Moving it after the loop makes the wrapped rule's ID stick and the outer rule's ID sort after it, matching the order IdentityRuleSignatureMorphDropTests expects.

Decisions, and why

The fenos test retries up to 100 fresh loads and passes on the first correct signature. Measured earlier: 41/100 correct with the fix, 0/100 with it reverted. Asserting every attempt would be red even with the fix; a single assertion would be flaky. Retry-until-first-success is the one assertion the data supports, and it goes red with the fix reverted. The cost is that it stays green if the fix works only once in 100 (F2).

Paths not taken

An earlier comment blamed BidirList's unseeded skip-list levels. The source does not support that: same-range annotations tie-break on Annotation.ListID, a call-order counter. The claim was removed.

Deferred

Making every fresh parse deterministic needs the ordering defect in #506 fixed first. Whether #556 is enough is not established; the cause of the per-run variation is not yet identified.

Surprising findings

An earlier revision reported "five green repetitions; suite 575 passed, one skipped, zero failed" and a seed experiment claiming 12/12 repeatability versus three unseeded outputs, attributed to forcing a fixed seed on BidirList's Random. Those numbers were not reproduced and the causal claim did not hold against the source; the 41/100 and 0/100 counts replace them.

🤖 Generated with Claude Code


This change is Reviewable

@codecov-commenter

codecov-commenter commented Sep 25, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 74.62%. Comparing base (41f319f) to head (f531fb1).

Additional details and impacted files
@@                       Coverage Diff                       @@
##           fix-506-directional-boundaries     #500   +/-   ##
===============================================================
  Coverage                           74.62%   74.62%           
===============================================================
  Files                                 453      453           
  Lines                               38345    38345           
  Branches                             5285     5285           
===============================================================
  Hits                                28614    28614           
  Misses                               8568     8568           
  Partials                             1163     1163           

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@johnml1135
johnml1135 force-pushed the fix/signature-drops-id-under-identity-rule branch from 96d6b7f to 5aa7b39 Compare September 25, 2026 22:54
@johnml1135
johnml1135 requested a review from jtmaxwell3 October 21, 2026 17:30
@johnml1135
johnml1135 force-pushed the fix/signature-drops-id-under-identity-rule branch from 5aa7b39 to 157c031 Compare October 23, 2026 11:09
@claude

This comment has been minimized.

johnml1135 added a commit that referenced this pull request Oct 24, 2026
Pin full parse multiplicity and application histories from combined Machine
#500 and #556 in three derivatives of the #480 truncation grammar. Update
fixture-derived coverage, interaction ledgers and their audit pins.

The original grammar is unchanged. The complete expectations intentionally
remain red on the conformance branch until both engine fixes are applied.
@johnml1135
johnml1135 force-pushed the fix/signature-drops-id-under-identity-rule branch from 157c031 to 857313b Compare October 26, 2026 11:50
johnml1135 added a commit that referenced this pull request Oct 26, 2026
Pin full parse multiplicity and application histories from combined Machine
#500 and #556 in three derivatives of the #480 truncation grammar. Update
fixture-derived coverage, interaction ledgers and their audit pins.

The original grammar is unchanged. The complete expectations intentionally
remain red on the conformance branch until both engine fixes are applied.
…the same node

In SynthesisAffixProcessAllomorphRuleSpec.ApplyRhs, a zero-width rule's
fallback "no new output morphs" marking claimed the shared final shape
node before the loop that re-marks the wrapped rule's own morph, so the
wrapped rule's morpheme ID could be overwritten or ordered ahead of the
outer rule's. Move the fallback marking after that loop so the wrapped
allomorph keeps its ID and the outer rule's ID sorts after it.

Adds IdentityRuleSignatureMorphDropTests with two deterministic controls
and one regression that retries fresh language loads. Measured over 100
fresh loads: with the fix, the correct signature is reached 41/100
times; with the fix reverted, 0/100 -- confirmed by reverting the
production change and rerunning. The remaining gap under the fix is a
separate, unfixed ordering defect (tracked in #506); this PR does not
make output deterministic for every ordering, only reachable.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@johnml1135
johnml1135 changed the base branch from master to fix-506-directional-boundaries October 29, 2026 18:45
@johnml1135
johnml1135 force-pushed the fix/signature-drops-id-under-identity-rule branch from 857313b to b86120f Compare October 29, 2026 18:46
@johnml1135
johnml1135 force-pushed the fix-506-directional-boundaries branch from 579b2ef to 41f319f Compare October 29, 2026 18:46
[TestFixture]
public class IdentityRuleSignatureMorphDropTests
{
private const string GrammarXml = """

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

FYI: F5. GrammarXml and WriteTempGrammar (line 99) use "grammar" for the HermitCrab configuration. AGENTS.md reserves that word and asks for Language or Stratum instead, e.g. LanguageXml and WriteTempLanguage.

@claude

claude Bot commented Oct 30, 2026

Copy link
Copy Markdown
Contributor
  1. Verdict: approve.
  2. Most important: nothing blocks. F1 is withdrawn: Word.MarkMorph builds each morph's range from its node endpoints (src/SIL.Machine.Morphology.HermitCrab/Word.cs:281), so moving the zero-width marking to SynthesisAffixProcessAllomorphRuleSpec.cs:201-207 only changes which morph has no node children. Before the change, the zero-width morph was the one with none. Because only one morph can own a node, two morphs still compete for a shared boundary node (line 185 does the same with Shape.First). That is older than this PR; if a later rule needs a fix for it, it is a design question for a separate change.
  3. Counts: Critical 0, Important 0, Low 1 (F5).
  4. What I ran: dotnet test on the HermitCrab test project in Release: 154 passed, 0 failed. gh pr checks 500: the ubuntu and windows builds and the NuGet package pass; the comment-hygiene check is advisory and passes, which does not stand in for the strict scan; claude-review and Reviewable are pending.
  5. Not verified: I could not run a scratch probe for a rule applied after the zero-width rule or after a truncation, because this session could not write files. The downstream morph order there is traced only. My trace shows both the old and the new code put a morph in the wrong position, so this PR does not make that case worse.

Public API, target frameworks, packages, and sillsdev/machine.py parity: None verified.

Findings: F1 withdrawn; F2 addressed; F3 addressed; F4 addressed; F5 new.

Reviewed at f531fb1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants