fix(secops): log investigation diagnostics instead of printing - #315
Open
abdeltaehass wants to merge 1 commit into
Open
fix(secops): log investigation diagnostics instead of printing#315abdeltaehass wants to merge 1 commit into
abdeltaehass wants to merge 1 commit into
Conversation
…e#267) stdout is the JSON-RPC channel on the MCP stdio transport, but all four tools in investigation_management.py wrote progress and error messages there with print(). Every other tool in the package already uses the module logger defined at the top of this file. The SDK wraps sys.stdout.buffer in its own TextIOWrapper rather than replacing sys.stdout, so print() and the JSON-RPC writer end up holding separate buffers on the same fd. sys.stdout block-buffers at 8KB when stdout is a pipe and flushes once it fills, which lands in the middle of a response. Running the server over stdio with 200 tool calls, 164 of 272 stdout lines came back unparseable; none do now. A single call never shows this, since the message just sits in the buffer and is dropped at exit, so today these messages reach nobody at all. Swap the 12 print() calls for logger.info(), and logger.error(..., exc_info=True) on the error paths to match the rest of the package. Message text is unchanged. Tests cover the success and failure path of each tool: stdout stays empty, the expected messages are logged at the expected level, and the error records carry the traceback.
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.
Fixes #267
investigation_management.pyreports progress and errors withprint(). Onthe MCP stdio transport stdout is the JSON-RPC channel, so those writes go
into the protocol stream.
The issue points at three calls in
list_investigations. There are twelve,across all four tools in the file —
list_investigations,get_investigation,trigger_investigationandfetch_associated_investigations. This routes allof them through the module logger the file already defines at line 22.
Why it breaks the stream
mcp/server/stdio.pywrapssys.stdout.bufferin its ownTextIOWrapperinstead of replacing
sys.stdout, soprint()and the JSON-RPC encoder holdseparate buffers on fd 1. When stdout is a pipe,
sys.stdoutblock-buffers at8KB and flushes whenever it fills, which is usually mid-response.
Driving the server over stdio with 200
list_investigationscalls:One call won't show it — the message sits in the buffer and gets dropped at
exit, which also means these diagnostics currently reach nobody. Sending them
through the logger puts them on stderr.
Changes
print()tologger.info(), message text unchanged.print(error_msg)tologger.error(error_msg, exc_info=True), which keepsthe traceback.
logger.error(..., exc_info=True)is used 38 times acrossthese tools and
logger.exceptionisn't used at all, so this matches.Return values are untouched.
Tests
server/secops/tests/test_investigation_management_unit.py, following thepatch+MagicMockfixture style intest_security_alerts_unit.py. Successand failure path per tool, eight tests. Each asserts stdout is empty, checks
the full sequence of (level, message) pairs from the logger, and on the failure
path checks the record carries
exc_infofor the right exception type.All eight fail against the current code. The full
server/secopssuite goesfrom 25 to 33 passing; the 52 errors are pre-existing and unrelated, they're
integration tests wanting a
tests/config.jsonwith live credentials.ruff check --isolated --select E9,F63,F7,F82,F401,T201and--select E501 --line-length 79are clean on both files.Out of scope
server/secops-soar/secops_soar_mcp/marketplace/has the same pattern in bulk(~6.4k calls over 294 files), but those look generated, so they're better
handled at the generator than by hand here.