Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughRemoved CircleCI and AppVeyor configs; added GitHub Actions workflows for CI and release. Migrated packaging from setuptools/setup.py/tox/MANIFEST.in to pyproject.toml with Hatch/uv dynamic versioning and a nox-based test/e2e/lint setup. Repository rebranded to deepnote-sshtunnel (import name remains sshtunnel); sshtunnel.py dropped Python‑2 compatibility and restricted host-key support to RSA/ECDSA/Ed25519. Tests modernized: ephemeral SSH key fixtures, generated ssh config, session-scoped Docker-based e2e infrastructure (SSH + PostgreSQL/MySQL/MongoDB), and new e2e tests exercising SSHTunnelForwarder. Removed static e2e artifacts and older test keys. Sequence Diagram(s)sequenceDiagram
participant Runner as Test runner
participant KeyGen as Key generator (paramiko)
participant Docker as Docker engine
participant SSH as SSH container
participant Postgres as PostgreSQL container
participant MySQL as MySQL container
participant Mongo as MongoDB container
participant Tunnel as SSHTunnelForwarder
Runner->>KeyGen: create ephemeral RSA host key + sshd_config
Runner->>Docker: create shared network
Docker->>Postgres: start postgres (alias: pg)
Docker->>MySQL: start mysql (alias: mysql)
Docker->>Mongo: start mongo (alias: mongo)
Docker->>SSH: start sshd container mounting keys/config
SSH-->>Runner: emit readiness logs
Runner->>Tunnel: construct tunnel with SSH host/port/key and remote binds
Tunnel->>SSH: establish SSH connection and remote port forwards
Runner->>Postgres: connect via local forwarded port and run query
Runner->>MySQL: connect via local forwarded port and run query
Runner->>Mongo: connect via local forwarded port and run query
Runner->>Tunnel: stop tunnel
Docker->>SSH: stop sshd container
Docker->>Postgres: stop postgres container
Docker->>MySQL: stop mysql container
Docker->>Mongo: stop mongo container
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@e2e_tests/conftest.py`:
- Around line 120-125: The extra time.sleep(2) after wait_for_logs indicates the
LogMessageWaitStrategy may not guarantee full readiness; remove the
unconditional sleep and instead perform explicit readiness checks: keep using
wait_for_logs(postgres/mysql/mongo, LogMessageWaitStrategy(...)) but follow each
with an active health check—e.g., attempt a TCP connect to ssh's port for the
ssh container (reference: ssh), and simple service-level checks for
postgres/mysql/mongo (attempt a DB TCP connection or run a lightweight query) to
confirm readiness; alternatively, if those checks are not feasible, add a clear
comment explaining why time.sleep(2) is required and what exact condition it’s
compensating for.
- Line 137: The shutil.rmtree call using ignore_errors=True can silently swallow
permission or filesystem errors for tmp_key_dir; change this to explicitly
handle failures by removing ignore_errors=True and wrapping the call in a
try/except around shutil.rmtree(tmp_key_dir) (or keep the call but remove the
flag), log the exception using the test/module logger or call pytest.fail with
the error message, and finally assert or check that tmp_key_dir no longer exists
(e.g., os.path.exists(tmp_key_dir) is False) so CI surfaces cleanup failures;
reference the shutil.rmtree(tmp_key_dir) invocation and tmp_key_dir variable
when making the change.
In `@e2e_tests/test_db_tunnel.py`:
- Around line 13-18: SSHTunnelForwarder is currently created without specifying
local_bind_addresses so it defaults to 0.0.0.0; update the SSHTunnelForwarder
instantiation to include local_bind_addresses set to loopback (e.g.,
local_bind_addresses=('127.0.0.1',) or [ ('127.0.0.1',) ]) alongside the
existing remote_bind_addresses to ensure the forwarded ports are bound to
127.0.0.1 only.
- Around line 29-35: Add client-side timeouts to the DB connections to avoid
long hangs: for the psycopg2 connection call (the psycopg2.connect invocation in
e2e_tests/test_db_tunnel.py) add connect_timeout=10; for the Mongo clients (the
pymongo.MongoClient constructions in this file) add
serverSelectionTimeoutMS=10000, connectTimeoutMS=10000 and socketTimeoutMS=10000
so they mirror the MySQL behavior and fail fast on connection issues.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 94a6221d-4adb-4495-80a5-5101e7212e94
⛔ Files ignored due to path filters (1)
e2e_tests/ssh-server-config/ssh_host_rsa_key.pubis excluded by!**/*.pub
📒 Files selected for processing (30)
.circleci/config.yml.github/workflows/ci.yml.github/workflows/database.yml.github/workflows/release.yml.gitignoreMANIFEST.inREADME.rstappveyor.ymle2e_tests/__init__.pye2e_tests/conftest.pye2e_tests/docker-compose.yamle2e_tests/run_docker_e2e_db_tests.pye2e_tests/run_docker_e2e_hangs_tests.pye2e_tests/ssh-server-config/ssh_host_rsa_keye2e_tests/ssh-server-config/sshd_confige2e_tests/test_db_tunnel.pye2e_tests/test_hang.pynoxfile.pypyproject.tomlsetup.cfgsetup.pysshtunnel.pytests/conftest.pytests/requirements-syntax.txttests/requirements.txttests/test_forwarder.pytests/testconfigtests/testrsa.keytests/testrsa_encrypted.keytox.ini
💤 Files with no reviewable changes (17)
- tests/requirements.txt
- tox.ini
- MANIFEST.in
- .github/workflows/database.yml
- e2e_tests/ssh-server-config/ssh_host_rsa_key
- e2e_tests/docker-compose.yaml
- tests/requirements-syntax.txt
- e2e_tests/ssh-server-config/sshd_config
- tests/testrsa.key
- tests/testconfig
- appveyor.yml
- e2e_tests/run_docker_e2e_db_tests.py
- tests/testrsa_encrypted.key
- setup.py
- e2e_tests/run_docker_e2e_hangs_tests.py
- setup.cfg
- .circleci/config.yml
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/ci.yml:
- Around line 35-40: Replace direct use of a globally-installed nox from "uv
tool install 'nox[uv]'" + plain "nox -s ..." invocations with uvx one-off
invocations to ensure deterministic execution; specifically, in the steps named
"Install nox" and "Run tests (paramiko ${{ matrix.paramiko-version }})" (and the
analogous step at lines 58–63) stop relying on PATH and invoke uvx --from
'nox[uv]' to run nox (e.g., use uvx --from 'nox[uv]' nox -s "tests-${{
matrix.python-version }}(paramiko='${{ matrix.paramiko-version }}')" instead of
the current two-step install + plain nox call).
In `@pyproject.toml`:
- Around line 74-77: The pyproject build target includes an invalid packages =
["."] for a single-file module; remove the packages = ["."] entry and rely only
on include = ["sshtunnel.py"] in the [tool.hatch.build.targets.wheel] section so
the wheel builds the top-level module correctly (keep the include line and
delete the packages line).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: d8c5b439-8db6-483a-948b-f2b7215da800
📒 Files selected for processing (4)
.github/workflows/ci.ymlpyproject.tomltests/conftest.pytests/test_forwarder.py
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@e2e_tests/conftest.py`:
- Line 114: The bind mount at .with_volume_mapping(str(tmp_key_dir),
"/config/ssh_host_keys") causes the container entrypoint chown to fail because
the host-owned files are not writable by container UID; fix by either making the
host dir owned by UID 1000 before mounting (e.g., call os.chown(tmp_key_dir,
1000, 1000) / ensure permissions permit container chown) or change the mount to
a writable bind (use the explicit bind mode string like
.with_volume_mapping(f"{str(tmp_key_dir)}:rw", "/config/ssh_host_keys") or use a
tmpfs instead) so the SSH container can successfully chown the keys.
In `@sshtunnel.py`:
- Around line 34-37: The except block that sets the package fallback version
should match pyproject.toml's fallback-version; update the except handler so
that when _get_version("deepnote-sshtunnel") fails it assigns __version__ =
"0.0.1" (instead of "0.0.0"), keeping the version source consistent; locate the
try/except around _get_version and change only the fallback string for
__version__.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: ecab7134-7f8b-4d3a-bd38-c5e3e65ecdac
📒 Files selected for processing (7)
.github/workflows/ci.ymle2e_tests/conftest.pye2e_tests/test_db_tunnel.pypyproject.tomlsshtunnel.pytests/conftest.pytests/test_forwarder.py
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/release.yml:
- Line 6: The tag pattern in the release workflow is using regex-style
characters (`[0-9]+.[0-9]+.[0-9]+*`) which GitHub Actions treats as a glob, so
update the tag pattern in .github/workflows/release.yml to a proper glob that
matches semantic version tags (for example use a glob like v*.*.* or *.*.*) so
tags like 1.2.3 or v1.2.3 will trigger the workflow; modify the value under the
tags key in the release workflow accordingly.
In `@e2e_tests/conftest.py`:
- Around line 79-152: Temp key directory tmp_key_dir may not be removed if an
exception occurs before the yield; wrap the container setup and waits in a
try/finally (or use a context manager like TemporaryDirectory) so that
shutil.rmtree(tmp_key_dir, ignore_errors=True) is executed in the finally block;
ensure the cleanup runs regardless of failures when calling
_generate_ssh_keypair, _generate_sshd_config, entering Network(), or while
waiting on containers (references: tmp_key_dir, _generate_ssh_keypair,
_generate_sshd_config, Network(), yield, shutil.rmtree).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: dc6c06b2-e7d2-463b-b2da-2d3dfa20679e
📒 Files selected for processing (2)
.github/workflows/release.ymle2e_tests/conftest.py
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
sshtunnel.py (1)
1608-1625:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
_bindlistbreaks documented UNIX-socket-Linputs.The parser always returns
(ip, port), so/tmp/foo.sockbecomes('/tmp/foo.sock', 22)instead of a socket path string.Suggested fix
def _bindlist(input_str): """Define type of data expected for remote and local bind address lists - Returns a tuple (ip_address, port) whose elements are (str, int) + Returns either: + - tuple (ip_address, port), or + - UNIX socket path (str) for local bind usage """ try: + if "/" in input_str and ":" not in input_str: + return input_str ip_port = input_str.split(":") if len(ip_port) == 1: _ip = ip_port[0] _port = None🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@sshtunnel.py` around lines 1608 - 1625, _bindlist currently always parses inputs into (ip, port) tuples which misinterprets UNIX-socket paths like "/tmp/foo.sock"; modify _bindlist to detect socket-path inputs (e.g., input_str contains '/' or startswith '/') and return the raw path string instead of a (ip, port) tuple, otherwise keep existing parsing logic (splitting on ":" and defaulting port to "22") and the argparse.ArgumentTypeError on invalid numeric ports; update the return type behavior in _bindlist so callers can receive either a str (UNIX socket path) or a (str, int) tuple for host:port.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@sshtunnel.py`:
- Around line 1467-1473: The __str__ implementation is including raw
self.ssh_password in the credentials dict which can leak secrets; update the
code that builds credentials (the credentials dict created alongside calls to
_remove_none_values) to never insert the plaintext password—either remove the
"password" key or replace its value with a redacted token (e.g., "REDACTED" or a
masked string) before calling _remove_none_values so logs/str output cannot
expose self.ssh_password; keep the pkeys behavior unchanged and ensure the
change is applied inside the same method that constructs credentials for
__str__.
- Around line 191-195: The current code unconditionally extends
pywarnings.handlers with logger.handlers
(pywarnings.handlers.extend(logger.handlers)), which can duplicate handlers on
repeated setup; replace this with a safe add that avoids duplicates—iterate over
logger.handlers and for each handler call pywarnings.addHandler(handler) only if
that handler is not already present in pywarnings.handlers (or compare by
identity) so repeated calls to the capture_warnings/capture_warnings block won’t
register the same handler multiple times; update the block that references
capture_warnings, pywarnings, and logger to perform this guarded add instead of
extend.
---
Outside diff comments:
In `@sshtunnel.py`:
- Around line 1608-1625: _bindlist currently always parses inputs into (ip,
port) tuples which misinterprets UNIX-socket paths like "/tmp/foo.sock"; modify
_bindlist to detect socket-path inputs (e.g., input_str contains '/' or
startswith '/') and return the raw path string instead of a (ip, port) tuple,
otherwise keep existing parsing logic (splitting on ":" and defaulting port to
"22") and the argparse.ArgumentTypeError on invalid numeric ports; update the
return type behavior in _bindlist so callers can receive either a str (UNIX
socket path) or a (str, int) tuple for host:port.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 2e695faf-e83f-4b50-a9ee-9d3c85bbecd4
📒 Files selected for processing (3)
.github/workflows/release.ymle2e_tests/conftest.pysshtunnel.py
mfranczel
reviewed
May 12, 2026
mfranczel
left a comment
mfranczel
left a comment
There was a problem hiding this comment.
Please consider updating changelog as well
mfranczel
approved these changes
May 12, 2026
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
2 participants
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 by CodeRabbit
Chores
Documentation
New Features
Bug Fixes