Skip to content

Fix directional annotation tie boundaries - #556

Open
johnml1135 wants to merge 1 commit into
masterfrom
fix-506-directional-boundaries
Open

johnml1135 wants to merge 1 commit into
masterfrom
fix-506-directional-boundaries

Conversation

@johnml1135

@johnml1135 johnml1135 commented Oct 23, 2026 •

Copy link
Copy Markdown
Collaborator

Quick summary

Word copying and segment deletion preserve tied annotations at their lookup boundaries. BidirList.Find now descends to level zero before returning an equal node, selecting the first match in the requested search direction. The patch changes the shared lookup and adds deterministic regression tests.

Where to look

  • BidirList.Find: both directions must reach the correct edge of an equal-range group.
  • AnnotationTieTests: fixed layouts exercise lookup, GetNodes, copied/cloned payloads and mapped endpoints, exact survivors, and tied children under shrinking multi-node owners.

Deliberately not included

The independent tied-container descendant lookup and flat spanning-annotation end-removal defects are tracked in #554 and #555. The a/ga conformance fixture update is being prepared separately for #480.

Validation

  • ./local_check.sh --agent-strict: passed with one-node WSL settings and the existing native CI build output restored; all normal stages ran.
  • Focused AnnotationTieTests on committed d98d12a5: 1,200 passed, 0 failed, 0 skipped; independently rerun by the orchestrator.
  • Independent fix-removed check of the final 1,200-case suite: 977 failed and 223 passed after reverting only the lookup change in a separate worktree; restoring the committed fix returns the suite to green.
  • git diff --check origin/master...HEAD: passed. Independent Sol source/test review passed; final exact-SHA binding recorded separately.
  • Ten fresh-process corrected-oracle runs matched the complete two a and five ga identity multisets, including application order, morph order, allomorph index and features.

Issue / porting context

Addresses #506's directional tie-boundary mechanism. Related: #500, #480, #554, #555.


Native prerequisite and evidence scope

Base: 259920414132bdd5a85542722d9af5509aed8943. Candidate: d98d12a5d514ce589d7495c71d055830e061a418.

The WSL environment lacks CMake. The existing sp4c-linux-x64 output from the successful exact-base CI run was restored after verifying the native wrapper's source inputs were unchanged. Its required tcmalloc runtime was downloaded, hash-checked and extracted under task-private /tmp; no system package was installed and no product dependency changed. The normal strict gate used a command-scoped LD_LIBRARY_PATH for that runtime.

The full local script does not collect CI-equivalent coverage. The finite identity checks and deterministic layouts establish the exercised cases; #554/#555 remain separate findings.

Generated with OpenAI Codex.


This change is Reviewable

@codecov-commenter

Copy link
Copy Markdown

Codecov Report

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

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #556      +/-   ##
==========================================
+ Coverage   74.57%   74.62%   +0.04%     
==========================================
  Files         453      453              
  Lines       38314    38317       +3     
  Branches     5280     5281       +1     
==========================================
+ Hits        28573    28594      +21     
+ Misses       8576     8561      -15     
+ Partials     1165     1162       -3     

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

return true;
}

break;

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: F1. The public Find(start, example, dir) overload can now return a tie that comes before start, and which tie it returns depends on random node levels. Example: start S ties example with Levels == 2, and tie T (level 1) sits between S.GetPrev(1) and S. Before this change the call returned S; now it drops to level 0 and returns T. If S has Levels == 1, it still returns S. Nothing in src/ calls this overload (searched for Find( in src/), so the visible effect is limited to outside callers. Unverified: I reasoned this through but did not reproduce it.

@claude

claude Bot commented Oct 23, 2026 •

Copy link
Copy Markdown
Contributor
  1. Verdict: approve. BidirList.Find now goes down to level 0 before it returns a match, so Shape lookups find the first tie in each direction.
  2. Most important: F1 (Low), src/SIL.Machine/DataStructures/BidirList.cs:273. The Find(start, ...) overload can now return a tie that comes before start. Nothing in src/ calls it.
  3. Counts: Critical 0, Important 0, Low 1.
  4. Ran the focused tests (AnnotationTieTests, AnnotationTests, SkipList, ShapeTests) at the PR head: 1,205 passed, 0 failed, 588 ms. CI gh pr checks 556: both builds (ubuntu-22.04, windows-latest) pass; Create NuGet package is pending.
  5. Not verified: I did not reproduce F1 with a test, and I did not run the HermitCrab conformance fixtures from the PR body.

Public API / TFM / package / machine.py parity: the return-value behavior of the public BidirList.Find overloads changed: with ties, they now return the first match in the search direction, where before they returned whichever match the search reached first. Signatures are unchanged. BidirList has no sillsdev/machine.py counterpart.

Findings: F1 new, unverified.

Reviewed at d98d12a

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

2 participants