Harden FilterExpressionParser against robustness issues and add tests#26
Merged
Conversation
- Guard against uncatchable StackOverflowException on deeply nested parenthesized/prefix-unary input by pre-scanning the (non-recursive) token stream for nesting depth before invoking ANTLR's recursive-descent parse, rejecting input beyond 200 levels with a diagnostic. - Broaden Parse's catch clause to catch Exception generically (in addition to RecognitionException) so ANTLR-internal failures such as the ArgumentException thrown by Lexer.GetErrorDisplay for astral-plane Unicode input become a diagnostic instead of propagating uncaught. - Check the token stream is positioned at EOF after ownedExpression() returns, reporting an 'unexpected trailing content' diagnostic instead of silently accepting a valid expression prefix followed by garbage tokens. - Reject integer/real literals that parse to a non-finite double (overflow, e.g. 3.14e400) in TryBuildLiteral, reporting a 'numeric literal out of range' diagnostic instead of silently producing an Infinity/NaN-valued literal that cannot round-trip through ToString(). All four fixes verified against the exact reproduction inputs from .agent-logs/code-review-filter-phase1-2a-retroactive-a91f3c.md. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Covers the exact reproduction inputs from the retroactive Filtering code-review report: 5000-level nested parentheses, astral-plane Unicode characters (embedded and trailing), trailing garbage after a valid expression prefix, and a numeric literal that overflows double (3.14e400). Also adds companion 'still works' tests confirming the new guards do not reject ordinary, moderately-nested, or large-but-finite Phase 1 input. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Updates the Core.Filtering design doc, the FilterExpressionEvaluator unit design doc's Error Handling section, and the corresponding reqstream/ verification docs to describe the deep-nesting depth guard, the broadened ANTLR-internal-failure catch, the trailing-content EOF check, and the numeric-literal-overflow guard, plus the new regression tests that verify each of them. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…ackets A follow-up code review found ExceedsMaxNestingDepth only tracked LPAREN/RPAREN and prefix unary operators, missing the grammar's 'ownedExpression LBRACK sequenceExpressionList? RBRACK' indexing production, which recurses through ownedExpression once per nesting level exactly like parens. Nested bracket indexing (a[a[a[...0...]]]) at just 500 levels still caused the same uncatchable STATUS_STACK_OVERFLOW process crash the original fix was meant to eliminate. - Add LBRACK to the 'push a pending frame' cases and RBRACK to the 'pop' cases in ExceedsMaxNestingDepth, mirroring the existing LPAREN/RPAREN handling (bracket frames behave like paren frames, not unary frames, since they are unambiguously balanced by a matching close token). - Add regression tests: Parse_DeeplyNestedBracketIndexing_ReturnsDiagnosticInsteadOfCrashing (exact 500-level repro from the follow-up review) and Parse_ShallowBracketIndexing_ReturnsUnsupportedConstructNotDeepNestingDiagnostic (confirms no false-positive rejection for ordinary-depth bracket expressions). - Update the FilterExpressionEvaluator design/reqstream/verification docs to describe the corrected guard and the new tests. Re-verified via a throwaway console probe against the built assembly: n=500, 3000, 5000 bracket nesting now return a clean diagnostic instead of crashing, while n=1/5/20 (below the ceiling) still return the expected 'Unsupported filter construct' diagnostic for bracket indexing (outside the Phase 1 subset) with no false-positive 'too deeply nested' rejection. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…RBRACE)
A second follow-up review found ExceedsMaxNestingDepth still did not track
LBRACE/RBRACE, leaving the grammar's bodyExpression : LBRACE functionBodyPart
RBRACE production (reachable via ownedExpression DOT_QUESTION bodyExpression,
and directly from baseExpression) free to still crash the process via the
same uncatchable StackOverflowException failure mode at 500 levels of nesting
("a.?{" * 500 + "0" + "}" * 500), just like the earlier LBRACK/RBRACK gap.
Independently re-derived the complete set of balanced-delimiter pairs
reachable from ownedExpression directly from SysMLv2Parser.g4 (rather than
trusting the report at face value): LPAREN/RPAREN, LBRACK/RBRACK, and
LBRACE/RBRACE are the only three pairs whose recursive-descent parse recurses
back into ownedExpression per nesting level. Documented this derivation in
detail in ExceedsMaxNestingDepth's XML doc remarks, explicitly stating this is
the complete list and instructing future maintainers to re-derive it if the
grammar changes, to prevent this class of gap from regressing silently again.
- Added LBRACE to the push cases and RBRACE to the pop cases in
ExceedsMaxNestingDepth, mirroring the existing paren/bracket handling.
- Added Parse_DeeplyNestedBodyExpressionBraces_ReturnsDiagnosticInsteadOfCrashing
(500-level a.?{...} repro) and
Parse_ShallowBodyExpressionBraces_ReturnsUnsupportedConstructNotDeepNestingDiagnostic
(5-level sanity check, confirming no false-positive rejection) regression tests.
- Updated the Core.Filtering design doc, reqstream, and verification docs to
describe the LBRACE/RBRACE gap and the independent grammar re-derivation.
- Re-verified via a throwaway console harness against the built assembly:
n=500/3000/5000 brace nesting now returns a clean diagnostic instead of
crashing; n=1/5/20 still correctly return 'Unsupported filter construct'
with no false positive; existing paren/bracket guards and ordinary
expressions are unaffected.
Verified pwsh ./fix.ps1 (clean), pwsh ./lint.ps1 (clean), and pwsh ./build.ps1
(393 tests x 3 target frameworks, all green, up from 391).
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This was referenced Jul 13, 2026
Malcolmnixon
added a commit
that referenced
this pull request
Jul 17, 2026
…ests (#43) * Fix InterconnectionView to depth-limit recursion for non-recursive expose scopes Add HasUnlimitedRecursion helper computed once per BuildLayout call from the resolved expose scope's Subjects, and thread the resulting boolean through LayOutInterior, CollectParts, BuildPartItem, and CollectTopLevelScopedParts. A purely non-recursive scope (MembershipExact and/or NamespaceDirectChildren subjects only) now limits interior expansion to the selected root's own direct part children, rendering any deeper container part as an intrinsic-sized leaf box instead of always recursing fully into its interior. A scope with at least one recursive subject, or no scope at all (--auto), continues to recurse fully at every depth, unchanged. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Add/rename InterconnectionView tests for depth-limited recursion - Rename ..._TopLevelFeatureIsContainer_RecursesInterior to ..._ContainerFeature_RendersAsLeaf and invert its assertion: it previously encoded the pre-fix over-recursion bug for a non-recursive scope; now asserts the top-level container feature renders as a leaf with no nested children, matching the corrected boxless-fallback depth-limiting. - Add ExposeNonRecursiveSubjects_DoesNotRecurseIntoNestedPartsInterior, confirming a purely non-recursive scope (MembershipExact + NamespaceDirectChildren) stops interior expansion at depth 0 on the normal container-rooted path. - The existing ExposeRootOnly_NestedSubsystemInDifferentNamespace_ RendersItsOwnUnits test (MembershipRecursive) now doubles as the regression guard proving full recursion is unchanged when the scope has an unlimited-depth subject. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Update companion docs for InterconnectionView depth-limited recursion - design doc: describe HasUnlimitedRecursion, conditional recursion in BuildLayout/CollectParts/BuildPartItem/CollectTopLevelScopedParts, and document the global-boolean simplification and its known limitation (a scope combining a recursive and a non-recursive subject uses unlimited recursion for the whole diagram). - reqstream: revise ScopedTopLevelFeatureFallback's title/tests to match the corrected fallback behavior; add new requirement ExposeScopingRecursionDepthLimit covering both the normal root path and the top-level fallback path. - verification doc: update Verification Approach, Acceptance Criteria, and Test Scenarios for the new/renamed tests. - user guide: clarify that Interconnection View interior recursion depth depends on whether the resolved expose scope is recursive. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Fix InterconnectionView per-branch recursion depth and top-level connection name collisions Fix 1: Replace the diagram-wide unlimitedRecursion boolean with per-branch tracking. Previously, HasUnlimitedRecursion computed one bool from the whole resolved scope and threaded it unchanged through every branch of the diagram, so a scope combining a recursive subject (expose A::**;) with a non-recursive subject (expose B;) applied unlimited recursion to the ENTIRE diagram, over-collapsing B's branch which should have stayed depth-limited to itself. Added ExposeScopeResolver.MatchesUnlimitedSubject(qualifiedName, scope) - a narrower sibling of IsInSubjectScope that reports whether a feature matches specifically a MembershipRecursive or NamespaceRecursive subject (never via ExplicitMembers). InterconnectionViewLayoutStrategy now decides recursion once per top-level branch (CollectParts at depth 0, and independently per feature in CollectTopLevelScopedParts) via a new DetermineBranchUnlimitedRecursion helper, and threads that concrete per-branch bool (ancestorUnlimitedRecursion) unchanged to every descendant, mirroring how the old global boolean was threaded but decided per-branch instead of per-diagram. HasUnlimitedRecursion is retained only as the rare depth-0-no-qualified-name fallback. Fix 2: Fix simple-name collision in the top-level boxless connector resolution. BuildPartIndex (used by the single-root ResolveConnections path) indexes by simple name only, which is safe there because one containing part def can't have two direct children with the same simple name. That assumption breaks for ResolveTopLevelConnections, which previously built one flat cross-workspace part index from every top-level part CollectTopLevelScopedParts collected across the entire workspace - if two different containing definitions each declared a same-named part (e.g. both part logger : Logger;), TryAdd silently kept only the first occurrence, causing bogus or dropped connections. Added a TopLevelPart(Part, OwnerQualifiedName) record; CollectTopLevelScopedParts now returns each matched feature paired with its owning definition's qualified name. ResolveTopLevelConnections groups top-level parts by OwnerQualifiedName and resolves each definition's own connections only against that definition's own restricted part index, never a flat cross-workspace union. The single-root BuildPartIndex/ResolveConnections path is unchanged. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Add tests for per-branch recursion and owner-scoped top-level connection resolution - ExposeScopeResolverTests: 7 new cases for MatchesUnlimitedSubject covering MembershipRecursive (self/subtree), NamespaceRecursive (subtree), MembershipExact, NamespaceDirectChildren, ExplicitMembers-only, and an unrelated name. - InterconnectionViewLayoutStrategyTests: - ExposeMixedRecursion_OnlyRecursiveBranchRecurses: a scope with expose A::**; expose B; on two sibling parts confirms A's branch fully recurses into its nested parts while B's branch stays depth-limited to itself, in the same diagram. - TopLevelNameCollisionAcrossOwners_ResolvesOwnConnection: two different part defs each declaring a same-named logger part and their own connection, both exposed as top-level scoped features (forcing the boxless fallback), confirms each definition's own connection resolves to its own logger/other parts, never the other definition's same-named part. All pre-existing tests continue to pass unchanged (35 -> 78 total InterconnectionView + ExposeScopeResolver tests, 0 regressions). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Update companion docs for per-branch recursion and owner-scoped connection resolution - design/interconnection-view-layout-strategy.md: rewrote "Depth-Limited Recursion" as "Per-Branch Depth-Limited Recursion", documenting DetermineBranchUnlimitedRecursion and explicitly superseding the earlier global-boolean simplification note; documented the TopLevelPart record and owner-scoped grouping in ResolveTopLevelConnections. - design/expose-scope-resolver.md: documented MatchesUnlimitedSubject alongside IsInSubjectScope. - reqstream/interconnection-view-layout-strategy.yaml: reworded ExposeScopingRecursionDepthLimit for per-branch semantics, added ExposeScopingPerBranchRecursionIndependence (Fix 1) and ScopedTopLevelFeatureFallbackOwnerScopedConnections (Fix 2) requirements. - reqstream/expose-scope-resolver.yaml: added UnlimitedSubjectMatch requirement. - verification/*.md: updated verification approach, acceptance criteria, and test-scenario tables for both units to reflect the new tests and per-branch/owner-scoped behavior. - user_guide/introduction.md: reworded the Interconnection View recursion-depth paragraph to describe the per-branch decision, and documented owner-scoped top-level connection resolution. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * docs: correct Dependencies bullet to reflect per-branch recursion gate Update the InterconnectionViewLayoutStrategy design doc's Dependencies section to name MatchesUnlimitedSubject and IsMoreSpecificCandidate, and clarify that MatchesUnlimitedSubject (via DetermineBranchUnlimitedRecursion) is now the primary per-branch depth-limiting gate, with HasUnlimitedRecursion retained only as the depth-0 no-qualified-name fallback. Addresses a low-severity documentation staleness finding from formal review. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * build: update DemaConsulting.Rendering to 0.1.0-beta.14 Picks up the graduated-falloff fix for ContainmentLayoutAlgorithm's column-count-estimate ratio guard (Rendering PR #26), which previously used a hard MaxColumnEstimateSizeRatio = 2.0 cutoff that collapsed realistic label-driven box sets into a single degenerate column whenever width variance barely exceeded 2x (observed with VersionMarkSystemView: 9 boxes, widths 168-351px, ratio 2.08). Verified: full suite still green (515 core tests x3 TFMs + 355 tool tests), and VersionMarkSystemView now packs into a balanced 2-3 box per row grid instead of a single tall column. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --------- Co-authored-by: Malcolm Nixon <Malcolm.Nixon@hiarc.inc> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
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
1 participant
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.
This pull request significantly strengthens the robustness of the filter expression parser and its documentation, ensuring that parsing and evaluation are resilient to adversarial or malformed input. The main improvements are the addition of hardening measures to prevent process crashes (especially stack overflows), better error reporting for edge cases, and comprehensive documentation and test coverage for these scenarios.
Parser hardening and error resilience:
FilterExpressionParser.Parseto detect and reject filter expressions with excessive nesting (parentheses, brackets, braces, or prefix unary operators) before invoking the recursive-descent parser, preventing uncatchable stack overflows that would otherwise crash the process. (src/DemaConsulting.SysML2Tools.Core/Filtering/FilterExpressionParser.cs[1] [2] [3]Parseto catch allExceptions (not justRecognitionException), ensuring that unexpected ANTLR-internal failures (e.g., astral-plane Unicode input) are reported as diagnostics instead of causing unhandled exceptions or crashes. (docs/design/sysml2-tools-core/filtering/filter-expression-evaluator.md[1] [2] [3]double(producing non-finite values), instead of silently accepting or producing unparsable output. (docs/design/sysml2-tools-core/filtering/filter-expression-evaluator.md[1] [2] [3]Documentation and requirements:
docs/design/sysml2-tools-core/filtering.md[1]docs/design/sysml2-tools-core/filtering/filter-expression-evaluator.md[2] [3] [4] [5]docs/verification/sysml2-tools-core/filtering/filter-expression-evaluator.md[6] [7]docs/reqstream/sysml2-tools-core/filtering/filter-expression-evaluator.yaml[1] [2]docs/verification/sysml2-tools-core/filtering/filter-expression-evaluator.md[3] [4]Test coverage:
docs/reqstream/sysml2-tools-core/filtering/filter-expression-evaluator.yaml[1]docs/verification/sysml2-tools-core/filtering/filter-expression-evaluator.md[2] [3]These changes collectively ensure that the filter expression subsystem is robust against malformed input and safe for use in live-editing GUI scenarios.