Completes MAM API migration and hardens auth - #17
Merged
Conversation
- Removed Playwright dependency and the old MAMScraper implementation. - Introduced MAMApiAdapter for API-based scraping, providing a cleaner and faster interface. - Updated MetadataCoordinator to utilize the new adapter. - Adjusted tests to reflect changes in method names and functionality. - Updated documentation and logging to align with the new API usage. - Removed backward compatibility alias for MAMScraper.
…integration tests Co-authored-by: Copilot <copilot@github.com>
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (14)
📝 WalkthroughWalkthroughMigration from a Playwright-based MAM scraper to an httpx-based MAM JSON API client/adapter. Config moved from local JSON to env cookie (MAM_ID) with optional MAM_TEST_TID for integration tests. Playwright removed from requirements; client+adapter centralize auth/response validation and update metadata wiring and tests. ChangesMAM JSON API Migration
Sequence DiagramsequenceDiagram
autonumber
actor Webhook
participant Coord as metadata_coordinator
participant Adapter as MAMApiAdapter
participant Client as MamClient (httpx)
participant MAM as MAM API
participant Validator as ResponseValidator
Webhook->>Coord: webhook(url)
Coord->>Adapter: get_asin_from_url(url)
Adapter->>Adapter: extract_tid_from_url(url)
Adapter->>Client: search/download request
Client->>MAM: HTTP GET /search or /download
MAM-->>Client: HTTP response (200 / redirect / 401/403 / HTML)
Client->>Validator: _api_response_json / _raise_for_api_response
alt Auth redirect or 401/403 detected
Validator-->>Client: raise MamApiError
Client-->>Adapter: MamApiError
Adapter-->>Coord: propagate MamApiError (log + re-raise)
else Valid JSON / torrent bytes
Validator-->>Client: parsed JSON or validated bytes
Client-->>Adapter: parsed data
Adapter->>Adapter: extract ASIN / metadata enrichment
Adapter-->>Coord: asin or metadata
Coord->>Coord: proceed with downstream lookups
end
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~45 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)
Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 62d3ff0259
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
src/mam_api/client.py (1)
249-263: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueConsider validating torrent file content on download.
- Issue: With
follow_redirects=False, a non-login redirect could pass_raise_for_api_responseand return unexpected content (error page HTML, etc.) as raw bytes.- Impact: Caller receives corrupted data silently. Low probability since login redirects are caught, but non-auth redirects could still occur.
- Fix: Validate response starts with torrent magic bytes (
d8:announceor bencode dict startd).- Test: Mock redirect to non-login URL, verify MamApiError raised.
🛡️ Optional validation snippet
def _validate_torrent_content(content: bytes) -> None: """Raise if content doesn't look like a .torrent file.""" if not content.startswith(b"d"): raise MamApiError("Downloaded content is not a valid torrent file")Apply after
_raise_for_api_response(r)in download methods:_raise_for_api_response(r) +_validate_torrent_content(r.content) log.debug("mam.download.complete", tid=tid, size=len(r.content))Also applies to: 265-283
🤖 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/mam_api/client.py` around lines 249 - 263, The download_torrent_by_tid method currently returns raw bytes from self._client.get which can be non-torrent HTML if a non-auth redirect occurred; after calling _raise_for_api_response(r) in download_torrent_by_tid (and the sibling download method around lines 265-283), validate the returned bytes before returning by checking torrent/bencode magic bytes (e.g., content.startswith(b"d") or content.startswith(b"d8:announce")) and raise MamApiError (or similar existing API error) when the check fails so callers never receive corrupted/non-torrent data.src/metadata_coordinator.py (1)
64-75:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftDo not demote MAM auth failures into the Audible fallback path.
- Issue: This block catches everything from the new adapter call and continues the normal fallback flow.
- Impact: An expired/invalid
MAM_IDbecomes indistinguishable from “no ASIN found,” which defeats the PR’s auth-hardening goal.- Fix: Let the adapter surface a distinct auth error, catch
MamApiErrorexplicitly here, and return/raise a controlled auth failure instead of falling through to Audnex/Audible.- Test: Add a webhook test where
mam_adapter.get_asin_from_url()raisesMamApiErrorand assert that the auth error is surfaced.🤖 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/metadata_coordinator.py` around lines 64 - 75, The current try/except around self.mam_adapter.get_asin_from_url(url) swallows all exceptions and treats auth failures like "no ASIN"; change it to explicitly catch MamApiError from the adapter (e.g., except MamApiError as e:) and surface a controlled auth failure (raise or return an auth-specific exception/result) instead of falling through to the Audible fallback, while keeping the existing handlers for httpx.RequestError, ValueError and a generic Exception; also add a unit/webhook test that simulates mam_adapter.get_asin_from_url() raising MamApiError and asserts the coordinator surfaces the auth error rather than proceeding to the Audible path.src/mam_api/adapter.py (1)
120-121:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRedact the incoming URL before logging extraction failures.
- Issue: This warning logs the full URL verbatim.
- Impact: If a caller passes a tokenized MAM link, that token lands in logs.
- Fix: Log only scheme/host/path, or strip query/fragment before logging.
- Test: Add a log assertion with a URL containing
?token=...and verify the token never appears.🛡️ Proposed fix
- log.warning("mam.adapter.tid_extract_failed", url=url) + parsed = urlparse(url) + redacted_url = f"{parsed.scheme}://{parsed.netloc}{parsed.path}" + log.warning("mam.adapter.tid_extract_failed", url=redacted_url)🤖 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/mam_api/adapter.py` around lines 120 - 121, The warning currently logs the full incoming URL (log.warning("mam.adapter.tid_extract_failed", url=url)); change this to sanitize the URL first by parsing it (e.g., urllib.parse.urlparse) and reconstructing only scheme, netloc, and path (omitting query and fragment) or otherwise stripping query/fragment, then pass that sanitized string to log.warning; update any test to assert that a URL with ?token=... does not appear in logs (e.g., replace the log call in the tid extraction failure path to use the sanitized URL variable and add a unit test verifying the token is not present).tests/test_metadata_coordinator.py (1)
259-381: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winAdd a
MamApiErrorwebhook test for the new auth path.
- Issue: The exception matrix covers network, parse, and generic failures, but not the explicit MAM auth error introduced by this migration.
- Impact: CI will not catch regressions where stale
MAM_IDhandling silently falls back instead of surfacing the auth problem.- Fix: Add a case where
coordinator.mam_adapter.get_asin_from_urlraisesMamApiErrorand assert the coordinator handles it distinctly.- Test: That case should fail against the current behavior and pass once auth failures stop being treated like normal misses.
🤖 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_metadata_coordinator.py` around lines 259 - 381, Add an async pytest that simulates an auth failure by setting coordinator.mam_adapter.get_asin_from_url = AsyncMock(side_effect=MamApiError("Auth failed")) and then call coordinator.get_metadata_from_webhook(sample_webhook_payload) expecting the MamApiError to be raised (use pytest.raises(MamApiError)); reference coordinator.get_metadata_from_webhook and coordinator.mam_adapter.get_asin_from_url so the test verifies auth errors are surfaced rather than treated as normal misses.
🤖 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 `@docs/MAM_API_MIGRATION.md`:
- Around line 190-196: The example command only sets MAM_TEST_TID but the text
states both MAM_ID and MAM_TEST_TID are required; update the runnable example to
set both environment variables (MAM_ID and MAM_TEST_TID) inline in the command
or explicitly state that MAM_ID must already be exported in the shell; modify
the snippet so it shows something like setting MAM_ID and MAM_TEST_TID together
before running pytest (or add a note that MAM_ID must be exported) to ensure the
test is actually executed rather than skipped.
In `@docs/user-guide/troubleshooting.md`:
- Around line 137-155: In the "MAM API Auth Failed" section (heading "MAM API
Auth Failed") the ordered list uses "1." twice; update the second list item (the
step that begins with "Refresh the cookie value:") to use "2." so the steps read
"1." then "2." for correct sequential numbering.
---
Outside diff comments:
In `@src/mam_api/adapter.py`:
- Around line 120-121: The warning currently logs the full incoming URL
(log.warning("mam.adapter.tid_extract_failed", url=url)); change this to
sanitize the URL first by parsing it (e.g., urllib.parse.urlparse) and
reconstructing only scheme, netloc, and path (omitting query and fragment) or
otherwise stripping query/fragment, then pass that sanitized string to
log.warning; update any test to assert that a URL with ?token=... does not
appear in logs (e.g., replace the log call in the tid extraction failure path to
use the sanitized URL variable and add a unit test verifying the token is not
present).
In `@src/mam_api/client.py`:
- Around line 249-263: The download_torrent_by_tid method currently returns raw
bytes from self._client.get which can be non-torrent HTML if a non-auth redirect
occurred; after calling _raise_for_api_response(r) in download_torrent_by_tid
(and the sibling download method around lines 265-283), validate the returned
bytes before returning by checking torrent/bencode magic bytes (e.g.,
content.startswith(b"d") or content.startswith(b"d8:announce")) and raise
MamApiError (or similar existing API error) when the check fails so callers
never receive corrupted/non-torrent data.
In `@src/metadata_coordinator.py`:
- Around line 64-75: The current try/except around
self.mam_adapter.get_asin_from_url(url) swallows all exceptions and treats auth
failures like "no ASIN"; change it to explicitly catch MamApiError from the
adapter (e.g., except MamApiError as e:) and surface a controlled auth failure
(raise or return an auth-specific exception/result) instead of falling through
to the Audible fallback, while keeping the existing handlers for
httpx.RequestError, ValueError and a generic Exception; also add a unit/webhook
test that simulates mam_adapter.get_asin_from_url() raising MamApiError and
asserts the coordinator surfaces the auth error rather than proceeding to the
Audible path.
In `@tests/test_metadata_coordinator.py`:
- Around line 259-381: Add an async pytest that simulates an auth failure by
setting coordinator.mam_adapter.get_asin_from_url =
AsyncMock(side_effect=MamApiError("Auth failed")) and then call
coordinator.get_metadata_from_webhook(sample_webhook_payload) expecting the
MamApiError to be raised (use pytest.raises(MamApiError)); reference
coordinator.get_metadata_from_webhook and
coordinator.mam_adapter.get_asin_from_url so the test verifies auth errors are
surfaced rather than treated as normal misses.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 889fdff6-8e67-45f2-b379-4e2efe15cfc0
📒 Files selected for processing (27)
.circleci/config.yml.env.example.github/workflows/ci.yml.gitignoreaudiobook_dev.egg-info/PKG-INFOaudiobook_dev.egg-info/SOURCES.txtaudiobook_dev.egg-info/dependency_links.txtaudiobook_dev.egg-info/requires.txtaudiobook_dev.egg-info/top_level.txtconfig/mam_config.json.exampledocs/CONCURRENCY_ANALYSIS.mddocs/MAM_API_MIGRATION.mddocs/PR1_REVIEW_FIXES.mddocs/SYSTEM_COMPLETION_SUMMARY.mddocs/development/PLAYWRIGHT_ASYNC_FIX_SUMMARY.mddocs/development/contributing.mddocs/user-guide/configuration.mddocs/user-guide/troubleshooting.mdrequirements.txtsrc/archive/mam_scraper.py.oldsrc/mam_api/__init__.pysrc/mam_api/adapter.pysrc/mam_api/client.pysrc/metadata_coordinator.pytests/test_audnex_direct.pytests/test_mam_api.pytests/test_metadata_coordinator.py
💤 Files with no reviewable changes (11)
- .github/workflows/ci.yml
- audiobook_dev.egg-info/PKG-INFO
- .circleci/config.yml
- audiobook_dev.egg-info/requires.txt
- docs/development/PLAYWRIGHT_ASYNC_FIX_SUMMARY.md
- audiobook_dev.egg-info/SOURCES.txt
- src/archive/mam_scraper.py.old
- requirements.txt
- src/mam_api/init.py
- config/mam_config.json.example
- audiobook_dev.egg-info/top_level.txt
…rectories from Bandit analysis, refining version retrieval logic, and optimizing qBittorrent configuration handling.
…rrent content, and updating documentation for cookie usage in tests Co-authored-by: Copilot <copilot@github.com>
…amApiError test Co-authored-by: Copilot <copilot@github.com>
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.
Why
What it improves
Summary by CodeRabbit
New Features
Improvements
Documentation