Skip to content

fix: prevent script injection in security workflow PR number step#154

Merged
adiadd merged 1 commit into
devfrom
fix/gha-script-injection-pr-number
Jun 26, 2026
Merged

fix: prevent script injection in security workflow PR number step#154
adiadd merged 1 commit into
devfrom
fix/gha-script-injection-pr-number

Conversation

@adiadd

@adiadd adiadd commented Jun 26, 2026

Copy link
Copy Markdown
Contributor

Summary

Fixes a script-injection issue in .github/workflows/security.yaml.

The Save PR number step interpolated an untrusted GitHub context value directly into an inline shell script:

run: echo "${{ github.event.pull_request.number }}" > pr-number.txt

GitHub Actions expands ${{ }} before the shell runs, substituting the raw value into the script text. Any field under the github.event context must be treated as untrusted input, so this is the classic script-injection pattern.

Fix

Bind the expression to an intermediate environment variable and reference it quoted in the script — GitHub's documented mitigation for inline scripts:

env:
  PR_NUMBER: ${{ github.event.pull_request.number }}
run: echo "$PR_NUMBER" > pr-number.txt

The value is now passed as environment data and the shell never re-evaluates it as code. No ${{ }} remains inside any run: block.

Verification

  • Scanned all six workflow files: this was the only ${{ }} interpolated into a run: block. Remaining expressions (matrix.python-version, secrets.GITHUB_TOKEN, the run-id: action input, and if: conditions) are not shell interpolations and are safe.
  • YAML parses cleanly.
  • Behavior is unchanged: pr-number.txt still receives the PR number consumed by security-pr-comment.yaml.

@sromoam sromoam left a comment

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.

Looks ok to me.

@github-actions

Copy link
Copy Markdown

Security Scan Results

ASH Security Scan Report

  • Report generated: 2026-06-26T20:12:52+00:00
  • Time since scan: 2 minutes

Scan Metadata

  • Project: ASH
  • Scan executed: 2026-06-26T20:10:39+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 High Medium Low Info Actionable Result Threshold
bandit 0 1 0 0 4036 0 1 FAILED MEDIUM (global)
cdk-nag 0 0 0 0 0 0 0 PASSED MEDIUM (global)
cfn-nag 0 0 0 0 0 0 0 MISSING MEDIUM (global)
checkov 0 1 0 0 0 0 1 SKIPPED MEDIUM (global)
detect-secrets 0 0 0 0 0 0 0 PASSED MEDIUM (global)
grype 0 0 0 0 0 0 0 MISSING MEDIUM (global)
npm-audit 0 0 0 0 0 0 0 PASSED MEDIUM (global)
opengrep 0 0 0 0 0 0 0 MISSING MEDIUM (global)
semgrep 0 0 0 0 0 0 0 PASSED MEDIUM (global)
syft 0 0 0 0 0 0 0 MISSING MEDIUM (global)

Top 2 Hotspots

Files with the highest number of security findings:

Finding Count File Location
1 .github/workflows/workflow.yml
1 tests/test_top_level_exports.py

Detailed Findings

Show 2 actionable findings

Finding 1: CKV2_GHA_1

  • Severity: HIGH
  • Scanner: checkov
  • Rule ID: CKV2_GHA_1
  • Location: .github/workflows/workflow.yml:11-12

Description:
Ensure top-level permissions are not set to write-all


Finding 2: B102

  • Severity: HIGH
  • Scanner: bandit
  • Rule ID: B102
  • Location: tests/test_top_level_exports.py:71-73

Description:
Use of exec detected.

Code Snippet:

ns = {}
        exec("from stickler import *", ns)  # noqa: S102
        exported_names = set(ns) - {"__builtins__"}

Report generated by Automated Security Helper (ASH) at 2026-06-26T20:12:53+00:00

@adiadd
adiadd merged commit a68a9d7 into dev Jun 26, 2026
10 checks passed
@adiadd
adiadd deleted the fix/gha-script-injection-pr-number branch June 26, 2026 20:13
adiadd added a commit that referenced this pull request Jun 26, 2026
* Update: add 4 file(s), modify 4 file(s)

* Update: modify 1 file(s)

* reverted .gitignore changes

* Update: modify 4 file(s)

* Update: modify 1 file(s)

* Update: modify 3 file(s)

* Update: modify 6 file(s)

* Update: modify 3 file(s)

* Fixing the issues refernced in Issue #33
#33

* Resolving Additya's feedback.

