codeant-ai-for-open-source[bot] commented on code in PR #40673:
URL: https://github.com/apache/superset/pull/40673#discussion_r3400924576
##########
tests/unit_tests/extensions/ssh_test.py:
##########
@@ -14,11 +14,41 @@
# KIND, either express or implied. See the License for the
# specific language governing permissions and limitations
# under the License.
-from unittest.mock import Mock
+from unittest.mock import Mock, patch
+import paramiko
+import pytest
import sshtunnel
-from superset.extensions.ssh import SSHManagerFactory
+from superset.commands.database.ssh_tunnel.exceptions import (
+ SSHTunnelHostKeyVerificationError,
+)
+from superset.extensions.ssh import SSHManager, SSHManagerFactory
+
+
+def _make_manager(strict: bool = False) -> SSHManager:
+ app = Mock()
+ app.config = {
+ "SSH_TUNNEL_MAX_RETRIES": 2,
+ "SSH_TUNNEL_LOCAL_BIND_ADDRESS": "127.0.0.1",
+ "SSH_TUNNEL_TIMEOUT_SEC": 123.0,
+ "SSH_TUNNEL_PACKET_TIMEOUT_SEC": 321.0,
+ "SSH_TUNNEL_MANAGER_CLASS": "superset.extensions.ssh.SSHManager",
+ "SSH_TUNNEL_STRICT_HOST_KEY_CHECKING": strict,
+ }
+ return SSHManager(app)
+
+
+def _authorized_key(key: paramiko.PKey) -> str:
Review Comment:
**Suggestion:** Add a docstring explaining that this helper converts a
`paramiko` key into authorized_keys text format. [custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
This helper is newly added and lacks a docstring.
Under the stated rule, new Python functions should be documented inline, so
this is a genuine violation.
</details>
[Fix in
Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=3f758ceb253b4476bd3f0acf5091f3e9&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
| [Fix in VSCode
Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=3f758ceb253b4476bd3f0acf5091f3e9&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** tests/unit_tests/extensions/ssh_test.py
**Line:** 42:42
**Comment:**
*Custom Rule: Add a docstring explaining that this helper converts a
`paramiko` key into authorized_keys text format.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40673&comment_hash=75ae12dffecbc72df2d50c3bbfaf36b8c00ce9dad704e568d71ece5d25f4cca0&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40673&comment_hash=75ae12dffecbc72df2d50c3bbfaf36b8c00ce9dad704e568d71ece5d25f4cca0&reaction=dislike'>👎</a>
##########
tests/unit_tests/extensions/ssh_test.py:
##########
@@ -14,11 +14,41 @@
# KIND, either express or implied. See the License for the
# specific language governing permissions and limitations
# under the License.
-from unittest.mock import Mock
+from unittest.mock import Mock, patch
+import paramiko
+import pytest
import sshtunnel
-from superset.extensions.ssh import SSHManagerFactory
+from superset.commands.database.ssh_tunnel.exceptions import (
+ SSHTunnelHostKeyVerificationError,
+)
+from superset.extensions.ssh import SSHManager, SSHManagerFactory
+
+
+def _make_manager(strict: bool = False) -> SSHManager:
+ app = Mock()
+ app.config = {
+ "SSH_TUNNEL_MAX_RETRIES": 2,
+ "SSH_TUNNEL_LOCAL_BIND_ADDRESS": "127.0.0.1",
+ "SSH_TUNNEL_TIMEOUT_SEC": 123.0,
+ "SSH_TUNNEL_PACKET_TIMEOUT_SEC": 321.0,
+ "SSH_TUNNEL_MANAGER_CLASS": "superset.extensions.ssh.SSHManager",
+ "SSH_TUNNEL_STRICT_HOST_KEY_CHECKING": strict,
+ }
+ return SSHManager(app)
+
+
+def _authorized_key(key: paramiko.PKey) -> str:
+ return f"{key.get_name()} {key.get_base64()}"
+
+
+def _ssh_tunnel(server_host_key: str | None) -> Mock:
Review Comment:
**Suggestion:** Add a docstring clarifying that this helper creates a mocked
SSH tunnel object populated with server connection fields for tests.
[custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
The function is newly introduced and does not include a docstring.
That matches the custom rule requiring new functions and classes to be
documented inline.
</details>
[Fix in
Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=64ce50d057ec4c25acdbd8b16a90841b&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
| [Fix in VSCode
Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=64ce50d057ec4c25acdbd8b16a90841b&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** tests/unit_tests/extensions/ssh_test.py
**Line:** 46:46
**Comment:**
*Custom Rule: Add a docstring clarifying that this helper creates a
mocked SSH tunnel object populated with server connection fields for tests.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40673&comment_hash=b06846dde7d12f8c0f8d23b56d7899985d012faf84602cf2bf4ccf02b567ec87&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40673&comment_hash=b06846dde7d12f8c0f8d23b56d7899985d012faf84602cf2bf4ccf02b567ec87&reaction=dislike'>👎</a>
##########
tests/unit_tests/extensions/ssh_test.py:
##########
@@ -34,3 +64,141 @@ def test_ssh_tunnel_timeout_setting() -> None:
factory.init_app(app)
assert sshtunnel.TUNNEL_TIMEOUT == 123.0
assert sshtunnel.SSH_TIMEOUT == 321.0
+
+
+@patch("superset.extensions.ssh.socket.create_connection")
+@patch("superset.extensions.ssh.paramiko.Transport")
+def test_verify_host_key_match(
+ mock_transport_cls: Mock, mock_create_connection: Mock
+) -> None:
Review Comment:
**Suggestion:** Add a test docstring summarizing the verification scenario
and expected behavior to satisfy the requirement that new functions are
documented inline. [custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
This is a newly added test function and it has no docstring.
The custom rule applies to all new Python functions, including tests, so the
suggestion is valid.
</details>
[Fix in
Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=e2fe4422563c480883f671663600320e&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
| [Fix in VSCode
Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=e2fe4422563c480883f671663600320e&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** tests/unit_tests/extensions/ssh_test.py
**Line:** 71:73
**Comment:**
*Custom Rule: Add a test docstring summarizing the verification
scenario and expected behavior to satisfy the requirement that new functions
are documented inline.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40673&comment_hash=93c43e96d3aa1715ad2eb8eb79800bc1c6d91e2f68079a20d880b13566f37376&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40673&comment_hash=93c43e96d3aa1715ad2eb8eb79800bc1c6d91e2f68079a20d880b13566f37376&reaction=dislike'>👎</a>
##########
tests/unit_tests/extensions/ssh_test.py:
##########
@@ -14,11 +14,41 @@
# KIND, either express or implied. See the License for the
# specific language governing permissions and limitations
# under the License.
-from unittest.mock import Mock
+from unittest.mock import Mock, patch
+import paramiko
+import pytest
import sshtunnel
-from superset.extensions.ssh import SSHManagerFactory
+from superset.commands.database.ssh_tunnel.exceptions import (
+ SSHTunnelHostKeyVerificationError,
+)
+from superset.extensions.ssh import SSHManager, SSHManagerFactory
+
+
+def _make_manager(strict: bool = False) -> SSHManager:
Review Comment:
**Suggestion:** Add a short docstring to this new helper function describing
that it builds an `SSHManager` test instance with configurable strict host-key
checking. [custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
This is a newly added Python function and it has no docstring in the current
file.
The rule requires new functions to be documented inline, so the suggestion
identifies a real violation.
</details>
[Fix in
Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=a9367acd02fa44a3ac2b39f0c4e13643&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
| [Fix in VSCode
Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=a9367acd02fa44a3ac2b39f0c4e13643&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** tests/unit_tests/extensions/ssh_test.py
**Line:** 29:29
**Comment:**
*Custom Rule: Add a short docstring to this new helper function
describing that it builds an `SSHManager` test instance with configurable
strict host-key checking.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40673&comment_hash=a791e249ea7c6ab877923543682cec8d8a5eb1c95a7dcf0817d170dcb2d69e9b&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40673&comment_hash=a791e249ea7c6ab877923543682cec8d8a5eb1c95a7dcf0817d170dcb2d69e9b&reaction=dislike'>👎</a>
##########
tests/unit_tests/extensions/ssh_test.py:
##########
@@ -34,3 +64,141 @@ def test_ssh_tunnel_timeout_setting() -> None:
factory.init_app(app)
assert sshtunnel.TUNNEL_TIMEOUT == 123.0
assert sshtunnel.SSH_TIMEOUT == 321.0
+
+
+@patch("superset.extensions.ssh.socket.create_connection")
+@patch("superset.extensions.ssh.paramiko.Transport")
+def test_verify_host_key_match(
+ mock_transport_cls: Mock, mock_create_connection: Mock
+) -> None:
+ # The server presents the same key we expect: verification passes.
+ server_key = paramiko.RSAKey.generate(2048)
+ manager = _make_manager(strict=False)
+ tunnel = _ssh_tunnel(_authorized_key(server_key))
+
+ transport = mock_transport_cls.return_value
+ transport.get_remote_server_key.return_value = server_key
+
+ manager._verify_host_key(tunnel) # should not raise
+
+ # The TCP connect is bounded by an explicit timeout, and the resulting
+ # socket is handed to Transport.
+ mock_create_connection.assert_called_once_with(
+ ("ssh.example.com", 22), timeout=321.0
+ )
+
mock_transport_cls.assert_called_once_with(mock_create_connection.return_value)
+ transport.start_client.assert_called_once()
+ transport.close.assert_called_once()
+
+
+@patch("superset.extensions.ssh.socket.create_connection")
+@patch("superset.extensions.ssh.paramiko.Transport")
+def test_verify_host_key_mismatch_raises(
+ mock_transport_cls: Mock, mock_create_connection: Mock
+) -> None:
Review Comment:
**Suggestion:** Add a docstring that states this test validates failure when
the presented host key does not match the configured expected key. [custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
This newly added function also lacks a docstring.
Since new Python functions must be documented inline, the suggestion
correctly flags a rule violation.
</details>
[Fix in
Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=5672a9b56e9f454296261f4089361b4e&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
| [Fix in VSCode
Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=5672a9b56e9f454296261f4089361b4e&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** tests/unit_tests/extensions/ssh_test.py
**Line:** 96:98
**Comment:**
*Custom Rule: Add a docstring that states this test validates failure
when the presented host key does not match the configured expected key.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40673&comment_hash=6075c44db376d632152ccfee2a1b2d9d66ffa6bd67dbf87942da99c41739a5e3&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40673&comment_hash=6075c44db376d632152ccfee2a1b2d9d66ffa6bd67dbf87942da99c41739a5e3&reaction=dislike'>👎</a>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
