Feature/logging bug fixes#105
Merged
Merged
Conversation
Canvas notify_type was setting msg_level to 'USER', causing canvas results to render with the red USER colour scheme. Set msg_level to 'CANVAS' and add a matching style branch in log_to_stdout. Fixes #90
The file result branch in StdoutLogger.log dereferenced
message.get('user').get('email') unguarded. The file worker
explicitly sets user to None when no user can be resolved
(watchman_processor.py:246, 262), causing AttributeError. Coerce
the user payload via (message.get('user') or {}) before reading
display_name and email so both lookups handle a missing or None
user safely.
Fixes #91
* develop: fix(loggers): drop pointless retry on log_to_stdout exception (#92)
The notify_type branches in StdoutLogger.log were mutually exclusive but each was an independent `if`, so every branch's equality check ran on every log call. Switch to an `elif` chain so only the matching branch is evaluated. Add a parameterised regression test that locks in the routing for each notify_type. Fixes #94
A formatting failure inside StdoutLogger.log_to_stdout used to call sys.exit(1) when debug mode was enabled, killing the entire scan over a single bad log line. Drop the sys.exit; the traceback is still printed via traceback.print_exc() in debug mode and the scan continues. Add a regression test that triggers the formatting error path and asserts sys.exit is not invoked. Fixes #95
The colourising regexes used by StdoutLogger.log_to_stdout were re.compile()'d on every invocation. Move them to module-level constants (_TYPE_COLORER, _HEADER_WORDS) so the patterns are compiled once at import time. Add a regression test that patches re.compile and asserts it is not called during a log emission. Fixes #96
The 13 elif branches in StdoutLogger.log_to_stdout each set the same three colour variables to one value plus a symbol. Replace with a module-level _LEVEL_STYLES dict mapping msg_level -> (color, style, symbol). Adding a new level is now a single dict entry instead of four near-identical lines. Add a parameterised regression test that locks in the colour mapping for every existing level. Fixes #97
… JSONLogger
JSONLogger inherited from logging.Logger but never used the base
class — every emission went through self.logger, which is fetched
from logging.getLogger('Slack Watchman'). Because that returns a
process-wide singleton, each new JSONLogger instance appended another
StreamHandler, causing N-fold duplicate output when multiple
instances exist (e.g. across tests).
Drop the Logger base class and guard addHandler so the singleton
keeps a single handler regardless of how many JSONLogger instances
are created. Also remove the now-unused `from logging import Logger`
import. Add regression tests covering both behaviours.
Fixes #98
JSONLogger.log compared the incoming level against 'WORKSPACE_PROBE_INFORMATION' but the caller in __init__.py:186 passes 'WORKSPACE_PROBE'. Probe results silently fell through to the catch-all else branch and were emitted as CRITICAL using the generic info_format, so structured consumers saw the wrong level and a flattened payload. Match the actual level string so the workspace_probe_format formatter is used. Add a regression test that asserts the right formatter is selected and logger.critical is not called. Fixes #99
JSONLogger.log routed every non-success, non-info level through the catch-all else, which always called self.logger.critical(msg). That meant warnings and errors were emitted as CRITICAL in JSON output, diverging from StdoutLogger and silently losing severity for SIEM consumers. Add explicit WARNING/ERROR/CRITICAL branches that call the matching logger.warning/error/critical method, so the %(levelname)s in info_format reflects the real level. Fixes #100
Every branch of JSONLogger.log called self.handler.setFormatter with one of eight pre-baked formatters. The mutation is not thread-safe and forced every code path to remember to set the formatter. Replace with a single _JSONFormatter subclass that builds the JSON envelope from record.msg and the log_level / scope / severity / detection_type fields supplied via `extra`. The formatter is installed once at __init__ time. Drop the eight `*_format` attributes. JSON schema is unchanged for every existing level. Update affected tests and add regression coverage for the WORKSPACE_PROBE envelope and the no- formatter-mutation invariant. Fixes #101
export_csv used to swallow every exception via `except Exception as e: print(e)` and return None unconditionally. The callers in __init__.py logged 'Users output to CSV file: ...' regardless, so a failed write looked successful to the user. Make export_csv return a bool. Both callers now log SUCCESS only on True and ERROR on False. Also corrected the channels success message which was mislabelled 'Users output to CSV file'. Fixes #102
dataclasses.asdict(export_data[0]) raises IndexError when the list is empty, but the bare `except Exception` swallowed it and reported the failure only as a stray `print(e)` line. Add an explicit empty-input guard at the top of export_csv that returns False without opening a file, and add a regression test that asserts open() is not called for an empty list. Fixes #103
The `with open(...) as f:` context manager already closes the file on exit. The trailing f.close() was a no-op. Add a regression test that asserts close() is not invoked explicitly after the with block. Fixes #104
Adds the missing docstring flagged by pylint (C0116). Brings loggers.py to a 10.00/10 pylint score.
- Add an autouse fixture that resets the process-wide 'Slack Watchman' logger handlers before and after each test, so JSONLogger handler state cannot leak between tests. - Hoist inline `import logging`, `import dataclasses` statements to the module level. - Replace the shallow test_export_csv with test_export_csv_writes_header_and_rows_in_order, which asserts open()/writeheader/writerow are called with the right arguments and rows are written in input order. Share a _CsvRow dataclass across the export_csv tests. - Cover the previously-missed branches in StdoutLogger.log: the DEBUG short-circuit, dataclass→dict conversion, the 'result' message branch (mapping/string user, public channel), the 'result' branch with neither message nor file, and the failure path that swallows log_to_stdout exceptions. - Cover log_to_stdout's non-debug formatting-error fallback. - Add JSONLogger tests for DEBUG routing, the NOTIFY envelope shape (scope/severity/detection_type/detection_data), the existing-handler reuse path, and explicit assertions on debug=True/False level. - Add a smoke test for StdoutLogger.print_header. Coverage on src/slack_watchman/loggers.py: 87% -> 100% (lines and branches).
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.
Summary
src/slack_watchman/loggers.py:StdoutLogger.logcorrectness (Canvas colour, file results with no user, retry-on-failure, debug-modesys.exit),JSONLoggercorrectness(broken
Loggerinheritance, handler duplication,WORKSPACE_PROBEsilent miss,WARNING/ERROR/CRITICALmislabelling), andexport_csvcorrectness (silent write failure, empty-input crash, redundantf.close()).StdoutLogger.log_to_stdout(13 colourelifs →_LEVEL_STYLESdict, regex compile hoisted to module level),StdoutLogger.log(cascade ofifs →elif), andJSONLogger.log(per-callsetFormattermutation → single
_JSONFormatterreading level fromextra).Slack Watchmanlogger between tests, the previously-shallowtest_export_csvnow asserts open args and row order, and 14 new tests bringloggers.pycoverage from 87% to 100% (lines and branches).