Skip to content

Fix directional annotation tie boundaries - #556

Open
johnml1135 wants to merge 2 commits into
masterfrom
fix-506-directional-boundaries
Open

johnml1135 wants to merge 2 commits into
masterfrom
fix-506-directional-boundaries

Conversation

@johnml1135

@johnml1135 johnml1135 commented Oct 23, 2026 •

Copy link
Copy Markdown
Collaborator

Start here: src/SIL.Machine/DataStructures/BidirList.cs, Find: the whole source change is a 9-line edit there.

Status: first of a stack — merge in order #556 → #500 → #615.

Ready for review. CI has no red checks (the claude-review job was skipped).

BidirList.Find now descends to level zero before returning an equal node, so it selects the first match in the requested search direction. Before, an equal node found on a higher skip-list level was returned at once, so word copying and segment deletion could land on the wrong member of a tied annotation group. The fix is in the shared lookup, and the tests are deterministic fixed layouts.

Reviewers arrive asking how a change to a shared data-structure lookup is pinned. Every case in AnnotationTieTests runs over 200 seeds (skip-list shapes), and reverting only the lookup change breaks most of them. The question worth your time: does returning the first tie in the search direction match what callers of Find expect when they pass an offset that starts or ends a tie?

Where to look:

  • BidirList.Find: both directions must reach the correct edge of an equal-range group. Pinned by Find_ReturnsFirstMatchingAnnotationInSearchDirection and GetNodes_ReturnsAnnotationsAtBothTiedBoundaries.
  • Word copy and clone: mapped ranges and payloads survive. Pinned by CopyAndClone_PreserveMappedAnnotationRangesAndPayloads.
  • Node removal: exact survivors, and tied children under shrinking multi-node owners. Pinned by RemoveNode_PreservesExactUnaffectedAnnotations, RemoveStartEndpoint_ShrinksMultiNodeOwnerAndRemovesEveryTie and RemoveEndEndpoint_ShrinksMultiNodeOwnerAndRemovesEveryTie.

Not here:

Verification (run on the earlier commit d98d12a5; the head differs only by src/AssemblyInfo.props from a rebase onto master): ./local_check.sh --agent-strict passed; focused AnnotationTieTests passed with 0 failed, 0 skipped. Not run: this branch combined with #557. Details in the accordions.

Next: approve, or say which Find caller needs a test for the new boundary.


Reading this a year from now -- start here

Addresses #506's directional tie-boundary mechanism (#506 is an open issue). Related: #500, #480, #554, #555. The reasoning and evidence scope live here because the PR description is the only record.

Evidence
  • ./local_check.sh --agent-strict: passed with one-node WSL settings and the existing native CI build output restored; all normal stages ran.
  • AnnotationTieTests: six test methods, each over 200 seeds, so 1,200 cases at the head (recounted). Earlier run on d98d12a5: 1,200 passed, 0 failed, 0 skipped, independently rerun.
  • Fix-removed check of the 1,200-case suite on a separate worktree, reverting only the lookup change: 977 failed, 223 passed. Restoring the fix returned the suite to green.
  • git diff --check origin/master...HEAD: passed. An independent source and test review passed.
  • 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.
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 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 and #555 remain separate findings.

Generated with OpenAI Codex.


This change is Reviewable

@codecov-commenter

codecov-commenter commented Oct 23, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 74.62%. Comparing base (c245476) to head (731385e).
⚠️ Report is 13 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #556      +/-   ##
==========================================
+ Coverage   74.57%   74.62%   +0.05%     
==========================================
  Files         453      453              
  Lines       38342    38353      +11     
  Branches     5284     5286       +2     
==========================================
+ Hits        28593    28622      +29     
+ Misses       8583     8568      -15     
+ Partials     1166     1163       -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.

@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-506-directional-boundaries branch from d98d12a to 579b2ef 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.
@johnml1135
johnml1135 requested a review from jtmaxwell3 October 27, 2026 12:11
@johnml1135
johnml1135 force-pushed the fix-506-directional-boundaries branch from 579b2ef to 41f319f Compare October 29, 2026 18:46
johnml1135 added a commit that referenced this pull request Nov 1, 2026
… reflect the current engine

