Repository navigation
Conversation
|
Thank you for your contribution. Your generosity and caring are unrivaled! Make sure to register your contribution by filling the Contribution Registration form, so our content wizard @kamalq97 will know the proposed changes are ready to be reviewed. |
🤖 AI-Powered Code Review AvailableHi @kamalq97, you can leverage AI-powered code review to assist with this PR! Available Commands:
|
|
Partner details validated with @nkanon and labels added to PR |
fa0367a to
3e574ed
Compare
|
🤖 Analysis started. Please wait for results... |
🤖 AI Review DisclaimerThis review was generated by an AI-powered tool and may contain inaccuracies. Please be advised, and we extend our sincere apologies for any inconvenience this may cause. |
marketplace-ai-reviewer
left a comment
There was a problem hiding this comment.
Thanks for contributing this new integration!
Overall, the pack looks promising, but there are a few standard Cortex XSOAR requirements and code improvements that need attention:
- Code & Error Handling: Refine exception handling in the integration script to catch
Exceptioninstead ofBaseException, ensure tracebacks are logged viademisto.debug(), and safely parse the API key parameter. - YAML & Metadata Schema: Move the integration
name/displayto the top of the YAML file, update argument and parameter descriptions to adhere to style guidelines, and add the missingcreatedandgithubUserfields topack_metadata.json. - Documentation: Update the integration description and README with standard headings, instructions on obtaining API credentials, and explicit
Requiredvalues in the configuration table.
Please take a look at these updates, and let us know once you're ready for another review!
Additionally, please address the following file-level notes:
Packs/CommandZeroMCP/pack_metadata.json: - The mandatory keycreatedis missing frompack_metadata.json. Please add thecreatedfield with the pack creation timestamp in ISO 8601 format (e.g.,"created": "2024-01-01T00:00:00Z").- The
githubUserfield is missing. For partner-supported packs (support: partner), please add thegithubUserfield containing a list of GitHub usernames of the pack maintainers.
@idovandijk, @RotemAmit, @Ni-Knight, @Moish-Gilboa, @Shellyber, @DeanArbel, @melamedbn, @omerKarkKatz, @JasBeilin please review and approve the results generated by the AI Reviewer by responding 👍 on this comment.
| else: | ||
| raise NotImplementedError(f"Command {command} is not implemented") | ||
|
|
||
| except BaseException as eg: |
There was a problem hiding this comment.
- Every
exceptblock that reports an error should log the full traceback viademisto.debug(traceback.format_exc())to assist with debugging. In addition, catchExceptioninstead ofBaseExceptionto prevent catching system-exiting exceptions such asSystemExit(whichreturn_erroruses) orKeyboardInterrupt, and passerror=egtoreturn_error.
| except BaseException as eg: | |
| except Exception as eg: | |
| demisto.debug(traceback.format_exc()) |
- Catch
Exceptioninstead ofBaseException:
except Exception as eg:|
|
||
| client = None | ||
| try: | ||
| api_key = params.get("api_key", {}).get("password") or "" |
There was a problem hiding this comment.
Consider safely handling cases where api_key is None or a string to avoid an AttributeError:
api_key = params.get("api_key")
api_key = api_key.get("password", "") if isinstance(api_key, dict) else (api_key or "")| @@ -0,0 +1,77 @@ | |||
| category: Utilities | |||
There was a problem hiding this comment.
According to Cortex XSOAR integration guidelines, the first key defined at the root level of the .yml file must always be name or display. Currently, line 1 defines category: Utilities while display and name are declared on lines 51-52.
Please move name and display to the very top of the YAML file.
| display: Custom Server URL | ||
| type: 0 | ||
| advanced: true | ||
| additionalinfo: Overrides the Region parameter. Use only when instructed by Command Zero. |
There was a problem hiding this comment.
Parameter additionalinfo descriptions must be phrased descriptively, begin with "The ", and end with a period.
| additionalinfo: Overrides the Region parameter. Use only when instructed by Command Zero. | |
| additionalinfo: The custom server URL that overrides the Region parameter. Use only when instructed by Command Zero. |
| display: Custom headers | ||
| type: 12 | ||
| advanced: true | ||
| additionalinfo: |- |
There was a problem hiding this comment.
Parameter additionalinfo descriptions must begin with "The " and use descriptive phrasing.
| additionalinfo: |- | |
| additionalinfo: |- | |
| The custom headers to be sent with each request. | |
| Enter each header on a new line in the format: HeaderName: HeaderValue. |
| description: The name of the tool to call. | ||
| required: true | ||
| - name: arguments | ||
| description: Parameters for the tool execution. |
There was a problem hiding this comment.
Argument descriptions must start with "The " and end with a period.
| description: Parameters for the tool execution. | |
| description: The parameters for the tool execution. |
| @@ -0,0 +1,8 @@ | |||
| To configure the integration: | |||
There was a problem hiding this comment.
According to the General integration description guidelines, the description file should begin with a top-level header ## <Integration Name> (e.g., ## CommandZeroMCP). Please add this heading before the configuration instructions.
| To configure the integration: | ||
|
|
||
| 1. Select the Region that hosts your Command Zero organization. | ||
| 2. Enter your Command Zero API key. |
There was a problem hiding this comment.
Please provide step-by-step instructions or an official vendor link explaining how users can acquire the Command Zero API key (e.g., UI navigation paths in bold).
| | --- | --- | --- | | ||
| | Region | The region that hosts your Command Zero organization. | True | | ||
| | API Key | The Command Zero API key. The integration can perform only the actions allowed by the key's role. | True | | ||
| | Organization ID | The ID of the Command Zero organization to connect to. Required only if the API key has access to more than one organization. | | |
There was a problem hiding this comment.
The Required column in the configuration table must explicitly specify True or False. For optional parameters (such as Organization ID on line 9, Custom Server URL on line 10, Custom headers on line 11, and Trust any certificate (not secure) on line 12), please set the value to False instead of leaving the cell empty.
| | Organization ID | The ID of the Command Zero organization to connect to. Required only if the API key has access to more than one organization. | | | |
| | Organization ID | The ID of the Command Zero organization to connect to. Required only if the API key has access to more than one organization. | False | |
For more details, see Creating Documentation.
| | Organization ID | The ID of the Command Zero organization to connect to. Required only if the API key has access to more than one organization. | | | ||
| | Custom Server URL | Overrides the Region parameter. Use only when instructed by Command Zero. | | | ||
| | Custom headers | Add custom headers to be sent with each request.<br/>Enter each header on a new line in the format: HeaderName: HeaderValue. | | | ||
| | Trust any certificate (not secure) | | | |
There was a problem hiding this comment.
The parameter Trust any certificate (not secure) has an empty Description cell. Please add an additionalinfo field for this parameter in the integration YAML and regenerate the README using Run demisto-sdk generate-docs. Also ensure the Required column is set to False.
There was a problem hiding this comment.
Thanks for the contribution! The integration follows the same pattern as our existing MCP integrations (GitHubMCP, GenericMCP, NodeZeroMCP).
Two things should be fixed before merge: the Docker image version and confirming the MCP endpoint path. The rest are minor suggestions.
| script: '-' | ||
| type: python | ||
| subtype: python3 | ||
| dockerimage: demisto/mcp:1.0.0.11195141 |
There was a problem hiding this comment.
Please update the Docker image to demisto/mcp:1.0.0.13526390. All the other MCP integrations on master use it (GitHub, Cloudflare, Atlassian, ServiceNow, Generic, NodeZero).
MCPApiModule has changed a lot since this branch was created, so please test against the current image.
| dockerimage: demisto/mcp:1.0.0.11195141 | |
| dockerimage: demisto/mcp:1.0.0.13526390 |
|
|
||
|
|
||
| REGION_URLS = { | ||
| "North America": "https://mcp.cmdzero.io/", |
There was a problem hiding this comment.
Can you confirm that the Command Zero MCP server serves streamable HTTP at the root path (https://mcp.cmdzero.io/)? Other vendors use a path such as /mcp (for example https://mcp.horizon3ai.com/mcp and https://api.githubcopilot.com/mcp). If Command Zero does too, please update both region URLs, the .secrets-ignore, and the tests.
There was a problem hiding this comment.
Confirmed; the HTTP path for the MCP server is correct.
|
|
||
| client = None | ||
| try: | ||
| api_key = params.get("api_key", {}).get("password") or "" |
There was a problem hiding this comment.
Nit: if api_key is present but None, .get("password") raises an AttributeError, and the user never sees your "API key is required" message. NodeZeroMCP uses a safer pattern:
| api_key = params.get("api_key", {}).get("password") or "" | |
| api_key = (params.get("api_key") or {}).get("password") or "" |
| except BaseException as eg: | ||
| root_msg = extract_root_error_message(eg) | ||
| return_error(f"Failed to execute {command} command.\nError:\n{root_msg}") |
There was a problem hiding this comment.
Nit: consider logging the full traceback and passing the exception to return_error, the way NodeZeroMCP does. It makes customer issues much easier to debug from logs. Catching Exception rather than BaseException also avoids swallowing KeyboardInterrupt/SystemExit. (You'll also need import traceback at the top of the file.)
| except BaseException as eg: | |
| root_msg = extract_root_error_message(eg) | |
| return_error(f"Failed to execute {command} command.\nError:\n{root_msg}") | |
| except Exception as eg: | |
| demisto.debug(traceback.format_exc()) | |
| root_msg = extract_root_error_message(eg) | |
| return_error(f"Failed to execute {command} command.\nError:\n{root_msg}", error=eg) |
| 3. If the API key has access to more than one organization, enter the ID of the Command Zero organization to connect to. | ||
| 4. Click **Test** to verify that everything is configured correctly. | ||
|
|
||
| The integration can perform only the actions allowed by the API key's role. We recommend using an API key with the Observer role for read-only access. No newline at end of file |
There was a problem hiding this comment.
Nit: the file doesn't end with a newline, so the end-of-file-fixer pre-commit hook will flag it.
| Tool availability depends on the role of the Command Zero API key. | ||
| The list below reflects the tools available at the time this pack was created and may change. | ||
|
|
||
| `get_caller_identity` `list_integrations` `list_investigations` `get_investigation` `get_investigation_report` `create_investigation` `create_scratch_investigation` `set_investigation` `update_investigation` `search_questions` `list_questions` `list_questions_summary` `get_question` `list_categories` `ask_question` `get_invocation` `list_recent_invocations` `investigate_user` |
There was a problem hiding this comment.
Suggestion: this list mixes read-only tools with tools that make changes (create_investigation, create_scratch_investigation, set_investigation, update_investigation, ask_question). The integration description recommends the Observer role for read-only access, so it would help users to label each tool as Read or Write, the way the NodeZeroMCP README does. That way they know which tools need a more privileged key.
| "serverMinVersion": "8.13.0", | ||
| "author": "Command Zero", | ||
| "url": "https://cmdzero.io", | ||
| "email": "support@cmdzero.io", |
There was a problem hiding this comment.
Nit: partner packs usually include devEmail and githubUser in pack_metadata.json, plus a CONTRIBUTORS.json file (see Packs/NodeZeroMCP). Please add them so we know who to contact about maintenance.
| assert "Command unknown-command is not implemented" in mock_return_error.call_args[0][0] | ||
|
|
||
| @pytest.mark.asyncio | ||
| async def test_exception_handling(self, mocker: MockerFixture): |
There was a problem hiding this comment.
Nit: please add a test that client.close() still runs when a command fails, like NodeZeroMCP_test.py::test_client_closed_on_exception. The finally cleanup is one of the nicer parts of this integration, so it's worth locking it in.
|
@fasenaj Looks really great! Have you tested it thoroughly? Please attach a demo, such as a screen recording showing the authentication, the imported tools, and one tool being used. |
@mayyagoldman Thank you!
In regards to the docker image, we had issues when testing with the later versions. We attempted to fix this by pinning the last working version. I will update with the latest and attempt it again. |
mayyagoldman
left a comment
There was a problem hiding this comment.
@fasenaj yes pls attache a link here would be easiest :) Let me know if you managed to test it with the latest docker image version
@mayyagoldman Yes, the test with the latest docker was successful. The latest commit has the adjustment to use the stated image. |
Contributing to Cortex XSOAR Content
Make sure to register your contribution by filling the contribution registration form
The Pull Request will be reviewed only after the contribution registration form is filled.
Status
Description
Must have
relates: https://jira-dc.paloaltonetworks.com/browse/CIAC-18567