refactor(395): remove the remaining dead confusion-matrix paths by Thanatos9404 · Pull Request #398 · awslabs/stickler

Thanatos9404 · 2026-10-15T16:24:38Z

Pull Request

Issue Reference

Part of #395. Items 1, 2 and 4 and both doc fixes are here. Item 3 turned out to be reachable, and item 5 needs a decision first; both are explained below.

Description of Changes

Follow-up to #390. Removes the two dead paths from #395, makes the per-pair classifier return only the six counts, and fixes the two points in the refactoring notes. No compare() or compare_with() result changes: the full suite gives the same results before and after, plus 6 new tests.

Changes Made

structured_model.py: delete _calculate_object_level_metrics (:1281, 55 lines) and the HungarianHelper import, which only it used. In the class docstring, the ConfusionMatrixCalculator and ConfusionMatrixBuilder bullets no longer describe list-level metrics or a third calculator.
confusion_matrix_builder.py: delete the calculator property, the _calculator slot, and the docstring lines that listed ConfusionMatrixCalculator as a dependency.
confusion_matrix_calculator.py: classify_field_for_confusion_matrix returns the six counts with no derived dict, and drops the MetricsHelper import. It is now a @staticmethod that takes the ground truth model, so StructuredModel._classify_field_for_confusion_matrix no longer builds a ConfusionMatrixCalculator per pair. With the builder property gone, nothing else constructed one.
docs/structured_model_REFACTORING.md: the size is now ~2030 lines (2029 after this PR; ~1486 was right when 47c25d0 wrote it in May). The shims paragraph names the three wrappers refactor(296): remove the dead ConfusionMatrixCalculator methods #390 removed and what does their work now. The builder's tree entry and the summary line no longer describe removed pieces.
New tests/structured_object_evaluator/test_list_subfield_classification.py, 6 tests: the classifier returns exactly the six counts (fails before this change, on the derived key), and a model-valued sub-field is compared as a model (passes before and after; see item 3).

Why These Changes Are Needed

Same kind of leftover as #296: a second copy of _calculate_object_level_metrics that could drift from the live one, a builder dependency that does not exist, and a derived-metrics dict built for every matched pair and primitive sub-field and then thrown away (N*K per list comparison).

Reachability check

Line numbers are at ad19ac8, as in the issue; src/ is identical on dev 6abbf26. Each name was searched with git grep over src/, tests/, examples/ (including the 11 notebooks) and docs/. A search for indirect access (getattr with built names, mock.patch, the names as strings) found none. Executed statements come from coverage json after a full coverage run -m pytest tests/ on dev 6abbf26.

Item	Code	Callers	Executed on dev
1	`StructuredModel._calculate_object_level_metrics`	none: `structured_list_comparator.py:70` calls `self._calculate_object_level_metrics`, and `StructuredListComparator` (:21) does not inherit from `StructuredModel`, so it reaches its own copy at :102 (15/15)	0/15
2	`ConfusionMatrixBuilder.calculator`, `_calculator`	none; `build_confusion_matrix` reads only `aggregate_calculator` and `derived_calculator`	0/4
3	`StructuredModel` branch in `classify_field_for_confusion_matrix` (:80-87)	reachable, see below	0/2 (the two statements inside it, :86-87)
4	`classify_field_for_confusion_matrix` via `_classify_field_for_confusion_matrix`	`structured_list_comparator.py:511`, which copies only the six counts (:515)	26/29, 3/3

Item 3: kept

_handle_non_hierarchical_field gets every sub-field that ConfigurationHelper.is_structured_field_type does not recognise. That covers List[M], Optional[List[M]], M and Optional[M], but not Union[A, B] without None, Any or object, which classification-logic.md names as annotations that can carry model values. On such a sub-field this branch is what compares two models as models. Without it, the field's default LevenshteinComparator scores the text of Inner(text="p") against Inner(text="q") at 0.96, so the confusion matrix counts the pair as a TP while non_matches and field_comparisons in the same result report items[0].value as a mismatch. TestModelValuedSubField pins this for Union[A, B] and Any; with the isinstance test replaced by False, its two mismatch cases fail. The import inside the branch is the lazy-import pattern the refactoring notes call intentional, so it stays as well.

