feat(creator): add notebook-backed youtube control review - #884
POWERFULMOVES merged 12 commits into
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis PR introduces YouTube control action management to the PMOVES platform. It extends the channel-monitor service with new request queueing and review workflows, adds a database schema for action persistence with row-level security, integrates Discord interaction handling in the messaging-gateway, and expands documentation with creator-focused traversal guides and a new worktree review runbook for PMOVES.YT integration testing. Changes
Sequence DiagramsequenceDiagram
participant Discord as Discord User
participant MG as Messaging Gateway
participant CM as Channel Monitor
participant DB as Database
participant YTC as YouTube Control<br/>(External)
Discord->>MG: Interaction (approve/reject ytcontrol)
MG->>MG: _handle_ytcontrol_interaction<br/>Extract actor & action
MG->>CM: POST /api/monitor/youtube-control/review<br/>(action_ids, approve, actor)
CM->>CM: review_youtube_control_actions<br/>Validate request & action
alt Approve
CM->>DB: Update status → approved<br/>Set approved_by, approval_note
CM->>YTC: _invoke_yt_control_action<br/>(action, details)
YTC-->>CM: Result (success/error)
CM->>DB: Update result & error fields
else Reject
CM->>DB: Update status → rejected<br/>Set approved_by, approval_note
end
CM-->>MG: Review results
MG-->>Discord: Interaction response<br/>(approval/rejection confirmation)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
|
@codex review\n@coderabbitai review\n@kilocode review\n\nFocused review asks:\n- notebook-backed creator-control request flow in channel-monitor\n- Discord approval/rejection UX and summary payloads\n- PMOVES.YT control-plane surface for playlist add/remove/reorder + comment actions |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
🧩 Analysis chain🏁 Script executed: Length of output: 148912
1. Notebook-backed creator-control request flow (
|
| Action | Endpoint | Execution payload keys |
|---|---|---|
playlist_add |
/yt/control/playlist/add |
playlist_id✱, video_id✱, position, execute, approved_by |
playlist_remove |
/yt/control/playlist/remove |
playlist_item_id✱, playlist_id, video_id, execute, approved_by |
playlist_reorder |
/yt/control/playlist/reorder |
playlist_item_id✱, playlist_id, video_id, position✱, execute, approved_by |
comment_create |
/yt/control/comment |
video_id✱, text✱, parent_comment_id, execute, approved_by |
✱ = logically required
🟡 Major: No upfront validation of required fields before DB persist
_build_youtube_control_execution_payload uses _compact(...) which strips None values, and falls back to {} if all are None. This means a playlist_add request missing both playlist_id and video_id is accepted and stored, only to fail silently at execution time against PMOVES.YT.
Add field guards in _prepare_youtube_control_details:
if action == "playlist_add":
if not details.get("playlist_id") or not details.get("video_id"):
raise ValueError("playlist_add requires playlist_id and video_id")
elif action == "playlist_remove":
if not details.get("playlist_item_id"):
raise ValueError("playlist_remove requires playlist_item_id")
elif action == "playlist_reorder":
if not details.get("playlist_item_id") or details.get("position") is None:
raise ValueError("playlist_reorder requires playlist_item_id and position")Minor / Nits
- No Prometheus metrics for YT control actions.
DISCORD_DROP_REVIEWS_TOTALhas parallels for Discord drops; aYOUTUBE_CONTROL_TOTALcounter withactionandresultlabels would make this observable. asyncio.wait([self._shutdown.wait()], ...)in_channel_looppasses a bare coroutine toasyncio.wait, which is deprecated since Python 3.8 and raises aDeprecationWarning. Wrap withasyncio.ensure_future(self._shutdown.wait())or useasyncio.wait_for/asyncio.sleepwithEventpolling. (Pre-existing, not introduced in this PR.)YouTubeControlRequestmodel has noactionfield validator. Adding@validator("action")to check membership inYOUTUBE_CONTROL_ACTION_LABELSat the API boundary would fail fast with a clear 422 rather than a 400 from deep in the call stack.
Summary
| # | Severity | Area | Issue |
|---|---|---|---|
| 1 | 🔴 Critical | monitor | Execution failure leaves action stuck in pending_review (no error capture) |
| 2 | 🔴 Critical | monitor | Race condition — no SELECT FOR UPDATE before concurrent approval |
| 3 | 🟡 Major | monitor | Invalid actions accepted at queue time, fail only at execution |
| 4 | 🟡 Major | monitor | _yt_control_base_url fragile suffix stripping |
| 5 | 🟡 Major | monitor | No required-field validation for playlist/comment payloads before DB persist |
| 6 | 🟡 Major | messaging-gateway | HTTP error response body discarded in _invoke_yt_control_action |
| 7 | 🟡 Minor | messaging-gateway | Full details in notification metadata; consider trimming |
| 8 | 🟢 Good | messaging-gateway | Ephemeral responses, notebook entry surfaced in confirmation |
| 9 | 🟢 Good | monitor | Notebook opt-in guard and error-state propagation both correct |
✅ Actions performed
Review triggered.
Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.
|
Addressed the review findings in the creator-control lane. Changes pushed in
Validation run locally:
@coderabbitai review |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 18
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
pmoves/services/channel-monitor/channel_monitor/config.py (1)
31-38:⚠️ Potential issue | 🟠 MajorDerive
source_classfromsource_typeinstead of hardcoding the channel default.This default is merged into all entries via
DEFAULT_CONFIG["channels"][0], so older configs with playlist rows that don't yet havesource_classwill now be silently classified as"watched"instead of"owned".🐛 Proposed fix
@@ for channel in merged.get("channels", []): merged_channel = DEFAULT_CONFIG["channels"][0].copy() merged_channel.update(channel) + if not merged_channel.get("source_class"): + merged_channel["source_class"] = ( + "owned" if merged_channel.get("source_type") == "playlist" else "watched" + ) if merged_channel.get("channel_metadata_fields") is None: merged_channel["channel_metadata_fields"] = None🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pmoves/services/channel-monitor/channel_monitor/config.py` around lines 31 - 38, DEFAULT_CONFIG currently hardcodes "source_class" in DEFAULT_CONFIG["channels"][0], causing entries missing source_class (e.g., old playlist rows) to inherit "watched"; remove the hardcoded "source_class" from the default channel entry and update the merge/normalize logic that applies DEFAULT_CONFIG["channels"][0] to per-row data so that when source_class is missing you set it based on source_type (e.g., if source_type == "playlist" -> "owned", otherwise -> "watched"); reference DEFAULT_CONFIG and the keys source_class and source_type when making these changes.pmoves/docs/PMOVES.AI PLANS/JELLYFIN_YOUTUBE_INTEGRATION.md (1)
152-167:⚠️ Potential issue | 🟡 MinorSetup instructions reference deprecated MCP YouTube Adapter.
The status update at lines 7-10 states that the MCP YouTube Adapter is "historical unless you explicitly choose to run that extra service," but Step 3 (lines 152-167) still instructs users to start the MCP YouTube Adapter via uvicorn. This creates confusion about which path to follow.
Consider updating Step 3 to reference the PMOVES.YT runtime validation from lines 65-71, or adding a note clarifying when users should follow the legacy MCP adapter path vs. the authoritative PMOVES.YT runtime.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pmoves/docs/PMOVES.AI` PLANS/JELLYFIN_YOUTUBE_INTEGRATION.md around lines 152 - 167, Update Step 3 to remove the default instruction to start the legacy MCP YouTube Adapter via the uvicorn command (services.mcp_youtube_adapter:app) and instead point users to the PMOVES.YT runtime validation path (referencing the PMOVES.YT runtime validation described earlier) as the authoritative option; alternatively add a short clarifying note in Step 3 that the uvicorn/mcp_youtube_adapter start is only required if users explicitly opt into the historical MCP YouTube Adapter, and show when to choose the legacy adapter versus PMOVES.YT runtime validation so readers are not confused.
🧹 Nitpick comments (7)
pmoves/supabase/migrations/20260312095500_youtube_control_actions.sql (1)
23-24: Index review-queue lookups bystatusfirst.The new flow lists/reviews pending actions newest-first.
(action, status)will not help aWHERE status = ... ORDER BY created_at DESCquery, so this will get more expensive as the audit table grows.Suggested index
create index if not exists idx_youtube_control_actions_action_status on pmoves_core.youtube_control_actions (action, status); + +create index if not exists idx_youtube_control_actions_status_created_at + on pmoves_core.youtube_control_actions (status, created_at desc);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pmoves/supabase/migrations/20260312095500_youtube_control_actions.sql` around lines 23 - 24, The current index idx_youtube_control_actions_action_status on pmoves_core.youtube_control_actions is ordered (action, status) which doesn't support queries like WHERE status = ... ORDER BY created_at DESC efficiently; add or replace with an index that begins with status and includes created_at (descending) and action (e.g., (status, created_at DESC, action)) so review-queue lookups (filter by status and order by created_at DESC) and any tie-breakers on action are covered; update the migration to create the new index name and drop or no-op the old idx_youtube_control_actions_action_status if desired.pmoves/services/messaging-gateway/main.py (1)
68-69: Consider using*_FILEsecret loading pattern for production hardening.Per the coding guidelines, focus services should resolve critical secrets through Docker/K8s-style file mounts via
services/common/env.py. TheCHANNEL_MONITOR_SECRETis loaded directly fromos.environ.get, which works but doesn't follow the preferred secret hardening convention.♻️ Example using *_FILE pattern
+from services.common.env import get_secret + # Environment configuration NATS_URL = os.environ.get("NATS_URL", "nats://nats:pmoves@nats:4222") ... CHANNEL_MONITOR_URL = os.environ.get("CHANNEL_MONITOR_URL", "http://channel-monitor:8097") -CHANNEL_MONITOR_SECRET = os.environ.get("CHANNEL_MONITOR_SECRET", "") +CHANNEL_MONITOR_SECRET = get_secret("CHANNEL_MONITOR_SECRET", "")As per coding guidelines, "Focus services now resolve critical secrets through Docker/K8s-style file mounts (
services/common/env.py), and the audit gate now blocks regressions to directos.getenvsecret reads."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pmoves/services/messaging-gateway/main.py` around lines 68 - 69, Replace the direct os.environ.get usage for CHANNEL_MONITOR_SECRET with the project *_FILE secret-loading pattern used by services/common/env.py: use the helper (or the same logic) to check CHANNEL_MONITOR_SECRET_FILE first and read the file contents, falling back to CHANNEL_MONITOR_SECRET env var only if the file isn't present; leave CHANNEL_MONITOR_URL behavior unchanged but ensure you reference CHANNEL_MONITOR_SECRET by name in the updated code so the audit gate recognizes the hardened secret loading.pmoves/services/messaging-gateway/test_main.py (1)
112-151: Consider adding error path test coverage.The approval and rejection happy paths are well tested. Consider adding tests for:
- HTTP error responses from channel monitor
- Invalid
custom_idformats (missing parts, wrong prefix)- Network failures (httpx exceptions)
💡 Example error path test
def test_handle_ytcontrol_interaction_http_error(monkeypatch): module = _load_main_module() class DummyResponse: status_code = 500 def raise_for_status(self): raise module.httpx.HTTPStatusError("Server Error", request=None, response=self) class DummyAsyncClient: def __init__(self, *args, **kwargs): pass async def __aenter__(self): return self async def __aexit__(self, exc_type, exc, tb): return False async def post(self, url, headers=None, json=None): return DummyResponse() monkeypatch.setattr(module.httpx, "AsyncClient", DummyAsyncClient) payload = { "type": 3, "data": {"custom_id": "ytcontrol:approve:test-id"}, "user": {"id": "99"}, } response = module.asyncio.run(module._handle_ytcontrol_interaction(payload)) assert response["type"] == 4 assert "failed" in response["data"]["content"].lower()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pmoves/services/messaging-gateway/test_main.py` around lines 112 - 151, Add error-path tests for _handle_ytcontrol_interaction by creating new test functions (e.g., test_handle_ytcontrol_interaction_http_error, test_handle_ytcontrol_interaction_invalid_custom_id, test_handle_ytcontrol_interaction_network_failure) that mirror the existing test style: monkeypatch module.httpx.AsyncClient with a DummyAsyncClient and DummyResponse, but simulate failure modes — have DummyResponse.raise_for_status raise module.httpx.HTTPStatusError to cover HTTP errors, provide payloads with malformed custom_id (missing parts or wrong prefix) to assert the function returns a user-facing failure message, and have DummyAsyncClient.post raise module.httpx.ConnectError (or similar) to test network failures; use module.asyncio.run(module._handle_ytcontrol_interaction(payload)) and assert response["type"] == 4 and that the content indicates the failure.pmoves/services/channel-monitor/tests/test_monitor.py (1)
623-677: Test does not cover execution failure scenarios.The PR comments identified a critical issue: exceptions during execution in
review_youtube_control_actionscan leave rows stuck in "pending_review" status. This test only covers the happy path where execution succeeds. Consider adding a test for execution failure to verify error handling and status updates.Do you want me to generate a test case for execution failure scenarios?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pmoves/services/channel-monitor/tests/test_monitor.py` around lines 623 - 677, Add a new test alongside test_review_youtube_control_actions_executes_pmoves_yt_call that simulates an HTTP execution failure for monitor.review_youtube_control_actions: create a DummyAsyncClient whose post method raises an exception (or returns a response whose raise_for_status raises), set CHANNEL_MONITOR_YT_API_KEY and monkeypatch channel_monitor.monitor.httpx.AsyncClient, and use a fake DB connection (as in the existing test) to assert the function marks the action as processed with an error (result shows failed/errored), increments processed count, and calls conn.execute to update the row status from "pending_review" to the appropriate failed state; reference monitor.review_youtube_control_actions, the test function name, DummyAsyncClient, and conn.execute for locating the changes.pmoves/services/channel-monitor/channel_monitor/main.py (1)
644-691: Consider adding Prometheus metrics for YouTube control actions.The existing Discord drop endpoints have corresponding metrics (
DISCORD_DROPS_TOTAL,DISCORD_DROP_REVIEWS_TOTAL). For observability parity, consider adding metrics for YouTube control actions to track queue, approval, and rejection rates.📊 Proposed metrics additions
YOUTUBE_CONTROL_TOTAL = Counter( "channel_monitor_youtube_control_total", "Total number of YouTube control requests queued", labelnames=("action", "result"), ) YOUTUBE_CONTROL_REVIEWS_TOTAL = Counter( "channel_monitor_youtube_control_reviews_total", "Total number of YouTube control review actions", labelnames=("action", "decision"), # decision: approved/rejected )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pmoves/services/channel-monitor/channel_monitor/main.py` around lines 644 - 691, Add Prometheus counters for YouTube control parity with Discord drops and increment them in the existing endpoints: declare YOUTUBE_CONTROL_TOTAL (labels: action, result) and YOUTUBE_CONTROL_REVIEWS_TOTAL (labels: action, decision) alongside the other metrics; in queue_youtube_control_action, after successful monitor.create_youtube_control_request increment YOUTUBE_CONTROL_TOTAL with action=payload.action and result="queued" (or "draft" if payload.draft is true) and on the ValueError path increment with result="error"; in review_youtube_control_actions increment YOUTUBE_CONTROL_REVIEWS_TOTAL for each reviewed action with action equal to the action type and decision "approved" or "rejected" based on payload.approve; place metric declarations near existing metric definitions and import/instrument them the same way as DISCORD_DROPS_TOTAL and DISCORD_DROP_REVIEWS_TOTAL.pmoves/services/channel-monitor/channel_monitor/monitor.py (2)
1976-1982: Consider trimming metadata payload sent to messaging gateway.The full
detailsdict (including drafts, rendered text, notebook metadata) is included in notification metadata. As noted in PR comments, this could be verbose. Consider trimming to a concise summary for the notification payload.✂️ Proposed trimmed metadata
payload = { "platforms": active_platforms, "content": content, "embeds": [...], "buttons": buttons, "metadata": { "action_id": action_id, "action": action, "request_source": request_source, - "details": details, "summary": summary, + "notebook_entry_id": notebook_meta.get("entry_id"), }, }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pmoves/services/channel-monitor/channel_monitor/monitor.py` around lines 1976 - 1982, The metadata currently embeds the full details dict (in the block building "metadata" with keys action_id, action, request_source, details, summary) which can be very verbose; replace the raw details value with a trimmed summary by implementing a small sanitizer (e.g., trim_notification_details or summarize_details) that extracts only essential fields (title/user/id/timestamps/concise change text or a generated short summary) and explicitly omits heavy fields like drafts, rendered_text, notebook metadata; then update the metadata construction to use the trimmed result (e.g., details_summary) instead of the full details object so notifications sent from monitor.py contain a compact payload.
2063-2068: API key loading doesn't support*_FILEsecret paths.Per coding guidelines, prefer central env helpers and
*_FILEsecret loading paths for hardened secret handling. The current implementation reads API keys directly from environment variables without supporting file-based secrets.🔐 Proposed enhancement for secret file support
+def _load_secret(env_var: str, file_env_var: str | None = None) -> str: + """Load secret from env var or file path.""" + if file_env_var: + file_path = os.getenv(file_env_var, "").strip() + if file_path and os.path.isfile(file_path): + with open(file_path) as f: + return f.read().strip() + return (os.getenv(env_var) or "").strip() async def _invoke_yt_control_action( # ... ) -> Dict[str, Any]: # ... headers: Dict[str, str] = {} - api_key = ( - os.getenv("CHANNEL_MONITOR_YT_API_KEY") - or os.getenv("NEXT_PUBLIC_BACKEND_API_KEY") - or os.getenv("BACKEND_API_KEY") - or "" - ).strip() + api_key = ( + _load_secret("CHANNEL_MONITOR_YT_API_KEY", "CHANNEL_MONITOR_YT_API_KEY_FILE") + or _load_secret("BACKEND_API_KEY", "BACKEND_API_KEY_FILE") + )As per coding guidelines, "Prefer central env helpers and *_FILE secret loading paths."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pmoves/services/channel-monitor/channel_monitor/monitor.py` around lines 2063 - 2068, Replace the direct os.getenv chain that assigns api_key in monitor.py with the project’s central env helper that supports *_FILE secret paths; specifically, ensure you resolve CHANNEL_MONITOR_YT_API_KEY, NEXT_PUBLIC_BACKEND_API_KEY, and BACKEND_API_KEY via the helper (or implement a small helper like get_secret_from_env_or_file(var)) which checks VAR_FILE first, reads the file contents if present, falls back to VAR, and returns a trimmed string, then assign that result to api_key (preserve the .strip() behavior).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@PMOVES-transcribe-and-fetch`:
- Line 1: The PMOVES-transcribe-and-fetch submodule update references a
non-existent commit SHA (a4968929b22f5f2625836aa9a1dfdcf6ba3e5459); verify the
SHA in the submodule repo and either push the missing commit to that remote or
update the submodule reference to a valid commit: checkout the
PMOVES-transcribe-and-fetch repository, confirm the correct commit SHA (or
create and push the intended commit), then in the superproject update the
submodule to that SHA (or run git submodule update --init --remote), commit the
updated submodule reference in the superproject, and push the change so the PR
points to an existing commit.
In `@pmoves/docs/AGENTS/PMOVES_YT_CONTROL_WORKTREE_REVIEW.md`:
- Around line 184-187: The sentence claiming "comment/reply actions" for the
first owned-channel YouTube Data API slice overstates capabilities; update the
PMOVES.YT description so it matches the supported-action list (`playlist_add`,
`playlist_remove`, `playlist_reorder`, `comment_create`) by removing "reply" or
explicitly stating only `comment_create` is supported (e.g., "comment create
only"), and ensure the phrase "comment/reply actions" is replaced wherever
present with the accurate term to keep the runbook and snapshot consistent.
In `@pmoves/docs/PMOVES.AI` PLANS/JELLYFIN_YOUTUBE_STATUS.md:
- Around line 5-10: The document's "Current Status" contradicts the quickstart:
it claims PMOVES.YT is the authoritative runtime and that POST /yt/search and
pmoves/scripts/backfill_jellyfin_metadata.py already implement the backfill, yet
the quickstart still instructs implementing that work; update the page so the
quickstart and status align by either (A) rewriting the quickstart into
validation/runbook steps that confirm POST /yt/search and the backfill script
behavior (e.g., tests to call POST /yt/search, expected responses, and how to
enable --link-youtube in pmoves/scripts/backfill_jellyfin_metadata.py), or (B)
moving the historical implementation notes into a clearly labeled "Historical
Appendix" and leaving the operational status and runbook/smoke-test instructions
in the main body; also ensure the status claims reference existing evidence
(PMOVES.YT runtime, POST /yt/search, and backfill script) and update any
runbook/smoke-test links mentioned in the doc.
In `@pmoves/docs/PMOVES.AI` PLANS/PMOVES.yt/CHANNEL_MONITOR_IMPLEMENTATION.md:
- Around line 14-17: The "Current status note (March 12, 2026)" claims the
service is beyond prototype but downstream sections (the header, checklist, and
status tracker) still present pending work; reconcile them by either updating
those sections to reflect completed items or explicitly marking them as
historical/archival. Specifically, edit the header to show "Production-ready"
(or "Historical" if archived), update the checklist items referenced in the
document to completed/removed and add dates/PR links for evidence, and adjust
the status tracker entries to reflect current production concerns (preferences,
observability, alignment) or prepend a clear "Historical (as of <date>):" label;
also add a short note linking to the authoritative PMOVES.YT runtime/docs and
runbooks/smoke tests to justify the status.
In `@pmoves/docs/services/pmoves-yt/README.md`:
- Around line 7-8: Replace the absolute Windows path in the README with a
repository-relative link to the PMOVES.YT submodule; specifically change the
link target "C:/Users/russe/Documents/GitHub/PMOVES.AI/PMOVES.YT" to a
repo-relative path like "pmoves_yt_service/" (or "./pmoves_yt_service/" or the
correct relative path to the submodule) so the authoritative runtime reference
under pmoves_yt_service/ resolves for all users and the note about
pmoves/services/pmoves-yt remaining a compatibility shim stays accurate.
In `@pmoves/services/channel-monitor/channel_monitor/main.py`:
- Around line 330-348: Add a Pydantic validator on YouTubeControlRequest.action
to enforce allowed values ("playlist_add", "playlist_remove",
"playlist_reorder", "comment_create") and raise a ValueError for any other
string so invalid actions fail fast at validation time; implement this by adding
a `@validator`("action") method on the YouTubeControlRequest class that checks
membership in the allowed set and returns the value or raises an error with a
clear message.
In `@pmoves/services/channel-monitor/channel_monitor/monitor.py`:
- Around line 344-351: Update the _yt_control_base_url method to first check for
an explicit CHANNEL_MONITOR_YT_CONTROL_URL environment variable and return it if
present; if not present, robustly derive the control base from self.queue_url by
parsing the URL (use urllib.parse) and removing only the known path segments
(/yt/ingest or /yt) rather than brittle string slicing, preserving scheme/netloc
and any query/fragment removal, and ensure the method returns a normalized URL
string; reference the _yt_control_base_url method and self.queue_url and use
os.getenv("CHANNEL_MONITOR_YT_CONTROL_URL") as the fallback lookup.
- Around line 2081-2129: The review_youtube_control_actions flow currently
fetches pending rows then processes them, causing a race where multiple workers
can pick the same id and no exception handling around _invoke_yt_control_action
means failures leave rows stuck in pending_review; fix by atomically claiming
rows from pmoves_core.youtube_control_actions (use SELECT ... FOR UPDATE SKIP
LOCKED inside a transaction or an UPDATE ... RETURNING to set a temporary status
like "processing" for the wanted_ids) before calling _invoke_yt_control_action,
and wrap the call in try/except so that on exception you update the row status
to a terminal failure state (e.g., "failed" or "rejected") and record the error
in result_payload/notes; ensure processed_ids/action_summaries and the final DB
UPDATE/INSERT use the new_status/result_payload values so no row remains pending
after an exception.
In `@pmoves/services/channel-monitor/README.md`:
- Around line 191-197: Add the five new control-plane environment variables to
the README Environment table: include entries for CHANNEL_MONITOR_YT_API_KEY,
CHANNEL_MONITOR_MESSAGING_URL, CHANNEL_MONITOR_YT_NOTEBOOK_ID,
OPEN_NOTEBOOK_API_URL, and OPEN_NOTEBOOK_API_TOKEN; for each entry state purpose
(e.g., API key for PMOVES.YT control endpoints, messaging gateway /v1/send for
approval notifications, notebook ID and OpenNotebook API URL/token for
publishing review artifacts), whether it is required, and an example format or
default; ensure the descriptions mention that CHANNEL_MONITOR_MESSAGING_URL is
used to route ytcontrol button interactions back to POST
/api/monitor/youtube-control/review and that CHANNEL_MONITOR_YT_API_KEY is
needed when PMOVES.YT requires X-API-Key.
- Around line 155-180: The README's owned-channel PMOVES.YT control action
example is using the wrong draft.source_class value; locate the sample payload
in the "owned-channel PMOVES.YT control actions" section and change the
draft.variables/draft.channel_name/draft.source_class block so that
"source_class" is "owned" (not "watched"), and update any nearby wording or
examples that imply third-party sources to reflect this is an owned-channel
flow.
In `@pmoves/services/channel-monitor/tests/test_monitor.py`:
- Around line 673-676: The test is brittle because _yt_control_base_url() only
handles a few hardcoded suffixes and there's no env override; update
_yt_control_base_url to accept a clear environment override (e.g.,
YT_CONTROL_BASE_URL) and robustly normalize queue URLs by trimming trailing
slashes and removing either "/yt/ingest" or "/yt" only if they appear as path
segments (not as substrings), then append "/control/comment"; update tests to
use the new env override and add cases for trailing slashes and non-standard
queue URLs to assert constructed URLs and preserve existing behavior when no
override is provided.
In `@pmoves/services/messaging-gateway/main.py`:
- Around line 184-198: The exception handler is losing server diagnostic info
because response.raise_for_status() is called before capturing the body; modify
the post call flow in the async block where httpx.AsyncClient is used so you
read response.text() or response.json() into a local (e.g., response_text /
body) immediately after receiving the response and before calling
response.raise_for_status(), then include that captured response in the
logger.warning and in the returned error "content" string (referencing response,
response.raise_for_status, logger, action_id, and review_payload to locate the
code). Ensure you still call raise_for_status() after capturing the body so HTTP
errors propagate to the except block with the response content available for
logs and the returned payload.
In `@pmoves/services/pmoves-yt/docs_catalog.py`:
- Around line 8-12: The code currently unconditionally inserts _SUBMODULE_ROOT
into sys.path and blindly imports pmoves_yt_service; instead verify the
authoritative submodule exists before mutating sys.path: check that
(_SUBMODULE_ROOT / "pmoves_yt_service") is a directory and contains a package
indicator (e.g., __init__.py) or that importlib.util.find_spec for that package
at that location would succeed, and if the check fails raise an explicit
ImportError/RuntimeError; only then insert str(_SUBMODULE_ROOT) into sys.path
and perform from pmoves_yt_service.docs_catalog import * so that
pmoves_yt_service is reliably loaded from the intended submodule rather than a
different installed package.
In `@pmoves/services/pmoves-yt/docs_sync.py`:
- Around line 8-12: The shim currently unconditionally inserts _SUBMODULE_ROOT
into sys.path which can mask a missing PMOVES.YT checkout; update the logic
around _SUBMODULE_ROOT (the pathlib Path defined at top) so you check
_SUBMODULE_ROOT.is_dir() before calling sys.path.insert(0,
str(_SUBMODULE_ROOT)), and if the directory does not exist raise a clear
exception (or call sys.exit) so the import from pmoves_yt_service.docs_sync
fails fast instead of silently pointing to an installed package.
In `@pmoves/services/pmoves-yt/README.md`:
- Around line 40-44: In the "Production defaults" section of README.md remove
the absolute Windows path reference to docker-compose.yml and replace it with a
relative or repository-root path (e.g., ./docker-compose.yml or a repo link) so
the docs are cross-platform; update the sentence that currently mentions
"docker-compose.yml" to reference the relative path and ensure the string
"YT_PLAYER_CLIENT=web_safari" and the note about keeping the compose override
aligned remain unchanged.
- Around line 5-15: The README currently contains absolute Windows paths (e.g.
"C:/Users/russe/Documents/GitHub/PMOVES.AI/PMOVES.YT/...") — update those to
relative repository paths so they work across environments; replace each
absolute path with a relative path from the repo root such as
"PMOVES.YT/pmoves_yt_service/yt.py", "PMOVES.YT/pmoves_yt_service/docs_sync.py",
"PMOVES.YT/pmoves_yt_service/docs_catalog.py" and "PMOVES.YT/docs/RUNTIME.md"
and ensure the PMOVES.YT submodule reference remains consistent (keep the
`pmoves_yt_service/` suffix used in imports and docs).
In `@pmoves/supabase/migrations/20260312095500_youtube_control_actions.sql`:
- Line 5: Add a DB-level constraint to reject unsupported action values by
altering the youtube_control_actions table: define the allowed set (e.g.,
"play","pause","stop","seek", etc.) and either create a Postgres ENUM type for
action and set the column to that type, or add a CHECK constraint on the action
column (e.g., ADD CONSTRAINT check_youtube_action CHECK (action IN (...))).
Update the migration script that currently defines "action text not null" to
instead create the enum type or add the CHECK constraint so inserts with invalid
actions fail at the DB level.
- Around line 36-39: The RLS policy "Public read youtube control actions" on
pmoves_core.youtube_control_actions uses using (true) which exposes sensitive
columns (approval_note, details, result, error); either remove this blanket
policy and associated grants and instead create a sanitized view/select-only
policy that explicitly allows only safe columns for anon/authenticated, or if
full access is intentional add
pmoves/supabase/migrations/20260312095500_youtube_control_actions.sql to the SQL
policy lint allowlist per LOCAL_CI_CHECKS.md Section 4 so the
chit-contract-check passes.
---
Outside diff comments:
In `@pmoves/docs/PMOVES.AI` PLANS/JELLYFIN_YOUTUBE_INTEGRATION.md:
- Around line 152-167: Update Step 3 to remove the default instruction to start
the legacy MCP YouTube Adapter via the uvicorn command
(services.mcp_youtube_adapter:app) and instead point users to the PMOVES.YT
runtime validation path (referencing the PMOVES.YT runtime validation described
earlier) as the authoritative option; alternatively add a short clarifying note
in Step 3 that the uvicorn/mcp_youtube_adapter start is only required if users
explicitly opt into the historical MCP YouTube Adapter, and show when to choose
the legacy adapter versus PMOVES.YT runtime validation so readers are not
confused.
In `@pmoves/services/channel-monitor/channel_monitor/config.py`:
- Around line 31-38: DEFAULT_CONFIG currently hardcodes "source_class" in
DEFAULT_CONFIG["channels"][0], causing entries missing source_class (e.g., old
playlist rows) to inherit "watched"; remove the hardcoded "source_class" from
the default channel entry and update the merge/normalize logic that applies
DEFAULT_CONFIG["channels"][0] to per-row data so that when source_class is
missing you set it based on source_type (e.g., if source_type == "playlist" ->
"owned", otherwise -> "watched"); reference DEFAULT_CONFIG and the keys
source_class and source_type when making these changes.
---
Nitpick comments:
In `@pmoves/services/channel-monitor/channel_monitor/main.py`:
- Around line 644-691: Add Prometheus counters for YouTube control parity with
Discord drops and increment them in the existing endpoints: declare
YOUTUBE_CONTROL_TOTAL (labels: action, result) and YOUTUBE_CONTROL_REVIEWS_TOTAL
(labels: action, decision) alongside the other metrics; in
queue_youtube_control_action, after successful
monitor.create_youtube_control_request increment YOUTUBE_CONTROL_TOTAL with
action=payload.action and result="queued" (or "draft" if payload.draft is true)
and on the ValueError path increment with result="error"; in
review_youtube_control_actions increment YOUTUBE_CONTROL_REVIEWS_TOTAL for each
reviewed action with action equal to the action type and decision "approved" or
"rejected" based on payload.approve; place metric declarations near existing
metric definitions and import/instrument them the same way as
DISCORD_DROPS_TOTAL and DISCORD_DROP_REVIEWS_TOTAL.
In `@pmoves/services/channel-monitor/channel_monitor/monitor.py`:
- Around line 1976-1982: The metadata currently embeds the full details dict (in
the block building "metadata" with keys action_id, action, request_source,
details, summary) which can be very verbose; replace the raw details value with
a trimmed summary by implementing a small sanitizer (e.g.,
trim_notification_details or summarize_details) that extracts only essential
fields (title/user/id/timestamps/concise change text or a generated short
summary) and explicitly omits heavy fields like drafts, rendered_text, notebook
metadata; then update the metadata construction to use the trimmed result (e.g.,
details_summary) instead of the full details object so notifications sent from
monitor.py contain a compact payload.
- Around line 2063-2068: Replace the direct os.getenv chain that assigns api_key
in monitor.py with the project’s central env helper that supports *_FILE secret
paths; specifically, ensure you resolve CHANNEL_MONITOR_YT_API_KEY,
NEXT_PUBLIC_BACKEND_API_KEY, and BACKEND_API_KEY via the helper (or implement a
small helper like get_secret_from_env_or_file(var)) which checks VAR_FILE first,
reads the file contents if present, falls back to VAR, and returns a trimmed
string, then assign that result to api_key (preserve the .strip() behavior).
In `@pmoves/services/channel-monitor/tests/test_monitor.py`:
- Around line 623-677: Add a new test alongside
test_review_youtube_control_actions_executes_pmoves_yt_call that simulates an
HTTP execution failure for monitor.review_youtube_control_actions: create a
DummyAsyncClient whose post method raises an exception (or returns a response
whose raise_for_status raises), set CHANNEL_MONITOR_YT_API_KEY and monkeypatch
channel_monitor.monitor.httpx.AsyncClient, and use a fake DB connection (as in
the existing test) to assert the function marks the action as processed with an
error (result shows failed/errored), increments processed count, and calls
conn.execute to update the row status from "pending_review" to the appropriate
failed state; reference monitor.review_youtube_control_actions, the test
function name, DummyAsyncClient, and conn.execute for locating the changes.
In `@pmoves/services/messaging-gateway/main.py`:
- Around line 68-69: Replace the direct os.environ.get usage for
CHANNEL_MONITOR_SECRET with the project *_FILE secret-loading pattern used by
services/common/env.py: use the helper (or the same logic) to check
CHANNEL_MONITOR_SECRET_FILE first and read the file contents, falling back to
CHANNEL_MONITOR_SECRET env var only if the file isn't present; leave
CHANNEL_MONITOR_URL behavior unchanged but ensure you reference
CHANNEL_MONITOR_SECRET by name in the updated code so the audit gate recognizes
the hardened secret loading.
In `@pmoves/services/messaging-gateway/test_main.py`:
- Around line 112-151: Add error-path tests for _handle_ytcontrol_interaction by
creating new test functions (e.g., test_handle_ytcontrol_interaction_http_error,
test_handle_ytcontrol_interaction_invalid_custom_id,
test_handle_ytcontrol_interaction_network_failure) that mirror the existing test
style: monkeypatch module.httpx.AsyncClient with a DummyAsyncClient and
DummyResponse, but simulate failure modes — have DummyResponse.raise_for_status
raise module.httpx.HTTPStatusError to cover HTTP errors, provide payloads with
malformed custom_id (missing parts or wrong prefix) to assert the function
returns a user-facing failure message, and have DummyAsyncClient.post raise
module.httpx.ConnectError (or similar) to test network failures; use
module.asyncio.run(module._handle_ytcontrol_interaction(payload)) and assert
response["type"] == 4 and that the content indicates the failure.
In `@pmoves/supabase/migrations/20260312095500_youtube_control_actions.sql`:
- Around line 23-24: The current index idx_youtube_control_actions_action_status
on pmoves_core.youtube_control_actions is ordered (action, status) which doesn't
support queries like WHERE status = ... ORDER BY created_at DESC efficiently;
add or replace with an index that begins with status and includes created_at
(descending) and action (e.g., (status, created_at DESC, action)) so
review-queue lookups (filter by status and order by created_at DESC) and any
tie-breakers on action are covered; update the migration to create the new index
name and drop or no-op the old idx_youtube_control_actions_action_status if
desired.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8465f1f8-1c94-4cc6-8076-422e07a1ff65
📒 Files selected for processing (35)
PMOVES-WealthPMOVES-supabasePMOVES-transcribe-and-fetchPMOVES.YTPmoves-Health-wgerpmoves/config/channel_monitor.jsonpmoves/docker-compose.ymlpmoves/docs/AGENTS/CODEX_OPERATOR_HOME.mdpmoves/docs/AGENTS/PMOVES_YT_CONTROL_WORKTREE_REVIEW.mdpmoves/docs/AGENTS/README.mdpmoves/docs/AGENTS/SUBMODULE_CODEX_HOMES/PMOVES-Creator.mdpmoves/docs/AGENTS/SUBMODULE_CODEX_HOMES/PMOVES-Open-Notebook.mdpmoves/docs/AGENTS/SUBMODULE_CODEX_HOMES/PMOVES-transcribe-and-fetch.mdpmoves/docs/AGENTS/SUBMODULE_CODEX_HOMES/PMOVES.YT.mdpmoves/docs/NEXT_STEPS.mdpmoves/docs/PMOVES.AI PLANS/CREATOR_NETWORK_CONTROL_PLANE.mdpmoves/docs/PMOVES.AI PLANS/JELLYFIN_YOUTUBE_INTEGRATION.mdpmoves/docs/PMOVES.AI PLANS/JELLYFIN_YOUTUBE_STATUS.mdpmoves/docs/PMOVES.AI PLANS/PMOVES.yt/CHANNEL_MONITOR_IMPLEMENTATION.mdpmoves/docs/PMOVES.AI PLANS/README_DOCS_INDEX.mdpmoves/docs/PMOVES.AI PLANS/ROADMAP.mdpmoves/docs/services/pmoves-yt/README.mdpmoves/env.shared.examplepmoves/services/channel-monitor/README.mdpmoves/services/channel-monitor/channel_monitor/config.pypmoves/services/channel-monitor/channel_monitor/main.pypmoves/services/channel-monitor/channel_monitor/monitor.pypmoves/services/channel-monitor/tests/test_monitor.pypmoves/services/messaging-gateway/main.pypmoves/services/messaging-gateway/test_main.pypmoves/services/pmoves-yt/README.mdpmoves/services/pmoves-yt/docs_catalog.pypmoves/services/pmoves-yt/docs_sync.pypmoves/services/pmoves-yt/yt.pypmoves/supabase/migrations/20260312095500_youtube_control_actions.sql
| @@ -1 +1 @@ | |||
| Subproject commit 5a57ca00185c6e04527cd2962feb2ac90491a0ad | |||
| Subproject commit a4968929b22f5f2625836aa9a1dfdcf6ba3e5459 | |||
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Verify the submodule commit exists and fetch its summary.
# Check if the submodule directory exists
if [ ! -d "PMOVES-transcribe-and-fetch" ]; then
echo "Error: PMOVES-transcribe-and-fetch submodule directory not found"
exit 1
fi
cd PMOVES-transcribe-and-fetch
# Fetch the latest refs to ensure we have the commit
git fetch --quiet 2>/dev/null || true
# Check if the commit exists
if git cat-file -e a4968929b22f5f2625836aa9a1dfdcf6ba3e5459 2>/dev/null; then
echo "✓ Commit exists"
echo ""
echo "Commit details:"
git log -1 --oneline a4968929b22f5f2625836aa9a1dfdcf6ba3e5459
echo ""
git log -1 --format="Author: %an <%ae>%nDate: %ad%nSubject: %s%n%nBody:%n%b" a4968929b22f5f2625836aa9a1dfdcf6ba3e5459
else
echo "✗ Commit a4968929b22f5f2625836aa9a1dfdcf6ba3e5459 not found in submodule"
exit 1
fiRepository: POWERFULMOVES/PMOVES.AI
Length of output: 139
Submodule commit does not exist in repository.
The commit a4968929b22f5f2625836aa9a1dfdcf6ba3e5459 referenced in the PMOVES-transcribe-and-fetch submodule update does not exist in the submodule repository. Verify that:
- The commit SHA is correct (check for typos).
- The commit has been pushed to the submodule repository.
- The correct branch or remote is being referenced.
Please provide the correct commit SHA or push the intended commit to the submodule repository before this change can be merged.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@PMOVES-transcribe-and-fetch` at line 1, The PMOVES-transcribe-and-fetch
submodule update references a non-existent commit SHA
(a4968929b22f5f2625836aa9a1dfdcf6ba3e5459); verify the SHA in the submodule repo
and either push the missing commit to that remote or update the submodule
reference to a valid commit: checkout the PMOVES-transcribe-and-fetch
repository, confirm the correct commit SHA (or create and push the intended
commit), then in the superproject update the submodule to that SHA (or run git
submodule update --init --remote), commit the updated submodule reference in the
superproject, and push the change so the PR points to an existing commit.
| - PMOVES.YT is still stronger on ingest/read/download than owned-channel mutation: | ||
| - yt-dlp + companion + Invidious cover extraction and fallback download paths | ||
| - PMOVES.YT now has a first owned-channel YouTube Data API control slice for playlist-add and | ||
| comment/reply actions, but it is intentionally narrow and approval-gated |
There was a problem hiding this comment.
Don't claim reply support unless it's actually wired.
This snapshot says the first owned-channel slice includes playlist-add plus "comment/reply actions", but the supported action list later in the runbook narrows that to playlist_add, playlist_remove, playlist_reorder, and comment_create. If replies are not live yet, this sentence overstates the current control surface.
As per coding guidelines, "pmoves/docs/**: Check docs for operational accuracy: Keep status claims aligned with evidence in runbooks and smokes."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pmoves/docs/AGENTS/PMOVES_YT_CONTROL_WORKTREE_REVIEW.md` around lines 184 -
187, The sentence claiming "comment/reply actions" for the first owned-channel
YouTube Data API slice overstates capabilities; update the PMOVES.YT description
so it matches the supported-action list (`playlist_add`, `playlist_remove`,
`playlist_reorder`, `comment_create`) by removing "reply" or explicitly stating
only `comment_create` is supported (e.g., "comment create only"), and ensure the
phrase "comment/reply actions" is replaced wherever present with the accurate
term to keep the runbook and snapshot consistent.
| Current status note (March 12, 2026): | ||
| - the service is no longer just a prototype backlog item | ||
| - Google OAuth token storage and API-first fetch paths are already present | ||
| - the active gap is production cleanup: preferences, observability, and alignment with the authoritative `PMOVES.YT` runtime/docs |
There was a problem hiding this comment.
Reconcile this status note with the rest of the implementation plan.
These bullets say the service is already past the prototype stage, but the header, checklist, and status tracker below still read like most of the work is pending. Please update the downstream sections or explicitly mark them as historical so operators do not get two different statuses from one page.
As per coding guidelines, pmoves/docs/**: "Check docs for operational accuracy: - Keep status claims aligned with evidence in runbooks and smokes."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pmoves/docs/PMOVES.AI` PLANS/PMOVES.yt/CHANNEL_MONITOR_IMPLEMENTATION.md
around lines 14 - 17, The "Current status note (March 12, 2026)" claims the
service is beyond prototype but downstream sections (the header, checklist, and
status tracker) still present pending work; reconcile them by either updating
those sections to reflect completed items or explicitly marking them as
historical/archival. Specifically, edit the header to show "Production-ready"
(or "Historical" if archived), update the checklist items referenced in the
document to completed/removed and add dates/PR links for evidence, and adjust
the status tracker entries to reflect current production concerns (preferences,
observability, alignment) or prepend a clear "Historical (as of <date>):" label;
also add a short note linking to the authoritative PMOVES.YT runtime/docs and
runbooks/smoke tests to justify the status.
| - Authoritative runtime now lives in the [PMOVES.YT submodule](C:/Users/russe/Documents/GitHub/PMOVES.AI/PMOVES.YT) under `pmoves_yt_service/`. | ||
| - Root `pmoves/services/pmoves-yt` is now a compatibility mirror/shim, not the source of truth. |
There was a problem hiding this comment.
Replace the local Windows path with a repo-relative link.
C:/Users/russe/... only works on one machine, so the rendered docs will carry a broken link for everyone else. Please point this at the submodule using a repository-relative path instead.
📝 Proposed fix
-- Authoritative runtime now lives in the [PMOVES.YT submodule](C:/Users/russe/Documents/GitHub/PMOVES.AI/PMOVES.YT) under `pmoves_yt_service/`.
+- Authoritative runtime now lives in the [PMOVES.YT submodule](../../../../PMOVES.YT/) under `pmoves_yt_service/`.As per coding guidelines, pmoves/docs/**: "Check docs for operational accuracy: - Flag command drift across Windows/WSL/Linux instructions."
📝 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.
| - Authoritative runtime now lives in the [PMOVES.YT submodule](C:/Users/russe/Documents/GitHub/PMOVES.AI/PMOVES.YT) under `pmoves_yt_service/`. | |
| - Root `pmoves/services/pmoves-yt` is now a compatibility mirror/shim, not the source of truth. | |
| - Authoritative runtime now lives in the [PMOVES.YT submodule](../../../../PMOVES.YT/) under `pmoves_yt_service/`. | |
| - Root `pmoves/services/pmoves-yt` is now a compatibility mirror/shim, not the source of truth. |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pmoves/docs/services/pmoves-yt/README.md` around lines 7 - 8, Replace the
absolute Windows path in the README with a repository-relative link to the
PMOVES.YT submodule; specifically change the link target
"C:/Users/russe/Documents/GitHub/PMOVES.AI/PMOVES.YT" to a repo-relative path
like "pmoves_yt_service/" (or "./pmoves_yt_service/" or the correct relative
path to the submodule) so the authoritative runtime reference under
pmoves_yt_service/ resolves for all users and the note about
pmoves/services/pmoves-yt remaining a compatibility shim stays accurate.
| _SUBMODULE_ROOT = pathlib.Path(__file__).resolve().parents[3] / "PMOVES.YT" | ||
| if str(_SUBMODULE_ROOT) not in sys.path: | ||
| sys.path.insert(0, str(_SUBMODULE_ROOT)) | ||
|
|
||
| def _capture_cmd(args: list[str]) -> str: | ||
| try: | ||
| proc = subprocess.run(args, capture_output=True, text=True, timeout=20) | ||
| if proc.returncode != 0: | ||
| return proc.stderr.strip() or proc.stdout.strip() | ||
| return proc.stdout | ||
| except Exception as exc: # best-effort | ||
| return f"<error: {exc}>" | ||
|
|
||
| def collect_yt_dlp_docs() -> Dict[str, Any]: | ||
| import yt_dlp # type: ignore | ||
| version = getattr(yt_dlp, "version", None) | ||
| if isinstance(version, str): | ||
| ver = version | ||
| else: | ||
| ver = getattr(yt_dlp, "__version__", "unknown") | ||
| docs: Dict[str, Any] = { | ||
| "version": ver, | ||
| "help_cli": _capture_cmd(["yt-dlp", "--help"]), | ||
| "extractors": _capture_cmd(["yt-dlp", "--list-extractors"]), | ||
| "user_agent": _capture_cmd(["yt-dlp", "--dump-user-agent"]), | ||
| "ts": datetime.now(timezone.utc).isoformat(), | ||
| } | ||
| return docs | ||
|
|
||
| def sync_to_supabase(docs: Dict[str, Any]) -> Dict[str, Any]: | ||
| keys = _candidate_keys() | ||
| if not keys: | ||
| raise RuntimeError("SUPABASE_SERVICE_ROLE_KEY (or equivalent) is required") | ||
| tool = "yt-dlp" | ||
| ver = docs.get("version") or "unknown" | ||
| rows = [] | ||
| for k in ("help_cli", "extractors", "user_agent"): | ||
| content = docs.get(k) | ||
| # Store as JSON with `text` field for consistency | ||
| rows.append({ | ||
| "tool": tool, | ||
| "version": str(ver), | ||
| "doc_type": k, | ||
| "content": {"text": content}, | ||
| }) | ||
| import requests | ||
| targets = [ | ||
| # Preferred: proper PostgREST profile headers for pmoves_core schema. | ||
| {"url": f"{SUPA}/tool_docs?on_conflict=tool,version,doc_type", "schema": "pmoves_core"}, | ||
| # Legacy fallback: existing callers that encode schema in table path. | ||
| {"url": f"{SUPA}/pmoves_core.tool_docs?on_conflict=tool,version,doc_type", "schema": None}, | ||
| # Last fallback if schema support is not configured. | ||
| {"url": f"{SUPA}/tool_docs?on_conflict=tool,version,doc_type", "schema": None}, | ||
| ] | ||
|
|
||
| last_error: str | None = None | ||
| for target in targets: | ||
| missing_relation = False | ||
| transport_error = False | ||
| for key in keys: | ||
| headers = { | ||
| "apikey": key, | ||
| "Authorization": f"Bearer {key}", | ||
| "content-type": "application/json", | ||
| "Prefer": "resolution=merge-duplicates", | ||
| } | ||
| if target["schema"]: | ||
| headers["Accept-Profile"] = target["schema"] | ||
| headers["Content-Profile"] = target["schema"] | ||
| try: | ||
| r = requests.post(target["url"], headers=headers, data=json.dumps(rows), timeout=20) | ||
| except requests.RequestException as exc: | ||
| last_error = f"transport error: {exc}" | ||
| transport_error = True | ||
| break | ||
| try: | ||
| body = r.json() | ||
| except ValueError: | ||
| body = {"text": r.text} | ||
| if r.ok: | ||
| return {"status": "ok", "count": len(rows), "version": ver} | ||
| last_error = f"{r.status_code} {body}" | ||
| # JWT/key mismatch can happen when layered env files contain stale aliases. | ||
| # Continue trying available keys before failing hard. | ||
| if r.status_code in (401, 403): | ||
| continue | ||
| # Missing schema/table: move to next target strategy. | ||
| if r.status_code in (404, 406): | ||
| missing_relation = True | ||
| break | ||
| if missing_relation or transport_error: | ||
| continue | ||
|
|
||
| raise RuntimeError(f"Supabase upsert failed: {last_error}") | ||
|
|
||
| if __name__ == "__main__": | ||
| data = collect_yt_dlp_docs() | ||
| out = sync_to_supabase(data) | ||
| print(json.dumps(out)) | ||
| from pmoves_yt_service.docs_sync import * # type: ignore # noqa: F401,F403 |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "docs_sync.py" -o -name "docs_catalog.py" | head -20Repository: POWERFULMOVES/PMOVES.AI
Length of output: 192
🏁 Script executed:
fd -t f "docs_sync.py|docs_catalog.py" pmoves/services/Repository: POWERFULMOVES/PMOVES.AI
Length of output: 239
🏁 Script executed:
cat -n pmoves/services/pmoves-yt/docs_sync.py | head -20Repository: POWERFULMOVES/PMOVES.AI
Length of output: 530
🏁 Script executed:
cat -n pmoves/services/pmoves-yt/docs_catalog.py | head -20Repository: POWERFULMOVES/PMOVES.AI
Length of output: 536
🏁 Script executed:
cat -n pmoves/services/pmoves_yt/docs_sync.py | head -20Repository: POWERFULMOVES/PMOVES.AI
Length of output: 699
🏁 Script executed:
ls -la pmoves/services/pmoves-yt/ | head -20Repository: POWERFULMOVES/PMOVES.AI
Length of output: 688
🏁 Script executed:
rg "is_dir\(\)" pmoves/services/ -A 2 -B 2Repository: POWERFULMOVES/PMOVES.AI
Length of output: 2106
Add a directory existence check before modifying sys.path.
This compatibility shim lacks protection against a missing PMOVES.YT checkout. If the expected sibling directory does not exist, sys.path.insert() will silently add a non-existent path, allowing import resolution to drift to a different installed pmoves_yt_service package. Add an _SUBMODULE_ROOT.is_dir() guard before inserting into sys.path to fail immediately when the submodule is missing.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pmoves/services/pmoves-yt/docs_sync.py` around lines 8 - 12, The shim
currently unconditionally inserts _SUBMODULE_ROOT into sys.path which can mask a
missing PMOVES.YT checkout; update the logic around _SUBMODULE_ROOT (the pathlib
Path defined at top) so you check _SUBMODULE_ROOT.is_dir() before calling
sys.path.insert(0, str(_SUBMODULE_ROOT)), and if the directory does not exist
raise a clear exception (or call sys.exit) so the import from
pmoves_yt_service.docs_sync fails fast instead of silently pointing to an
installed package.
| Canonical source now lives in the [PMOVES.YT submodule](C:/Users/russe/Documents/GitHub/PMOVES.AI/PMOVES.YT) under `pmoves_yt_service/`. | ||
|
|
||
| PMOVES.AI keeps this path only as a compatibility shim for: | ||
| - existing imports under `pmoves.services.pmoves_yt` | ||
| - older docs/test references that still point at `pmoves/services/pmoves-yt` | ||
|
|
||
| For real runtime/docs changes, edit: | ||
| - [PMOVES.YT/pmoves_yt_service/yt.py](C:/Users/russe/Documents/GitHub/PMOVES.AI/PMOVES.YT/pmoves_yt_service/yt.py) | ||
| - [PMOVES.YT/pmoves_yt_service/docs_sync.py](C:/Users/russe/Documents/GitHub/PMOVES.AI/PMOVES.YT/pmoves_yt_service/docs_sync.py) | ||
| - [PMOVES.YT/pmoves_yt_service/docs_catalog.py](C:/Users/russe/Documents/GitHub/PMOVES.AI/PMOVES.YT/pmoves_yt_service/docs_catalog.py) | ||
| - [PMOVES.YT/docs/RUNTIME.md](C:/Users/russe/Documents/GitHub/PMOVES.AI/PMOVES.YT/docs/RUNTIME.md) |
There was a problem hiding this comment.
Replace absolute Windows paths with relative repository paths.
The paths reference a specific Windows user directory (C:/Users/russe/Documents/GitHub/PMOVES.AI/...) which won't work for other developers or in CI environments. Use relative paths from the repository root instead.
🔧 Proposed fix
-Canonical source now lives in the [PMOVES.YT submodule](C:/Users/russe/Documents/GitHub/PMOVES.AI/PMOVES.YT) under `pmoves_yt_service/`.
+Canonical source now lives in the [PMOVES.YT submodule](../../../PMOVES.YT) under `pmoves_yt_service/`.
PMOVES.AI keeps this path only as a compatibility shim for:
- existing imports under `pmoves.services.pmoves_yt`
- older docs/test references that still point at `pmoves/services/pmoves-yt`
For real runtime/docs changes, edit:
-- [PMOVES.YT/pmoves_yt_service/yt.py](C:/Users/russe/Documents/GitHub/PMOVES.AI/PMOVES.YT/pmoves_yt_service/yt.py)
-- [PMOVES.YT/pmoves_yt_service/docs_sync.py](C:/Users/russe/Documents/GitHub/PMOVES.AI/PMOVES.YT/pmoves_yt_service/docs_sync.py)
-- [PMOVES.YT/pmoves_yt_service/docs_catalog.py](C:/Users/russe/Documents/GitHub/PMOVES.AI/PMOVES.YT/pmoves_yt_service/docs_catalog.py)
-- [PMOVES.YT/docs/RUNTIME.md](C:/Users/russe/Documents/GitHub/PMOVES.AI/PMOVES.YT/docs/RUNTIME.md)
+- `PMOVES.YT/pmoves_yt_service/yt.py`
+- `PMOVES.YT/pmoves_yt_service/docs_sync.py`
+- `PMOVES.YT/pmoves_yt_service/docs_catalog.py`
+- `PMOVES.YT/docs/RUNTIME.md`As per coding guidelines, "Flag command drift across Windows/WSL/Linux instructions."
📝 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.
| Canonical source now lives in the [PMOVES.YT submodule](C:/Users/russe/Documents/GitHub/PMOVES.AI/PMOVES.YT) under `pmoves_yt_service/`. | |
| PMOVES.AI keeps this path only as a compatibility shim for: | |
| - existing imports under `pmoves.services.pmoves_yt` | |
| - older docs/test references that still point at `pmoves/services/pmoves-yt` | |
| For real runtime/docs changes, edit: | |
| - [PMOVES.YT/pmoves_yt_service/yt.py](C:/Users/russe/Documents/GitHub/PMOVES.AI/PMOVES.YT/pmoves_yt_service/yt.py) | |
| - [PMOVES.YT/pmoves_yt_service/docs_sync.py](C:/Users/russe/Documents/GitHub/PMOVES.AI/PMOVES.YT/pmoves_yt_service/docs_sync.py) | |
| - [PMOVES.YT/pmoves_yt_service/docs_catalog.py](C:/Users/russe/Documents/GitHub/PMOVES.AI/PMOVES.YT/pmoves_yt_service/docs_catalog.py) | |
| - [PMOVES.YT/docs/RUNTIME.md](C:/Users/russe/Documents/GitHub/PMOVES.AI/PMOVES.YT/docs/RUNTIME.md) | |
| Canonical source now lives in the [PMOVES.YT submodule](../../../PMOVES.YT) under `pmoves_yt_service/`. | |
| PMOVES.AI keeps this path only as a compatibility shim for: | |
| - existing imports under `pmoves.services.pmoves_yt` | |
| - older docs/test references that still point at `pmoves/services/pmoves-yt` | |
| For real runtime/docs changes, edit: | |
| - `PMOVES.YT/pmoves_yt_service/yt.py` | |
| - `PMOVES.YT/pmoves_yt_service/docs_sync.py` | |
| - `PMOVES.YT/pmoves_yt_service/docs_catalog.py` | |
| - `PMOVES.YT/docs/RUNTIME.md` |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pmoves/services/pmoves-yt/README.md` around lines 5 - 15, The README
currently contains absolute Windows paths (e.g.
"C:/Users/russe/Documents/GitHub/PMOVES.AI/PMOVES.YT/...") — update those to
relative repository paths so they work across environments; replace each
absolute path with a relative path from the repo root such as
"PMOVES.YT/pmoves_yt_service/yt.py", "PMOVES.YT/pmoves_yt_service/docs_sync.py",
"PMOVES.YT/pmoves_yt_service/docs_catalog.py" and "PMOVES.YT/docs/RUNTIME.md"
and ensure the PMOVES.YT submodule reference remains consistent (keep the
`pmoves_yt_service/` suffix used in imports and docs).
| ## Production defaults | ||
|
|
||
| - Root compose intentionally overrides the submodule runtime with `YT_PLAYER_CLIENT=web_safari` plus a Safari user agent. | ||
| - bgutil POT provider is the preferred production path; companion/Invidious stay available as fallback lanes. | ||
| - For downloader/client changes, update the submodule first and keep the compose override in [docker-compose.yml](C:/Users/russe/Documents/GitHub/PMOVES.AI/pmoves/docker-compose.yml) aligned. |
There was a problem hiding this comment.
Same issue: absolute Windows path in production defaults section.
Line 44 contains another absolute Windows path reference to docker-compose.yml.
🔧 Proposed fix
-- For downloader/client changes, update the submodule first and keep the compose override in [docker-compose.yml](C:/Users/russe/Documents/GitHub/PMOVES.AI/pmoves/docker-compose.yml) aligned.
+- For downloader/client changes, update the submodule first and keep the compose override in `pmoves/docker-compose.yml` aligned.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pmoves/services/pmoves-yt/README.md` around lines 40 - 44, In the "Production
defaults" section of README.md remove the absolute Windows path reference to
docker-compose.yml and replace it with a relative or repository-root path (e.g.,
./docker-compose.yml or a repo link) so the docs are cross-platform; update the
sentence that currently mentions "docker-compose.yml" to reference the relative
path and ensure the string "YT_PLAYER_CLIENT=web_safari" and the note about
keeping the compose override aligned remain unchanged.
🧩 Analysis chain🏁 Script executed: Length of output: 142813
✅ All previous findings addressed
Minor residual items (no action required now)Orphaned
If you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! ✅ Actions performedReview triggered.
|
4e5a8dd to
24cb93d
Compare
|
Restacked this PR onto the current Current scope is now the intended 9-commit creator-control lane only:
Local validation rerun on the restacked branch:
GitHub now reports the PR branch as mergeable; waiting on the fresh CI/review cycle. @coderabbitai review |
|
Triggering a fresh incremental review on the restacked branch now. ✅ Actions performedReview triggered.
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
|
||
| ytcontrol_response = await _handle_ytcontrol_interaction(payload) | ||
| if ytcontrol_response is not None: | ||
| return ytcontrol_response |
Check warning
Code scanning / CodeQL
Information exposure through an exception Medium
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 6 months ago
In general, the fix is to avoid embedding raw exception information (exc) into the HTTP response that is returned to the Discord interaction caller. Instead, log the detailed error server-side and return a generic, user-friendly message that doesn’t include stack traces, status codes, or internal details.
Concretely, in pmoves/services/messaging-gateway/main.py, inside the except Exception as exc: block around the httpx.AsyncClient call (lines 199–204), we should keep the logging line as-is to preserve diagnostics, but change the returned dictionary so that the "content" field does not interpolate {exc}. We can keep the action id to give some context to the user, but the problem-specific detail should be removed or replaced with a generic phrase like “internal error” or “upstream service error”. No new imports are required.
So, update the block:
except Exception as exc:
logger.warning("YouTube control Discord interaction failed for %s: %s", action_id, exc)
return {
"type": 4,
"data": {"content": f"Review action failed for {action_id}: {exc}", "flags": 64},
}to something like:
except Exception as exc:
logger.warning("YouTube control Discord interaction failed for %s: %s", action_id, exc)
return {
"type": 4,
"data": {
"content": f"Review action failed for {action_id} due to an internal error.",
"flags": 64,
},
}This preserves existing behavior (user still sees that the review failed for a given action_id, the message type and flags are unchanged) while eliminating exposure of exception details. The tainted data (exc) now flows only into the log, not into the response.
| @@ -200,7 +200,10 @@ | ||
| logger.warning("YouTube control Discord interaction failed for %s: %s", action_id, exc) | ||
| return { | ||
| "type": 4, | ||
| "data": {"content": f"Review action failed for {action_id}: {exc}", "flags": 64}, | ||
| "data": { | ||
| "content": f"Review action failed for {action_id} due to an internal error.", | ||
| "flags": 64, | ||
| }, | ||
| } | ||
|
|
||
| content = _format_ytcontrol_response(body, action_id, approve) |
|
Follow-up CI fixes pushed in This addresses the two failing root PR checks:
Local validation:
@coderabbitai review |
🧩 Analysis chain🏁 Script executed: Length of output: 138812
✅ CI fixes verified1. 2.
The new composite index 3. All previous findings remain closed. The lane is clean — triggering a fresh review pass now. ✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pmoves/docs/AGENTS/SUBMODULE_CODEX_HOMES/PMOVES-transcribe-and-fetch.md (1)
33-33:⚠️ Potential issue | 🟡 MinorClarify indirect routing through PMOVES.YT.
The operating rule states "align transcript/fetch outputs with Discord, Jellyfin, retrieval, and Tokenism consumers", but the implementation shows channel-monitor POSTs to a single
queue_urlendpoint (typically PMOVES.YT), not directly to these consumers. This is aspirational and depends on downstream routing from PMOVES.YT. Consider clarifying that alignment happens indirectly through PMOVES.YT's queue endpoint.As per coding guidelines, "pmoves/docs/**: Check docs for operational accuracy: Keep status claims aligned with evidence in runbooks and smokes."
📝 Suggested clarification
-- align transcript/fetch outputs with Discord, Jellyfin, retrieval, and Tokenism consumers +- align transcript/fetch outputs with downstream consumers (Discord, Jellyfin, retrieval, Tokenism) via PMOVES.YT queue routing🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pmoves/docs/AGENTS/SUBMODULE_CODEX_HOMES/PMOVES-transcribe-and-fetch.md` at line 33, Update the docs to clarify that transcript/fetch outputs are routed indirectly via PMOVES.YT by the channel-monitor POSTs to the single queue_url endpoint rather than being sent directly to downstream consumers (Discord, Jellyfin, retrieval, Tokenism); mention that alignment with those consumers depends on PMOVES.YT's downstream routing and include a short note near the "align transcript/fetch outputs..." line indicating this indirection and dependency on PMOVES.YT handling of the queue_url requests.
♻️ Duplicate comments (2)
pmoves/docs/AGENTS/PMOVES_YT_CONTROL_WORKTREE_REVIEW.md (1)
172-175:⚠️ Potential issue | 🟡 MinorDon't claim reply support unless it's actually wired.
Lines 174-175 state "PMOVES.YT now has a first owned-channel YouTube Data API control slice for playlist-add and comment/reply actions", but line 197 and the code evidence (YouTubeControlRequest model) show that only
comment_createis supported, not reply actions. Update "comment/reply actions" to "comment create only" or similar to match the actual supported actions.As per coding guidelines, "pmoves/docs/**: Check docs for operational accuracy: Keep status claims aligned with evidence in runbooks and smokes."
📝 Suggested fix
-- PMOVES.YT now has a first owned-channel YouTube Data API control slice for playlist-add and - comment/reply actions, but it is intentionally narrow and approval-gated +- PMOVES.YT now has a first owned-channel YouTube Data API control slice for playlist-add and + comment create actions (not reply), but it is intentionally narrow and approval-gated🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pmoves/docs/AGENTS/PMOVES_YT_CONTROL_WORKTREE_REVIEW.md` around lines 172 - 175, The doc incorrectly claims reply support for PMOVES.YT; update the phrasing around PMOVES.YT to reflect the actual supported actions by changing "comment/reply actions" to "comment create only" (or similar), and note that the YouTubeControlRequest currently supports only comment_create (no reply actions) and that the owned-channel Data API slice is approval-gated; adjust the sentence referencing playlist-add and comment/reply to read something like "playlist-add and comment_create (comment create only)" so it aligns with the YouTubeControlRequest model.pmoves/services/channel-monitor/README.md (1)
155-179:⚠️ Potential issue | 🟡 MinorUse
ownedin the owned-channel example.This section is explicitly about owned-channel mutations, but the sample payload still sets
"source_class": "watched", which points operators at the wrong lane.✏️ Suggested edit
- "source_class": "watched" + "source_class": "owned"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pmoves/services/channel-monitor/README.md` around lines 155 - 179, The sample payload for the owned-channel control action incorrectly sets the draft source class to "watched"; update the example under "owned-channel PMOVES.YT control actions" (the JSON block used for the comment_create action and the draft object) to set "source_class": "owned" so the draft, notify lanes and operators correctly reflect an owned-channel mutation.
🧹 Nitpick comments (1)
pmoves/supabase/migrations/20260312095500_youtube_control_actions.sql (1)
17-34: Consider adding a CHECK constraint on thestatuscolumn.The
actioncolumn is properly constrained, butstatusaccepts any text value. Based on the review workflow inmonitor.py, valid status values appear to be'pending_review','processing','approved','rejected', and'failed'. Adding a constraint would prevent invalid states at the database level.♻️ Suggested constraint
status text not null, + check ( + status in ('pending_review', 'processing', 'approved', 'rejected', 'failed') + ),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pmoves/supabase/migrations/20260312095500_youtube_control_actions.sql` around lines 17 - 34, The status column in pmoves_core.youtube_control_actions lacks a constraint allowing invalid states; add a CHECK constraint (e.g., youtube_control_actions_status_check) on the status column limiting values to 'pending_review','processing','approved','rejected','failed' by either updating the CREATE TABLE statement to include the CHECK on status or issuing an ALTER TABLE ... ADD CONSTRAINT for the existing table so the database enforces valid workflow states.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pmoves/services/channel-monitor/channel_monitor/main.py`:
- Around line 154-157: The source_class Field currently accepts any string and
relies on monitor._normalize_source_class() to fallback silently; change the API
models to validate and reject unknown values up front by restricting
source_class to the allowed set {"owned","partner","watched","candidate"}
(either via a Pydantic validator that checks the value against that set and
raises ValueError or by using an Enum/Literal type), update the Field
declaration for source_class and add the same validator/type to
DiscordDropRequest so typos like "owend" fail at the API boundary rather than
being normalized later.
In `@pmoves/services/channel-monitor/channel_monitor/monitor.py`:
- Around line 2165-2170: The audit row update currently leaves approved_by NULL
when actor is omitted; change the code that writes the DB/audit row to use the
same resolved reviewer identity passed to _invoke_yt_control_action (i.e., use
resolved_approver = actor or "channel-monitor") so both the payload and the
stored row record the same approver; update the places around the
_invoke_yt_control_action call (and the similar block at 2190-2205) to reference
resolved_approver for approved_by instead of actor directly.
- Around line 2091-2096: The code is falling back to a public API key when
building api_key (the os.getenv chain referencing NEXT_PUBLIC_BACKEND_API_KEY
and plain ""), which allows control-plane writes with a potentially exposed
credential; update the api_key loading in monitor.py to use the project's
central env helper that supports *_FILE secret loading and only read the
service-private secret (e.g., CHANNEL_MONITOR_YT_API_KEY via the helper), remove
the NEXT_PUBLIC_BACKEND_API_KEY and empty-string fallback, and ensure the
resulting logic (the variable api_key) fails loudly or logs an error if the
private secret is missing.
- Around line 173-184: In the comment_create branch (where action ==
"comment_create") add a guard after the text/template handling to validate that
a target is present: check normalized and draft_dict for a non-empty "video_id"
or "parent_comment_id" (e.g., normalized.get("video_id") or
draft_dict.get("video_id") or normalized.get("parent_comment_id") or
draft_dict.get("parent_comment_id")), and if neither exists raise a ValueError
(similar to the existing message style) so rows without a target cannot be
persisted or queued for PMOVES.YT.
In `@pmoves/services/messaging-gateway/main.py`:
- Around line 191-203: The code currently raises a RuntimeError including
exc.response.text and then returns str(exc) to Discord, leaking upstream
response bodies; change the error handling so you do not include upstream
response details in either the raised exception message or the ephemeral reply:
in the httpx.HTTPStatusError except block (around response.raise_for_status /
httpx.HTTPStatusError) remove or redact exc.response.text from the RuntimeError
(keep only status_code or a generic message) and log the full response or
exception internally (e.g., logger.exception or logger.error) for debugging;
then update the outer except handler that builds the Discord reply (the dict
returned with "type": 4 and "data": {"content": ...}) to return a generic
user-facing message like "Review action failed for {action_id}" without
including exc or its text, while still logging the full exception via
logger.warning/logger.exception for operators.
In `@pmoves/supabase/migrations/20260312095500_youtube_control_actions.sql`:
- Around line 84-85: Update the table comment for
pmoves_core.youtube_control_actions to reflect that rows are mutable (they are
updated by monitor.py: fields like status, approved_by, approval_note, result,
error are modified) instead of claiming "Append-only audit trail"; change the
description to indicate it is an audit/history of YouTube API control actions
that may be updated with approval and result metadata, referencing the table
name pmoves_core.youtube_control_actions and the monitored fields (status,
approved_by, approval_note, result, error).
---
Outside diff comments:
In `@pmoves/docs/AGENTS/SUBMODULE_CODEX_HOMES/PMOVES-transcribe-and-fetch.md`:
- Line 33: Update the docs to clarify that transcript/fetch outputs are routed
indirectly via PMOVES.YT by the channel-monitor POSTs to the single queue_url
endpoint rather than being sent directly to downstream consumers (Discord,
Jellyfin, retrieval, Tokenism); mention that alignment with those consumers
depends on PMOVES.YT's downstream routing and include a short note near the
"align transcript/fetch outputs..." line indicating this indirection and
dependency on PMOVES.YT handling of the queue_url requests.
---
Duplicate comments:
In `@pmoves/docs/AGENTS/PMOVES_YT_CONTROL_WORKTREE_REVIEW.md`:
- Around line 172-175: The doc incorrectly claims reply support for PMOVES.YT;
update the phrasing around PMOVES.YT to reflect the actual supported actions by
changing "comment/reply actions" to "comment create only" (or similar), and note
that the YouTubeControlRequest currently supports only comment_create (no reply
actions) and that the owned-channel Data API slice is approval-gated; adjust the
sentence referencing playlist-add and comment/reply to read something like
"playlist-add and comment_create (comment create only)" so it aligns with the
YouTubeControlRequest model.
In `@pmoves/services/channel-monitor/README.md`:
- Around line 155-179: The sample payload for the owned-channel control action
incorrectly sets the draft source class to "watched"; update the example under
"owned-channel PMOVES.YT control actions" (the JSON block used for the
comment_create action and the draft object) to set "source_class": "owned" so
the draft, notify lanes and operators correctly reflect an owned-channel
mutation.
---
Nitpick comments:
In `@pmoves/supabase/migrations/20260312095500_youtube_control_actions.sql`:
- Around line 17-34: The status column in pmoves_core.youtube_control_actions
lacks a constraint allowing invalid states; add a CHECK constraint (e.g.,
youtube_control_actions_status_check) on the status column limiting values to
'pending_review','processing','approved','rejected','failed' by either updating
the CREATE TABLE statement to include the CHECK on status or issuing an ALTER
TABLE ... ADD CONSTRAINT for the existing table so the database enforces valid
workflow states.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: dce71686-e948-4344-82a9-71395ddcdbdb
📒 Files selected for processing (21)
.github/workflows/python-tests.ymlPMOVES.YTpmoves/config/channel_monitor.jsonpmoves/docs/AGENTS/CODEX_OPERATOR_HOME.mdpmoves/docs/AGENTS/PMOVES_YT_CONTROL_WORKTREE_REVIEW.mdpmoves/docs/AGENTS/README.mdpmoves/docs/AGENTS/SUBMODULE_CODEX_HOMES/PMOVES-Creator.mdpmoves/docs/AGENTS/SUBMODULE_CODEX_HOMES/PMOVES-Open-Notebook.mdpmoves/docs/AGENTS/SUBMODULE_CODEX_HOMES/PMOVES-transcribe-and-fetch.mdpmoves/docs/AGENTS/SUBMODULE_CODEX_HOMES/PMOVES.YT.mdpmoves/docs/NEXT_STEPS.mdpmoves/docs/PMOVES.AI PLANS/ROADMAP.mdpmoves/services/channel-monitor/README.mdpmoves/services/channel-monitor/channel_monitor/config.pypmoves/services/channel-monitor/channel_monitor/main.pypmoves/services/channel-monitor/channel_monitor/monitor.pypmoves/services/channel-monitor/tests/test_monitor.pypmoves/services/messaging-gateway/main.pypmoves/services/messaging-gateway/test_main.pypmoves/services/pmoves-yt/yt.pypmoves/supabase/migrations/20260312095500_youtube_control_actions.sql
✅ Files skipped from review due to trivial changes (1)
- PMOVES.YT
🚧 Files skipped from review as they are similar to previous changes (6)
- pmoves/docs/NEXT_STEPS.md
- pmoves/docs/AGENTS/SUBMODULE_CODEX_HOMES/PMOVES-Open-Notebook.md
- pmoves/docs/PMOVES.AI PLANS/ROADMAP.md
- pmoves/docs/AGENTS/README.md
- pmoves/services/channel-monitor/channel_monitor/config.py
- pmoves/docs/AGENTS/SUBMODULE_CODEX_HOMES/PMOVES.YT.md
| source_class: str | None = Field( | ||
| None, | ||
| description="Operator intent class (owned, partner, watched, candidate)", | ||
| ) |
There was a problem hiding this comment.
Reject unknown source_class values at the API boundary.
These fields currently accept any string, and monitor._normalize_source_class() later falls back silently to watched or candidate. A typo like "owend" will succeed with the wrong routing semantics instead of failing fast.
🛡️ Suggested validator
class AddChannelRequest(BaseModel):
@@
source_class: str | None = Field(
None,
description="Operator intent class (owned, partner, watched, candidate)",
)
+
+ `@validator`("source_class")
+ def _validate_source_class(cls, value: str | None) -> str | None:
+ if value is None:
+ return None
+ normalized = value.strip().lower()
+ allowed = {"owned", "partner", "watched", "candidate"}
+ if normalized not in allowed:
+ raise ValueError(f"source_class must be one of {sorted(allowed)}")
+ return normalizedApply the same validator to DiscordDropRequest.
Also applies to: 301-304
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pmoves/services/channel-monitor/channel_monitor/main.py` around lines 154 -
157, The source_class Field currently accepts any string and relies on
monitor._normalize_source_class() to fallback silently; change the API models to
validate and reject unknown values up front by restricting source_class to the
allowed set {"owned","partner","watched","candidate"} (either via a Pydantic
validator that checks the value against that set and raises ValueError or by
using an Enum/Literal type), update the Field declaration for source_class and
add the same validator/type to DiscordDropRequest so typos like "owend" fail at
the API boundary rather than being normalized later.
| if action == "comment_create": | ||
| text_value = normalized.get("text") | ||
| if not isinstance(text_value, str) or not text_value.strip(): | ||
| template = normalized.get("text_template") or draft_dict.get("text_template") or draft_dict.get("template") | ||
| template_vars = normalized.get("template_vars") or draft_dict.get("template_vars") or draft_dict.get("variables") or {} | ||
| if isinstance(template, str) and template.strip(): | ||
| rendered = _render_template_text(template, template_vars if isinstance(template_vars, dict) else {}) | ||
| normalized["text"] = rendered.strip() | ||
| normalized["template_rendered"] = True | ||
| else: | ||
| raise ValueError("comment_create requires text or text_template") | ||
| normalized["text_preview"] = _truncate_text(normalized.get("text"), limit=160) |
There was a problem hiding this comment.
Validate the target for comment_create.
comment_create currently validates the text/template, but it still accepts requests with neither video_id nor parent_comment_id. Those rows can be persisted and later sent to PMOVES.YT with no target object.
🔧 Suggested guard
if action == "comment_create":
+ if normalized.get("video_id") in (None, "") and normalized.get("parent_comment_id") in (None, ""):
+ raise ValueError("comment_create requires video_id or parent_comment_id")
text_value = normalized.get("text")
if not isinstance(text_value, str) or not text_value.strip():
template = normalized.get("text_template") or draft_dict.get("text_template") or draft_dict.get("template")🧰 Tools
🪛 Ruff (0.15.5)
[warning] 183-183: Avoid specifying long messages outside the exception class
(TRY003)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pmoves/services/channel-monitor/channel_monitor/monitor.py` around lines 173
- 184, In the comment_create branch (where action == "comment_create") add a
guard after the text/template handling to validate that a target is present:
check normalized and draft_dict for a non-empty "video_id" or
"parent_comment_id" (e.g., normalized.get("video_id") or
draft_dict.get("video_id") or normalized.get("parent_comment_id") or
draft_dict.get("parent_comment_id")), and if neither exists raise a ValueError
(similar to the existing message style) so rows without a target cannot be
persisted or queued for PMOVES.YT.
| api_key = ( | ||
| os.getenv("CHANNEL_MONITOR_YT_API_KEY") | ||
| or os.getenv("NEXT_PUBLIC_BACKEND_API_KEY") | ||
| or os.getenv("BACKEND_API_KEY") | ||
| or "" | ||
| ).strip() |
There was a problem hiding this comment.
Don't fall back to a public API key for control-plane writes.
Using NEXT_PUBLIC_BACKEND_API_KEY here makes it easy to authorize PMOVES.YT mutations with a credential that may already be exposed to clients. This path should read only a service-private secret via the shared env helper / _FILE mechanism.
As per coding guidelines, "Prefer central env helpers and *_FILE secret loading paths. Flag direct critical-secret reads and plaintext fallbacks."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pmoves/services/channel-monitor/channel_monitor/monitor.py` around lines 2091
- 2096, The code is falling back to a public API key when building api_key (the
os.getenv chain referencing NEXT_PUBLIC_BACKEND_API_KEY and plain ""), which
allows control-plane writes with a potentially exposed credential; update the
api_key loading in monitor.py to use the project's central env helper that
supports *_FILE secret loading and only read the service-private secret (e.g.,
CHANNEL_MONITOR_YT_API_KEY via the helper), remove the
NEXT_PUBLIC_BACKEND_API_KEY and empty-string fallback, and ensure the resulting
logic (the variable api_key) fails loudly or logs an error if the private secret
is missing.
| async with self._pool.acquire() as conn: | ||
| async with conn.transaction(): | ||
| rows = await conn.fetch( | ||
| """ | ||
| WITH claimed AS ( | ||
| SELECT id | ||
| FROM pmoves_core.youtube_control_actions | ||
| WHERE status = 'pending_review' | ||
| AND id = ANY($1::uuid[]) | ||
| ORDER BY created_at ASC | ||
| FOR UPDATE SKIP LOCKED | ||
| ) | ||
| UPDATE pmoves_core.youtube_control_actions AS actions | ||
| SET status = 'processing' | ||
| FROM claimed | ||
| WHERE actions.id = claimed.id | ||
| RETURNING actions.id, actions.action, actions.details | ||
| """, | ||
| wanted_ids, | ||
| ) |
There was a problem hiding this comment.
Claimed actions can get stranded in processing.
This claim step commits status='processing' before the PMOVES.YT call runs. If the worker dies after the claim and before the final update, the row stops appearing in list_pending_youtube_control_actions() and never becomes reviewable again. Please add a lease/retry path for stale processing rows.
| try: | ||
| result_payload = await self._invoke_yt_control_action( | ||
| action=action, | ||
| details=details, | ||
| approved_by=actor or "channel-monitor", | ||
| approval_note=reason, |
There was a problem hiding this comment.
Persist the same reviewer identity you send downstream.
When actor is omitted, _invoke_yt_control_action() uses "channel-monitor" but the row update still writes approved_by = NULL. That leaves the audit row inconsistent with the executed payload.
🧭 Suggested normalization
for row in rows:
action_id = str(row.get("id"))
action = row.get("action")
details = row.get("details") if isinstance(row.get("details"), dict) else {}
+ resolved_actor = actor or "channel-monitor"
summary = details.get("request_summary") or _build_youtube_control_summary(action, details, details.get("draft"))
notebook_meta = details.get("notebook") if isinstance(details.get("notebook"), dict) else {}
@@
try:
result_payload = await self._invoke_yt_control_action(
action=action,
details=details,
- approved_by=actor or "channel-monitor",
+ approved_by=resolved_actor,
approval_note=reason,
)
@@
action_id,
new_status,
- actor,
+ resolved_actor,
reason,Also applies to: 2190-2205
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pmoves/services/channel-monitor/channel_monitor/monitor.py` around lines 2165
- 2170, The audit row update currently leaves approved_by NULL when actor is
omitted; change the code that writes the DB/audit row to use the same resolved
reviewer identity passed to _invoke_yt_control_action (i.e., use
resolved_approver = actor or "channel-monitor") so both the payload and the
stored row record the same approver; update the places around the
_invoke_yt_control_action call (and the similar block at 2190-2205) to reference
resolved_approver for approved_by instead of actor directly.
| try: | ||
| response.raise_for_status() | ||
| except httpx.HTTPStatusError as exc: | ||
| raise RuntimeError( | ||
| f"channel-monitor returned {exc.response.status_code}: " | ||
| f"{exc.response.text[:500]}" | ||
| ) from exc | ||
| body = response.json() | ||
| except Exception as exc: | ||
| logger.warning("YouTube control Discord interaction failed for %s: %s", action_id, exc) | ||
| return { | ||
| "type": 4, | ||
| "data": {"content": f"Review action failed for {action_id}: {exc}", "flags": 64}, |
There was a problem hiding this comment.
Don't echo upstream error details back to Discord users.
The inner RuntimeError includes the channel-monitor response body, and the outer handler returns str(exc) verbatim in the ephemeral message. That leaks internal diagnostics to anyone who clicks the button.
🔒 Suggested response handling
except Exception as exc:
logger.warning("YouTube control Discord interaction failed for %s: %s", action_id, exc)
return {
"type": 4,
- "data": {"content": f"Review action failed for {action_id}: {exc}", "flags": 64},
+ "data": {
+ "content": f"Review action failed for {action_id}. Please retry or check service logs.",
+ "flags": 64,
+ },
}🧰 Tools
🪛 Ruff (0.15.5)
[warning] 194-197: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 199-199: Do not catch blind exception: Exception
(BLE001)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pmoves/services/messaging-gateway/main.py` around lines 191 - 203, The code
currently raises a RuntimeError including exc.response.text and then returns
str(exc) to Discord, leaking upstream response bodies; change the error handling
so you do not include upstream response details in either the raised exception
message or the ephemeral reply: in the httpx.HTTPStatusError except block
(around response.raise_for_status / httpx.HTTPStatusError) remove or redact
exc.response.text from the RuntimeError (keep only status_code or a generic
message) and log the full response or exception internally (e.g.,
logger.exception or logger.error) for debugging; then update the outer except
handler that builds the Discord reply (the dict returned with "type": 4 and
"data": {"content": ...}) to return a generic user-facing message like "Review
action failed for {action_id}" without including exc or its text, while still
logging the full exception via logger.warning/logger.exception for operators.
| comment on table pmoves_core.youtube_control_actions is | ||
| 'Append-only audit trail for PMOVES.YT YouTube Data API playlist/comment control actions.'; |
There was a problem hiding this comment.
Table comment is misleading - rows are updated, not append-only.
The comment states "Append-only audit trail," but the review workflow in monitor.py updates rows (modifying status, approved_by, approval_note, result, error). Consider revising to accurately describe the table's mutability.
📝 Suggested comment revision
comment on table pmoves_core.youtube_control_actions is
- 'Append-only audit trail for PMOVES.YT YouTube Data API playlist/comment control actions.';
+ 'Audit trail for PMOVES.YT YouTube Data API playlist/comment control actions with status-tracking updates.';📝 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.
| comment on table pmoves_core.youtube_control_actions is | |
| 'Append-only audit trail for PMOVES.YT YouTube Data API playlist/comment control actions.'; | |
| comment on table pmoves_core.youtube_control_actions is | |
| 'Audit trail for PMOVES.YT YouTube Data API playlist/comment control actions with status-tracking updates.'; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pmoves/supabase/migrations/20260312095500_youtube_control_actions.sql` around
lines 84 - 85, Update the table comment for pmoves_core.youtube_control_actions
to reflect that rows are mutable (they are updated by monitor.py: fields like
status, approved_by, approval_note, result, error are modified) instead of
claiming "Append-only audit trail"; change the description to indicate it is an
audit/history of YouTube API control actions that may be updated with approval
and result metadata, referencing the table name
pmoves_core.youtube_control_actions and the monitored fields (status,
approved_by, approval_note, result, error).
010e8c8
into
PMOVES.AI-Edition-Hardened
3 participants
Summary
What Changed
Testing
Reviewer Notes
Summary by CodeRabbit
Release Notes
New Features
Documentation
Chores