fix(media): correct fallback logic, ACL bypass, and migration cleanup#169
Merged
Conversation
- Extract shared media_utils.py with proper arithmetic for Telegram marked-ID resolution (fixes channel fallback producing wrong paths) - Fix ACL bypass: enforce access control against resolved path, not URL - Fix entrypoint crash when media table doesn't exist on pre-v6 databases - Remove dead _BATCH_SIZE constant and identical dialect branches in migration - Add try/except for RuntimeError (symlink loops) in fallback paths - Add debug logging to fallback and ACL legacy grant paths - Fix except syntax to use parenthesized tuple form - Add 25 unit tests for media_utils with roundtrip consistency checks - Bump version to 7.10.15
📝 WalkthroughWalkthroughVersion 7.10.15 centralizes legacy Telegram media folder/chat-ID conversions in a new media_utils module, integrates it into media serving and thumbnail fallback logic, adds unit tests, and guards pre-Alembic DB stamping against missing media tables. ChangesLegacy Media Utilities and Serving Compatibility
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 |
Return empty list for non-numeric folders (e.g., "chat1") instead of raising ValueError. Fixes test_web_thumbnails failure on CI.
|
🐳 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (1)
tests/test_media_utils.py (1)
17-174: ⚡ Quick winAdd invalid-folder tests for
legacy_folder_alternates.Please add cases like
"avatars"and"avatars/chats"to assert safe behavior (e.g.,[]) so fallback callers don’t regress into 500s.🤖 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_media_utils.py` around lines 17 - 174, Add tests exercising legacy_folder_alternates with invalid folder names so it returns an empty list instead of raising: add assertions calling legacy_folder_alternates("avatars") and legacy_folder_alternates("avatars/chats") and assertEqual(result, []) (or assertEqual(len(result), 0)); place them alongside the other forward-resolution tests in tests/test_media_utils.py (e.g., inside TestLegacyFolderAlternatesForward) so callers relying on safe fallback behavior don’t regress to 500s.
🤖 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 `@src/web/main.py`:
- Line 826: The debug statements emit PII (chat/folder IDs and request paths);
change the logger.debug calls in the ACL/fallback logic so they do not log
identifiers—replace the message that references media_chat_id (the "ACL legacy
grant" log) and the request path/folder-details log with aggregated/no-ID
diagnostics (e.g., "ACL legacy grant: positive folder mapped to allowed chat
(count=1)" or "request route examined for ACL fallback (folders_checked=3)") and
ensure no message content or IDs are included; update the two logger.debug sites
to report only counts, booleans, or sanitized flags and keep the original log
level and context variables intact.
- Around line 932-941: legacy_folder_alternates(folder) can raise ValueError for
non-numeric folder names before the per-alt try/except, causing a 500; to fix,
guard generation by validating `folder` (e.g., only call
legacy_folder_alternates if `folder.isnumeric()` or otherwise matches the
expected pattern) or wrap the call in a try/except that sets `alt_folders = []`
on ValueError; update the code around the `alt_folders =
legacy_folder_alternates(folder)` line and ensure the rest of the loop uses
`alt_folders`, leaving `resolved` as None so missing files still raise the 404
HTTPException.
- Line 938: Replace Python-2 style comma-separated exception handlers with tuple
form: locate the flawed except clause using the pattern "except OSError,
ValueError, RuntimeError:" in src/web/main.py (and similar patterns at the other
noted locations, e.g., the earlier handler around line 925 and in
src/web/thumbnails.py around line 141) and change it to use the tuple form
"except (OSError, ValueError, RuntimeError):" so the code is valid under Python
3; ensure any accompanying except blocks or variable bindings are adapted
consistently.
In `@src/web/media_utils.py`:
- Around line 20-23: In the legacy folder resolver (the block that checks
folder.startswith and calls int(folder)), defensively handle non-numeric folder
values by validating or catching conversion errors and returning an empty list
instead of letting ValueError propagate; update the logic around the two
int(folder) calls to either check folder.isdigit() (or a suitable numeric test)
before conversion or wrap the int(...) in a try/except ValueError that returns
[] as a safe no-op, referencing the existing variables folder and
CHANNEL_ID_OFFSET so the returned values are only produced for valid numeric
folder inputs.
In `@src/web/thumbnails.py`:
- Line 137: Remove the detailed PII-bearing values from the logger.debug call
that currently prints "Thumbnail legacy fallback: %s/%s via alt folder %s" (the
call referencing variables folder, filename, alt); instead log an aggregate
outcome such as a simple fallback event (e.g., "Thumbnail legacy fallback
occurred") or a boolean/count (e.g., "Thumbnail legacy fallback: true") so no
folder, filename or alt values are emitted; update the logger.debug invocation
in the same scope where that message appears (the call using logger.debug with
the exact string above) to only include non-identifying aggregated information.
- Around line 131-132: legacy_folder_alternates(folder) can throw for
non-numeric folder values and break thumbnail requests; guard the call by
validating or normalizing folder first and treat invalid formats as "no
alternates" (empty list). Update the code around the call to
legacy_folder_alternates (where alt_folders and found are set) to either check
the folder is numeric (e.g., str(folder).isdigit() or isinstance(folder, int))
before calling, or wrap the call in a try/except that catches the specific
exception (e.g., ValueError) and sets alt_folders = [] on failure so thumbnail
paths continue to be resolved safely.
- Line 141: The except line uses Python2-style syntax "except OSError,
RuntimeError:" which is invalid in Python 3; update the exception handling in
the thumbnails module (around the try/except block that currently references
OSError and RuntimeError) to use modern exception tuple syntax by catching both
exceptions with "except (OSError, RuntimeError):" or split into separate "except
OSError:" and "except RuntimeError:" clauses as appropriate, preserving the
existing error handling logic in the surrounding function/method.
---
Nitpick comments:
In `@tests/test_media_utils.py`:
- Around line 17-174: Add tests exercising legacy_folder_alternates with invalid
folder names so it returns an empty list instead of raising: add assertions
calling legacy_folder_alternates("avatars") and
legacy_folder_alternates("avatars/chats") and assertEqual(result, []) (or
assertEqual(len(result), 0)); place them alongside the other forward-resolution
tests in tests/test_media_utils.py (e.g., inside
TestLegacyFolderAlternatesForward) so callers relying on safe fallback behavior
don’t regress to 500s.
🪄 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: 7029b331-64b1-4694-87b5-71418fb3477b
⛔ Files ignored due to path filters (1)
alembic/versions/20260524_013_fix_media_file_paths.pyis excluded by!alembic/versions/**
📒 Files selected for processing (7)
pyproject.tomlscripts/entrypoint.shsrc/__init__.pysrc/web/main.pysrc/web/media_utils.pysrc/web/thumbnails.pytests/test_media_utils.py
Comment on lines
+932
to
941
| alt_folders = legacy_folder_alternates(folder) | ||
| for alt in alt_folders: | ||
| try: | ||
| resolved = (_media_root / alt / rest).resolve(strict=True) | ||
| logger.debug("Legacy fallback: served %s via alt folder %s", path, alt) | ||
| break | ||
| except OSError, ValueError: | ||
| except OSError, ValueError, RuntimeError: | ||
| continue | ||
| if resolved is None: | ||
| raise HTTPException(status_code=404, detail="File not found") |
There was a problem hiding this comment.
Guard alternate-folder generation for non-numeric folder names.
Line 932 can raise ValueError before the per-alt try, so missing non-chat paths can fail with 500 instead of clean 404 handling.
Proposed fix
- alt_folders = legacy_folder_alternates(folder)
+ try:
+ alt_folders = legacy_folder_alternates(folder)
+ except ValueError:
+ alt_folders = []🤖 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/web/main.py` around lines 932 - 941, legacy_folder_alternates(folder) can
raise ValueError for non-numeric folder names before the per-alt try/except,
causing a 500; to fix, guard generation by validating `folder` (e.g., only call
legacy_folder_alternates if `folder.isnumeric()` or otherwise matches the
expected pattern) or wrap the call in a try/except that sets `alt_folders = []`
on ValueError; update the code around the `alt_folders =
legacy_folder_alternates(folder)` line and ensure the rest of the loop uses
`alt_folders`, leaving `resolved` as None so missing files still raise the 404
HTTPException.
| logger.debug("Legacy fallback: served %s via alt folder %s", path, alt) | ||
| break | ||
| except OSError, ValueError: | ||
| except OSError, ValueError, RuntimeError: |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -nP 'except\s+[^(\n:]+,\s*[^:\n]+:' src/web/main.py src/web/thumbnails.pyRepository: GeiserX/Telegram-Archive
Length of output: 876
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the exact exception block around line 938 in src/web/main.py
nl -ba src/web/main.py | sed -n '920,950p'Repository: GeiserX/Telegram-Archive
Length of output: 111
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Print src/web/main.py lines 920-950 with line numbers (no nl dependency)
awk 'NR>=920 && NR<=950 {printf "%d:%s\n", NR, $0}' src/web/main.pyRepository: GeiserX/Telegram-Archive
Length of output: 1636
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check Python version / runtime hints
ls -1 | rg -i 'pyproject\.toml|setup\.cfg|setup\.py|requirements.*|Pipfile|tox\.ini'
[ -f pyproject.toml ] && { echo '--- pyproject.toml ---'; sed -n '1,120p' pyproject.toml; }
[ -f setup.cfg ] && { echo '--- setup.cfg ---'; sed -n '1,160p' setup.cfg; }
[ -f setup.py ] && { echo '--- setup.py ---'; sed -n '1,120p' setup.py; }
[ -f requirements.txt ] && { echo '--- requirements.txt ---'; sed -n '1,80p' requirements.txt; }
# Inspect beginning of main.py for shebang / future imports
echo '--- src/web/main.py (top) ---'
sed -n '1,80p' src/web/main.pyRepository: GeiserX/Telegram-Archive
Length of output: 7691
Fix Python 3–invalid except syntax (line 938).
src/web/main.py uses Python-2 comma exception syntax: except OSError, ValueError, RuntimeError: which won’t parse under the project’s requires-python >= 3.14. Use tuple form; this comma-except pattern also appears elsewhere (e.g., src/web/main.py:925, src/web/thumbnails.py:141).
Diff
- except OSError, ValueError, RuntimeError:
+ except (OSError, ValueError, RuntimeError):📝 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.
Suggested change
| except OSError, ValueError, RuntimeError: | |
| except (OSError, ValueError, RuntimeError): |
🤖 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/web/main.py` at line 938, Replace Python-2 style comma-separated
exception handlers with tuple form: locate the flawed except clause using the
pattern "except OSError, ValueError, RuntimeError:" in src/web/main.py (and
similar patterns at the other noted locations, e.g., the earlier handler around
line 925 and in src/web/thumbnails.py around line 141) and change it to use the
tuple form "except (OSError, ValueError, RuntimeError):" so the code is valid
under Python 3; ensure any accompanying except blocks or variable bindings are
adapted consistently.
Comment on lines
+131
to
132
| alt_folders = legacy_folder_alternates(folder) | ||
| found = False |
There was a problem hiding this comment.
Protect fallback from invalid folder formats.
If folder is non-numeric, legacy_folder_alternates(folder) can raise and break thumbnail requests. Treat invalid folders as “no alternates”.
Proposed fix
- alt_folders = legacy_folder_alternates(folder)
+ try:
+ alt_folders = legacy_folder_alternates(folder)
+ except ValueError:
+ alt_folders = []🤖 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/web/thumbnails.py` around lines 131 - 132,
legacy_folder_alternates(folder) can throw for non-numeric folder values and
break thumbnail requests; guard the call by validating or normalizing folder
first and treat invalid formats as "no alternates" (empty list). Update the code
around the call to legacy_folder_alternates (where alt_folders and found are
set) to either check the folder is numeric (e.g., str(folder).isdigit() or
isinstance(folder, int)) before calling, or wrap the call in a try/except that
catches the specific exception (e.g., ValueError) and sets alt_folders = [] on
failure so thumbnail paths continue to be resolved safely.
| source = alt_source | ||
| found = True | ||
| break | ||
| except OSError, RuntimeError: |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -nP 'except\s+[^(\n:]+,\s*[^:\n]+:' src/web/main.py src/web/thumbnails.pyRepository: GeiserX/Telegram-Archive
Length of output: 876
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the exact code around the reported line
nl -ba src/web/thumbnails.py | sed -n '120,170p'
# Find all occurrences of the specific invalid syntax pattern in the repo
rg -n "except OSError, RuntimeError" -S src/web
# Check declared Python version / requirements (if present)
ls -la
rg -n "python_requires|Programming Language :: Python|requires-python" -S pyproject.toml setup.cfg setup.py requirements*.txt .github/workflows 2>/dev/null || trueRepository: GeiserX/Telegram-Archive
Length of output: 111
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Code context around the reported line
cat -n src/web/thumbnails.py | sed -n '120,165p'
# Locate the exact offending syntax in the repo
rg -n "except OSError, RuntimeError" -S src/web || true
# Show declared Python version (if any)
rg -n "python_requires|Programming Language :: Python|requires-python" -S pyproject.toml setup.cfg setup.py requirements*.txt .github/workflows 2>/dev/null || true
# Parse the file as Python 3 to check syntax validity
python3 - <<'PY'
import ast, pathlib, sys
p = pathlib.Path("src/web/thumbnails.py")
src = p.read_text(encoding="utf-8")
try:
ast.parse(src)
print("PY3_AST_PARSE_OK")
except SyntaxError as e:
print("PY3_AST_PARSE_SYNTAX_ERROR")
print(e)
PYRepository: GeiserX/Telegram-Archive
Length of output: 1871
Fix invalid multi-exception syntax in src/web/thumbnails.py (line 141).
except OSError, RuntimeError: is Python 2-style syntax and is invalid on Python 3 (the repo targets Python >=3.14), so it will fail module import.
Suggested fix
- except OSError, RuntimeError:
+ except (OSError, RuntimeError):📝 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.
Suggested change
| except OSError, RuntimeError: | |
| except (OSError, RuntimeError): |
🤖 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/web/thumbnails.py` at line 141, The except line uses Python2-style syntax
"except OSError, RuntimeError:" which is invalid in Python 3; update the
exception handling in the thumbnails module (around the try/except block that
currently references OSError and RuntimeError) to use modern exception tuple
syntax by catching both exceptions with "except (OSError, RuntimeError):" or
split into separate "except OSError:" and "except RuntimeError:" clauses as
appropriate, preserving the existing error handling logic in the surrounding
function/method.
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_media_utils.py`:
- Line 48: The test function test_non_numeric_folder_returns_empty_list lacks a
return type hint; update its signature to include "-> None" (i.e., change def
test_non_numeric_folder_returns_empty_list(self) to def
test_non_numeric_folder_returns_empty_list(self) -> None:) so it conforms to the
repository's typing rule for Python function signatures.
🪄 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: 7ebf9969-033a-441e-a37b-183577dc5081
📒 Files selected for processing (2)
src/web/media_utils.pytests/test_media_utils.py
✅ Files skipped from review due to trivial changes (1)
- src/web/media_utils.py
| self.assertEqual(len(result), 2) | ||
|
|
||
|
|
||
| def test_non_numeric_folder_returns_empty_list(self): |
There was a problem hiding this comment.
Add a return type hint to the new test method.
Please annotate the signature as def test_non_numeric_folder_returns_empty_list(self) -> None: to comply with the repo typing rule for Python function signatures.
As per coding guidelines, "**/*.py`: Use type hints for function signatures".
🤖 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_media_utils.py` at line 48, The test function
test_non_numeric_folder_returns_empty_list lacks a return type hint; update its
signature to include "-> None" (i.e., change def
test_non_numeric_folder_returns_empty_list(self) to def
test_non_numeric_folder_returns_empty_list(self) -> None:) so it conforms to the
repository's typing rule for Python function signatures.
|
🐳 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
❌ Your patch check has failed because the patch coverage (75.00%) is below the target coverage (90.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #169 +/- ##
==========================================
+ Coverage 92.55% 92.61% +0.06%
==========================================
Files 22 23 +1
Lines 6527 6557 +30
==========================================
+ Hits 6041 6073 +32
+ Misses 486 484 -2
🚀 New features to boost your workflow:
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| found = True | ||
| break | ||
| try: | ||
| alt_source = (media_root / alt / filename).resolve() |
| break | ||
| try: | ||
| alt_source = (media_root / alt / filename).resolve() | ||
| if alt_source.is_relative_to(media_root_resolved) and alt_source.exists(): |
…ry tests - Strip chat IDs from all debug log statements (PII compliance) - Fix thumbnail ACL bypass: ensure_thumbnail now returns resolved folder so serve_thumbnail checks ACL on the actual resolved path - Simplify serve_media path reconstruction (str(relative_to) vs manual join) - Guard folder_int <= 0 in legacy_folder_alternates - Fix import style in thumbnails.py (absolute → relative) - Add boundary tests for exact CHANNEL_ID_OFFSET inflection point - Add integration tests for ACL bypass scenario (deny + allow)
|
🐳 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/web/thumbnails.py (1)
143-144:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winUse tuple-style multi-exception syntax.
except OSError, RuntimeError:is Python 2 syntax; switch to tuple form for Python 3 compatibility.Proposed diff
- except OSError, RuntimeError: + except (OSError, RuntimeError):🤖 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/web/thumbnails.py` around lines 143 - 144, Replace the old Python 2-style exception clause `except OSError, RuntimeError:` with the Python 3 tuple-style form to catch both exceptions correctly; update the `except` in the thumbnails handling block to use `except (OSError, RuntimeError):` (keeping the same handling body like `continue`) and ensure no other nearby exception clauses use the deprecated syntax.src/web/main.py (1)
925-926:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winFix Python 3-invalid
exceptsyntax in media fallback block.
except OSError, ValueError:andexcept OSError, ValueError, RuntimeError:are invalid in Python 3 and can break module import.Proposed diff
- except OSError, ValueError: + except (OSError, ValueError): @@ - except OSError, ValueError, RuntimeError: + except (OSError, ValueError, RuntimeError):#!/bin/bash set -euo pipefail # Verify no Python2-style comma except clauses remain in this file. rg -nP 'except\s+[^(\n:]+,\s*[^:\n]+:' src/web/main.pyExpected result: no matches.
Also applies to: 938-939
🤖 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/web/main.py` around lines 925 - 926, Replace Python2-style comma-separated except clauses in the media fallback block with Python3 tuple-style exception groups: find the clauses written as "except OSError, ValueError:" and "except OSError, ValueError, RuntimeError:" in src/web/main.py (media fallback block) and change them to use parenthesized tuples like "except (OSError, ValueError):" and "except (OSError, ValueError, RuntimeError):"; ensure the except blocks retain their existing handling logic and then run the supplied rg command to verify no remaining comma-style excepts.
🤖 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_media_utils.py`:
- Around line 53-56: Add explicit return type hints "-> None" to the new test
function signatures so they conform to the repo typing rules: update the
signature of test_zero_folder_returns_empty_list to include "-> None" and do the
same for the other newly added test methods in this module (the other test
methods added in the same change); ensure each test def line uses the form "def
<function_name>(self) -> None:" (preserve existing names like
test_zero_folder_returns_empty_list).
---
Duplicate comments:
In `@src/web/main.py`:
- Around line 925-926: Replace Python2-style comma-separated except clauses in
the media fallback block with Python3 tuple-style exception groups: find the
clauses written as "except OSError, ValueError:" and "except OSError,
ValueError, RuntimeError:" in src/web/main.py (media fallback block) and change
them to use parenthesized tuples like "except (OSError, ValueError):" and
"except (OSError, ValueError, RuntimeError):"; ensure the except blocks retain
their existing handling logic and then run the supplied rg command to verify no
remaining comma-style excepts.
In `@src/web/thumbnails.py`:
- Around line 143-144: Replace the old Python 2-style exception clause `except
OSError, RuntimeError:` with the Python 3 tuple-style form to catch both
exceptions correctly; update the `except` in the thumbnails handling block to
use `except (OSError, RuntimeError):` (keeping the same handling body like
`continue`) and ensure no other nearby exception clauses use the deprecated
syntax.
🪄 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: 01222649-7d13-4b4f-af64-1058102c1368
📒 Files selected for processing (6)
src/web/main.pysrc/web/media_utils.pysrc/web/thumbnails.pytests/test_media_utils.pytests/test_web_coverage.pytests/test_web_thumbnails.py
Comment on lines
+53
to
+56
| def test_zero_folder_returns_empty_list(self): | ||
| """Folder '0' is not a valid Telegram entity ID.""" | ||
| self.assertEqual(legacy_folder_alternates("0"), []) | ||
|
|
There was a problem hiding this comment.
Add return type hints to new test methods.
Please annotate these new test signatures with -> None to match repo typing rules.
Proposed diff
- def test_zero_folder_returns_empty_list(self):
+ def test_zero_folder_returns_empty_list(self) -> None:
@@
- def test_reverse_exact_offset_treated_as_basic_group(self):
+ def test_reverse_exact_offset_treated_as_basic_group(self) -> None:
@@
- def test_exact_offset_treated_as_basic_group(self):
+ def test_exact_offset_treated_as_basic_group(self) -> None:Also applies to: 76-80, 142-145
🤖 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_media_utils.py` around lines 53 - 56, Add explicit return type
hints "-> None" to the new test function signatures so they conform to the repo
typing rules: update the signature of test_zero_folder_returns_empty_list to
include "-> None" and do the same for the other newly added test methods in this
module (the other test methods added in the same change); ensure each test def
line uses the form "def <function_name>(self) -> None:" (preserve existing names
like test_zero_folder_returns_empty_list).
…-depth Keep the pre-resolution ACL check (prevents file existence leakage), and add a secondary check only when the resolved folder differs from the requested folder (prevents bypass via legacy fallback).
|
🐳 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 |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/web/main.py (1)
930-930:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winFix Python-2
exceptsyntax insrc/web/main.py(breaks Python 3.14+).
pyproject.tomlrequires Python >=3.14, so these clauses are invalid syntax and will prevent the module from running:
except OSError, ValueError:(line 930) →except (OSError, ValueError):except OSError, ValueError, RuntimeError:(line 943) →except (OSError, ValueError, RuntimeError):Proposed fix
- except OSError, ValueError: + except (OSError, ValueError):🤖 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/web/main.py` at line 930, Replace Python-2 style comma-separated except clauses with tuple-style exceptions: change the clause "except OSError, ValueError:" to "except (OSError, ValueError):" and similarly change "except OSError, ValueError, RuntimeError:" to "except (OSError, ValueError, RuntimeError):" in src/web/main.py so the exception handlers use valid Python 3 syntax; update the two except statements found around the error-handling blocks (the handlers catching OSError and ValueError, and the one catching OSError, ValueError, RuntimeError) accordingly.
🤖 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.
Outside diff comments:
In `@src/web/main.py`:
- Line 930: Replace Python-2 style comma-separated except clauses with
tuple-style exceptions: change the clause "except OSError, ValueError:" to
"except (OSError, ValueError):" and similarly change "except OSError,
ValueError, RuntimeError:" to "except (OSError, ValueError, RuntimeError):" in
src/web/main.py so the exception handlers use valid Python 3 syntax; update the
two except statements found around the error-handling blocks (the handlers
catching OSError and ValueError, and the one catching OSError, ValueError,
RuntimeError) accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: bf4fd90b-54ab-440c-b415-d1c3d026f3e6
📒 Files selected for processing (1)
src/web/main.py
5 tasks
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
Addresses all critical, high, and medium findings from the 22-agent code review of PR #168.
folder[1:]on-1001234567890produced1001234567890(never existed on disk). Now uses proper arithmetic via sharedmedia_utils.py.mediatable. Added existence guard.-100Xvs-(10^12+X)). Fixed with arithmetic._BATCH_SIZE, collapse identical dialect branches, extractCHANNEL_ID_OFFSETconstant, add debug logging, fix except syntax.Changes
src/web/media_utils.pylegacy_folder_alternates(),legacy_marked_chat_ids(),derive_stale_folder()src/web/main.pysrc/web/thumbnails.pyalembic/versions/...013...scripts/entrypoint.shtests/test_media_utils.pyTest plan
pytest tests/test_media_utils.py— 25 tests passruff check+ruff format --check— cleanbash -n scripts/entrypoint.sh— syntax validSummary by CodeRabbit
New Version
New Features
Bug Fixes
Security / Access Control
Tests