fix: resolve relative paths, add session log, remove /app ownership#147
Merged
fix: resolve relative paths, add session log, remove /app ownership#147
Conversation
- Apply os.path.abspath() to all DB/session path resolution (#144) so relative env vars resolve against the filesystem root, not WORKDIR - Log Telethon session DB path at INFO before client creation (#142) to distinguish session DB errors from main application DB errors - Remove /app from chown in Dockerfile (#145 comment) — static code should not be writable at runtime; add PYTHONDONTWRITEBYTECODE=1 and PYTHONUNBUFFERED=1 - Align entrypoint.sh path priority with Python code (DATABASE_PATH → DATABASE_DIR → DB_PATH → BACKUP_PATH) and resolve to absolute Closes #142, closes #144
1 task
|
🐳 Dev images published!
The dev/test instance will pick up these changes automatically (Portainer GitOps). To test locally: docker pull drumsergio/telegram-archive:dev
docker pull drumsergio/telegram-archive-viewer:dev |
|
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughNormalize DB/session filesystem paths to absolute values, log resolved Telethon session DB paths before client creation, limit Docker chown to /data and extend ENV, update entrypoint DB resolution, and add tests validating logging and path normalization. ChangesAbsolute Path Resolution & Session Logging Path normalization & session logging
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
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 (3)
src/db/base.py (1)
191-207:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
_safe_url()doesn't applyabspath, so the logged path diverges from the actual URL
_safe_url()re-derivesdb_pathfrom env vars withoutos.path.abspath(), so when a relative path is configured the "Initializing database" log line will show the relative path while the real connection uses the absolute one. Low impact (logging only), but confusing.✏️ Suggested fix
if not db_path: backup_path = os.getenv("BACKUP_PATH", "/data/backups") db_path = os.path.join(backup_path, "telegram_backup.db") + db_path = os.path.abspath(db_path) return f"sqlite+aiosqlite:///{db_path}"🤖 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 `@src/db/base.py` around lines 191 - 207, _safe_url currently reconstructs the SQLite db_path from env vars (DATABASE_PATH/DB_PATH, DATABASE_DIR, BACKUP_PATH) but does not call os.path.abspath, so logged path can differ from the actual connection path; update the _safe_url method to normalize the computed db_path with os.path.abspath() before returning (apply after determining db_path in the SQLite branch) so the returned "sqlite+aiosqlite:///{db_path}" matches the real absolute path used by the connection.scripts/entrypoint.sh (1)
229-237:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
realpath -mis bypassed whenDATABASE_URLcontains a relative SQLite pathLines 233–237 overwrite
DB_PATHby stripping the URL prefix afterrealpath -mhas already run. IfDATABASE_URL=sqlite:///relative/path.dbis set, the extracted path is still relative and the migration check at line 239 (if [ -f "$DB_PATH" ]) will resolve against$PWD(/app) instead of the intended root.✏️ Suggested fix
- if [[ "$DATABASE_URL" == sqlite+aiosqlite:///* ]]; then - DB_PATH="${DATABASE_URL#sqlite+aiosqlite:///}" - elif [[ "$DATABASE_URL" == sqlite:///* ]]; then - DB_PATH="${DATABASE_URL#sqlite:///}" - fi + if [[ "$DATABASE_URL" == sqlite+aiosqlite:///* ]]; then + DB_PATH="$(realpath -m "${DATABASE_URL#sqlite+aiosqlite:///}")" + elif [[ "$DATABASE_URL" == sqlite:///* ]]; then + DB_PATH="$(realpath -m "${DATABASE_URL#sqlite:///}")" + fi🤖 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 `@scripts/entrypoint.sh` around lines 229 - 237, The current flow runs realpath -m on DB_PATH before you strip SQLite URL prefixes from DATABASE_URL, so when DATABASE_URL contains a relative path (handled by the conditions that check sqlite+aiosqlite:/// and sqlite:///), the extracted path remains relative; move or repeat the realpath -m normalization to occur after you handle DATABASE_URL (i.e., after the if [[ "$DATABASE_URL" == sqlite+aiosqlite:///* ]] / elif [[ "$DATABASE_URL" == sqlite:///* ]] blocks that set DB_PATH) so DB_PATH is always an absolute path before the subsequent file existence check.src/telegram_backup.py (1)
136-137:⚠️ Potential issue | 🔴 Critical | ⚡ Quick win
SyntaxError: Python 2exceptsyntax
except ValueError, TypeError:is invalid in Python 3 — it's aSyntaxErrorat parse time. The entire module will fail to import.🐛 Fix
- except ValueError, TypeError: + except (ValueError, TypeError):🤖 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 `@src/telegram_backup.py` around lines 136 - 137, The except clause uses Python 2 syntax ("except ValueError, TypeError:") which causes a SyntaxError; update the exception handling around the block that sets log_threshold_seconds so it uses modern syntax — either a tuple form "except (ValueError, TypeError):" or separate except blocks — ensuring the same fallback assignment to log_threshold_seconds = 10 is preserved and no other exceptions are swallowed.
🧹 Nitpick comments (4)
src/db/migrate.py (1)
91-91: 💤 Low value
abspathonly applied to env-var-derived paths, not caller-supplied pathsIf a caller passes an explicit relative
sqlite_path, it won't be normalized to absolute. Consider movingabspathoutside theif sqlite_path is None:block to cover both cases (same applies toverify_migrationat line 199).✏️ Suggested fix
if sqlite_path is None: sqlite_path = os.getenv("DATABASE_PATH") if not sqlite_path: db_dir = os.getenv("DATABASE_DIR") if db_dir: sqlite_path = os.path.join(db_dir, "telegram_backup.db") if not sqlite_path: sqlite_path = os.getenv("DB_PATH") if not sqlite_path: backup_path = os.getenv("BACKUP_PATH", "/data/backups") sqlite_path = os.path.join(backup_path, "telegram_backup.db") - sqlite_path = os.path.abspath(sqlite_path) + sqlite_path = os.path.abspath(sqlite_path)🤖 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 `@src/db/migrate.py` at line 91, The code only normalizes sqlite_path with os.path.abspath when sqlite_path is None, so caller-supplied relative paths are left unnormalized; move the os.path.abspath(sqlite_path) call out of the if-block so sqlite_path is normalized regardless of being provided or derived, and make the same change inside verify_migration (ensure the variable sqlite_path in both migrate-related functions is passed through os.path.abspath immediately after determination). Ensure you update any subsequent logic that assumes None vs str accordingly (keep the same variable name sqlite_path).tests/test_issue_fixes.py (3)
287-297: ⚡ Quick winScan is too narrow to catch a
/appwrite regression.You're scanning only
telegram_backup.pyandlistener.py, but#144/#145 affectconfig.py,db/base.py,db/migrate.py,alembic/env.py, etc. A regression that re-introduces"/app/..."in any of those wouldn't be caught.- src_dir = pathlib.Path(__file__).parent.parent / "src" - for py_file in (src_dir / "telegram_backup.py", src_dir / "listener.py"): - content = py_file.read_text() - self.assertNotIn("APP_DIR", content) - self.assertNotIn('"/app/', content) + src_dir = pathlib.Path(__file__).parent.parent / "src" + for py_file in src_dir.rglob("*.py"): + content = py_file.read_text() + self.assertNotIn("APP_DIR", content, f"{py_file} contains APP_DIR") + self.assertNotIn('"/app/', content, f"{py_file} contains hardcoded /app/ path")🤖 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_issue_fixes.py` around lines 287 - 297, Update the test_no_hardcoded_app_directory_writes test to scan all source .py files instead of only telegram_backup.py and listener.py: use the existing src_dir (Path(__file__).parent.parent / "src") and iterate over src_dir.rglob("*.py") (or explicitly include config.py, db/base.py, db/migrate.py, alembic/env.py) and assert that neither "APP_DIR" nor '"/app/' appears in any file content; keep the same assertions but run them per-file so regressions in any module are caught.
299-306: 💤 Low valueThis test is essentially a tautology.
hasattr(sys, "dont_write_bytecode")is True on every CPython build. It doesn't verify thatPYTHONDONTWRITEBYTECODE=1is honored, nor that the Dockerfile sets it. Either drop it, or check the Dockerfile content directly (similar totest_no_hardcoded_app_directory_writes):- def test_pythondontwritebytecode_prevents_pyc_creation(self): - """PYTHONDONTWRITEBYTECODE=1 means sys.dont_write_bytecode is respected.""" - import sys - self.assertTrue(hasattr(sys, "dont_write_bytecode")) + def test_dockerfile_sets_pythondontwritebytecode(self): + """Dockerfile must set PYTHONDONTWRITEBYTECODE=1 so /app stays read-only.""" + import pathlib + dockerfile = (pathlib.Path(__file__).parent.parent / "Dockerfile").read_text() + self.assertIn("PYTHONDONTWRITEBYTECODE=1", dockerfile) + self.assertIn("PYTHONUNBUFFERED=1", dockerfile)🤖 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_issue_fixes.py` around lines 299 - 306, The test test_pythondontwritebytecode_prevents_pyc_creation is a tautology because hasattr(sys, "dont_write_bytecode") is always True; replace it with a meaningful assertion that inspects the project Dockerfile (or build config) to ensure PYTHONDONTWRITEBYTECODE=1 is set (mirroring the approach used in test_no_hardcoded_app_directory_writes), or simply remove the test; update the test function name and body accordingly so it reads the Dockerfile content and asserts the presence of the PYTHONDONTWRITEBYTECODE=1 directive.
186-271: 💤 Low valuePath assertions are POSIX-only, but CI is Linux-only so no action required.
os.path.isabs("C:\\foo")is True on Windows buturl_path.endswith("/telegram_backup.db")won't be, andassertEqual(url_path, "/data/backups/telegram_backup.db")would fail on Windows. All CI workflows run onubuntu-latestonly, so this is safe in practice. If you want defensive future-proofing, consider adding@unittest.skipUnless(os.name == "posix", ...)to these tests.🤖 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_issue_fixes.py` around lines 186 - 271, Several tests (e.g., test_relative_db_path_gets_resolved_to_absolute, test_absolute_db_path_remains_unchanged, test_relative_database_path_env_gets_resolved, test_absolute_database_path_env_remains_unchanged, test_default_path_is_absolute, test_relative_database_dir_gets_resolved, test_relative_backup_path_gets_resolved) make POSIX-specific path assertions; guard them with a platform check to avoid Windows failures by adding `@unittest.skipUnless`(os.name == "posix", "POSIX-only path assertions") above each test or the test class, and import os and unittest.skipUnless where tests are defined so DatabaseManager path behavior is only asserted on POSIX platforms.
🤖 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_issue_fixes.py`:
- Line 17: The async test class TestSessionPathLogging (and any other bare async
test classes in this file) should inherit from unittest.IsolatedAsyncioTestCase
instead of being bare classes so asyncio tests run under unittest; update their
class definition(s) to subclass IsolatedAsyncioTestCase and convert any plain
assert statements to unittest assertions (e.g., self.assertEqual,
self.assertTrue) and use MagicMock/AsyncMock for async dependencies where
applicable; adjust any async test methods to be async def test_* methods that
use await as needed.
- Around line 123-175: The tests are calling TelegramListener.create() but the
session-path/info log and TelegramClient instantiation happen in connect(), so
change both tests to call listener.connect(listener.config) instead of create();
keep the TelegramClient patch (patch("src.listener.TelegramClient") /
side_effect=track_client) and logger patch as before, and additionally stub out
get_db_manager (patch or set listener.get_db_manager to a simple stub returning
a db with get_all_chats), RealtimeNotifier (patch
"src.listener.RealtimeNotifier"), and db.get_all_chats to avoid downstream calls
during connect() so the test focuses only on the ordering/logging behavior of
connect() vs TelegramClient creation.
---
Outside diff comments:
In `@scripts/entrypoint.sh`:
- Around line 229-237: The current flow runs realpath -m on DB_PATH before you
strip SQLite URL prefixes from DATABASE_URL, so when DATABASE_URL contains a
relative path (handled by the conditions that check sqlite+aiosqlite:/// and
sqlite:///), the extracted path remains relative; move or repeat the realpath -m
normalization to occur after you handle DATABASE_URL (i.e., after the if [[
"$DATABASE_URL" == sqlite+aiosqlite:///* ]] / elif [[ "$DATABASE_URL" ==
sqlite:///* ]] blocks that set DB_PATH) so DB_PATH is always an absolute path
before the subsequent file existence check.
In `@src/db/base.py`:
- Around line 191-207: _safe_url currently reconstructs the SQLite db_path from
env vars (DATABASE_PATH/DB_PATH, DATABASE_DIR, BACKUP_PATH) but does not call
os.path.abspath, so logged path can differ from the actual connection path;
update the _safe_url method to normalize the computed db_path with
os.path.abspath() before returning (apply after determining db_path in the
SQLite branch) so the returned "sqlite+aiosqlite:///{db_path}" matches the real
absolute path used by the connection.
In `@src/telegram_backup.py`:
- Around line 136-137: The except clause uses Python 2 syntax ("except
ValueError, TypeError:") which causes a SyntaxError; update the exception
handling around the block that sets log_threshold_seconds so it uses modern
syntax — either a tuple form "except (ValueError, TypeError):" or separate
except blocks — ensuring the same fallback assignment to log_threshold_seconds =
10 is preserved and no other exceptions are swallowed.
---
Nitpick comments:
In `@src/db/migrate.py`:
- Line 91: The code only normalizes sqlite_path with os.path.abspath when
sqlite_path is None, so caller-supplied relative paths are left unnormalized;
move the os.path.abspath(sqlite_path) call out of the if-block so sqlite_path is
normalized regardless of being provided or derived, and make the same change
inside verify_migration (ensure the variable sqlite_path in both migrate-related
functions is passed through os.path.abspath immediately after determination).
Ensure you update any subsequent logic that assumes None vs str accordingly
(keep the same variable name sqlite_path).
In `@tests/test_issue_fixes.py`:
- Around line 287-297: Update the test_no_hardcoded_app_directory_writes test to
scan all source .py files instead of only telegram_backup.py and listener.py:
use the existing src_dir (Path(__file__).parent.parent / "src") and iterate over
src_dir.rglob("*.py") (or explicitly include config.py, db/base.py,
db/migrate.py, alembic/env.py) and assert that neither "APP_DIR" nor '"/app/'
appears in any file content; keep the same assertions but run them per-file so
regressions in any module are caught.
- Around line 299-306: The test
test_pythondontwritebytecode_prevents_pyc_creation is a tautology because
hasattr(sys, "dont_write_bytecode") is always True; replace it with a meaningful
assertion that inspects the project Dockerfile (or build config) to ensure
PYTHONDONTWRITEBYTECODE=1 is set (mirroring the approach used in
test_no_hardcoded_app_directory_writes), or simply remove the test; update the
test function name and body accordingly so it reads the Dockerfile content and
asserts the presence of the PYTHONDONTWRITEBYTECODE=1 directive.
- Around line 186-271: Several tests (e.g.,
test_relative_db_path_gets_resolved_to_absolute,
test_absolute_db_path_remains_unchanged,
test_relative_database_path_env_gets_resolved,
test_absolute_database_path_env_remains_unchanged,
test_default_path_is_absolute, test_relative_database_dir_gets_resolved,
test_relative_backup_path_gets_resolved) make POSIX-specific path assertions;
guard them with a platform check to avoid Windows failures by adding
`@unittest.skipUnless`(os.name == "posix", "POSIX-only path assertions") above
each test or the test class, and import os and unittest.skipUnless where tests
are defined so DatabaseManager path behavior is only asserted on POSIX
platforms.
🪄 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: CHILL
Plan: Pro
Run ID: b8e57e46-267a-4acb-af2f-4796758388cd
📒 Files selected for processing (10)
Dockerfilealembic/env.pyscripts/entrypoint.shsrc/config.pysrc/connection.pysrc/db/base.pysrc/db/migrate.pysrc/listener.pysrc/telegram_backup.pytests/test_issue_fixes.py
| # ============================================================ | ||
|
|
||
|
|
||
| class TestSessionPathLogging: |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Make the async test classes use unittest.IsolatedAsyncioTestCase.
Per coding guidelines for tests/**/*.py ("Use unittest.TestCase with MagicMock/AsyncMock for test consistency"), and to match the rest of this file (TestRelativeDbPathResolution, TestDockerfileAssumptions). Right now these two classes are bare classes that only run because pytest-asyncio happens to collect them — they’re also using assert instead of self.assert*.
-class TestSessionPathLogging:
+class TestSessionPathLogging(unittest.IsolatedAsyncioTestCase):
...
-class TestListenerSessionPathLogging:
+class TestListenerSessionPathLogging(unittest.IsolatedAsyncioTestCase):As per coding guidelines: "Use unittest.TestCase with MagicMock/AsyncMock for test consistency".
Also applies to: 86-86
🤖 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_issue_fixes.py` at line 17, The async test class
TestSessionPathLogging (and any other bare async test classes in this file)
should inherit from unittest.IsolatedAsyncioTestCase instead of being bare
classes so asyncio tests run under unittest; update their class definition(s) to
subclass IsolatedAsyncioTestCase and convert any plain assert statements to
unittest assertions (e.g., self.assertEqual, self.assertTrue) and use
MagicMock/AsyncMock for async dependencies where applicable; adjust any async
test methods to be async def test_* methods that use await as needed.
- Add DB_PATH env var check to src/config.py (was missing, causing inconsistency with db/base.py, alembic/env.py, and migrate.py) - Fix entrypoint.sh to respect DB_PATH env var without overwriting a pre-set value, using intermediate _DB_FILE variable - Add priority comment in entrypoint.sh documenting the fallback chain: DATABASE_PATH > DATABASE_DIR > DB_PATH > BACKUP_PATH/telegram_backup.db - Remove stale unittest.main() from test_issue_fixes.py (pytest-style classes don't need it, and it would skip async tests)
|
🐳 Dev images published!
The dev/test instance will pick up these changes automatically (Portainer GitOps). To test locally: docker pull drumsergio/telegram-archive:dev
docker pull drumsergio/telegram-archive-viewer:dev |
The session path log is emitted in connect(), not in the create() factory method. Tests now properly exercise the connect() path and mock get_db_manager + RealtimeNotifier dependencies.
|
🐳 Dev images published!
The dev/test instance will pick up these changes automatically (Portainer GitOps). To test locally: docker pull drumsergio/telegram-archive:dev
docker pull drumsergio/telegram-archive-viewer:dev |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #147 +/- ##
==========================================
- Coverage 94.07% 94.06% -0.01%
==========================================
Files 21 21
Lines 6127 6136 +9
==========================================
+ Hits 5764 5772 +8
- Misses 363 364 +1
🚀 New features to boost your workflow:
|
This was referenced May 7, 2026
Closed
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
1 participant
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
os.path.abspath()to all 7 independent path resolution sites (config.py, db/base.py, alembic/env.py, db/migrate.py, entrypoint.sh) so relative env vars likeBACKUP_PATH=backupsresolve against/not/app(Docker WORKDIR)TelegramClient()instantiation in all 3 creation sites (connection.py, telegram_backup.py, listener.py) — users can now distinguish session DB errors from application DB errors/appfromchownin Dockerfile (static code shouldn't be writable), addPYTHONDONTWRITEBYTECODE=1+PYTHONUNBUFFERED=1DATABASE_DIRcheck)Test plan
ruff check .+ruff format --check .pass/appCloses #142, closes #144
Summary by CodeRabbit
New Features
Improvements
Tests