[johnml1135]

Retargeted to master. The conformance suite (#480) is deferred until after this release, so this PR no longer stacks on it. Its edge-cases/interposed-optional-multi-rhs fixture is held back and will be added to #480 after this merges; the unit tests here cover the fix.

Rebased onto #539. Master now includes #539's union-aligned encoding, whose Unapply had the same raw targetNode.Next walk in two places (the union loop and the "make the rest optional" loop). Both now read each replacement position from its own group capture. #539's union behaviour, its HasVariables guards and its tests (MergeWithBoundary, AlphaAgreementMergeFindsAbbaFromApa) are unchanged. A position whose group captured nothing ends the union and is not marked, and only each capture's start node is marked.

Quick summary

A root is no longer lost in analysis when a deletion rule ordered after a narrowing rule reinserts an optional node inside the narrowing rule's replacement. Only parses that were silently dropped change; the rest of the HermitCrab suite is unchanged. Synthesis, feature rules, and #539's union behaviour are untouched.

Where to look

• The unapply walk now reads each replacement position from its own group capture, as FeatureAnalysisRewriteRuleSpec already does -- RewriteRuleTests.MergeRuleUnappliesAcrossInterposedOptionalSegment (red on master, line 636 assertion; green here).
• A boundary interposed in the same span (the analysis matcher filters boundaries out, so the raw walk stepped onto it too) -- RewriteRuleTests.MergeRuleUnappliesAcrossInterposedOptionalBoundary (red on master, green here).
• The same loss right to left -- RewriteRuleTests.RightToLeftMergeRuleUnappliesAcrossInterposedOptionalSegment (red on master, green here).
• Expansion rules (u -> i i) share this spec and this walk -- RewriteRuleTests.ExpandRuleUnappliesAcrossInterposedOptionalSegment (root lost on master, green here); RewriteRuleTests.ExpandRules still passes.
• A replacement is not validated to be plain constraints, so a quantifier there can leave its group capture empty -- RewriteRuleTests.MergeRuleWithOptionalReplacementSegmentSkipsEmptyCapture (NullReferenceException without the Success guard, green here; it also passes on master, whose raw walk happens to land on real nodes for this word).

Deliberately not included

• Rejecting a quantifier in a narrowing rule's replacement at compile time, as 6304028 does for feature rules, and the unchecked cast on the synthesis side (NarrowSynthesisRewriteSubruleSpec.ApplyRhs) that such a rule still trips when it fires. Both are pre-existing and separate from this walk.

Validation (head 3e99f8e)

• dotnet test tests/SIL.Machine.Morphology.HermitCrab.Tests -c Release -m:4 -nodeReuse:false with master's NarrowAnalysisRewriteRuleSpec.cs (which includes Fix a performance bug in NarrowAnalysisRewriteRuleSpec #539) -- the interposed-node tests fail, including the right-to-left and expansion cases (the expansion test returns no analyses).
• Same command at HEAD, full suite -- Failed: 0, Passed: 115.
• dotnet test tests/SIL.Machine.Tests -c Release -m:4 -nodeReuse:false -- Passed: 887, Skipped: 3, Failed: 0.
• pwsh ./scripts/comment-hygiene.ps1 -- clean.
• dotnet csharpier check . -- Checked 720 files, clean.
• git diff --check origin/master...HEAD -- clean.
• Not run: ./local_check.sh --agent-strict as a whole.

Not CI-equivalent: no coverage collected.

---

Reading this a year from now

NarrowAnalysisRewriteRuleSpec.Unapply handles a rule l1..ln -> r1..rm in analysis by inserting l1..ln as optional nodes after the match and marking the matched r1..rm optional, so lexical lookup can read either the surface form or the underlying one. It found r1..rm by walking range.Start.Next m times. The analysis matcher skips optional nodes and never sees boundaries (its filter is Segment|Anchor), so a match of r1 r2 can span an interior optional node or a boundary. The raw walk then marked that node instead of r2; r2 stayed mandatory and the l1..ln reading was unreachable.

Reproduction (test fixture language, Morphophonemic stratum): synthesis order s a g -> n i, then z -> 0 / n _ i, which is vacuous in synthesis. In analysis the deletion unapplies first and inserts (z) between n and i; the merge then matches n (z) i and the walk marks n and (z), leaving i mandatory. Surface ni should parse to root 32 (sag) and did not. Replacing z with the boundary + reproduces the same loss through the matcher's filter instead of through an optional node.

The fix wraps each replacement constraint in a named group in the analysis pattern and marks the captured node of each group optional. That is the encoding FeatureAnalysisRewriteRuleSpec already uses for its per-position read-back.

Paths not taken

• Walking range.Start.Next while skipping optional and boundary nodes. Rejected: it re-derives what the matcher already decided, and would disagree with the matcher whenever its skip rules change.
• Folding this into Fix a performance bug in NarrowAnalysisRewriteRuleSpec #539. Rejected: the bug predates Fix a performance bug in NarrowAnalysisRewriteRuleSpec #539 and is reachable without it; this PR now sits on Fix a performance bug in NarrowAnalysisRewriteRuleSpec #539 and replaces its walk.

🤖 Generated with Claude Code

---

This change is 

[codecov-commenter]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 74.41%. Comparing base (8983141) to head (3e99f8e).

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #540      +/-   ##
==========================================
+ Coverage   74.38%   74.41%   +0.03%     
==========================================
  Files         456      456              
  Lines       38343    38355      +12     
  Branches     5264     5268       +4     
==========================================
+ Hits        28522    28543      +21     
+ Misses       8665     8656       -9     
  Partials     1156     1156              

☔ 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.

[claude]

1. Verdict: approve. Since the last round the only change is one test, and it closes the last open finding. Nothing blocks the PR.
2. Most important: none open. The F2 gap is closed by ExpandRuleUnappliesAcrossInterposedOptionalSegment (RewriteRuleTests.cs:650), which covers an expansion rule whose RHS is matched across an interposed optional node.
3. Counts: Critical 0, Important 0, Low 0 open.
4. Ran: dotnet test tests/SIL.Machine.Morphology.HermitCrab.Tests -c Release -m:4 -nodeReuse:false on the merge checkout: Failed 0, Passed 115, Total 115. Compare against bd15f64 reports ahead, and git diff bd15f64 3e99f8e touches only RewriteRuleTests.cs (+51). I read that diff by hand and found nothing new. Head CI: Build on ubuntu-22.04 and both comment-hygiene reports are green; Build on windows-latest was still running.
5. Not verified: this environment refused a scratch worktree and a copy of the tree, so I could not confirm the new test is red with master's NarrowAnalysisRewriteRuleSpec.cs. It also refused ./local_check.sh --agent-strict and comment-hygiene.ps1, so the strict gate is unchecked; the advisory green check does not stand in for it. CI coverage is unchecked too.

Public API, target framework, package, machine.py parity: None verified.

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

Reviewed at 3e99f8e

[jtmaxwell3]

Looks good! Thanks!