Prevent hanging in svn+ssh#1230
Conversation
|
Warning Review limit reached
More reviews will be available in 41 minutes and 19 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (7)
WalkthroughThis PR centralizes non-interactive SVN-over-SSH handling: injects BatchMode=yes into SVN_SSH, detects SSH host-key verification failures and raises a dedicated error with ssh-keyscan/ssh -T hints, applies these helpers across SvnRemote and SvnRepo methods, and adds tests, docs, and config updates. ChangesNon-Interactive SVN SSH Mode
🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs:
Suggested labels: Suggested reviewers:
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@CHANGELOG.rst`:
- Line 23: Update the CHANGELOG line that currently reads "* Run ``svn+ssh://``
connections in non-interactive mode to prevent hanging (`#0`)" to reference the
correct issue/PR number; replace the placeholder "(`#0`)" with "(`#1230`)" so the
entry reads "...prevent hanging (`#1230`)". Ensure only the numeric reference is
changed and formatting (backticks and punctuation) is preserved.
In `@dfetch/vcs/svn.py`:
- Around line 37-55: The remediation message currently uses the output of
_ssh_target_from_url (which returns "user@host") for both commands; change
_raise_if_ssh_host_key_error to parse the URL (urlparse(url)) locally, build
host_only = parsed.hostname or url for the ssh-keyscan suggestion, and keep
target = _ssh_target_from_url(url) (user@host) for the ssh -T suggestion so the
ssh-keyscan line uses only the hostname/IP while the ssh -T line still shows
user@host.
In `@example/dfetch.yaml`:
- Line 10: Update the example near the url-base entry (url-base:
svn+ssh://svn.code.sf.net/p/) to add a brief note explaining that SourceForge
requires users to register an SSH public key in their account and to accept the
host's SSH key (add guidance to add the host key to ~/.ssh/known_hosts or run
ssh-keyscan for the host), and include a short smoke-test instruction to verify
the repository is reachable via svn+ssh (e.g., attempt an svn list/ls of the
tortoisesvn/code path) so reviewers know how to validate the ssh-key/known-hosts
setup non-interactively.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: a4056c4b-b3e0-4ab1-b83c-6b6a4029179a
📒 Files selected for processing (4)
CHANGELOG.rstdfetch/vcs/svn.pyexample/dfetch.yamltests/test_svn.py
spoorcc
force-pushed
the
svn-ssh-non-onteractive
branch
from
16d4f3a to
81387c8
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@tests/test_svn.py`:
- Around line 569-583: Add an autouse pytest fixture in this test module that
centralizes clearing the lru_cache for _extend_env_for_non_interactive_mode:
create a fixture (autouse=True) which imports
dfetch.vcs.svn._extend_env_for_non_interactive_mode and calls
_extend_env_for_non_interactive_mode.cache_clear() before each test (and
optionally after) so the cached process-global state is reset; then remove the
repetitive per-test imports and explicit
_extend_env_for_non_interactive_mode.cache_clear() calls from tests like
test_svn_ssh_env_has_batch_mode, test_svn_ssh_env_preserves_existing_batch_mode
and the *_passes_batch_mode_env tests.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 86c26433-10d8-4f73-b002-421224f1f319
📒 Files selected for processing (3)
CHANGELOG.rstdfetch/vcs/svn.pytests/test_svn.py
spoorcc
force-pushed
the
svn-ssh-non-onteractive
branch
2 times, most recently
from
e470253 to
d798bd0
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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 `@dfetch/project/svnsubproject.py`:
- Around line 165-169: The except block catching SubprocessCommandError should
preserve and log the underlying failure details; change the handler to capture
the exception (e.g., "except SubprocessCommandError as e:") and include the
revision and the exception/return-code information in the logger.warning call
for the externals-report skip (use the existing complete_path and the local
revision variable and e or e.returncode/str(e) as available), or include
exc_info=True to preserve traceback.
In `@dfetch/util/cmdline.py`:
- Around line 49-51: The inline Bandit suppression should be removed and the
root cause fixed in run_on_cmdline: stop relying on "# nosec" by ensuring
subprocess.run is invoked safely (use shell=False and a sequence for cmd or
parse cmd with shlex.split), validate or sanitize the cmd input (or enforce a
whitelist of allowed commands) before calling subprocess.run, and keep existing
arguments (env, capture_output, check, timeout) while removing the "# nosec"
comment next to subprocess.run so Bandit no longer flags an unsafe shell
invocation of cmd.
- Around line 52-58: The TimeoutExpired handler in run_on_cmdline currently
drops captured output; change the except block to extract exc.stdout and
exc.stderr (they may be bytes or None), decode bytes to str using
errors="replace", default to "" when None, and pass those decoded stdout/stderr
into the SubprocessCommandError constructor (retain the timeout message and exit
code 124) so partial output is preserved for diagnostics; reference
run_on_cmdline and SubprocessCommandError to locate and update the handler.
In `@dfetch/vcs/svn.py`:
- Around line 239-261: externals_from_url should handle SSH host-key
verification errors like other SVN helpers: wrap the run_on_cmdline call in
externals_from_url in a try/except that catches SubprocessCommandError and calls
_raise_if_ssh_host_key_error(logger, err) (or re-raise if not an SSH host-key
issue) before proceeding; ensure you still decode result.stdout and call
SvnRepo._normalize_url_prefix and SvnRepo._parse_externals after successful
run_on_cmdline.
- Around line 25-35: The cached function _extend_env_for_non_interactive_mode
currently returns a mutable dict which lru_cache will reuse across calls; change
it to return an immutable mapping so callers cannot mutate the cached object:
keep the caching but wrap the computed env in types.MappingProxyType (import
MappingProxyType from types) and update the return type annotation to
Mapping[str, str] (or similar typing.Mapping) so the cached value is an
immutable view of the environment.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: e6748593-13b3-4b61-a828-c15382bb17b7
📒 Files selected for processing (9)
CHANGELOG.rstdfetch/project/svnsubproject.pydfetch/util/cmdline.pydfetch/vcs/svn.pydoc/explanation/threat_model_usage.rstsecurity/tm_usage.pytests/test_cmdline.pytests/test_svn.pytests/test_svnsubproject.py
| except SubprocessCommandError: | ||
| logger.warning( | ||
| "Could not retrieve SVN externals from '%s' — skipping report.", | ||
| complete_path, | ||
| ) |
There was a problem hiding this comment.
Preserve the subprocess failure details in this warning.
This warning drops the reason externals reporting was skipped. Please include at least the revision plus the underlying SVN error/return code so operators can tell an expected timeout/host-key case from an unexpected repository/configuration failure.
🤖 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 `@dfetch/project/svnsubproject.py` around lines 165 - 169, The except block
catching SubprocessCommandError should preserve and log the underlying failure
details; change the handler to capture the exception (e.g., "except
SubprocessCommandError as e:") and include the revision and the
exception/return-code information in the logger.warning call for the
externals-report skip (use the existing complete_path and the local revision
variable and e or e.returncode/str(e) as available), or include exc_info=True to
preserve traceback.
| proc = subprocess.run( | ||
| cmd, env=env, capture_output=True, check=True, timeout=timeout | ||
| ) # nosec |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Remove the inline Bandit suppression from the subprocess wrapper.
This change still relies on # nosec in production code. Since run_on_cmdline is already the single subprocess boundary, fix the underlying Bandit concern here once instead of suppressing the call inline.
As per coding guidelines, dfetch/**/*.py: "Avoid inline lint suppressions (# noqa, # type: ignore, # pylint: disable, # pyright: ignore) without fixing the root cause; exception permitted for module-level tool headers (lines 1–5) in test files".
🤖 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 `@dfetch/util/cmdline.py` around lines 49 - 51, The inline Bandit suppression
should be removed and the root cause fixed in run_on_cmdline: stop relying on "#
nosec" by ensuring subprocess.run is invoked safely (use shell=False and a
sequence for cmd or parse cmd with shlex.split), validate or sanitize the cmd
input (or enforce a whitelist of allowed commands) before calling
subprocess.run, and keep existing arguments (env, capture_output, check,
timeout) while removing the "# nosec" comment next to subprocess.run so Bandit
no longer flags an unsafe shell invocation of cmd.
| except subprocess.TimeoutExpired as exc: | ||
| raise SubprocessCommandError( | ||
| list(exc.cmd) if isinstance(exc.cmd, (list, tuple)) else [str(exc.cmd)], | ||
| "", | ||
| f"Command timed out after {exc.timeout:.0f}s", | ||
| 124, | ||
| ) from exc |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the target file and nearby context
FILE="dfetch/util/cmdline.py"
echo "== $FILE =="
nl -ba "$FILE" | sed -n '1,140p'
echo
# Locate SubprocessCommandError definition
echo "== SubprocessCommandError definition/usages =="
rg -n "class SubprocessCommandError|SubprocessCommandError\(" -S dfetch/util/cmdline.py dfetch -g'*.py'
# Show the exact SubprocessCommandError class if found
matches=$(rg -n "class SubprocessCommandError" -S dfetch -g'*.py' || true)
if [ -n "$matches" ]; then
echo
echo "== Showing class SubprocessCommandError block =="
# Print file sections around each match
echo "$matches" | while read -r line; do
f=$(echo "$line" | cut -d: -f1)
ln=$(echo "$line" | cut -d: -f2)
start=$((ln-20))
end=$((ln+80))
nl -ba "$f" | sed -n "${start},${end}p"
echo
done
fi
# Check whether os is already imported in cmdline.py
echo "== Imports in dfetch/util/cmdline.py =="
rg -n "^import|^from" "$FILE"Repository: dfetch-org/dfetch
Length of output: 151
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="dfetch/util/cmdline.py"
echo "== File exists?"
test -f "$FILE" && echo "OK: $FILE" || (echo "MISSING: $FILE" && exit 1)
echo
echo "== $FILE (lines 1-140)"
cat -n "$FILE" | sed -n '1,140p'
echo
echo "== TimeoutExpired branch (exact matches)"
rg -n "except subprocess\.TimeoutExpired as exc" -n "$FILE" || true
rg -n "TimeoutExpired" "$FILE" || true
echo
echo "== SubprocessCommandError definition/constructor"
rg -n "class SubprocessCommandError|def __init__\(|SubprocessCommandError\(" -S "$FILE" || true
# Print block around the first class definition match, if present
class_line=$(rg -n "class SubprocessCommandError" "$FILE" | head -n1 | cut -d: -f1 || true)
if [ -n "${class_line:-}" ]; then
start=$((class_line-20))
end=$((class_line+120))
echo
echo "== Showing around SubprocessCommandError at ~line $class_line"
cat -n "$FILE" | sed -n "${start},${end}p"
fi
echo
echo "== Imports in $FILE"
rg -n "^(import|from )" "$FILE" || true
echo
echo "== SubprocessCommandError usages elsewhere (show a few)"
rg -n "SubprocessCommandError\(" -S dfetch -g'*.py' | head -n 20Repository: dfetch-org/dfetch
Length of output: 6117
🌐 Web query:
Python subprocess.TimeoutExpired attributes output stdout stderr capture_output run
💡 Result:
In Python, when using subprocess.run with a timeout parameter, a subprocess.TimeoutExpired exception is raised if the process does not terminate within the specified time [1][2]. Important details regarding this exception and output capture include: 1. Accessing Captured Output: When a TimeoutExpired exception is raised, you can access the output captured up to the point of the timeout via the exception's attributes [3]. Specifically: - stdout: Contains the standard output captured [1][2]. - stderr: Contains the standard error captured [1][2]. - output: An alias for stdout, provided for symmetry with stderr [1][2]. 2. Data Format: Regardless of whether you set text=True or encoding in your subprocess.run call, the stdout and stderr attributes on the TimeoutExpired exception will always be in bytes [1][2][4]. They will be None if no output was captured or if capture_output=True (or stdout=PIPE/stderr=PIPE) was not used [1][2]. 3. Usage Pattern: To access this data, you must catch the exception [3]. For example: try: result = subprocess.run(["your_command"], capture_output=True, text=True, timeout=5) except subprocess.TimeoutExpired as e: # Access bytes output here captured_stdout = e.stdout captured_stderr = e.stderr # If text=True was used, you may need to decode manually: # decoded_stdout = e.stdout.decode if e.stdout else None 4. Behavior with capture_output: When you use capture_output=True in subprocess.run, Python automatically configures stdout and stderr as pipes (stdout=PIPE and stderr=PIPE) [5]. If a timeout occurs, the subprocess library ensures that whatever data was already buffered in these pipes is attached to the exception [1][2]. Note: Before Python 3.5, the stdout and stderr attributes were not present on the TimeoutExpired exception [1][2]. Always ensure your code accounts for these attributes being bytes, even when operating in text mode [6][1].
Citations:
- 1: https://docs.python.org/3.10/library/subprocess.html
- 2: https://docs.python.org/3/library/subprocess.html
- 3: https://stackoverflow.com/questions/64640506/capture-output-from-subprocess-with-a-timeout-set
- 4: https://github.com/python/cpython/blob/0274d830/Doc/library/subprocess.rst
- 5: https://docs.python.org/release/3.11.5/library/subprocess.html
- 6: Subprocess timeout causes output to be returned as bytes in text mode python/cpython#87597
Preserve captured stdout/stderr on subprocess timeouts for better diagnostics.
run_on_cmdline uses subprocess.run(..., capture_output=True), and subprocess.TimeoutExpired can include the buffered stdout/stderr from the timed-out process. The current handler replaces stdout with "" and only keeps the timeout message, dropping the most useful partial output.
File: dfetch/util/cmdline.py
Lines: 52-58
except subprocess.TimeoutExpired as exc:
raise SubprocessCommandError(
list(exc.cmd) if isinstance(exc.cmd, (list, tuple)) else [str(exc.cmd)],
"",
f"Command timed out after {exc.timeout:.0f}s",
124,
) from exc
Suggested fix
except subprocess.TimeoutExpired as exc:
+ stdout = exc.output.decode(errors="replace").strip() if isinstance(exc.output, bytes) else (exc.output or "")
+ stderr = exc.stderr.decode(errors="replace").strip() if isinstance(exc.stderr, bytes) else (exc.stderr or "")
+ timeout_msg = f"Command timed out after {exc.timeout:g}s"
raise SubprocessCommandError(
- list(exc.cmd) if isinstance(exc.cmd, (list, tuple)) else [str(exc.cmd)],
- "",
- f"Command timed out after {exc.timeout:.0f}s",
+ list(exc.cmd) if isinstance(exc.cmd, (list, tuple)) else [str(exc.cmd)],
+ stdout,
+ f"{stderr}{os.linesep if stderr else ''}{timeout_msg}",
124,
) from exc🤖 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 `@dfetch/util/cmdline.py` around lines 52 - 58, The TimeoutExpired handler in
run_on_cmdline currently drops captured output; change the except block to
extract exc.stdout and exc.stderr (they may be bytes or None), decode bytes to
str using errors="replace", default to "" when None, and pass those decoded
stdout/stderr into the SubprocessCommandError constructor (retain the timeout
message and exit code 124) so partial output is preserved for diagnostics;
reference run_on_cmdline and SubprocessCommandError to locate and update the
handler.
d798bd0 to
f6f8068
Compare
spoorcc
force-pushed
the
svn-ssh-non-onteractive
branch
from
f6f8068 to
ce9644c
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
tests/test_svn.py (1)
577-584: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winVerify that existing SSH options are preserved.
The test correctly validates that
BatchMode=yesis not duplicated, but it doesn't verify that other SSH options (like-i /my/key) are preserved in the result. Consider adding an assertion to ensure the full original value is maintained.🔍 Suggested assertion
monkeypatch.setenv("SVN_SSH", "ssh -o BatchMode=yes -i /my/key") _extend_env_for_non_interactive_mode.cache_clear() env = _extend_env_for_non_interactive_mode() assert env["SVN_SSH"].count("BatchMode=yes") == 1 + assert "-i /my/key" in env["SVN_SSH"]🤖 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 `@tests/test_svn.py` around lines 577 - 584, The test test_svn_ssh_env_preserves_existing_batch_mode should also assert that existing SSH options are preserved; after calling _extend_env_for_non_interactive_mode(), add an assertion that env["SVN_SSH"] still contains the original key option (e.g. assert "-i /my/key" in env["SVN_SSH"] or assert env["SVN_SSH"].startswith("ssh -o BatchMode=yes -i /my/key") to ensure the full original value is maintained) while keeping the existing BatchMode=yes duplication check.dfetch/vcs/svn.py (1)
389-404:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFinish applying the centralized SSH host-key handling to the remaining remote entry points.
get_last_changed_revision()for remote targets,export(), andfiles_in_path()all passBatchMode=yes, but they still surface rawSubprocessCommandErroron host-key verification failures. Wrap theserun_on_cmdline()calls the same way asget_info_from_target()so callers get the same remediation path everywhere.Also applies to: 443-449, 454-460
🤖 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 `@dfetch/vcs/svn.py` around lines 389 - 404, get_last_changed_revision(), export(), and files_in_path() still call run_on_cmdline() directly and will raise raw SubprocessCommandError on SSH host-key verification failures; update each to mirror get_info_from_target() by wrapping their run_on_cmdline() calls in the same try/except used there (catch SubprocessCommandError), invoke the same remediation/host-key handling helper used by get_info_from_target(), and re-run or re-raise as appropriate so callers receive the centralized remediation path instead of raw subprocess errors; locate the run_on_cmdline invocations in get_last_changed_revision(), export(), and files_in_path() and apply the identical exception-handling pattern found in get_info_from_target().
♻️ Duplicate comments (4)
dfetch/vcs/svn.py (2)
48-54:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse a host-only target in the
ssh-keyscanhint.
_ssh_target_from_url()can returnuser@host, butssh-keyscanexpects the host portion. Keepuser@hostonly for thessh -Texample.Does `ssh-keyscan` accept targets in `user@host` form, or only hostnames/IP addresses?🤖 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 `@dfetch/vcs/svn.py` around lines 48 - 54, The ssh hint uses _ssh_target_from_url(url) which may return "user@host" but ssh-keyscan requires just the host; update the code to extract a host-only value (e.g., call or add a helper to parse host from url or strip the user prefix from the result of _ssh_target_from_url) and use that host-only string in the ssh-keyscan line, while keeping the original target (user@host) for the ssh -T example; adjust the two formatted f-strings in the RuntimeError construction around _ssh_target_from_url(url) accordingly (use host_only for the ssh-keyscan hint and the original target for the ssh -T hint).
238-241:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winHandle host-key failures in
externals_from_url().This
svn propgetcall now runs with the non-interactive SSH env, but it still skips_raise_if_ssh_host_key_error(), so missing known-hosts state leaks a rawSubprocessCommandErrorinstead of the new remediation message.♻️ Minimal fix
- result = run_on_cmdline(logger, cmd, env=_extend_env_for_non_interactive_mode()) + try: + result = run_on_cmdline( + logger, + cmd, + env=_extend_env_for_non_interactive_mode(), + ) + except SubprocessCommandError as exc: + _raise_if_ssh_host_key_error(url, exc) + raise🤖 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 `@dfetch/vcs/svn.py` around lines 238 - 241, The svn propget call in externals_from_url (the run_on_cmdline invocation) runs with non-interactive SSH but doesn't check for SSH host-key failures; after capturing the subprocess result, call SvnRepo._raise_if_ssh_host_key_error(result, url) (or the existing helper that inspects the subprocess result) before calling SvnRepo._normalize_url_prefix and SvnRepo._parse_externals so host-key issues produce the remediation message instead of a raw SubprocessCommandError.CHANGELOG.rst (1)
24-24:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate placeholder issue reference.
The changelog entry still references
(#0), which should be(#1230)based on the PR objectives. This issue was previously flagged and marked as addressed in commit 81387c8, but remains in the current code.📝 Proposed fix
-* Run ``svn+ssh://`` connections in non-interactive mode to prevent hanging (`#0`) +* Run ``svn+ssh://`` connections in non-interactive mode to prevent hanging (`#1230`)🤖 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 `@CHANGELOG.rst` at line 24, Update the placeholder issue reference in the changelog entry that reads "* Run ``svn+ssh://`` connections in non-interactive mode to prevent hanging (`#0`)" to use the correct issue number "(`#1230`)"; locate the changelog line containing the phrase "Run ``svn+ssh://`` connections in non-interactive mode to prevent hanging" and replace the "(`#0`)" token with "(`#1230`)" so the entry reflects the proper issue reference.example/dfetch.yaml (1)
10-10:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winVerify SSH prerequisites for the svn+ssh example.
The change from
svn://tosvn+ssh://demonstrates the non-interactive SSH mode feature. However, documentation explaining SSH key registration and known-hosts setup for SourceForge was previously requested and marked as addressed in commit 81387c8, but is not visible in the current code.🤖 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 `@example/dfetch.yaml` at line 10, The example changed the config key value url-base: svn+ssh://svn.code.sf.net/p/ to use non-interactive SSH but lacks documentation on SSH prerequisites; add a short doc/README note referenced from the example that explains registering your SSH public key with SourceForge, adding SourceForge's host key to users' known_hosts (or using a secured ssh config entry), and testing non-interactive SSH access (ssh -T or svn list) before using the url-base value, and link or mention this guidance next to the example so users of the url-base: svn+ssh://... snippet know the required SSH setup steps.
🤖 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 @.claude/settings.json:
- Around line 5-31: Split the unrelated expanded Claude assistant permission
entries (e.g., "Bash(git stash *)", "Bash(pip install *)", "Bash(pytest *)",
"Bash(dfetch add *)", etc.) out of the current commit into a separate PR focused
on dev-tooling/config changes, leaving only the svn+ssh hang fix in this PR;
alternatively, if any of these entries are required to test or validate the
svn+ssh change, add a short note in the PR description explaining which specific
permission(s) (reference the exact entries above) are required and why so
reviewers understand their relevance.
In `@tests/test_svn.py`:
- Around line 542-549: Add a new parametrized test in tests/test_svn.py that
mirrors test_get_info_from_target_raises_hint_on_ssh_host_key_failure but covers
SvnRepo.externals_from_url, SvnRepo.get_last_changed_revision,
SvnRepo("...").export, and SvnRepo.files_in_path: for each method call patch
dfetch.vcs.svn.run_on_cmdline to raise SubprocessCommandError with stderr "Host
key verification failed." and assert the call raises a RuntimeError matching
"known hosts"; reference the SvnRepo class and the four method names
(externals_from_url, get_last_changed_revision, export, files_in_path) so the
test targets the updated non-interactive SSH behavior.
- Around line 525-540: The parameterized test
test_svn_remote_raises_hint_on_ssh_host_key_failure is missing coverage for
SvnRemote.ls_tree; update the param list in the pytest.mark.parametrize tuple to
include ("ls_tree", "svn+ssh://svn.code.sf.net/project") so ls_tree is exercised
the same way as is_svn, list_of_tags, and list_of_branches and will raise the
same SSH host-key hint on SubprocessCommandError.
---
Outside diff comments:
In `@dfetch/vcs/svn.py`:
- Around line 389-404: get_last_changed_revision(), export(), and
files_in_path() still call run_on_cmdline() directly and will raise raw
SubprocessCommandError on SSH host-key verification failures; update each to
mirror get_info_from_target() by wrapping their run_on_cmdline() calls in the
same try/except used there (catch SubprocessCommandError), invoke the same
remediation/host-key handling helper used by get_info_from_target(), and re-run
or re-raise as appropriate so callers receive the centralized remediation path
instead of raw subprocess errors; locate the run_on_cmdline invocations in
get_last_changed_revision(), export(), and files_in_path() and apply the
identical exception-handling pattern found in get_info_from_target().
In `@tests/test_svn.py`:
- Around line 577-584: The test test_svn_ssh_env_preserves_existing_batch_mode
should also assert that existing SSH options are preserved; after calling
_extend_env_for_non_interactive_mode(), add an assertion that env["SVN_SSH"]
still contains the original key option (e.g. assert "-i /my/key" in
env["SVN_SSH"] or assert env["SVN_SSH"].startswith("ssh -o BatchMode=yes -i
/my/key") to ensure the full original value is maintained) while keeping the
existing BatchMode=yes duplication check.
---
Duplicate comments:
In `@CHANGELOG.rst`:
- Line 24: Update the placeholder issue reference in the changelog entry that
reads "* Run ``svn+ssh://`` connections in non-interactive mode to prevent
hanging (`#0`)" to use the correct issue number "(`#1230`)"; locate the changelog
line containing the phrase "Run ``svn+ssh://`` connections in non-interactive
mode to prevent hanging" and replace the "(`#0`)" token with "(`#1230`)" so the
entry reflects the proper issue reference.
In `@dfetch/vcs/svn.py`:
- Around line 48-54: The ssh hint uses _ssh_target_from_url(url) which may
return "user@host" but ssh-keyscan requires just the host; update the code to
extract a host-only value (e.g., call or add a helper to parse host from url or
strip the user prefix from the result of _ssh_target_from_url) and use that
host-only string in the ssh-keyscan line, while keeping the original target
(user@host) for the ssh -T example; adjust the two formatted f-strings in the
RuntimeError construction around _ssh_target_from_url(url) accordingly (use
host_only for the ssh-keyscan hint and the original target for the ssh -T hint).
- Around line 238-241: The svn propget call in externals_from_url (the
run_on_cmdline invocation) runs with non-interactive SSH but doesn't check for
SSH host-key failures; after capturing the subprocess result, call
SvnRepo._raise_if_ssh_host_key_error(result, url) (or the existing helper that
inspects the subprocess result) before calling SvnRepo._normalize_url_prefix and
SvnRepo._parse_externals so host-key issues produce the remediation message
instead of a raw SubprocessCommandError.
In `@example/dfetch.yaml`:
- Line 10: The example changed the config key value url-base:
svn+ssh://svn.code.sf.net/p/ to use non-interactive SSH but lacks documentation
on SSH prerequisites; add a short doc/README note referenced from the example
that explains registering your SSH public key with SourceForge, adding
SourceForge's host key to users' known_hosts (or using a secured ssh config
entry), and testing non-interactive SSH access (ssh -T or svn list) before using
the url-base value, and link or mention this guidance next to the example so
users of the url-base: svn+ssh://... snippet know the required SSH setup steps.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: ee5c747c-98e6-4f39-abc2-48501f388bab
📒 Files selected for processing (5)
.claude/settings.jsonCHANGELOG.rstdfetch/vcs/svn.pyexample/dfetch.yamltests/test_svn.py
spoorcc
force-pushed
the
svn-ssh-non-onteractive
branch
from
ce9644c to
5d550c7
Compare
spoorcc
force-pushed
the
svn-ssh-non-onteractive
branch
from
5d550c7 to
1b58c1d
Compare
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 `@dfetch/vcs/svn.py`:
- Line 71: Replace the redundant str() wrapper around the decoded stdout: change
the return of "return str(result.stdout.decode())" to simply return the decoded
string (i.e., return result.stdout.decode()). This removes the unnecessary
conversion while preserving existing behavior in the function containing that
return expression.
- Around line 135-138: list_of_tags() currently calls _run_svn directly and can
raise SubprocessCommandError if the tags/ path is missing; update list_of_tags
to mirror list_of_branches by wrapping the _run_svn call in a try/except
SubprocessCommandError block (catch the exception and return an empty list),
keeping the same output parsing logic (strip "/\r" and split by "\n") and
referencing list_of_tags, list_of_branches, _run_svn, and SubprocessCommandError
to locate where to add the handler.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 57862597-74b9-4d2e-a123-1c822ff2b03b
📒 Files selected for processing (5)
.readthedocs.ymlCHANGELOG.rstdfetch/vcs/svn.pytests/test_svn.pytests/test_svn_vcs.py
| def list_of_tags(self) -> list[str]: | ||
| """Get list of all available tags.""" | ||
| result = run_on_cmdline( | ||
| logger, ["svn", "ls", "--non-interactive", f"{self._remote}/tags"] | ||
| ) | ||
| return [ | ||
| str(tag).strip("/\r") for tag in result.stdout.decode().split("\n") if tag | ||
| ] | ||
| output = _run_svn(["ls", f"{self._remote}/tags"], url=self._remote) | ||
| return [str(tag).strip("/\r") for tag in output.split("\n") if tag] |
There was a problem hiding this comment.
Missing exception handling inconsistent with list_of_branches().
list_of_tags() lacks the try/except block that list_of_branches() has. If the tags/ directory doesn't exist, this will raise SubprocessCommandError instead of returning an empty list. This behavioral inconsistency could break callers that expect both methods to handle missing directories gracefully.
🐛 Suggested fix to match list_of_branches() pattern
def list_of_tags(self) -> list[str]:
"""Get list of all available tags."""
- output = _run_svn(["ls", f"{self._remote}/tags"], url=self._remote)
- return [str(tag).strip("/\r") for tag in output.split("\n") if tag]
+ try:
+ output = _run_svn(["ls", f"{self._remote}/tags"], url=self._remote)
+ return [str(tag).strip("/\r") for tag in output.split("\n") if tag]
+ except SshHostKeyError:
+ raise
+ except (SubprocessCommandError, RuntimeError):
+ return []🤖 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 `@dfetch/vcs/svn.py` around lines 135 - 138, list_of_tags() currently calls
_run_svn directly and can raise SubprocessCommandError if the tags/ path is
missing; update list_of_tags to mirror list_of_branches by wrapping the _run_svn
call in a try/except SubprocessCommandError block (catch the exception and
return an empty list), keeping the same output parsing logic (strip "/\r" and
split by "\n") and referencing list_of_tags, list_of_branches, _run_svn, and
SubprocessCommandError to locate where to add the handler.
spoorcc
force-pushed
the
svn-ssh-non-onteractive
branch
from
1b58c1d to
0cc082e
Compare
|
@coderabbitai resume |
|
✅ Action performedReviews resumed. Review finished.
|
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/test_svn.py (1)
538-671:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd Google-style docstrings to the new test functions.
The newly added tests in this block are missing function docstrings, which violates the test-style guideline.
📝 Example pattern
def test_ssh_hint_includes_hostname(): + """Raises a known-hosts hint that includes the parsed SSH hostname.""" stderr = "Host key verification failed."As per coding guidelines, “
tests/**/*.py: Docstrings in test functions should follow Google style convention.”🤖 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 `@tests/test_svn.py` around lines 538 - 671, Several new test functions lack Google-style docstrings; add a one- to two-sentence Google-style docstring to each test function (e.g., test_svn_remote_raises_hint_on_ssh_host_key_failure, test_get_info_from_target_raises_hint_on_ssh_host_key_failure, test_svn_repo_raises_hint_on_ssh_host_key_failure, test_ssh_hint_includes_hostname, test_ssh_hint_includes_user_when_present_in_url, test_svn_ssh_env_has_batch_mode, test_svn_ssh_env_preserves_existing_batch_mode, test_run_svn_passes_non_interactive_env_to_subprocess, test_ssh_hint_on_authenticity_of_host_message, test_ssh_hint_without_url_omits_hostname_commands, test_browse_tree_raises_hint_on_ssh_host_key_failure, test_create_diff_handles_non_utf8_diff_output) that describes the test purpose and any important conditions; follow Google docstring style (short summary, optional blank line, and brief notes on the scenario/assertions) and place it immediately under each def line.Source: Coding guidelines
🤖 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 @.claude/settings.json:
- Around line 23-24: The allowlist entries "Bash(pip install *)" and
"Bash(pytest *)" (and the similar wildcard entries around lines 28–31) are too
permissive; replace these star-based patterns with least-privilege shapes by
enumerating allowed package/flag patterns or using constrained argument regexes
instead of "*" — e.g., restrict "pip install" to specific package
names/versions, constrain "pytest" to specific test paths/flags, and tighten any
"dfetch" or other Bash(...) entries to explicit, validated argument forms so
arbitrary installs/tests/commands cannot be executed.
In @.readthedocs.yml:
- Line 22: The post_checkout step currently suppresses all errors by running
"git fetch --unshallow || true"; change it to only run git fetch --unshallow
when the repo is actually shallow (e.g., test git rev-parse
--is-shallow-repository or inspect .git/shallow) so real fetch failures are not
hidden; update the post_checkout sequence to conditionally execute "git fetch
--unshallow" when shallow, otherwise skip it, keeping the step name
(post_checkout) and the command string ("git fetch --unshallow") so reviewers
can find and verify the change.
In `@tests/test_svn.py`:
- Around line 543-544: Update the host-key failure tests to assert the specific
SshHostKeyError instead of a generic RuntimeError: replace
pytest.raises(RuntimeError, match="known hosts") with
pytest.raises(SshHostKeyError, match="known hosts") for the calls invoking
getattr(SvnRemote(url), method)(*call_args) (and apply the same change to the
other test occurrences referenced), ensuring the tests import or reference the
SshHostKeyError class used by the SvnRemote implementation.
---
Outside diff comments:
In `@tests/test_svn.py`:
- Around line 538-671: Several new test functions lack Google-style docstrings;
add a one- to two-sentence Google-style docstring to each test function (e.g.,
test_svn_remote_raises_hint_on_ssh_host_key_failure,
test_get_info_from_target_raises_hint_on_ssh_host_key_failure,
test_svn_repo_raises_hint_on_ssh_host_key_failure,
test_ssh_hint_includes_hostname,
test_ssh_hint_includes_user_when_present_in_url,
test_svn_ssh_env_has_batch_mode, test_svn_ssh_env_preserves_existing_batch_mode,
test_run_svn_passes_non_interactive_env_to_subprocess,
test_ssh_hint_on_authenticity_of_host_message,
test_ssh_hint_without_url_omits_hostname_commands,
test_browse_tree_raises_hint_on_ssh_host_key_failure,
test_create_diff_handles_non_utf8_diff_output) that describes the test purpose
and any important conditions; follow Google docstring style (short summary,
optional blank line, and brief notes on the scenario/assertions) and place it
immediately under each def 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 52bb39b5-6274-45b5-9c87-fd00a910db43
📒 Files selected for processing (8)
.claude/settings.json.readthedocs.ymlCHANGELOG.rstdfetch/vcs/svn.pydoc/howto/troubleshooting.rstfeatures/check-svn-repo.featuretests/test_svn.pytests/test_svn_vcs.py
| "Bash(pip install *)", | ||
| "Bash(pytest *)", |
There was a problem hiding this comment.
Restrict wildcard Bash permissions to least privilege.
These *-based entries materially widen what the assistant can execute and weaken the allowlist boundary. For high-risk commands (pip install, broad pytest, and unconstrained dfetch args), prefer explicit command/arg shapes.
🔧 Suggested tightening example
- "Bash(pip install *)",
- "Bash(pytest *)",
+ "Bash(pip install -e .[docs])",
+ "Bash(pytest tests/*)",Also applies to: 28-31
🤖 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 @.claude/settings.json around lines 23 - 24, The allowlist entries "Bash(pip
install *)" and "Bash(pytest *)" (and the similar wildcard entries around lines
28–31) are too permissive; replace these star-based patterns with
least-privilege shapes by enumerating allowed package/flag patterns or using
constrained argument regexes instead of "*" — e.g., restrict "pip install" to
specific package names/versions, constrain "pytest" to specific test
paths/flags, and tighten any "dfetch" or other Bash(...) entries to explicit,
validated argument forms so arbitrary installs/tests/commands cannot be
executed.
0cc082e to
5fd2026
Compare
1 participant
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests
Chores