Fix 86 Pyright errors in security-critical paths (Phase 3)#301
Fix 86 Pyright errors in security-critical paths (Phase 3)#301nitrobass24 merged 1 commit intodevelopfrom
Conversation
Type-only changes, no behavior modifications. Reduces Pyright error count from 149 to 64. Files fixed: - lftp/lftp.py (33) — pexpect spawn null guards, descriptor pattern type ignores, env dict cast, kill() None handling - ssh/sshcp.py (19) — pexpect before/after narrowing with isinstance guards, replacing != pexpect.EOF checks Pyright couldn't narrow - controller/extract/extract_process.py (14) — datetime module vs class confusion, Optional params, dispatch null guard - web/web_app.py (13) — Bottle DictProperty type ignores on route decorators - controller/auto_queue.py (7) — cls TypeVar fix, None guard on pair_id dict lookup, shadowed variable rename Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughMultiple modules receive type-safety enhancements: classmethod signatures updated with explicit typing, Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
📝 Coding Plan
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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/python/lftp/lftp.py`:
- Around line 548-551: The kill() method currently returns False when status()
is None, which silently no-ops; update kill() in src/python/lftp/lftp.py so that
when statuses is None it logs an error (self.logger.error) with the job name and
then raises a runtime exception (e.g., RuntimeError("Kill failed - status
unavailable for job '<name>'")) instead of returning False; keep the existing
debug/error message content and ensure the exception includes the job name so
callers (e.g., controller.py callers of kill()) can detect and handle the
failure.
In `@src/python/web/web_app.py`:
- Around line 83-93: CI's python-typecheck step doesn't install runtime
dependencies (e.g., bottle, pexpect, tblib) so Pyright can't resolve imports
used by web_app.py (e.g., references to __index, __static, __web_stream via
bottle routes). Fix by updating the python-typecheck job to install the
project's dependencies before running pyright (e.g., pip install -r
requirements.txt or pip install .[dev]), or alternatively add appropriate search
paths / package entries to pyrightconfig.json (extraPaths or
executionEnvironments) so Pyright can locate the installed packages used in
web_app.py.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5ea8285a-cb9b-4e44-a245-61b96a93936e
📒 Files selected for processing (5)
src/python/controller/auto_queue.pysrc/python/controller/extract/extract_process.pysrc/python/lftp/lftp.pysrc/python/ssh/sshcp.pysrc/python/web/web_app.py
| statuses = self.status() | ||
| if statuses is None: | ||
| self.logger.debug("Kill failed - status unavailable for job '{}'".format(name)) | ||
| return False |
There was a problem hiding this comment.
kill() now silently no-ops when status is unavailable.
At Line 549-551, returning False on status() is None is effectively ignored by the current caller (src/python/controller/controller.py Line 1057-1061), so kill failures can pass silently.
Proposed fix
statuses = self.status()
if statuses is None:
- self.logger.debug("Kill failed - status unavailable for job '{}'".format(name))
- return False
+ self.logger.warning("Kill failed - status unavailable for job '{}'".format(name))
+ raise LftpError("Kill failed - status unavailable")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| statuses = self.status() | |
| if statuses is None: | |
| self.logger.debug("Kill failed - status unavailable for job '{}'".format(name)) | |
| return False | |
| statuses = self.status() | |
| if statuses is None: | |
| self.logger.warning("Kill failed - status unavailable for job '{}'".format(name)) | |
| raise LftpError("Kill failed - status unavailable") |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/python/lftp/lftp.py` around lines 548 - 551, The kill() method currently
returns False when status() is None, which silently no-ops; update kill() in
src/python/lftp/lftp.py so that when statuses is None it logs an error
(self.logger.error) with the job name and then raises a runtime exception (e.g.,
RuntimeError("Kill failed - status unavailable for job '<name>'")) instead of
returning False; keep the existing debug/error message content and ensure the
exception includes the job name so callers (e.g., controller.py callers of
kill()) can detect and handle the failure.
There was a problem hiding this comment.
Returning False is the correct behavior here. kill() returns bool indicating whether the job was found and killed. When status is unavailable
(transient parse error), returning False lets the caller retry on the next cycle. Raising an exception would crash the controller — the same pattern we fixed in
PR #296 (don't crash on transient failures). The caller (controller.py) already handles kill() returning False gracefully.
There was a problem hiding this comment.
@nitrobass24, understood — that's a clear design rationale. The bool return is the correct contract for kill(): False means "job not found or status temporarily unavailable, retry next cycle," not a hard failure. Raising an exception here would be inconsistent with the transient-failure tolerance pattern established in #296. My suggestion was mistaken in this context.
✏️ Learnings added
Learnt from: nitrobass24
Repo: nitrobass24/seedsync PR: 301
File: src/python/lftp/lftp.py:548-551
Timestamp: 2026-03-18T23:09:07.947Z
Learning: In seedsync (nitrobass24/seedsync), `Lftp.kill()` in `src/python/lftp/lftp.py` intentionally returns `False` (not raises) when `status()` returns `None` due to a transient parse error. Returning `False` allows the controller to retry on the next polling cycle without crashing. Do not suggest raising an exception here. The caller in `controller.py` handles `kill()` returning `False` gracefully. This pattern of returning False on transient failures (rather than raising) was established deliberately in PR `#296`.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: nitrobass24
Repo: nitrobass24/seedsync PR: 74
File: src/python/controller/controller.py:5-5
Timestamp: 2026-02-21T15:50:25.495Z
Learning: Maintain consistency by using typing.Dict, typing.List, and typing.Set across Python code. Do not suggest converting to built-in generic types (dict, list, set) on a per-file basis. If a change is needed, apply it project-wide or ensure any modernization decisions are deliberate and reviewed, rather than triggering mixed usage in individual files.
Learnt from: nitrobass24
Repo: nitrobass24/seedsync PR: 146
File: src/python/tests/unittests/test_controller/test_exclude_patterns.py:11-11
Timestamp: 2026-03-05T20:23:51.664Z
Learning: In this repository Ruff is not configured (no ruff.toml, .ruff.toml, or [tool.ruff] in pyproject.toml). Do not flag Ruff-specific rules (e.g., FBT003, ANN) for any Python files. Do not enforce Ruff rules unless explicitly configured. Also, the project uses positional boolean arguments for SystemFile constructor calls throughout; preserve this pattern unless there is a specific refactor. Apply this guidance to all Python files in the repo.
| self.get("/server/stream")(self.__web_stream) # type: ignore[operator] | ||
|
|
||
| # Front-end routes | ||
| self.route("/")(self.__index) | ||
| self.route("/dashboard")(self.__index) | ||
| self.route("/settings")(self.__index) | ||
| self.route("/autoqueue")(self.__index) | ||
| self.route("/logs")(self.__index) | ||
| self.route("/about")(self.__index) | ||
| self.route("/")(self.__index) # type: ignore[operator] | ||
| self.route("/dashboard")(self.__index) # type: ignore[operator] | ||
| self.route("/settings")(self.__index) # type: ignore[operator] | ||
| self.route("/autoqueue")(self.__index) # type: ignore[operator] | ||
| self.route("/logs")(self.__index) # type: ignore[operator] | ||
| self.route("/about")(self.__index) # type: ignore[operator] | ||
| # For static files | ||
| self.route("/<file_path:path>")(self.__static) | ||
| self.route("/<file_path:path>")(self.__static) # type: ignore[operator] |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Candidate config/manifests =="
fd -HI 'pyrightconfig.json|pyproject.toml|requirements.*|Pipfile|setup.cfg|tox.ini'
echo
echo "== Dependency declarations for bottle/pexpect/tblib =="
for f in $(fd -HI 'pyproject.toml|requirements.*|Pipfile|setup.cfg'); do
echo "--- $f ---"
rg -n 'bottle|pexpect|tblib' "$f" || true
done
echo
echo "== Pyright settings that affect import resolution =="
for f in $(fd -HI 'pyrightconfig.json|pyproject.toml|setup.cfg'); do
echo "--- $f ---"
rg -n 'pythonVersion|venvPath|venv|extraPaths|executionEnvironments|reportMissingImports|reportMissingModuleSource' "$f" || true
doneRepository: nitrobass24/seedsync
Length of output: 654
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Full pyrightconfig.json content =="
cat -n src/python/pyrightconfig.json
echo
echo "== CI workflow type-check job =="
rg -A 20 'python-test' .github/workflows/ci.yml | head -40
echo
echo "== Check if bottle import exists at line 8 of web_app.py =="
sed -n '1,15p' src/python/web/web_app.py | cat -nRepository: nitrobass24/seedsync
Length of output: 1927
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Check for pyright/type-check steps in CI =="
rg -A 10 'pyright|type-?check|mypy|pylint' .github/workflows/ci.yml | head -60
echo
echo "== Full python-test job in CI =="
rg -A 30 'python-test:' .github/workflows/ci.yml | head -50Repository: nitrobass24/seedsync
Length of output: 1630
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Full python-typecheck job =="
rg -A 20 'python-typecheck:' .github/workflows/ci.yml
echo
echo "== Check if there's a separate dependency install step in typecheck =="
rg -B 5 -A 15 'pip install pyright' .github/workflows/ci.ymlRepository: nitrobass24/seedsync
Length of output: 1074
CI type-check environment is missing project dependencies, causing Pyright import resolution failures.
The python-typecheck job in the CI workflow installs only Pyright but not project dependencies (bottle, pexpect, tblib), so Pyright cannot resolve the bottle import at line 8. The job is soft-blocked with continue-on-error: true, but this needs fixing: either install dependencies in the type-check job or configure pyrightconfig.json to help Pyright locate them.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/python/web/web_app.py` around lines 83 - 93, CI's python-typecheck step
doesn't install runtime dependencies (e.g., bottle, pexpect, tblib) so Pyright
can't resolve imports used by web_app.py (e.g., references to __index, __static,
__web_stream via bottle routes). Fix by updating the python-typecheck job to
install the project's dependencies before running pyright (e.g., pip install -r
requirements.txt or pip install .[dev]), or alternatively add appropriate search
paths / package entries to pyrightconfig.json (extraPaths or
executionEnvironments) so Pyright can locate the installed packages used in
web_app.py.
There was a problem hiding this comment.
This is by design. The python-typecheck job is informational (continue-on-error: true). Installing all runtime deps would slow CI for a job that's not blocking.
The reportMissingImports errors from missing bottle/pexpect are expected and will resolve naturally when we make Pyright required (after reaching 0 errors). Not
fixing now.
There was a problem hiding this comment.
@nitrobass24, understood — that's a sensible staged approach. Keeping the typecheck job informational until errors reach zero, then hardening it (with deps installed and continue-on-error: false), is a clean migration path. I'll note this for future reviews.
✏️ Learnings added
Learnt from: nitrobass24
Repo: nitrobass24/seedsync PR: 301
File: src/python/web/web_app.py:83-93
Timestamp: 2026-03-18T23:09:21.223Z
Learning: In nitrobass24/seedsync, the `python-typecheck` CI job intentionally does NOT install runtime dependencies (bottle, pexpect, tblib, etc.) to keep CI fast. The job runs with `continue-on-error: true` and is purely informational. `reportMissingImports` errors from unresolved imports are expected and acceptable. The plan is to install runtime deps and make the job blocking only after Pyright errors reach 0. Do not flag the missing runtime deps in the typecheck job as an issue.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: nitrobass24
Repo: nitrobass24/seedsync PR: 287
File: .github/workflows/ci.yml:55-62
Timestamp: 2026-03-17T17:35:30.153Z
Learning: In nitrobass24/seedsync, the CI python-test job installs from src/python/requirements.txt and src/python/requirements-test.txt rather than directly from src/python/pyproject.toml. This is an intentional short-term design decision: adding a sync-check tool (e.g., pip-tools) was explicitly deferred to issue `#286` (uv evaluation), after which the requirements files may be eliminated entirely in favour of `uv sync --group test` reading pyproject.toml directly. The drift risk is considered acceptable given the small dep count (3 runtime, 6 test) and the fact that a missing dep would surface as an ImportError in CI. Do not flag the lack of a pyproject.toml ↔ requirements file consistency check as an issue in this repo.
Learnt from: nitrobass24
Repo: nitrobass24/seedsync PR: 146
File: src/python/tests/unittests/test_controller/test_exclude_patterns.py:11-11
Timestamp: 2026-03-05T20:23:51.664Z
Learning: In this repository Ruff is not configured (no ruff.toml, .ruff.toml, or [tool.ruff] in pyproject.toml). Do not flag Ruff-specific rules (e.g., FBT003, ANN) for any Python files. Do not enforce Ruff rules unless explicitly configured. Also, the project uses positional boolean arguments for SystemFile constructor calls throughout; preserve this pattern unless there is a specific refactor. Apply this guidance to all Python files in the repo.
Learnt from: nitrobass24
Repo: nitrobass24/seedsync PR: 74
File: src/python/controller/controller.py:5-5
Timestamp: 2026-02-21T15:50:25.495Z
Learning: Maintain consistency by using typing.Dict, typing.List, and typing.Set across Python code. Do not suggest converting to built-in generic types (dict, list, set) on a per-file basis. If a change is needed, apply it project-wide or ensure any modernization decisions are deliberate and reviewed, rather than triggering mixed usage in individual files.
1 participant
Summary
Phase 3 of the Pyright rollout (#249). Fixes 86 type errors in the security-critical code paths, reducing total from 149 to 64.
Error reduction
Files fixed
lftp/lftp.pyssh/sshcp.pycontroller/extract/extract_process.pyweb/web_app.pycontroller/auto_queue.pyRemaining 64 errors (Phase 4)
Concentrated in
controller/controller.py(~50) andcommon/config.py(~14) — the dynamic Config property system and large controller state machine.Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit