Skip to content

Trace HermitCrab failure reasons and template slot outcomes - #534

Open
johnml1135 wants to merge 2 commits into
masterfrom
fix/hc-trace-reasons
Open

johnml1135 wants to merge 2 commits into
masterfrom
fix/hc-trace-reasons

Conversation

@johnml1135

@johnml1135 johnml1135 commented Oct 1, 2026 •

Copy link
Copy Markdown
Collaborator

Retargeted to master. The conformance suite (#480) is deferred until after this release, so this PR no longer stacks on it. Its two suite-side commits (FailureRuleAttributor crediting through Trace.Allomorph, and the FieldWorks-producibility rows for the four new failure reasons) are held back and will be added to #480 after this merges.

HermitCrab computes several facts that Try A Word cannot display because they do not reach the trace or are discarded by the trace manager. This change passes the available facts through an optional IDetailedTraceManager capability and retains them in Machine's TraceManager, without rerunning linguistic checks.

  • Analysis affix rejection now carries the first failed gate and its operand: maximum applications, output/required syntactic features, realizational features, or no pattern match.
  • Template slots report applied, optional skipped, and required unfilled branches, with the template, slot index, input, output, and direction. Optional slots can report both an applied and a skipped branch. Sequential and parallel analysis and synthesis are covered.
  • Partial parses retain the completion cause: realizational feature mismatch, pending morphological rules (including the next rule), a non-final template applied last, or applicable templates not applied.
  • Completed lexical lookups report yielded Word-candidate counts and whether the lookup was a guess. Zero candidates is explicit; an interrupted lookup remains unknown. Synthesis nodes stay attached to the corresponding lookup when inputs interleave.
  • Compounding analysis reports the computed limit, feature, non-head lookup, productivity, and pattern gates to IDetailedTraceManager, with the subrule index and available operand. ITraceManager-only consumers receive exactly the callback they did before: the productivity-restriction rejection, with subrule index -1. FieldWorks writes a trace node for every CompoundingRuleNotUnapplied call, so sending them the new gates would add reasonless failures to Try A Word.
  • Blocking already passes the replacement HermitCrab Word through the existing callback. Machine's trace now retains the blocking lexical entry from that Word's root allomorph.
  • Environment failure already passes the allomorph and all failed alternatives. Machine's trace now retains both, along with existing failure operands elsewhere.
  • Phonological non-unapplication remains reasonless because that analysis path does not compute a reason. A regression test ensures no reason is invented.

ITraceManager has no new members. Existing implementations keep the original callbacks; detailed consumers opt into IDetailedTraceManager. Enum additions preserve existing values, and morphological analyses remain unchanged in tracing-on/off and sequential/parallel tests.

Validation: ./local_check.sh --agent-strict passed on Windows (full solution build and every test project). In WSL, all 151 tests in SIL.Machine.Morphology.HermitCrab.Tests passed in a fresh WSL Release run, including 45 synthetic TraceReasonTests covering the facts above, legacy callbacks, interrupted/interleaved lookups, and analysis equivalence. Strict comment hygiene passed and CSharpier checked 724 files. Both ./local_check.sh and ./local_check.sh --agent-strict exited at the solution build because WSL lacks cmake for SentencePiece; the standard rerun also reported MSBuild child-node shutdown errors. The full solution test suite has not passed on this tree. These local checks do not collect CI-equivalent coverage. After 371ba39, on Windows: Release build clean; dotnet test passed in every test project that ran (880 passed and 3 skipped, 4, 155, 93); strict comment hygiene clean; CSharpier 1.2.6 check clean, run from a scratch install because the manifest's tool would not resolve in this environment, so local_check.sh itself stopped at that step.

FieldWorks follow-up (outside this PR):

  • Implement IDetailedTraceManager in FwXmlTraceManager: add the reason-bearing MorphologicalRuleNotUnapplied overload, TemplateSlotProcessed, and LexicalLookupCompleted. Serialize the new operands, branch outcomes, and completed counts. Keep lookup/synthesis associations correct in LexicalLookup and SynthesizeWord when other trace nodes are inserted.
  • Implement Blocked to serialize the replacement Word's root entry. Extend CompoundingRuleNotUnapplied to handle each reason rather than only productivity restrictions.
  • In Failed, serialize PartialParseFailure.Cause and its pending rule; for Environments, serialize each failed alternative alongside the supplied allomorph instead of passing the collection directly to XElement. Existing NonFinalTemplateAppliedLast and ApplicableTemplatesNotApplied callbacks already distinguish those two template completion failures.
  • In MorphologicalRuleNotApplied, fix the prefixEnv/suffixEnv branches at FwXmlTraceManager.cs:299-302: they currently write env instead of their respective values.
  • Update FormatHCTrace.xsl's ShowAnyFailure (environment branch around line 597 and partial-parse branch around line 661) and ShowCompoundingRuleFailure (around line 1149) to display the detailed facts. Extend ShowTracePath (around line 1237), which selects synthesis children of LexLookupTrace but excludes the lookup node itself, to include empty lookup, slot, and blocking events; adjust DetermineIfMoreToShow for the added nodes. Display unknown phonological causes as unknown.

🤖 Generated with Claude Code


This change is Reviewable

@codecov-commenter

codecov-commenter commented Oct 1, 2026 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 91.93548% with 20 lines in your changes missing coverage. Please review.
✅ Project coverage is 74.98%. Comparing base (a2f80e2) to head (54393fd).

Files with missing lines Patch % Lines
...rab/MorphologicalRules/AnalysisAffixProcessRule.cs 75.00% 7 Missing and 1 partial ⚠️
...Crab/MorphologicalRules/AnalysisCompoundingRule.cs 85.36% 6 Missing ⚠️
.../SIL.Machine.Morphology.HermitCrab/TraceManager.cs 91.66% 3 Missing and 2 partials ⚠️
src/SIL.Machine.Morphology.HermitCrab/Word.cs 0.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #534      +/-   ##
==========================================
+ Coverage   74.36%   74.98%   +0.62%     
==========================================
  Files         456      458       +2     
  Lines       38325    38539     +214     
  Branches     5263     5297      +34     
==========================================
+ Hits        28499    28900     +401     
+ Misses       8669     8511     -158     
+ Partials     1157     1128      -29     

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

@jtmaxwell3

Copy link
Copy Markdown
Collaborator

@johnml1135 Did you ask Beth about this? I'm worried that this will make it harder to find a needle in a haystack because the haystack will be so much bigger. It may also slow Try A Word down.

@johnml1135

Copy link
Copy Markdown
Collaborator Author

@jtmaxwell3 I measured both concerns, on a real Indonesian grammar exported from FLEx (70 wordforms, two compounding rules), comparing the merge-base with this branch.

Speed with tracing off (normal parsing): no measurable change. All 70 words give identical parses, and total time is 4.4 s on both, within 2%. Every new callback sits behind IsTracing; the only always-on addition is an integer counter per lexical-lookup candidate.

Trace size in Try A Word: you were right, and it was worse than the branch description said. FwXmlTraceManager implements only ITraceManager, and this branch was sending it CompoundingRuleNotUnapplied for every new compounding gate (stem limit, feature mismatches, non-head lookup, pattern). FieldWorks writes a node for each of those calls, and none of them would say why the rule failed. Over 40 of the words, that was 0 → 4,412 extra compounding-failure nodes in Try A Word, from the Machine bump alone. The affix-rule fallbacks don't reach the trace, because FwXmlTraceManager.MorphologicalRuleNotUnapplied is empty.

Fixed in 371ba39. ITraceManager-only consumers now get exactly the compounding callback they got before (the productivity restriction, with subrule index -1), and the new gates reach only IDetailedTraceManager. Re-measured on the same 40 words, every callback FieldWorks records now matches the merge-base, and a new test pins this. So updating Machine leaves Try A Word as it is. Whether to show any of the new facts, and how to keep the trace readable, is a decision for the FieldWorks follow-up.

I haven't timed parsing with tracing on beyond those 40 words.

if (_morpher.TraceManager.IsTracing && !unapplied)
_morpher.TraceManager.MorphologicalRuleNotUnapplied(_rule, i, input);
if (!matched)
TraceFailure(i, input, FailureReason.Pattern, null);

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.

Minor: F1. Plain ITraceManager implementations no longer receive MorphologicalRuleNotUnapplied(rule, i, input) for a compounding subrule that fails. Base called it whenever srOutput produced nothing; now every subrule failure goes through CompoundingRuleRejected, which passes only NonHeadProdRestrictMprFeatures to plain managers. This contradicts "receive exactly the callback they did before", and LegacyTraceManager_ReceivesOnlyTheExistingCompoundingRejection checks only for an absence. Send the old call to non-detailed managers at the subrule loop exit.

input.GetUnapplicationCount(_rule) >= _rule.MaxApplicationCount
|| !_outputFS.IsUnifiable(input.SyntacticFeatureStruct)
)
if (input.GetUnapplicationCount(_rule) >= _rule.MaxApplicationCount)

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.

