Conversation
PIIProblem never got the `level` attribute that show_problems() reads for --no-warnings, so the flag crashed with AttributeError as soon as a file had a finding. Findings now have level "error" (score 1.0) or "warning", the same split the colored output already used. show_problems() always returned 0 and run() overwrote its result for every file, so the CLI exited with 0 even when it reported PII. show_problems() now returns the number of findings it printed and run() adds them up, so the exit code is 1 when any finding is reported. -f github printed `::<score> file=...`, which is not a workflow command, so GitHub Actions never created annotations. It now prints ::warning and ::error commands with escaped property values and message, and drops the ./ prefix that `presidio .` adds to file paths. The problems test fixture used Mock objects, which create any attribute on access and so hid the missing `level`; it now builds real PIIProblem objects.
PresidioCLIConfig.parse() range-checked the current threshold (the default 0) instead of the configured value, and only converted the configured value afterwards. `threshold: 5` was accepted and filtered out every finding, and `threshold: abc` raised an uncaught ValueError. The configured value is now converted and range-checked before it is stored, and invalid values raise PresidioCLIConfigError. This also covers YAML booleans such as `true`, which float() would accept as 1.0, and integers too large for a float.
Contributor
There was a problem hiding this comment.
🟢 Approval recommended
The requested fixes are covered by tests and documentation with no unresolved blocking issues.
Pull request overview
Fixes presidio-cli warning filtering, exit codes, GitHub Actions annotations, and threshold validation.
Changes:
- Adds severity classification and correct problem counting.
- Formats and escapes GitHub annotations.
- Validates configured thresholds.
- Updates tests and documentation.
File summaries
| File | Description |
|---|---|
presidio-cli/tests/test_config.py |
Tests threshold validation. |
presidio-cli/tests/test_cli.py |
Tests output and exit-code behavior. |
presidio-cli/tests/test_analyzer.py |
Tests severity classification. |
presidio-cli/README.md |
Documents updated behavior. |
presidio-cli/presidio_cli/config.py |
Validates threshold values. |
presidio-cli/presidio_cli/cli.py |
Fixes filtering, exit codes, and GitHub output. |
presidio-cli/presidio_cli/analyzer.py |
Adds finding severity classification. |
Review details
- Files reviewed: 7/7 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
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
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.
Change Description
Fixes four presidio-cli bugs described in #2265:
--no-warningscrash:PIIProblemnow haslevel:"error"when the score is 1.0,"warning"otherwise. This is the splitFormat.standard_coloralready used, and it now readsproblem.level.show_problems()returns the number of problems it printed (after--no-warningsfiltering), andrun()adds these up across files and stdin, so the existingif prob_num > 0check works.-f github: prints::warning/::errorworkflow commands, with property values and the message escaped the way@actions/coredoes it (%, CR, LF, plus:and,in properties). The./prefix thatpresidio .adds is removed from thefileproperty, asrun()already does beforeanalyze(). Example:::error file=pii.txt,line=2,col=51::2:51 [CREDIT_CARD] score=1.0. Escaping also means a file name with a newline can no longer print its own workflow command into the log.threshold: the configured value is converted and range-checked before it is stored. Out-of-range values (threshold: 5), non-numbers (abc), YAML booleans (true, whichfloat()would read as 1.0) and integers too large for a float now raisePresidioCLIConfigErrorinstead of being accepted silently or crashing.Tests: the Mock-based
problemsfixture, which hid the missing attribute, now uses realPIIProblem/RecognizerResultobjects. Tests that asserted the old return value and exit code (rc == 0, exit 0 on a workspace with a card number) are updated. Each bug has new tests that fail on main and pass with this change; the color-split and valid-threshold tests are regression guards that pass on both. README: corrected github example, plus new "Warnings and errors" and "Exit codes" sections.Behavior changes
presidionow exits 1 when it reports at least one finding, warnings included (it always exited 0 before). CI jobs or hooks that run it will fail on findings;presidio . || truekeeps the old behavior. If you would rather follow yamllint (exit 1 only for errors, warnings exit 0 unless--strict), I can change it.--no-warningsprints only score-1.0 findings (before, it crashed on the first finding).-f github(andautoon GitHub Actions) output changes from::<score> file=...::L:C [TYPE]to::warning|error file=...::L:C [TYPE] score=Sand now creates annotations. For stdin the file property is stillstdin; that can be a follow-up.thresholdis now a config error (exit 1 with a message).standard,coloredandparsableoutput text is unchanged.Scoring 1.0 as "error" is a judgment call that follows the existing colored output. I can make it configurable if you prefer. The two commits are independent (the config threshold fix is the second one) and can be split into separate PRs if that is easier to review.
Issue reference
Fixes #2265
Checklist