* docs: restructure documentation with new sections and navigation (#86)

* docs: restructure documentation with new sections and navigation

* docs: add dark/light mode theme toggle and site URL config

* docs: update code examples and installation requirements

* docs: update deployment section to reflect implemented workflow

* docs: add comparison engine architecture guide for contributors

* docs: restructure documentation with consolidated navigation

* docs: address PR review feedback — add KIE framing, architecture diagrams, sample outputs, SME calibration guide, and namespaced extras

* Adding in the docsplit metrics. (#82)

* Adding in the docsplit metrics.
* Impementing 1:1 metrics porting
* Added a namespace for Stickler docsplit metrics
* Added tests.

* ruff linting

* Updating the packet split metrics to include the new metrics in addition to classical
* Updating the mkdocs UI
* Adding tests
* Adding docs.
* Updating with the new evaluation tooling

Changes were made to the tooling to support JSON based invocation.

* Adding an example for docsplit.

* resolving the remaining comments
* Fixing the docs for the page number clarity.
* Removing the extra init file.
* Adding a case to cover empty data frame.

* Removing an empty readme.
Adding init py file to a test directory.

* chore: bump version to 0.2.0 for release

* Sr/fix 17 (#84)

* Fixing the issues refernced in Issue #33
#33

* This PR includes tests that cover the complaint in #17

* This PR adds testing to prove that we aren't suffering the error pointed out in Issue #17.
#17

* reverting the unneeded file change from dev.

* Fixing the ruff linting issues in this PR.

* fix: update packet evaluation metric (#95)

* fix: update packet evaluation metric

* test: update single-page group ordering test to match new behavior

* test: add test_single_page_groups_score_perfect per reviewer feedback

---------

Co-authored-by: Sirajus Salekin <salekin@amazon.com>

* chore: migrate conda to uv (#97)

* chore: migrate from conda to uv for environment and dependency management

* docs: update commands to uv run with pip+venv fallback notes and sync pyproject.toml dev extras

* docs: add uv pip install alternative to installation guide

* fix: address PR review feedback for lint parity, docs accuracy, and notebook format

* fix: include static assets (CSS, JS) in package distribution (#91)

The HTML report styling and interactive JS files were not included
in the built wheel because setuptools only packages .py files by
default. This resulted in unstyled HTML reports when installed from
PyPI.

* fix: remove unused inspect import in structured_model.py (#100)

* fix: resolve PyPI release issues (missing __init__.py, dependency bounds, metadata) (#101)

* fix: resolve PyPI release issues (missing __init__.py, dependency bounds, metadata)

* fix: correct dependency lower bounds for Python 3.12 compatibility

* fix: add dependency upper bounds and ignore dist/ for PyPI release readiness

* chore: bump version to 0.2.1 for release

* fix: bump pillow, cryptography, and pytest to resolve dependabot alerts

* Bump python-multipart in the uv group across 1 directory

Bumps the uv group with 1 update in the / directory: [python-multipart](https://github.com/Kludex/python-multipart).


Updates `python-multipart` from 0.0.22 to 0.0.26
- [Release notes](https://github.com/Kludex/python-multipart/releases)
- [Changelog](https://github.com/Kludex/python-multipart/blob/master/CHANGELOG.md)
- [Commits](Kludex/python-multipart@0.0.22...0.0.26)

---
updated-dependencies:
- dependency-name: python-multipart
  dependency-version: 0.0.26
  dependency-type: indirect
  dependency-group: uv
...

Signed-off-by: dependabot[bot] <support@github.com>

* fix: restore dropped notebook cell from merge conflict resolution

* fix: remove duplicate docs targets from merge conflict resolution

* fix: resolve PR #102 review feedback — round-trip fidelity, doc corrections, version bump to 0.3.0

* fix: repair broken notebooks — kernel specs and evaluator_format crash

* fix: keep semantic model_id as string (#116)

* chore(deps): bump python-multipart in the uv group across 1 directory (#126)

Bumps the uv group with 1 update in the / directory: [python-multipart](https://github.com/Kludex/python-multipart).


Updates `python-multipart` from 0.0.26 to 0.0.27
- [Release notes](https://github.com/Kludex/python-multipart/releases)
- [Changelog](https://github.com/Kludex/python-multipart/blob/main/CHANGELOG.md)
- [Commits](Kludex/python-multipart@0.0.26...0.0.27)

---
updated-dependencies:
- dependency-name: python-multipart
  dependency-version: 0.0.27
  dependency-type: indirect
  dependency-group: uv
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* chore(deps): bump urllib3 in the uv group across 1 directory (#128)

Bumps the uv group with 1 update in the / directory: [urllib3](https://github.com/urllib3/urllib3).


Updates `urllib3` from 2.6.3 to 2.7.0
- [Release notes](https://github.com/urllib3/urllib3/releases)
- [Changelog](https://github.com/urllib3/urllib3/blob/main/CHANGES.rst)
- [Commits](urllib3/urllib3@2.6.3...2.7.0)

---
updated-dependencies:
- dependency-name: urllib3
  dependency-version: 2.7.0
  dependency-type: indirect
  dependency-group: uv
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* fix: expose weight-aware aggregate in BulkStructuredModelEvaluator (#122)

* fix: prevent defaultdict mutation and cover list-of-models in weighted score aggregation

* refactor: simplify weighted score accumulators and drop dead defensive branch

* chore: address copilot review comments on PR #124

* fix: distinguish unscored paths from zero-scored paths in bulk field_metrics

* test: cover reset, mid-stream polling, batch, aggregate helper, and disjoint-merge for weighted score

* refactor: drop redundant verbose guards and tighten comments

* docs: propagate weighted_overall_score and mean_score across docsite

* Confidence Evaluation tooling  (#98)

* This branch has capabilities to enable bulk estimation of confidence quality.
* Breakout of metrics, ECE, AUROC, Brier Score.
* Enhancement of the 'rich field' to be tested, and support extension.
* Documentation of the confidence estimation capabilities.
* Notebooks demonstrating single and multi-doc confidence estimation.

* Adding in notebooks, and changes to the bulk structured model eval.

* updating to add ECARB. Error capture at review budget.
Metric to compute gain from reviewing documents.
testing and notebooks to show using these metrics.

* Updating the docs to add the confidence estimation tools to the main guides page.

* Fixing copilot flagged issues.
* Adding todo about the rich value 'value' key bing potentially an issue.
* Fixing edge cases on several modules.
* resolving incorrect coverage accounting.

* whoops, missed files in the commit.

* Fixing many of the nits on the PR.

* Big refactor.
* Creating mechanism for bulk evlaaution against JSON's from individual comparison 'map-reduce pattern'
* creating helpers for these additional evlauations to fit into the bulk evaluator.
* Adding testing.

* feat: add pluggable confidence metrics framework and rich value pattern

- Add pluggable confidence metrics framework (AUROC, Brier Score, ECE, Error Capture at Review Budget) under `stickler.structured_object_evaluator.models.confidence`
- Rename Rich Value Pattern convention keys from `{"value", "confidence"}` to `{"_value", "_confidence"}` to prevent collision with user data fields
- Update `StructuredModel.from_json(process_confidence=...)` to `from_json(process_rich_values=...)` to reflect broader scope
- Rename single-doc confidence result key from `auroc_confidence_metric` to nested `confidence_metrics` dict with `overall`, `fields`, and `coverage` keys
- Add `BulkStructuredModelEvaluator` confidence metrics accumulation across documents in aggregated results
- Add `StructuredModel.compare_with(..., add_confidence_metrics=True)` to include structured confidence block in single-doc results
- Introduce `PostComparisonAccumulator` interface for pluggable accumulators (e.g. bbox mAP) in bulk pipeline
- Add `prediction_raw` round-tripping through comparison results for lossless map/reduce aggregation from JSONL corpora
- Fix `ConfidencePair` validation to ensure `confidence` and `similarity` are finite floats in `[0.0, 1.0]`
- Fix `ErrorCaptureAtBudgetMetric` to report gain against actual reviewed fraction rather than requested budget
- Fix `ConfidenceCalculator.extract` to skip field comparison rows with `actual_key=None` to prevent coverage bias
- Fix `StructuredModel` to always capture `_raw_json` when `process_rich_values=True` for map/reduce aggregation
- Include deprecation shims for one release to support old `{"value", "confidence"}` format and legacy API calls
- Add CHANGELOG.md documenting all changes, deprecations, and migration guide
- Update documentation and notebooks to reflect Rich Value Pattern rename and new confidence metrics API

* fix: address PR #98 review comments for confidence evaluation

* docs: clarify update_from_comparison_result error and jsonl ordering

* chore: refresh uv.lock after dev merge

* fix: address PR #98 review (drop changelog, document accumulators, add bbox jsonl tests, guard duplicate accumulator names)

* fix: tighten legacy rich-value detector and address PR #98 review followups

* refactor: simplify confidence pipeline and close out PR #98 pre-merge gaps

* chore: pin deprecation removal to 0.5.0 (0.4.0 introduces shims)

* fix: log handle close failures in BulkStructuredModelEvaluator.close()/__del__

---------

Co-authored-by: Aditya Addepalli <adiadd@amazon.com>

* chore(deps-dev): bump pymdown-extensions (#129)

Bumps the uv group with 1 update in the / directory: [pymdown-extensions](https://github.com/facelessuser/pymdown-extensions).


Updates `pymdown-extensions` from 10.21.2 to 10.21.3
- [Release notes](https://github.com/facelessuser/pymdown-extensions/releases)
- [Commits](facelessuser/pymdown-extensions@10.21.2...10.21.3)

---
updated-dependencies:
- dependency-name: pymdown-extensions
  dependency-version: 10.21.3
  dependency-type: direct:development
  dependency-group: uv
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* chore: bump version to 0.4.0 for release

* chore(deps): bump idna in the uv group across 1 directory (#130)

Bumps the uv group with 1 update in the / directory: [idna](https://github.com/kjd/idna).


Updates `idna` from 3.11 to 3.15
- [Release notes](https://github.com/kjd/idna/releases)
- [Changelog](https://github.com/kjd/idna/blob/master/HISTORY.md)
- [Commits](kjd/idna@v3.11...v3.15)

---
updated-dependencies:
- dependency-name: idna
  dependency-version: '3.15'
  dependency-type: indirect
  dependency-group: uv
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* docs: fix v0.4.0 release doc bugs flagged on PR #131 (#132)

- rich-value-pattern.md: legacy shape removal is 0.5.0, not 0.4.0
  (matches rich_value_helper.py docstring and the 0.4.0 deprecation
  shim — 0.4.0 introduces the shim, 0.5.0 removes it)
- confidence-evaluation.md, confidence-metrics.md: cast budget to float
  before formatting with :.0%. ECARB budget keys are strings ("0.10")
  for JSON round-trip, so the previous snippet would TypeError when
  copied verbatim.

* feat: add BERTComparator example script (#111) (#120)

Co-authored-by: Heinrich Kreuser <heikreus@amazon.com>

* ci: split security scan PR comment into workflow_run trigger for fork PR support (#125)

PR comments from fork-triggered pull_request runs were failing with 403
because GITHUB_TOKEN is read-only for fork PRs. Move the comment step to
a separate workflow_run workflow that executes in the base repo context
with write permissions, and upsert the comment to avoid duplicates.

* fix: use innerHTML instead of textContent in createElement helper (#92)

* fix: use innerHTML instead of textContent in createElement helper

The createElement helper named its third parameter 'innerHTML' but
assigned it via textContent, causing HTML strings (filter controls,
table rows, PDF navigation) to render as escaped text instead of
being interpreted as HTML.

* refactor: hoist escapeHtml helper to module scope

Move escapeHtml from inside updateDocumentFiles to module scope
next to createElement, making it accessible to all functions.

* fix: escape interpolated data in innerHTML to prevent XSS

Wrap user-controlled values with escapeHtml in updateNonMatchesTable
and loadPDF to prevent cross-site scripting via doc_id, field_path,
and other interpolated fields.

* fix: build PDF nav via DOM + addEventListener and unescape .id assignments

Co-authored-by: Simone Gaiarin <simgunz@gmail.com>

---------

Co-authored-by: Aditya Addepalli <adiadd@amazon.com>

* test: implement 8 LLMComparator placeholder unit tests (#114)

* test: implement 8 LLMComparator unit tests

Remove @pytest.mark.skip decorators and rewrite the 8 placeholder test
bodies to target the current strands-agents-based LLMComparator API.

The stubs were written against an older BedrockRuntime/invoke_model
pattern. The comparator now uses an eagerly-initialized strands Agent,
accepts `model` + `eval_guidelines` (not `model_name`/`temperature`/
`client`/`prompt_template`), and HTML-escapes values before rendering
the Jinja2 prompt template.

Each of the 8 tests covers a distinct scenario not already exercised
by the existing implemented tests:
- test_init: attribute shape at construction time
- test_init_with_client: init with a Model object (not a string ID)
- test_client_initialization: Agent eagerly initialized with correct kwargs
- test_compare_values_equal: "true" response yields 1.0; values in prompt
- test_compare_values_not_equal: "false" response yields 0.0; values in prompt
- test_compare_with_special_values: HTML-special chars in values are escaped
- test_compare_with_custom_prompt: eval_guidelines escaped + in <guidelines>
- test_compare_exception_handling: exceptions propagate; comparator recovers

Also removes the now-unused `import json` to keep the module clean.

Closes #109

* test: address PR #114 review — dedupe and tighten LLMComparator tests

Apply option 1 from the review thread: keep the rewritten stub tests,
drop pre-existing duplicates, and tighten assertions per the inline
comments.

- pin test_init to _default_system_prompt() and the parsing invariant
- rename test_init_with_client → test_init_with_model_instance
- scope test_compare_with_special_values negative to Value 1/2 lines
- rename test_compare_with_custom_prompt → test_compare_escapes_eval_guidelines
  with a regex pinning the escaped value inside <guidelines>...</guidelines>
- swap RuntimeError for the module's NoCredentialsError so we hit the
  dedicated except branch; recovery now uses 'true' → 1.0
- delete duplicates: test_no_match, test_case_variations,
  test_default_initialization, test_agent_initialization,
  test_prompt_template_with_guidelines, test_agent_exception_handling,
  test_error_recovery_after_exception, test_no_credentials_error_handling
- add fixture comment about the shared mock_agent reference
- fix typo in module docstring

Co-authored-by: Yarizakura <126939795+CrepuscularIRIS@users.noreply.github.com>

* test: parametrize true/false response tests across case and whitespace

---------

Co-authored-by: Aditya Addepalli <adiadd@amazon.com>

* docs: update structuredmodel refactoring documentation

* docs: update structuredmodel architecture documentation

* 🙈 chore: ignore .claude and working directories (#147)

* chore(deps): bump aiohttp in the uv group across 1 directory (#145)

---
updated-dependencies:
- dependency-name: aiohttp
  dependency-version: 3.14.0
  dependency-type: indirect
  dependency-group: uv
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* chore(deps): bump starlette in the uv group across 1 directory (#148)

Bumps the uv group with 1 update in the / directory: [starlette](https://github.com/Kludex/starlette).


Updates `starlette` from 1.0.0 to 1.0.1
- [Release notes](https://github.com/Kludex/starlette/releases)
- [Changelog](https://github.com/Kludex/starlette/blob/main/docs/release-notes.md)
- [Commits](Kludex/starlette@1.0.0...1.0.1)

---
updated-dependencies:
- dependency-name: starlette
  dependency-version: 1.0.1
  dependency-type: indirect
  dependency-group: uv
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* fix: log semantic embedding fallback failures (#119)

* fix: log semantic embedding fallback failures

* fix: include semantic fallback log context

* feat: export comparators from top-level stickler package (#110) (#121)

* feat: export comparators from top-level stickler package (#110)

* fix: broaden BERT gate to ImportError, finish import sweep, harden export tests

---------

Co-authored-by: Heinrich Kreuser <heikreus@amazon.com>
Co-authored-by: Aditya Addepalli <adiadd@amazon.com>

* chore(deps): bump the uv group across 1 directory with 2 updates (#153)

Bumps the uv group with 2 updates in the / directory: [torch](https://github.com/pytorch/pytorch) and [pyjwt](https://github.com/jpadilla/pyjwt).


Updates `torch` from 2.11.0 to 2.12.0
- [Release notes](https://github.com/pytorch/pytorch/releases)
- [Changelog](https://github.com/pytorch/pytorch/blob/main/RELEASE.md)
- [Commits](pytorch/pytorch@v2.11.0...v2.12.0)

Updates `pyjwt` from 2.12.1 to 2.13.0
- [Release notes](https://github.com/jpadilla/pyjwt/releases)
- [Changelog](https://github.com/jpadilla/pyjwt/blob/master/CHANGELOG.rst)
- [Commits](jpadilla/pyjwt@2.12.1...2.13.0)

---
updated-dependencies:
- dependency-name: torch
  dependency-version: 2.12.0
  dependency-type: direct:production
  dependency-group: uv
- dependency-name: pyjwt
  dependency-version: 2.13.0
  dependency-type: indirect
  dependency-group: uv
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* Date Comparator (#141)

* feat(comparators): add DateComparator with comprehensive date matching

- Add DateComparator implementation with semantic date parsing and
  comparison
- Support configurable matching tiers: surface-form normalization,
  partial dates, date ranges
- Add date_requirements.md specification document detailing all matching
  behaviors and edge cases
- Implement configurable options: allow_partial_year,
  partial_match_score, range_match_score, range_endpoints_score,
  two_digit_year_pivot, dayfirst, case_insensitive_textual,
  strip_weekday_prefix, warn_on_corrupted_input
- Wire DateComparator into ComparatorRegistry and comparators package
  exports
- Declare python-dateutil as a direct dependency
- Add comprehensive test coverage (143 tests) for date parsing,
  normalization, range matching, and edge cases
- Add example script demonstrating DateComparator usage patterns
- Update comparators documentation and navigation to include
  date-comparator guide
- Handle year hallucination, zero-padding, separator normalization,
  locale-aware parsing, and data quality warnings
- Grounded in empirical analysis of 6,630 real date-field failures from
  document extraction evaluation

* docs(comparators): add Date_Comparator_Demo notebook

Walks through DateComparator section-by-section: surface-form
normalization, partial-year handling, range scoring under all four
range_mode policies, two-digit-year resolution, dayfirst locale, edge
cases, and a StructuredModel example with three date fields each tuned
for its own situation. Includes a configuration cheat-sheet and a brief
note on the empirical grounding (6,630 real failures).

Notebook executes end-to-end against the implementation. Each numeric
claim in the prose was verified against the actual cell output (Jaccard
math uses textual months for unambiguous parsing, two-digit-year row
choices avoid the 50-year sliding-window boundary).

* test(date): lock in JSON-schema config flow for IDP integration

DateComparator was already JSON-configurable through both
StructuredModel.model_from_json (the dict flow) and
StructuredModel.from_json_schema (the x-aws-stickler-* extension flow
used by the IDP accelerator), but neither path had test coverage.

Adds 35 tests covering:
- ComparatorRegistry registration and create_comparator() factory
- DateComparator's `config` property round-trip (timedelta -> int days,
  whole vs sub-day, JSON-serializable, omits zero-tolerance)
- model_from_json() propagating every comparator_config option
- from_json_schema() propagating x-aws-stickler-comparator-config across
  every option (tolerance, dayfirst, allow_partial_year, range_mode)
- Realistic IDP invoice scenario end-to-end via both entry points
- Error pathway: invalid range_mode / dayfirst surface with field
  attribution at schema-load time

Also adds Section 8b to the Date_Comparator_Demo notebook showing the
schema-driven flow alongside the Python-class flow, and drops the
extraneous date_requirements.md design doc.

* feat(comparators): add precision_mode parameter to DateComparator

- Add `precision_mode` parameter to control how month/day resolution mismatches are scored (`Jan 2024` vs `Jan 1, 2024`)
- Implement three precision modes: `"exact"` (default, strict), `"gt_loose"` (partial credit), and `"overlap"` (range-based scoring)
- Update DateComparator documentation with new parameter, behavior matrix, and configuration examples
- Add comprehensive test coverage for precision_mode across all three modes and edge cases
- Update API reference and configuration guide to reflect new parameter
- Document two-digit year handling using python-dateutil's sliding 50-year window
- Clarify relationship between comparator threshold and ComparableField threshold in documentation
- Update dependencies in pyproject.toml and uv.lock
- Add precision_mode support to JSON schema configuration round-trip serialization

* fix(date): gate range-vs-single by precision_mode; fix doc row and lint

---------

Co-authored-by: Aditya Addepalli <adiadd@amazon.com>

* fix: prevent script injection in security workflow PR number step (#154)

* feat: mAP scoring for bounding box evaluation (#89) (#151)

* feat: add BBoxIoUComparator for bounding box similarity via IoU

Computes Intersection over Union between two bounding boxes as a
similarity score. Supports two-point ([[x1,y1],[x2,y2]]) and flat
([x1,y1,x2,y2]) formats with coordinate normalization. Registered in
the comparator registry and exported from stickler.comparators.

* feat: add MAPCalculator and BBoxMAPAccumulator for mAP scoring

Adds mean Average Precision scoring for bounding box localization,
built on the rich-value (_bbox) pattern and the PostComparisonAccumulator
interface.

- MAPCalculator: extract_from_dicts() joins field_comparisons with gt/pred
  bounding boxes into keyed pairs; compute_metrics() produces per-field
  IoU/precision/recall/F1/AP plus overall mean AP and coverage. Mirrors
  ConfidenceCalculator's calculator/accumulator split.
- BBoxMAPAccumulator: PostComparisonAccumulator implementation
  (name 'bbox_map_metrics') for bulk evaluation, with get/load/merge_state
  for checkpointing and distributed runs.
- comparison_engine: pre-extracts ground_truth_bboxes and prediction_bboxes
  into the result (mirrors prediction_confidences) so the accumulator can
  join both sides without the model instances; adds a single-document
  add_bbox_metrics sanity-check path.
- structured_model: forwards add_bbox_metrics and bbox_iou_threshold.

* test: add comprehensive tests for bbox mAP scoring

43 tests covering BBoxIoUComparator IoU math and edge cases,
MAPCalculator extraction/metrics/coverage, BBoxMAPAccumulator
accumulation and state round-trip/merge, bulk evaluator integration
(including coexistence with ConfidenceAccumulator), and the
single-document add_bbox_metrics path.

* docs: add bounding box mAP metrics documentation

Adds the Advanced/bbox-map-metrics.md page (accumulator-first usage with
the _bbox rich-value key, plus single-document sanity-check path), wires
it into the Advanced nav, and documents BBoxIoUComparator in the
Comparators guide.

* fix: guard non-finite bbox coords and drop dead margin_percent param

- Reject NaN/inf coordinates in _normalize_bbox (return None -> scores as a
  miss) so they no longer return nan from compare() and leak into JSON output.
- Remove the margin_percent parameter, its config serialization, and the
  docstring describing a snap correction that was never implemented; update the
  comparators guide that told users to configure a no-op.
- Document the zero-area-box (point annotation) IoU=0.0 behavior.

Addresses review: bbox.py:49, bbox.py:109.

* fix: correct GT/pred bbox join and compute real Average Precision

Blocking review fixes (map_calculator.py:166, :221).

GT/pred join (B1):
- Look up ground-truth boxes by the GT-side expected_key and prediction boxes
  by the prediction-side actual_key, which diverge once Hungarian matching
  reorders list items. The previous code joined both by actual_key, so
  reordered-but-correct list items scored mean_ap=0.0.
- FN rows (actual_key None, often reported at the object level) now record a
  localization miss for every GT bbox at or under the expected_key prefix, so
  missed ground-truth boxes deflate recall instead of being dropped.
- Group observations by a list-index-normalized class key
  (LineItems[2].StartDate -> LineItems[].StartDate) so AP is per field-type.

Real AP (B2):
- Replace ap = precision * recall with true Average Precision: rank a field's
  predicted boxes by _confidence, label TP/FP at the IoU threshold, and
  integrate the precision-recall curve (Pascal VOC 2010+ all-points). mean_ap
  macro-averages per-field AP over fields with at least one GT box.
- BBoxObservation now carries has_gt/has_pred/iou/confidence; accumulator state
  and the prediction-confidence join updated accordingly.
- Drop the useless prediction_raw fallback (GT boxes are unrecoverable from it)
  and the dead KeyedBBoxPairsState alias.

Addresses review: bbox_map_accumulator.py:78, :137.

* fix: harden add_bbox_metrics edge cases in compare_with

- Warn when add_bbox_metrics is combined with evaluator_format=True, which
  rebuilds the result from a fixed key set and silently drops bbox_metrics.
- Guard an empty field_comparisons list (e.g. a model whose only list field is
  empty on both sides) and return a coverage-only result instead of letting
  MAPCalculator.extract raise ValueError.
- Collapse the two in-function MAPCalculator imports into one module-level
  import (no circular-import risk).

Addresses review: comparison_engine.py:394, :419, :431.

* test: cover real AP, list-field join, FN misses, and edge cases

- Confidence-ranking AP tests (asymmetric fixtures distinguishing orderings).
- Reordered list-item test proving mean_ap=1.0 (the join-correctness regression)
  and a missing-item test proving the recall miss is recorded.
- NaN/inf coordinate, evaluator_format warning, empty-list coverage-only, and
  JSONL update_from_comparison_result round-trip parity.
- Strengthen state round-trip to assert known values; class_key normalization.
- Drop the margin_percent config assertions.

* docs: document real AP, dual FP+FN counting, and macro-averaging

- Describe Average Precision as the confidence-ranked PR-curve area (VOC
  2010+), the _confidence requirement, and the no-confidence fallback.
- State that a below-threshold detection counts as both a false positive and a
  false negative (the example numbers depend on it).
- Document the macro-average-over-classes denominator, list-index normalization,
  and how mean_ap differs from coverage.
- Update the result-structure example to the new per-field keys; drop the
  margin_percent row from the comparators guide.

Addresses review: bbox-map-metrics.md:188, map_calculator.py:261.

* docs: note bool coordinate acceptance in BBoxIoUComparator

bool is an int subclass, so True/False pass coordinate validation as 1/0.
Document it for users working with point annotations.

Addresses review nit: bbox.py.

* refactor: move bbox mAP modules into models/bbox subpackage

Mirror the sibling models/confidence package layout:
- map_calculator.py -> bbox/calculator.py
- bbox_map_accumulator.py -> bbox/accumulator.py
- add bbox/__init__.py re-exporting the public API (MAPCalculator,
  BBoxMAPAccumulator, BBoxObservation, etc.)
- add bbox/README.md and the previously-missing models/README.md per AGENTS.md.

Imports updated in comparison_engine.py and the tests. No behavior change.

Addresses review nit (models/bbox subpackage + README).

* test: add multi-detection AP curve tests with hand-derived VOC values

Strengthen Average Precision coverage beyond the 2-detection cases:
- FP,TP,TP (num_gt=2): asserts AP=2/3 and that it exceeds the
  non-interpolated value (~0.583), proving the VOC precision envelope is
  applied rather than a plain step sum.
- TP,FP,TP,FP,TP (num_gt=3): a richer alternating curve hand-derived to
  34/45, exercising the curve integration across multiple recall steps.

Note: not cross-checked against sklearn.average_precision_score on purpose
- that computes a non-interpolated AP and differs from VOC 2010+ all-points
(0.583 vs 0.667 on the first case).

* feat: COCO-style multi-threshold mAP with 101-point interpolation

Align AP computation with the pycocotools / torchmetrics standard:

- 101-point interpolation: apply the precision envelope ('remove zig-zags')
  then average precision sampled at 101 fixed recall points, instead of the
  previous exact-area integration. Matches torchmetrics on the envelope (and
  so deliberately differs from sklearn's non-interpolated AP).
- IoU-threshold range: AP is computed per (field-type class, IoU threshold)
  and mean_ap now averages over a configurable range, defaulting to the COCO
  set [0.50, 0.55, ..., 0.95]. Adds map_50 / map_75 and iou_thresholds to the
  result; per-field entries gain ap_50 / ap_75 (ap is the over-range mean).
- MAPCalculator/BBoxMAPAccumulator take iou_thresholds (float or iterable);
  compare_with takes bbox_iou_thresholds. Raw iou+confidence are already stored
  per observation, so re-thresholding needs no data-model change.

Note: the 'mean' in mAP is the average over classes (field-types), which we
already did; this adds the separate COCO averaging over the IoU range.

* test: cover COCO 101-point AP and multi-threshold mAP

- AP unit tests updated to the (confidence, iou) + threshold signature and
  COCO 101-point values (Case A 2/3 unchanged; Case B 0.7564; ranking
  51/101 vs 25.5/101); envelope assertion (> sklearn-style 0.583) retained.
- New multi-threshold test: a detection with IoU 0.7 is TP at 5/10 COCO
  thresholds -> mean_ap 0.5, map_50 1.0, map_75 0.0.
- New single-threshold test: map_75 is None when 0.75 isn't configured.
- Missing-item list test updated to the 101-point value (51/101).

* docs: describe COCO 101-point AP and IoU-threshold-range mAP

Update the mAP docs and bbox package README: AP now matches pycocotools /
torchmetrics (precision envelope + 101-point sampling), mean_ap averages over
the COCO IoU range with map_50 / map_75, and the result-structure example and
iou_thresholds usage reflect the new API.

* docs: note metric scope (COCO AP definition, not many-to-many detection)

Clarify that the metric matches COCO's AP definition (envelope + 101-point +
IoU range) but is not a full object-detection evaluator: each document field
carries one GT and one predicted box, paired by field path, with no
many-to-many box assignment per image.

* docs: showcase bbox mAP across the docsite (v2 review)

Address the v2 review's docs requests so the new functionality is visible:

- API-Reference/comparators.md: add the missing BBoxIoUComparator block.
- Guides/Evaluation/README.md: document add_bbox_metrics / bbox_iou_thresholds
  in the compare_with parameter table, and add an 'Evaluating bounding boxes?'
  tip callout mirroring the confidence one.
- Advanced/rich-value-pattern.md: flip bbox from 'future' to supported, drop the
  get_field_bbox() promise (the calculator reads boxes from get_all_extras()),
  and add a See Also link.
- Advanced/README.md: add the bbox-map-metrics.md entry to Contents.
- index.md: mention bounding-box (IoU) in the comparators card.

Also annotate bbox_iou_thresholds / iou_thresholds params (Union[float,
Iterable[float]]) so they render in the API docs, and add a comment noting the
unconditional bbox pre-extraction is deliberate (the bulk accumulator path
reads the stashed maps without setting add_bbox_metrics).

* style: ruff format two dict comprehensions in bbox calculator

* docs: list BBoxIoUComparator in remaining comparator catalogs

Second pass for bbox coverage beyond the docs site:

- Top-level README.md: add BBoxIoUComparator to the 'Available Comparators'
  table (it's registered and usable by string name) and mention IoU matching in
  the comparator overview sentence.
- src/stickler/comparators/Comparators.md: add a Spatial Comparators section for
  BBoxIoUComparator and list it under Best Practices.
- Getting-Started overview: include bounding-box IoU in the comparator list.

* fix: keep bbox coverage ratio consistent for object-level FN rows

---------

Co-authored-by: Divya Bhargavi <dbharga@amazon.com>
Co-authored-by: Aditya Addepalli <adiadd@amazon.com>

* chore: bump version to 0.5.0 for release

---------

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: Vincil Bishop <vincilb@amazon.com>
Co-authored-by: vawsgit <147627358+vawsgit@users.noreply.github.com>
Co-authored-by: Spencer Romo <sromo@amazon.com>
Co-authored-by: Spencer Romo <136653019+sromoam@users.noreply.github.com>
Co-authored-by: regularizer <26374641+regularizer@users.noreply.github.com>
Co-authored-by: Sirajus Salekin <salekin@amazon.com>
Co-authored-by: Simone Gaiarin <simgunz@gmail.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Erick Aleman <erick@eacognitive.com>
Co-authored-by: Heinrich <48674623+HeinrichWizardKreuser@users.noreply.github.com>
Co-authored-by: Heinrich Kreuser <heikreus@amazon.com>
Co-authored-by: Yarizakura <126939795+CrepuscularIRIS@users.noreply.github.com>
Co-authored-by: guyua9 <haiming2zhao@gmail.com>
Co-authored-by: Divya-Bhargavi <vbhargavi@dons.usfca.edu>
Co-authored-by: Divya Bhargavi <dbharga@amazon.com>
adiadd added a commit that referenced this pull request Jul 16, 2026
adiadd added a commit that referenced this pull request Jul 16, 2026
…pt-injection fix from #154)

# Conflicts:
#	.gitignore
#	uv.lock
adiadd added a commit that referenced this pull request Jul 16, 2026
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