Add a grammar health checker for two unenforced preconditions - #475
Open
johnml1135 wants to merge 1 commit into
Open
Add a grammar health checker for two unenforced preconditions#475johnml1135 wants to merge 1 commit into
johnml1135 wants to merge 1 commit into
Conversation
johnml1135
force-pushed
the
feature/grammar-health-checker
branch
from
August 20, 2026 09:32
b12553f to
0e80833
Compare
johnml1135
changed the title
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #475 +/- ##
==========================================
- Coverage 73.53% 73.48% -0.05%
==========================================
Files 449 451 +2
Lines 37633 37830 +197
Branches 5174 5202 +28
==========================================
+ Hits 27673 27799 +126
- Misses 8824 8888 +64
- Partials 1136 1143 +7 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
johnml1135
added a commit
that referenced
this pull request
Aug 20, 2026
GrammarHealthChecker and GrammarHealthFinding are a diagnostic feature, not part of the conformance suite: nothing in conformance/ or in the Conformance project calls them, and their only caller here was their own test file. They are wanted in FieldWorks on a schedule of their own, so they ship in #475 instead, which now carries the newer copies this branch had developed. Two tests go with them that cannot follow them there. Both load real conformance fixtures, and one uses the Conformance project's Fixture.DiscoverAll, so neither compiles on a branch that has no fixture tree. They belong here rather than in #475, and should come back once that lands; until then they are recoverable from 9dfb2f6. What remains of this branch's engine footprint is Trace.FailureAllomorph, its assignment in TraceManager from an argument that method already received and discarded, and the inert SemanticBranch capture point.
15 tasks
johnml1135
added a commit
that referenced
this pull request
Sep 1, 2026
Fixtures with per-fixture hashes, a published manifest, and a documented contract for running an external engine against them. This is what a consumer receives: the fixtures, the manifest, and PROTOCOL.md, and nothing else in this branch. Also a census of the compilations the pinned compiler produced, a denominator for rule interactions, and per-phase parse observation. The interaction ledger is stated as a corpus statistic rather than a bound. It counts what these grammars happen to contain, which is not a claim about what a grammar could contain. Derive coverage denominators from the DTD and the engine A denominator read off the corpus measures the corpus. So the interface inventory comes from the DTD, and RequiredToLoad splits into RequiredByDtd and RequiredByLoader, because "the DTD demands it" and "the loader demands it" are different facts about a grammar. The surfaces no consumer reads are named rather than quietly counted, and the host configuration the fixtures assume becomes part of the contract. Real grammars supply the SHAPE of a feature and never its scale. Establish presence-is-not-coverage and the layers enforcing it A construct can be present, referenced correctly, load cleanly, and change no parse. Only severance that changes a parse counts as evidence. Adds the interaction-chain layer, per-grammar witness traceability that holds presence and witness apart, exercises: corroboration, and fold-in candidates. The witness ledger records every fixture that witnesses a surface, not just the first. The earlier strictly-better tie-break silently preferred a fabricated edge case over a language grammar on every tie, which hid how much coverage the real grammars already carried. Add the authoring harness The doer skill, the review skill, the measurement, and the revision loop, with the suite documentation shipped alongside the fixtures. Measurement and revision stay separate deliberately. Doing both in one pass makes a change impossible to attribute to either. Make the mutant sweep reproducible, and add conformance CI Morpher.Synthesize sizes its Parallel.ForEach off ProcessorCount, so a mutant could return a different verdict per run. Pinning DOTNET_PROCESSOR_COUNT=1 in the child makes two independent sweeps byte-identical, which is the property the drift gates already assumed and did not have. Test child processes are bounded to the test's own lifetime. Adds the conformance workflow, the fixture index, and the RuleName/FailureAllomorph threading that lets an infinite-loop crash name the rule responsible. The expensive sweep runs on a schedule, not on every pull request. The dead-rule step is advisory: --coverage-report reports dead rules and still exits 0, and making that a hard failure while 14 exist would fail CI on its first run. Key coverage to the engine's own gates and three constraint layers An obligation is worth covering only where the engine can act on it, the DTD can declare it, and a real FieldWorks project can produce it. Failing any one of the three is an exclusion, not a gap. Layer one is FailureReason, HermitCrab's own 23-member enumeration of the decisions it makes when declining to apply something. Witness status comes from a real traced run, and six of those gates are reachable through no XML attribute at all, so an attribute-keyed denominator cannot see them. Layer three is HCLoader, the component that turns a LibLCM model into an HC grammar and therefore integrates both the model and what is reachable from it. Of 83 subjects, 61 are producible and 22 are not. Keying MC/DC to the gates also fixes a miscount: the attribute-keyed ledger emits four arms per chain but can certify at most one, because it reads the arm off the reader attribute's spelling. The older ledger is kept as a cross-check on writer-reader chains, which the gate ledger does not measure, and the docs say which number is the claim. Ledger assertions read the checked-in file; only a freshness check recomputes, behind [Explicit]. An earlier version reswept four times in the default suite. Triage every obligation, and record what authors keep relearning Fifty-one unmet obligations, each assigned a bucket from the blocker its own ledger records, with the construct stated in plain language so a reader can judge whether covering it is worth anything. The split that matters: thirteen are a HARNESS gap, not a coverage gap, and no amount of authoring touches them. Seven control arms already have their word in the corpus and fail only because GrammarRuleIndex cannot resolve Allomorph, MorphologicalInput, AffixTemplate or PhonologicalSubrule to a fired-rule id. Six more are reachable only through element content, for which no severance primitive exists. Also writes down the engine and tooling facts each author had been rediscovering at real cost: severance is fixture-wide and can unblock as easily as block; a DTD #REQUIRED attribute can never be severed, which makes both co-occurrence gates unwitnessable by any word; outputPartOfSpeech overrides rather than sets; stem names compare by object identity; and a Timeout is a statement about the machine, not the grammar. check-obligation-feasibility.ps1 answers the three mechanical questions before a budget is committed, and claims only that no known mechanism forbids a witness. Witness lexically-conditioned phonology and affix-conferred gates No grammar in the corpus declared an MPR gate on a phonological subrule, so exception features and minor rules -- one of the most ordinary things a real grammar does -- had no coverage at all. HCLoader emits both at lines 2057 and 2058, from FLEx's own Required and Excluded rule features. Satisfied cells go from four to nine, and the recurring shape is worth knowing: a feature the ROOT presets cannot witness a required-gate chain, because severing it can only turn a passing word into a failing one. What works is a feature an affix confers or destroys before the reader checks it. Two items are foreclosed by the engine rather than unattempted. Stem names compare by bare object identity, and a root-preset feature meets a plain set-membership test, so in both cases removing the payload can only break a match and never repair one. Raises the mutant budget from 45s to 180s and drops the confirmation retry. A Timeout roved to a new fixture that proved Unobservable in isolation while needing about 128s uncontended, and genuine non-termination has never once been observed here. Record what this measures, and where it deliberately stops Every figure was read from a checked-in ledger at the time of writing. Interface edges: 60 declared, 44 present, 19 witnessed. Obligation cells: 346 enumerated, 18 worth covering, 8 defensible. Gate arms: 46, of which 42 are worth covering and 14 are evidenced, giving 5 of 23 gates both MC/DC arms. Those are findings, not a backlog, and the docs now say so. The apparatus is frozen here on purpose: it established what the fixtures witness and produced the map of what remains, and its marginal return then fell to roughly nothing. obligation-triage.tsv classifies every unmet obligation by the blocker its own ledger records, and most of the remainder is impossible or strained rather than merely undone. One row changed what it MEASURES, and the docs say so rather than swapping a number quietly. The interaction-chain row reported chains whose writer and reader are each evidenced somewhere, which two separate words satisfy; it now reports chains with a same-word paired witness. Resolving a control arm to its nearest rule ancestor makes three more attributable. Four others never will be: Allomorph is always a child of LexicalEntry and never of a rule, and the DTD gives AffixTemplate no id at all, so neither has any identity to attribute a control to. An adversarial review then broke one claim. The affix-conferred blocking cell cited HCLoader.cs:1717 as its read, but that line populates ExcludedMprFeatures only from slot.ReferringObjects.OfType<ILexEntryInflType>(), FieldWorks' own irregular-form blocking. The witness is behaviourally genuine and the fixture stays as an engine test, but the claim is withdrawn. The general fact matters more than the cell. Producibility is keyed per attribute and does not compose along a chain, and no gate in this repository can catch that, because it requires reading another one. The branch's own working documents come out with it. Twenty plans, censuses and rationale files are deleted and their reasoning moves to the pull request, which is the one place a reader always looks and which survives being read a year later. Two were genuinely durable and move into conformance/docs instead: the HermitCrab XML semantic catalogue, and the pipeline design. Nothing shipped now points at a document a consumer does not receive. That eviction was not cosmetic. docs/conformance-migration-ledger.md was still load-bearing in code: it sat in ConformanceManifestGenerator's AdditionalSourceFiles, which is hashed into every manifest's SourceHash, so deleting the file would have permanently recorded an absent contributor to that hash, and a parametrized test would have thrown on a missing file. Make CI able to build, and stop gating on a deliberate backlog Two independent reasons every job failed. global.json pinned SDK 10.0.303 with rollForward disabled. Hosted runners carry 10.0.110, 10.0.204, 10.0.302 and 10.0.400 -- never 303 -- so the pin was unsatisfiable by construction and all three jobs died on their first dotnet invocation. That version was one developer's local SDK. The pin now states a floor with latestFeature instead of an exact patch. The part that matters is unchanged: the census still analyses this repository with the same Roslyn that builds it, taken from $(MSBuildToolsPath)/Roslyn/bincore rather than a NuGet package. An exact patch bought nothing beyond that, because SdkVersion feeds a graph hash that is recomputed every run and compared against nothing checked in, so any .NET 10 SDK is self-consistent. Pinning one that is not installable everywhere only made the repository unbuildable. Second, ci.yml gated on the semantic-coverage CLI, which exits non-zero whenever the catalog is incomplete. The catalog is INTENTIONALLY an incomplete proposal backlog, and unclassified-mapping is its expected state: the CLI reported zero new gaps, zero stale lines and zero unbacked quotients while still failing. The test named TheCheckedInCatalogMapsEveryRealSurfaceExactlyOnce already runs that identical audit under dotnet test, and asserts both that the catalog is incomplete and that unclassified-mapping is the only diagnostic class allowed, which is a stronger gate than an exit code and can express the intent. The redundant step is gone and the comment names its real gate. The CLI's exit code conflating "not complete" with "regressed" is left as it is, and recorded in the pull request rather than changed under time pressure. Apply CSharpier, and brace the bodies it wraps CI runs dotnet csharpier check, and 200 files in this branch had never been run through the formatter. Formatting them exposed a genuine disagreement between two pre-existing tools: .editorconfig sets csharp_prefer_braces to when_multiline, so a brace-less if is legal with a one-line body and an IDE0011 error once the body wraps. CSharpier wrapped 83 such bodies, all of them long argument lists, which turned legal code into build errors under TreatWarningsAsErrors. Master never hits this because its brace-less bodies are short enough not to wrap. Adding braces satisfies both tools, so neither .editorconfig nor the CSharpier configuration is touched -- the repository's style is the repository's own call. Convergence took two passes. The second was not redundant: while the Conformance project failed to build, the Tests project could not compile at all, so six of the 83 sites stayed invisible until the first pass fixed its project reference. Verified that no logic moved, rather than assuming it: comparing token streams with whitespace and braces stripped, every difference across all 108 changed files is either a trailing comma CSharpier adds to a multi-line initialiser or a using-directive reorder. Both are inert in C#. Handle both path shapes on either platform, and name mismatched words The compilation-graph hash is relocation-invariant so a graph captured on one machine can be verified on another, which means the path vocabulary has to understand a Windows-shaped path and a Unix-shaped one regardless of the host. LogicalPathTokens carries its own IsAbsolute and NormalizeAbsolute for exactly that reason. Three places reached for the platform's own path handling instead, and all three failed only on Linux. A csc switch is syntactically an absolute path on Unix, so switch shape must be decided before any path handling: /noconfig carries no value, /doc:/tmp/x.xml carries one, and /home/me/src/X.cs is a real path. The discriminator is what follows the switch name -- nothing, a colon, or a further separator. The ancestor .editorconfig probe used Path.GetFileName, whose notion of a separator is the host's, so a Windows-shaped path there returned the whole string as its filename on Linux and the probe silently declined a file it should have admitted. It now segments with this class's own rules. Finally, a mismatch reason gave only a count. A count cannot be diagnosed from a CI log on a machine you cannot reach, so it now names the words and their expected and actual analyses, truncated and capped at five. Read census specimens from code this branch owns Master replaced the SINGLE_THREADED compile symbol with a runtime MaxDegreeOfParallelism, so the engine no longer contains any code that exists in only some configurations. Two SemanticCoverage tests were reading engine code as their live specimen and lost it. Both point the census at real repository source rather than at a string literal, which is the whole reason they exist -- the synthetic-source tests beside each already cover the same logic on fixtures. So they still read real source, but source whose shape this branch is answerable for. The local-function keying test reads FailureRuleAttributor's Walk in place of Morpher's deleted GenerateSynthesis. The configuration-only census test reads Morpher's surviving #if OUTPUT_ANALYSES region, scoped to the type rather than to one method signature, so that an upstream rename cannot present itself here as a census defect. The second test's claim is narrower than it was, and the name now says so: it witnessed a method that existed only under a symbol, and no such method remains anywhere in the engine. What it still witnesses is a configuration-gated region censused under exactly the configurations that contain it. Its expected configuration string was confirmed by running the test, not derived. Stop naming the rule that hit the epenthesis loop cap Threading rule.Name down to the 256-node throw site cost a required parameter on two public constructors, SynthesisRewriteRuleSpec and EpenthesisSynthesisRewriteSubruleSpec. Both ship in the HermitCrab NuGet package, so that is a source and binary break for anyone constructing them. Nothing read what it bought. InfiniteLoopException.RuleName has no reader anywhere in the repository: the conformance runner mentions the exception only in a comment, the fixture that provokes it asserts expect_crash and not a message, and the engine test asserts only Throws.TypeOf<InfiniteLoopException>(). A capability with no consumer does not justify breaking a published constructor, so the four files go back to what master has. The 256-node cap itself is untouched, and edge-cases/simultaneous- epenthesis-cascade still pins it. Split the grammar health checker out to its own change GrammarHealthChecker and GrammarHealthFinding are a diagnostic feature, not part of the conformance suite: nothing in conformance/ or in the Conformance project calls them, and their only caller here was their own test file. They are wanted in FieldWorks on a schedule of their own, so they ship in #475 instead, which now carries the newer copies this branch had developed. Two tests go with them that cannot follow them there. Both load real conformance fixtures, and one uses the Conformance project's Fixture.DiscoverAll, so neither compiles on a branch that has no fixture tree. They belong here rather than in #475, and should come back once that lands; until then they are recoverable from 9dfb2f6. What remains of this branch's engine footprint is Trace.FailureAllomorph, its assignment in TraceManager from an argument that method already received and discarded, and the inert SemanticBranch capture point. Remove the semantic branch marker channel SemanticBranch was introduced by this branch's first commit as a way for engine code to declare a semantic path that the census would then count. It was never wired up. No engine or tool code has called SemanticBranch.Hit in any commit here, no ledger, catalog or coverage file has ever carried a branch: id or a branch-marker surface, and no document names it as a mechanism anyone intends to use. Every coverage number this branch reports comes from severance sweeps and trace evidence instead. So it was a public type in the shipped netstandard2.0 package with no consumer, and the census carried a fail-closed resolution path that could never fire. It could not move to the Conformance project either: that project references the engine and not the reverse, so a marker channel living there could never be called from the engine that is its only intended caller. Removed with it: CollectMarkers and its two resolution helpers in CSharpInventoryReader, the branch-marker catalog family, and three tests that existed only to exercise markers. Three further tests used a marker as an incidental probe and keep their subject. Two lose nothing -- the if/else-if arms keep an inert body, and the reachable-versus-dead claim was already carried by the xml-read assertions beside it. The third is narrower than it was and its name now says so: it asserted that an unresolved marker in dead code does not trip the fail-closed check, and with no such check left it now asserts only that an unresolved call reached from dead code raises no diagnostic. What remains of this branch's engine footprint is Trace.FailureAllomorph and its assignment in TraceManager: two files, thirteen lines, no new public type, and both read by FailureRuleAttributor.
HermitCrab imposes two requirements on a grammar that nothing reports today, so a grammar author only learns of a violation as a parse that silently returns nothing. Every segment used must be declared in a CharacterDefinitionTable: an undeclared segment makes the parser refuse every word containing it. And each segment needs a distinct phonological feature bundle within its table: when two share one, the parser cannot reliably determine which morphemes are involved. GrammarHealthChecker.Check(Language) reports both as findings with a severity, a stable code and the offending declarations named. It is diagnostic only -- a grammar the engine would load still loads, and nothing throws. Lives in the netstandard2.0 engine library rather than a tool, so FieldWorks and any other host can call it directly on a loaded Language. The duplicate-bundle check is skipped for a grammar that declares no PhonologicalFeatureSystem at all. Such a grammar distinguishes segments by their representation alone, so every bundle is the same empty struct by construction and reporting it would be a false positive on a correct grammar.
johnml1135
force-pushed
the
feature/grammar-health-checker
branch
from
September 1, 2026 18:45
0e80833 to
9818f12
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
HermitCrab imposes two requirements on a grammar that nothing currently reports. A grammar author only discovers a violation as a parse that silently returns nothing, which is a hard failure to diagnose from the outside.
Every segment used must be declared in a
CharacterDefinitionTable. An undeclared segment makes the parser refuse every word containing it — total, and silent-looking.Each segment needs a distinct phonological feature bundle within its table. When two segments share one, the parser cannot reliably determine which morphemes are involved.
GrammarHealthChecker.Check(Language)reports both as findings carrying a severity, a stable code, and the offending declarations named so a host can navigate to them.Design notes
Diagnostic only. It never throws and never changes parse behaviour — a grammar the engine would load still loads. It reports; the host decides what to do.
In the
netstandard2.0engine library, not a tool. So it ships in theSIL.Machine.Morphology.HermitCrabNuGet package and any host — FieldWorks, or anyone testing their own grammar — can call it directly on a loadedLanguage. Verified by packing:lib/netstandard2.0/SIL.Machine.Morphology.HermitCrab.dll.The duplicate-bundle check is skipped when a grammar declares no
PhonologicalFeatureSystemat all. Such a grammar distinguishes segments by their representation alone, so every bundle is the same empty struct by construction. Reporting that would be a false positive on a correct grammar, and grammars of exactly this shape exist.Tests
Five tests covering both checks, the clean case, and the feature-less-grammar case. They build the object model directly rather than loading XML, so they have no external fixture dependency. Full HermitCrab suite passes (98/98), zero-warning build, CSharpier and
gitlintclean.Provenance
Rebased onto current
master(it was based on 3.9.2). The three files here are also present on the HermitCrab conformance branch (#480), where they were developed further; this branch now carries that newer version, and #480 drops them so the checker lands on its own schedule. Nothing in the conformance suite calls the checker, so the split is clean. Two additional tests live on that branch which run the checker across the real conformance fixtures — they need the fixture tree and the Conformance project, so they stay there rather than move here.🤖 Generated with Claude Code
This change is