A fresh sweep no longer reproduces the stale pre-#556/#500 baselines; five
truncate-morphotactic rows move from Evidenced to Unobservable and the pinned
counts follow (evidenced 237, unobservable 285).

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
johnml1135 added a commit to sillsdev/PanGloss that referenced this pull request Nov 5, 2026
…the #18 waivers are gone (#184)

* Align truncation morph attribution with corrected C# semantics

Replay copied annotation transfers before the zero-width last-node claim.
Preserve current node ownership, genuine descendant grouping and repeated
allomorph suppression using existing flat morph records and shared decoded
ranges. Keep compound descendant transfer non-recursive, matching C#.

Reuse the #480 identity, subsumption and copy-U fixtures with complete
occurrence and application histories. Add the root-boundary regression and
stage the existing #553 partial-root corpus with separate prune counters.

References: sillsdev/machine#500, sillsdev/machine#506,
sillsdev/machine#556, sillsdev/machine#480, sillsdev/machine#553.

* test: register new parser modules in harness inventory

* Match the pinned oracle on truncation identities and remove obsolete waivers.

Drop graduated fixture copies and remove the five-word and shell-runner
exceptions. Ratchet measured faithfulness failures from 24 to 11 and expose
the corrected ground truth's tuned-surface proposal gap in the scoreboard.

Co-Authored-By: Codex GPT-6.1 Sol <noreply@openai.com>

* Restore pruning coverage and document truncation proposal identities.

Count both template skip mechanisms in the clitic-state guard. Pin a
Floating annotation's later anchor-copy transition with real synthesis.
Explain the HC-Rust dependency of scoreboard ground truth and composite
FST proposal tags using before/after gau identities.

Co-Authored-By: Codex GPT-6.1 Sol <noreply@openai.com>

---------

Co-authored-by: Codex GPT-6.1 Sol <noreply@openai.com>
Check the starting node before descending from its skip-list predecessor, which can expose an earlier tie or miss result.
using NUnit.Framework;
using SIL.Machine.DataStructures;

namespace SIL.Machine.Tests.DataStructures;

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: F2. This namespace breaks the test project convention: SIL.Machine.Tests.DataStructures is the only SIL.Machine.Tests.* namespace under tests/. Every other file uses the namespace of the code under test, as AnnotationTieTests in this PR does with SIL.Machine.Annotations. Use namespace SIL.Machine.DataStructures;.

@claude

claude Bot commented Nov 7, 2026

Copy link
Copy Markdown
Contributor
  1. Verdict: approve. History was rewritten since d98d12a5, so this round re-read the whole PR. The new Find(start, example, dir) wrapper keeps every hit and miss at or after start, and the shared lookup still goes down to level 0, so it returns the first tie in each direction.
  2. Most important: F2 (Low), tests/SIL.Machine.Tests/DataStructures/BidirListTests.cs:5. The test namespace breaks the project convention. Nothing affects behavior.
  3. Counts: Critical 0, Important 0, Low 1 new; F1 addressed.
  4. Ran at the merge checkout: focused AnnotationTieTests|BidirListTests|AnnotationTests|SkipList|ShapeTests gave 1,211 passed, 0 failed. The full HermitCrab test project gave 176 passed, 0 failed. gh pr checks 556: Build on ubuntu-22.04 passes; Build on windows-latest and claude-review are pending.
  5. Not verified: I reasoned that Find_WithStart_DoesNotReturnEarlierTiedNode fails without the wrapper but did not run it that way. I did not run the HermitCrab conformance fixtures named in the PR body.

Public API / TFM / package / machine.py parity: the signatures did not change, but BidirList.Find and the IBidirList.Find(start, ...) contract now behave differently. With ties, they return the first match in the search direction. start is inclusive, and on a miss the result is never before start. OrderedBidirList already behaved that way. BidirList has no sillsdev/machine.py counterpart.

Findings: F1 addressed (731385e), F2 new.

Reviewed at 731385e

This branch has not been deployed

No deployments
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