Skip to content

fix(pretty_print): restore stdout after printer failures. - #551

Merged
sromoam merged 3 commits into
awslabs:devfrom
mangeshraut712:fix/424-printer-stdout-restoration
Nov 13, 2026
Merged

sromoam merged 3 commits into
awslabs:devfrom
mangeshraut712:fix/424-printer-stdout-restoration

Conversation

@mangeshraut712

@mangeshraut712 mangeshraut712 commented Nov 9, 2026 •

Copy link
Copy Markdown
Contributor

Restores stdout after printer failures using context-managed redirection. File and terminal rendering now share a path, including the bulk summary in confusion-matrix file output. File-opening errors are reported separately from rendering errors.

Adds regression tests and a changelog entry. The separate HTML escaping work in #548 is outside this change.

Testing: merged current dev (7034ec0) in d0a4ee1, preserving the printer entry and all upstream changelog entries. The merge retains upstream HTML escaping. On Python 3.12.16, the dependency-enabled suite passed 3,526 tests (2 skipped, 1 xfailed); the separate Strands Evals integration suite passed 821 tests (1 xfailed). Repository-wide Ruff and whitespace checks pass. Independent AI review found no blocking merge issues. Hosted checks must validate the new head.

AI-assisted implementation, testing and review.

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

@mangeshraut712 mangeshraut712 changed the title Issue: Fixes #424 fix(pretty_print): restore stdout after printer failures. Nov 9, 2026

@vawsgit vawsgit 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.

✅ LGTM!

Verified at head a5b79b6, with stickler imported from that checkout's src:

  • The new tests give 17 passed. With dev's pretty_print.py and these tests, 10 of 17 fail: the stdout-restore tests for print_non_matches, print_evaluation_results and print_confusion_matrix_html, plus the bulk file-output and invalid-regex tests. The full suite gives 4118 passed, 13 skipped, 1 xfailed, as the PR body says.
  • All four items of #424 are delivered: stdout is restored in the three printers, and the bulk info reaches file output.
  • Mutations are caught: dropping the bulk info from file output fails 2 tests, and removing the open-error message fails 1.
  • Nested redirects (print_evaluation_results calling print_confusion_matrix and print_non_matches) write to the file correctly. No caller in src/ passes output_file, so making render errors raise breaks no internal caller.
  • No conflict with dev. With #548, pretty_print.py merges automatically, and both PRs' tests pass on the merged file (23 passed). Only CHANGELOG.md conflicts. Whichever PR merges second keeps both entries.

Non-blocking (low):

  • ruff format --check fails on pretty_print.py:1293 (line over 88 characters). CI runs only ruff check, so CI will not catch it.
  • The body does not say Closes #424, so merging leaves #424 open.
  • The CHANGELOG entry has no bold lead sentence and no #424 link, unlike the entries next to it.
  • The early returns for unrecognizable results (print_confusion_matrix_html, print_non_matches) leave an old output file in place. That predates this PR, but the new README's "same rendering path" line overstates it.

@github-actions

github-actions Bot commented Nov 12, 2026 •

Copy link
Copy Markdown

Security Scan Results

ASH Security Scan Report

  • Report generated: 2026-11-13T16:23:35+00:00
  • Time since scan: 2 minutes

Scan Metadata

  • Project: ASH
  • Scan executed: 2026-11-13T16:20:44+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 0 0 0 6738 0 0 PASSED 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 0 0 0 0 0 0 PASSED 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 3 0 0 0 0 0 0 PASSED MEDIUM (global)
syft 0 0 0 0 0 0 0 MISSING MEDIUM (global)

Report generated by Automated Security Helper (ASH) at 2026-11-13T16:23:35+00:00

@sromoam

sromoam commented Nov 13, 2026

Copy link
Copy Markdown
Contributor

Hi! I merged the latest dev into this branch so it can be merged. dev now requires a new Dependency cooldown check, and this branch was created before that check existed, so it was never going to report and the PR stayed blocked. No changes to your work; the merge just brings in the new CI. Once the checks are green I'll merge it. Thanks for your patience!

@sromoam
sromoam merged commit ae01626 into awslabs:dev Nov 13, 2026
11 checks passed
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.

3 participants