Item 5: not in this PR

ThresholdHelper.is_above_threshold here makes the confusion matrix agree with a List[str] element, but not with the rest of the same result. I tried it: for one list item whose sub-field scores 0.7 + 0.1 against 0.8, one compare_with then reports

`items[0].name`	now	with the tolerant test
confusion matrix	FD	TP
`non_matches`	false_discovery	false_discovery
`field_comparisons`	match false, score 0.0	match false, score 0.0

non_matches and field_comparisons come from the item's own compare_with, which goes through FieldComparator.compare_primitive_with_scores and its bare >= (field_comparator.py:68). That comparison also clips the score feeding the pair's similarity, and it is what a top-level or nested-model field gets, so the same score is an FD there too. Today the tolerant test is used by ComparisonHelper (primitive list elements) and the bare one by FieldComparator and everything that reads it. Inside a List[StructuredModel], the matched objects themselves are also counted with a bare >= (structured_list_comparator.py:127).

So there are three ways to go:

keep the bare test here, so one result follows one rule. If that works for you, this PR can close Remove the remaining dead confusion-matrix paths left after #390 #395.
the tolerant test here only, as the issue describes. I have it ready (one line, a regression test and a CHANGELOG entry), but it ships the mismatch above.
move every field-level verdict to ThresholdHelper together (FieldComparator, the field_comparisons collectors and this classifier), as its own PR with a CHANGELOG entry, since it changes verdicts and clipped scores at the margin.

I would keep the bare test. Which do you prefer?

Testing

Unit tests added/updated
Integration tests added/updated
Manual testing completed
All existing tests pass (apart from 6 tests that need the POSIX-only signal.SIGALRM; details below)

Windows, Python 3.12.5, uv sync --frozen --extra llm --no-group docs (CI also installs the bert extra). inspect.getfile(stickler) pointed at this checkout's src/stickler/__init__.py for every run.

uv run --no-sync coverage run -m pytest tests/
uv run --no-sync coverage report -i --include="src/*"
uv run --no-sync coverage json -i --include="src/*"

-i skips a temporary file that test_the_canary_is_wired_up creates and deletes.

	`dev` `6abbf26`	this branch (`623a408`)
pytest under coverage	2782 passed, 6 failed, 10 skipped, 1 xfailed	2788 passed, 6 failed, 10 skipped, 1 xfailed
`confusion_matrix_calculator.py`	33/36 statements	31/32
`confusion_matrix_builder.py`	27/31	24/24
`structured_model.py`	427/475	424/457
`structured_list_comparator.py`	147/157	147/157
`src/` total	6573/7583 (86.68%)	6565/7554 (86.91%)

Missed statements in src/ drop by 21 (1010 to 989): the 19 of the two dead paths, plus the two lines of the item 3 branch, which the new test now runs.

The 6 failures are the same on both trees: 5 tests in TestRealisticDeepNesting and test_integration in test_realistic_deep_nesting.py, all AttributeError: module 'signal' has no attribute 'SIGALRM'. Linux CI runs them.

On the changed Python files, uv run ruff check passes, and uv run ruff format --check passes for all but confusion_matrix_builder.py, which it already flagged on dev (the same 22 diff lines of whitespace and one trailing comma, none on lines this PR touches).

Reviewers

@sromoam

Checklist

Code follows the project's coding standards
Self-review of code completed
Documentation updated (if applicable)
CHANGELOG.md entry added under [Unreleased], or not needed because
nothing user-visible changed (see RELEASING.md): dead-code removal and an internal return shape, no entry
Tests added/updated for new functionality
All CI checks are passing
Ready for review

Additional Notes