Minor: F2. Plain managers now get calls they never got before: MorphologicalRuleNotUnapplied(rule, -1, input) from the MaxApplicationCount and output-FS gates here, and from the realizational gate at AnalysisRealizationalAffixProcessRule.cs:53. Base returned silently, and -1 was never a subrule index on this callback, so a consumer that indexes Subrules[subruleIndex] throws. Unverified against FieldWorks, whose handler the description says is empty. Gate these the way CompoundingRuleRejected does.

_rule,
i,
outWord,
FailureReason.RequiredSyntacticFeatureStruct,

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: F3. The legacy callback changes shape here. When every match fails Required, base made one call with input; now a plain manager gets one call per rejected outWord, each carrying the output with Out stripped, and none with input.

word,
FailureReason.PartialParse,
null,
new PartialParseFailure(

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: F4. Failed(..., PartialParse, null, failureObj) now passes a PartialParseFailure to plain managers, where base passed null; SynthesisStratumRule.cs:74 does the same. A legacy manager that serializes failureObj gets an unknown type. Unverified: I did not read FwXmlTraceManager.Failed.

TraceFailure(
i,
tempInput,
FailureReason.NonHeadProdRestrictMprFeatures,

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. The public TraceManager now builds a different tree for compounding failures: CompoundingRuleAnalysis nodes with the subrule index replace MorphologicalRuleAnalysis nodes, and the productivity rejection carries i instead of -1. A consumer that filters on MorphologicalRuleAnalysis loses compounding rules. If this is intended, the description should say so.

case TraceType.StratumAnalysisOutput:
case TraceType.TemplateAnalysisInput:
case TraceType.TemplateAnalysisOutput:
case TraceType.TemplateSlotAnalysis:

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: F6. The HC tool now prints slot traces without SlotIndex or SlotOutcome: PrintTrace writes only source, input, output, and reason. Applied, skipped, and unfilled lines for the same template look alike, so the tool trace gets longer without telling the reader more.

}

/// <summary>
/// ITraceManager implementations previously received only the productivity-restriction rejection,

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: F7. This comment narrates history ("previously received"), which .claude/skills/code-comments/SKILL.md:24 forbids; the strict scan has no pattern for it. State the current contract: plain managers receive only the productivity-restriction rejection, with subrule index -1.

@claude

This comment has been minimized.

johnml1135 added a commit that referenced this pull request Oct 14, 2026
The suite added Trace.FailureAllomorph so FailureRuleAttributor could credit
an affix rule whose allomorph failed an environment or co-occurrence check.
That is an engine API change riding in a test-suite PR, and #534 adds the
same fact as Trace.Allomorph. Drop it here; the attribution returns with
#534. No generated ledger changes: coverage.csv and rules.csv regenerate
byte-identical.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@johnml1135
johnml1135 force-pushed the fix/hc-trace-reasons branch from 371ba39 to 9124922 Compare October 14, 2026 20:41
@johnml1135
johnml1135 changed the base branch from master to integrate-conformance-framework October 14, 2026 20:41
Comment thread src/SIL.Machine.Morphology.HermitCrab.Conformance/FailureRuleAttributor.cs Outdated
NonPartialRuleProhibitedAfterFinalTemplate,
NonPartialRuleRequiredAfterNonFinalTemplate,
MaxApplicationCount,
OutputSyntacticFeatureStruct,

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: F9. The four new FailureReason values have no row in conformance/engine-gate-inventory.tsv, which still lists 23 gates. EngineGateInventoryLedger.Compute emits one row per member. The [Explicit] CheckedInEngineGateInventoryLedgerIsUpToDate fails at this head (expected length 16331, got 28388). Some of that drift is older, from a stale line number in the untouched SynthesisAffixProcessRule.cs. gate-obligations.tsv and the "23" counts in both ledger doc comments are stale in the same way.

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.

F9 still applies at 54393fd: the ledger now comes from master, and this PR still makes it stale on merge. At merge commit 9fd7e57, the [Explicit] CheckedInEngineGateInventoryLedgerIsUpToDate fails: expected length 16780, got 27164. The first difference is this PR's new AnalysisCompoundingRule.cs raise site. Regenerate with hc-conformance --write-engine-gate-inventory --repository-root . after rebasing.

@claude

This comment has been minimized.

@claude

This comment has been minimized.

johnml1135 added a commit that referenced this pull request Oct 15, 2026
The suite added Trace.FailureAllomorph so FailureRuleAttributor could credit
an affix rule whose allomorph failed an environment or co-occurrence check.
That is an engine API change riding in a test-suite PR, and #534 adds the
same fact as Trace.Allomorph. Drop it here; the attribution returns with
#534. No generated ledger changes: coverage.csv and rules.csv regenerate
byte-identical.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@johnml1135
johnml1135 force-pushed the integrate-conformance-framework branch from 3a63132 to df2cf21 Compare October 15, 2026 21:21
johnml1135 and others added 2 commits October 16, 2026 09:16
Add optional detailed trace callbacks for analysis rejections, slot branches,
and completed lexical lookup counts. Retain failure operands, blocking entries,
and partial-parse causes in TraceManager without replaying linguistic checks.

Preserve the ITraceManager member contract and verify legacy callbacks,
parallel slot outcomes, interrupted lookups, and morphological analyses.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
ITraceManager implementations previously received CompoundingRuleNotUnapplied only for
the productivity-restriction rejection, with subrule index -1. This branch had sent them
every new compounding gate as well. FieldWorks writes a trace node for each call, so
Try A Word would have gained a reasonless failure node per rejected compound analysis.

They now receive exactly the original call. Detailed consumers still get every gate with
its subrule index.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
@johnml1135
johnml1135 force-pushed the fix/hc-trace-reasons branch from 928d7fa to 54393fd Compare October 16, 2026 13:19
@johnml1135
johnml1135 changed the base branch from integrate-conformance-framework to master October 16, 2026 13:19

internal int MorphologicalRuleApplicationCount => _mruleAppCount;

internal IMorphologicalRule NextMorphologicalRule => _mruleAppIndex < 0 ? null : _mruleApps[_mruleAppIndex];

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: F10. NextMorphologicalRule can be null while a rule remains: GenerateWords pushes (null, allo) for a compound non-head (Morpher.cs:340), and MorphologicalRuleUnapplied stores it in _mruleApps. Then PartialParseFailure(UnappliedMorphologicalRules, null) cannot be told apart from "no rule known". Unverified by a test; read from the code.

@claude

claude Bot commented Oct 16, 2026

Copy link
Copy Markdown
Contributor

Verdict: approve with fixes. This push rewrote the branch onto a2f80e2 (#529) and dropped the conformance commits, so only the two engine commits are left. The engine code behind F1-F7 is the same as last round. Three files have new blob SHAs (Morpher.cs, AnalysisAffixProcessRule.cs, SynthesisStratumRule.cs), but their PR diff hunks match what was reviewed before.

  1. Most important: F1 (AnalysisCompoundingRule.cs:189) still has no reply. Plain ITraceManager implementations lose MorphologicalRuleNotUnapplied for a failed compounding subrule. A fresh /code-review high found it again, and it also found F2 (AnalysisAffixProcessRule.cs:52) again, the same plain-manager regression that 54393fd fixed for compounding.
  2. Counts: Critical 0, Important 2 (F1, F2), Low 7 (F3-F7, F9, F10). F8 is addressed.
  3. Ran: /code-review high 534, which returned 9 items. Four repeat F1/F5, F2, F3 and F6. F10 is new. The other four are not posted: untraced template-level gates are unchanged code, the triplicated slot-tracing block is a cleanup, Trace field memory is speculative, and the CurrentTrace assignment is redundant. dotnet test tests/SIL.Machine.Morphology.HermitCrab.Tests -c Release filtered to TraceReasonTests and gate tests at merge commit 9fd7e57: 56 passed, 0 failed. The [Explicit] CheckedInEngineGateInventoryLedgerIsUpToDate fails at the merge (F9).
  4. Not verified: ./local_check.sh --agent-strict and gh pr checks did not run, because this session did not get permission. The green advisory hygiene check does not replace the strict scan. F2 and F4 are still unverified against FieldWorks. F10 was read from the code; no test was run for it.

Public API: unchanged since last round. The PR adds IDetailedTraceManager, PartialParseFailure, TemplateSlotOutcome, PartialParseCause, four FailureReason values, TraceType.TemplateSlotAnalysis/TemplateSlotSynthesis and new Trace properties. It also changes the compounding trace shape in TraceManager (F5). Target framework and package: none. sillsdev/machine.py parity: None verified.

Findings: F1 open, F2 open (unverified), F3 open, F4 open (unverified), F5 open, F6 open, F7 open, F8 addressed, F9 open, F10 new (unverified).

Reviewed at 54393fd

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