Repository navigation
fix(gti-mcp): fix collection/file bugs from #327 - #335
Conversation
dandye
left a comment
There was a problem hiding this comment.
Thanks for picking up #327, @VISHNU0906!
Could you please split this work into two separate PRs so we can land the bug fixes right away?
- This PR (#335): Bug fixes 1 to 3 only (
get_collection_rulesrule_typesfilter,get_entities_related_to_a_filereturn annotation, andget_collections_commonalitiesmissing-dataguard, plus their unit tests). - A separate PR: Scoped response trimming (
VULNERABILITY_EXCLUDED_ATTRS,FILE_BEHAVIOUR_EXCLUDED_ATTRS,FILE_BEHAVIOR_SUMMARY_EXCLUDED_ATTRS,COLLECTION_COMMONALITIES_FILES_EXCLUDED_ATTRS,utils.remove_fields, and the trimming tests). The field-trimming tradeoffs need a bit more design discussion on what gets dropped vs. truncated, and we can have that conversation directly on the new PR without blocking the bug fixes here.
Also, once split, please change Fixes #327 in this PR's description to Part of #327 so merging the bug fixes does not auto-close the issue before the response-trimming PR is resolved.
Requested changes on the bug fixes in this PR
-
get_entities_related_to_a_filetest and docstring (server/gti/tests/test_files_errors.py,server/gti/gti_mcp/tools/files.py):- Calling
await files.get_entities_related_to_a_file(...)directly intest_get_entities_related_to_a_file_invalid_relationship_returns_error_dictbypasses FastMCP'soutputSchema/ structured-output validation, meaning the test would still pass even if theUnion[...]return annotation were reverted. Please invoke the tool viaawait server.call_tool("get_entities_related_to_a_file", {"hash": "abc", "relationship_name": "not_a_real_relationship", "descriptors_only": True})so the regression test exercises FastMCP's structured-output validation path end-to-end. - Minor: please update the
Returns:docstring onget_entities_related_to_a_fileto note"or an error dictionary"(matchingget_collection_rules).
- Calling
-
get_collections_commonalitiesguard (server/gti/gti_mcp/tools/collections.py,server/gti/tests/test_collections_errors.py):if "data" not in data:still lets{"data": None}(or a non-dict top-level payload) fall through:utils.sanitize_response(None)returnsNone, which raisesAttributeErrorinsideutils.parse_collection_commonalities.- Please tighten this check to
if not isinstance(data, dict) or not isinstance(data.get("data"), dict):and add an assertion covering{"data": None}intest_collections_errors.py.
The get_collection_rules fix and strengthened sigma_lookups / yara_lookups assertions in test_get_collection_rules_filter_types look good as-is. Once this PR is narrowed to the three bug fixes with the two updates above, we can get it merged quickly!
Fix the broken rule_types filter in get_collection_rules, widen the get_entities_related_to_a_file return annotation so invalid relationships return a clean error dict through FastMCP outputSchema validation, and guard missing/non-dict data in get_collections_commonalities. Signed-off-by: KVR Vishnu <46535652+VISHNU0906@users.noreply.github.com>
ddc5a57 to
71bc251
Compare
|
Thanks @dandye — addressed as requested:
Scoped response trimming is in #337 (stacked on this branch; happy to discuss drop vs truncate there). |
dandye
left a comment
There was a problem hiding this comment.
Thanks for the quick turnaround and for splitting this out cleanly, @VISHNU0906!
The three bug fixes look good, and the server.call_tool and {"data": None} tests cover the exact failure paths from #327. Merging this now, and I will follow up on the response-trimming discussion over in #337.
Hi @dandye — narrowing this PR to the three bug fixes from #327 (response trimming moved to a follow-up PR).
Fixes
get_collection_rules: correct therule_typesfilter toif rule_type not in rule_types or not rules:so unrequested types with non-empty aggregations are skipped.get_entities_related_to_a_file: widen the return annotation toUnion[List[Dict], Dict[str, str]]so an invalid relationship returns a clean error dict instead of failing FastMCP structured-output validation. Regression test now goes throughserver.call_toolsooutputSchemais exercised; docstring notes "or an error dictionary".get_collections_commonalities: guard withif not isinstance(data, dict) or not isinstance(data.get("data"), dict):(covers missingdataand{"data": None}) and return a clean error string.Tests
rule_typesfilter regression (mutation-checked).server.call_tool.Nonedatacommonalities paths.server/gtiunit suite: 61 passed.Part of #327