classify_field_for_confusion_matrix has no caller outside StructuredModel._classify_field_for_confusion_matrix, and ConfusionMatrixCalculator is not exported, so making it a @staticmethod changes no public signature. The wrapper and its row in the refactoring notes stay.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

StructuredModel._calculate_object_level_metrics has no caller. The one call site is inside StructuredListComparator, which does not inherit from StructuredModel and calls its own copy. ConfusionMatrixBuilder's calculator property and _calculator slot are never read. Both had zero executed lines across the full suite. Remove them, the HungarianHelper import that only the first used, and the docstrings that describe the builder as coordinating a ConfusionMatrixCalculator. classify_field_for_confusion_matrix now returns only the six counts. Its one caller, StructuredListComparator._handle_non_hierarchical_field, adds up those counts for every matched pair and primitive sub-field and never read the derived metrics that came with them. It is now a staticmethod that takes the ground truth model, so the StructuredModel wrapper no longer builds a ConfusionMatrixCalculator on every call. The StructuredModel branch in classify_field_for_confusion_matrix stays. is_structured_field_type does not recognise a list element sub-field annotated Union[A, B] or Any, so model values on such a field reach the classifier. Without the branch the default LevenshteinComparator would score the text of the two models, and a pair that scores 0.0 as models would count as a TP. A new test pins it. Also correct the StructuredModel size in the refactoring notes (2029 lines now, not ~1486), and name the shims awslabs#390 removed and what does their work now. Refs awslabs#395

sromoam

Thanks, this is a clean removal. I checked it against 623a408: the 6 new tests pass, and on dev the two TestVerdictShape tests fail, as the description says.

The only change I'm asking for is one sentence of docs. In docs/structured_model_REFACTORING.md, line 159 still says the private wrappers "were not removed", and the new paragraph on line 161 lists four that were. Rewording 159 to match is all that stands between this and an approval. Everything else below is optional.

Optional, outside the diff: comparison_engine.py:25, aggregate_metrics_calculator.py:21 and derived_metrics_calculator.py:21 still list ConfusionMatrixCalculator as a component that calculates the matrix, the same claim this PR corrects in the builder and StructuredModel docstrings.

Not for this PR: while testing I found that a dict in an Any sub-field of a list item crashes compare_with(). It fails the same way on dev and this PR doesn't touch that path, so I filed it as #400, with a direction that would also cover #395's remaining items 3 and 5.

sromoam · 2026-10-15T19:01:58Z


 Older callers (and many tests) reach for private methods on `StructuredModel` such as `_dispatch_field_comparison`, `compare_recursive`, `_collect_enhanced_non_matches`, `_add_derived_metrics_to_result`, `_format_for_evaluator`, etc. These were **not removed** — they now exist as one-line wrappers that build the appropriate component and forward the call. They are retained for backward compatibility; new code should call the component directly.

+Three wrappers that nothing called were removed in [#390](https://github.com/awslabs/stickler/pull/390): `_calculate_list_confusion_matrix`, `_calculate_nested_field_metrics` and `_calculate_single_nested_field_metrics`. Their work is done by `StructuredListComparator` for a list of models, including the fields inside matched items, and by `FieldComparator.compare_structured_field` for a single nested model. [#395](https://github.com/awslabs/stickler/issues/395) removed `_calculate_object_level_metrics`, an uncalled copy of `StructuredListComparator._calculate_object_level_metrics`.


The one requested change. Line 159 still says these wrappers "were not removed" and that each one "build[s] the appropriate component". This paragraph now lists four that were removed, and _classify_field_for_confusion_matrix no longer builds a component. Could you reword 159 to match? While you're here, the removal is in this PR, so the link should point to #398 rather than issue #395.

Reworded in fc7e762. Line 159 now says the wrappers that still have callers are kept and forward the call to the component doing the work, and that four private methods nothing called have been removed, as listed in the next paragraph. The _calculate_object_level_metrics link now points to #398.

sromoam · 2026-10-15T19:01:58Z


        This method orchestrates the calculation of a complete confusion matrix
-        by coordinating between the three calculator classes:
+        in three steps:


Optional: the docstring now counts three steps, but the inline comments below number aggregate and derived as Step 1 and Step 2.

Done in fc7e762: the inline comments are now Step 1 to Step 3, matching the docstring.

sromoam · 2026-10-15T19:01:58Z

+    ``ConfigurationHelper.is_structured_field_type`` recognises neither
+    annotation, so the pair takes the primitive route. The field's default
+    comparator is ``LevenshteinComparator``, which scores the text of
+    ``_Inner(text="p")`` against ``_Inner(text="q")`` at about 0.95. Compared as


Optional: measured at 0.9583, so "about 0.96" (the PR body says 0.96).

Thanks, changed to "about 0.96" in fc7e762.

sromoam · 2026-10-15T19:01:58Z

-        """
-        self.model = model
-
+    @staticmethod


Nit, take it or leave it: this is now one static method behind a forwarding wrapper. A module-level function called from StructuredListComparator would do the same job.

I'd leave this one as is, if that's OK: keeping the class keeps the ConfusionMatrixCalculator import path and the component list in the refactoring notes unchanged. Happy to switch it to a module-level function if you prefer.

The refactoring notes still said the private wrappers "were not removed" right above the paragraph listing the four that were. Say the ones that still have callers are kept as forwarding wrappers, and link the _calculate_object_level_metrics removal to awslabs#398 instead of the issue. Also stop listing ConfusionMatrixCalculator as the component that calculates the matrix in the ComparisonEngine, AggregateMetricsCalculator and DerivedMetricsCalculator docstrings, number the builder's inline step comments to match its docstring, and give the Levenshtein score in the model-valued sub-field test as about 0.96 (measured 0.9583). Refs awslabs#395

Thanatos9404 · 2026-10-15T20:17:54Z

Thanks for the review, @sromoam. fc7e762 rewords line 159 and fixes the link, and also takes the optional items: the three docstrings outside the diff (comparison_engine.py, aggregate_metrics_calculator.py, derived_metrics_calculator.py), the step numbering in the builder, and the 0.96 comment. The full suite passes apart from the six signal.SIGALRM timeout tests, which fail the same way on dev on Windows. And thanks for filing #400.

Thanatos9404 · 2026-10-18T20:38:13Z

@sromoam the docs sentence you asked for is in fc7e762, and dev is merged in as of the 17th. Could you take another look when you have time?

sromoam

Thanks @Thanatos9404, fc7e762 covers what I asked for, and thanks for taking the optional items too. Approving. One small wording fix if you're touching it again: the ConfusionMatrixCalculator docstring bullet (structured_model.py:325) and the test module docstring say "primitive" sub-fields, but this PR keeps the model-valued branch on purpose, so "non-list sub-fields" is closer. Separately, that kept branch scores with compare_with() while the dispatcher uses compare(). That's older than this PR and is filed as #461.

Model-valued sub-fields annotated Union or Any also reach the classifier, so "non-list" describes what it handles; simple list sub-fields still go through the element-level dispatch.

Thanatos9404 · 2026-10-28T18:49:39Z

Thanks! Switched both the ConfusionMatrixCalculator bullet and the test module docstring to "non-list sub-fields" in e330b54. And thanks for filing #461 for the compare_with() mismatch.

github-actions · 2026-10-28T21:07:05Z

Security Scan Results

ASH Security Scan Report

Report generated: 2026-10-28T19:51:21+00:00
Time since scan: 2 minutes

Scan Metadata

Project: ASH
Scan executed: 2026-10-28T19:48:27+00:00
ASH version: 3.1.2

Summary

Scanner Results

The table below shows findings by scanner, with status based on severity thresholds and dependencies:

Severity levels:
- Suppressed (S): Findings that have been explicitly suppressed and don't affect scanner status
- Critical (C): Highest severity findings that require immediate attention
- High (H): Serious findings that should be addressed soon
- Medium (M): Moderate risk findings
- Low (L): Lower risk findings
- Info (I): Informational findings with minimal risk
Duration (Time): Time taken by the scanner to complete its execution
Actionable: Number of findings at or above the threshold severity level that require attention
Result:
- PASSED = No findings at or above threshold
- FAILED = Findings at or above threshold
- MISSING = Required dependencies not available
- SKIPPED = Scanner explicitly disabled
- ERROR = Scanner execution error
Threshold: The minimum severity level that will cause a scanner to fail
- Thresholds: ALL, LOW, MEDIUM, HIGH, CRITICAL
- Source: Values in parentheses indicate where the threshold is set:
  - global (global_settings section in the ASH_CONFIG used)
  - config (scanner config section in the ASH_CONFIG used)
  - scanner (default configuration in the plugin, if explicitly set)
Statistics calculation:
- All statistics are calculated from the final aggregated SARIF report
- Suppressed findings are counted separately and do not contribute to actionable findings
- Scanner status is determined by comparing actionable findings to the threshold

Scanner	Suppressed	Critical	Low	Actionable	Result	Threshold
bandit	0	4	6307	4	FAILED	MEDIUM (global)
cdk-nag	0	0	0	0	PASSED	MEDIUM (global)
cfn-nag	0	0	0	0	MISSING	MEDIUM (global)
checkov	0	0	0	0	PASSED	MEDIUM (global)
detect-secrets	0	1	0	1	SKIPPED	MEDIUM (global)
grype	0	0	0	0	MISSING	MEDIUM (global)
npm-audit	0	0	0	0	PASSED	MEDIUM (global)
opengrep	0	0	0	0	MISSING	MEDIUM (global)
semgrep	3	0	0	0	PASSED	MEDIUM (global)
syft	0	0	0	0	MISSING	MEDIUM (global)

Top 3 Hotspots

Files with the highest number of security findings:

Finding Count	File Location
2	tests/comparators/test_anls_leaf_semantics.py
2	tests/structured_object_evaluator/test_rollup_node_semantics.py
1	tests/test_readme_examples.py

Detailed Findings

Show 5 actionable findings

Finding 1: SECRET-HEX-HIGH-ENTROPY-STRING

Severity: HIGH
Scanner: detect-secrets
Rule ID: SECRET-HEX-HIGH-ENTROPY-STRING
Location: tests/comparators/test_anls_leaf_semantics.py:89

Description:
Secret of type 'Hex High Entropy String' detected in file 'tests/comparators/test_anls_leaf_semantics.py' at line 89

Code Snippet:

Secret of type Hex High Entropy String detected

Finding 2: B108

Severity: HIGH
Scanner: bandit
Rule ID: B108
Location: tests/comparators/test_anls_leaf_semantics.py:210-212

Description:
Probable insecure usage of temp file/directory.

Code Snippet:

uuid.UUID(int=1),
            pathlib.Path("/tmp/x"),
            b"bytes",

Finding 3: B307

Severity: HIGH
Scanner: bandit
Rule ID: B307
Location: tests/structured_object_evaluator/test_rollup_node_semantics.py:1363-1369

Description:
Use of possibly insecure function - consider using safer ast.literal_eval.

Code Snippet:

def published_label(node: dict, section: str, object_lists: set) -> str:
            counts_objects = eval(  # noqa: S307
                cond.group(1),
                {},
                {"data": node, "section": section, "OBJECT_LISTS": object_lists},
            )
            return eval(unit.group(1), {}, {"counts_objects": counts_objects})  # noqa: S307

Finding 4: B307

Severity: HIGH
Scanner: bandit
Rule ID: B307
Location: tests/structured_object_evaluator/test_rollup_node_semantics.py:1368-1370

Description:
Use of possibly insecure function - consider using safer ast.literal_eval.

Code Snippet:

)
            return eval(unit.group(1), {}, {"counts_objects": counts_objects})  # noqa: S307

Finding 5: B102

Severity: HIGH
Scanner: bandit
Rule ID: B102
Location: tests/test_readme_examples.py:55-57

Description:
Use of exec detected.

Code Snippet:

with contextlib.redirect_stdout(out):
        exec(code, namespace)
    prints = [line for line in code.splitlines() if line.startswith("print(")]

Report generated by Automated Security Helper (ASH) at 2026-10-28T19:51:22+00:00

Thanatos9404 requested a review from sromoam as a code owner October 15, 2026 16:24

sromoam mentioned this pull request Oct 15, 2026

A dict in an Any sub-field of a list item crashes compare_with() #400

Open

sromoam requested changes Oct 15, 2026

View reviewed changes

This was referenced Oct 16, 2026

fix(400)!: classify a list element's sub-field as it is at top level #406

Open

fix(340): read and check Stickler extensions inside $defs #402

Closed

chore: merge dev into refactor/395-remaining-dead-confusion-matrix-paths

a894cc1

Thanatos9404 mentioned this pull request Oct 18, 2026

fix(417)!: use one threshold tolerance at every classification gate #421

Merged

9 of 11 tasks

sromoam added this to the 1.1.0 milestone Oct 19, 2026

sromoam mentioned this pull request Oct 28, 2026

[BUG]: a model-valued Any sub-field of a list item is counted FD while field_comparisons reports a match #461

Open

sromoam approved these changes Oct 28, 2026

View reviewed changes

docs(395): call the classified list sub-fields non-list, not primitive

e330b54

Model-valued sub-fields annotated Union or Any also reach the classifier, so "non-list" describes what it handles; simple list sub-fields still go through the element-level dispatch.

Thanatos9404 requested a review from digalaviz as a code owner October 28, 2026 18:49

sromoam merged commit 13a4523 into awslabs:dev Oct 28, 2026
10 checks passed

sromoam mentioned this pull request Oct 30, 2026

release(v1.0.1): HTML report security fixes and scoring corrections #473

Closed

7 tasks

digalaviz mentioned this pull request Nov 2, 2026

release(v1.0.1): HTML report security fixes and scoring corrections #475

Merged

1 of 7 tasks


		Older callers (and many tests) reach for private methods on `StructuredModel` such as `_dispatch_field_comparison`, `compare_recursive`, `_collect_enhanced_non_matches`, `_add_derived_metrics_to_result`, `_format_for_evaluator`, etc. These were not removed — they now exist as one-line wrappers that build the appropriate component and forward the call. They are retained for backward compatibility; new code should call the component directly.

		Three wrappers that nothing called were removed in [#390](https://github.com/awslabs/stickler/pull/390): `_calculate_list_confusion_matrix`, `_calculate_nested_field_metrics` and `_calculate_single_nested_field_metrics`. Their work is done by `StructuredListComparator` for a list of models, including the fields inside matched items, and by `FieldComparator.compare_structured_field` for a single nested model. [#395](https://github.com/awslabs/stickler/issues/395) removed `_calculate_object_level_metrics`, an uncalled copy of `StructuredListComparator._calculate_object_level_metrics`.

Conversation

Thanatos9404 commented Oct 15, 2026

Pull Request

Issue Reference

Description of Changes

Changes Made

Why These Changes Are Needed

Reachability check

Item 3: kept

Item 5: not in this PR

Testing

Reviewers

Checklist

Additional Notes

Uh oh!

sromoam left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Thanatos9404 commented Oct 15, 2026

Uh oh!

Thanatos9404 commented Oct 18, 2026

Uh oh!

sromoam left a comment

Choose a reason for hiding this comment

Uh oh!

Thanatos9404 commented Oct 28, 2026

Uh oh!

Uh oh!

github-actions Bot commented Oct 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Security Scan Results

ASH Security Scan Report

Scan Metadata

Summary

Scanner Results

Top 3 Hotspots

Detailed Findings

Finding 1: SECRET-HEX-HIGH-ENTROPY-STRING

Finding 2: B108

Finding 3: B307

Finding 4: B307

Finding 5: B102

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

github-actions Bot commented Oct 28, 2026 •

edited

Loading