Conversation
There was a problem hiding this comment.
Pull request overview
Updates the GitHub Actions E2E workflow to start the Conduwuit homeserver via an explicit docker run with a generated TOML config, aiming to stabilize the integration test setup.
Changes:
- Removes the GitHub Actions
services:container definition for Conduwuit. - Adds a step that writes
conduwuit.tomland starts Conduwuit viadocker run. - Drops passing
CONDUWUIT_CONTAINER_IDinto the integration test step.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 2 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="docker/test_integration.py">
<violation number="1" location="docker/test_integration.py:22">
P1: Fail fast when token discovery fails instead of silently falling back to a hardcoded token.</violation>
<violation number="2" location="docker/test_integration.py:44">
P2: Do not print the full registration token in logs; this leaks a credential used for registration.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
1 issue found across 5 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="matrix_premid.py">
<violation number="1" location="matrix_premid.py:206">
P2: Adding the new `Watching` prefix without updating `update()` quality parsing regresses metadata prioritization, so lower-quality statuses can overwrite richer ones for the same title.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
1 issue found across 4 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="Makefile">
<violation number="1" location="Makefile:53">
P1: `test` no longer depends on `deps`, so `make test` can fail on fresh setups because pytest/dev dependencies are not installed.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.
Comments suppressed due to low confidence (1)
tests/test_updater.py:172
test_main_execution_mocked_gathercallsmain(), butmain()now exits early ifplayerctlis not found viashutil.which. This test (and the othermain()test) should patchmatrix_premid.shutil.whichto return a fake path so the assertions exercise the intended code paths and don't become environment-dependent.
@pytest.mark.asyncio
@patch("matrix_premid.sys.exit")
@patch("matrix_premid.acquire_lock")
@patch("matrix_premid.HOMESERVER", "mock")
@patch("matrix_premid.USERNAME", "@user")
@patch("matrix_premid.ACCESS_TOKEN", "tok")
@patch("matrix_premid.DEVICE_ID", "dev")
async def test_main_execution_mocked_gather(_mock_lock, mock_exit):
"""Test main entrypoint setups everything cleanly resolving without errors."""
with patch("matrix_premid.AsyncClient") as mock_client:
mock_instance = AsyncMock()
mock_client.return_value = mock_instance
mock_event = MagicMock()
mock_event.user_id = "@user"
mock_event.presence = "dnd"
mock_resp = MagicMock()
mock_resp.presence.events = [mock_event]
# Emit native valid presence payload once, then gracefully exit loop
mock_instance.sync.side_effect = [mock_resp, asyncio.CancelledError()]
with patch(
"matrix_premid.MatrixStatusUpdater.update", new_callable=AsyncMock
) as mock_update:
mock_update.side_effect = asyncio.CancelledError()
with patch("matrix_premid.asyncio.create_subprocess_exec") as mock_exec:
mock_proc = AsyncMock()
mock_proc.communicate.side_effect = asyncio.CancelledError()
mock_exec.return_value = mock_proc
await main()
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
1 issue found across 4 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="tests/test_updater.py">
<violation number="1" location="tests/test_updater.py:156">
P2: This test fixture now uses `online`, which makes it unable to catch regressions where native non-default presence is ignored and forced back to online.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="docker/test_integration.py">
<violation number="1" location="docker/test_integration.py:29">
P2: Using `docker logs --since 1s` can miss the one-time registration-token log line and make this integration test flaky.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/matrix_premid/__main__.py">
<violation number="1" location="src/matrix_premid/__main__.py:658">
P2: The new `set` command is incorrectly blocked by the global `playerctl` dependency check. `set` should work without `playerctl` since it does not use MPRIS.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| ) | ||
| ) | ||
|
|
||
| if args.command == "set": |
There was a problem hiding this comment.
P2: The new set command is incorrectly blocked by the global playerctl dependency check. set should work without playerctl since it does not use MPRIS.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/matrix_premid/__main__.py, line 658:
<comment>The new `set` command is incorrectly blocked by the global `playerctl` dependency check. `set` should work without `playerctl` since it does not use MPRIS.</comment>
<file context>
@@ -652,6 +655,32 @@ async def main(args=None):
)
)
+ if args.command == "set":
+ status_msg = " ".join(args.status_args)
+ if not status_msg:
</file context>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 15 out of 18 changed files in this pull request and generated 7 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| 1. **Install dependencies**: ``pip install .`` (or use installation methods above). | ||
| 2. **Setup environment**: Place your ``.env`` file in the current directory or at ``~/.config/matrix-premid/.env``. | ||
| 3. **Run the script**: ``matrix-premid`` |
There was a problem hiding this comment.
The configuration instructions still mention placing a .env file and exporting env vars, but the CLI now loads ~/.config/matrix-premid/config.json and resolves tokens via keyring. Update these steps to describe config.json + keyring (or embedding access_token in config.json).
| sudo make install | ||
|
|
||
| This creates the directory ``/opt/matrix-premid``, copies the script and ``.env`` there, sets up an isolated Python virtual environment exclusively for the service, and symlinks the script to ``/usr/local/bin/matrix_premid``. The systemd service is placed in ``/etc/systemd/system/``. | ||
| This creates the directory ``/opt/matrix-premid``, copies the script and ``.env`` there, sets up an isolated Python virtual environment exclusively for the service, and symlinks the script to ``/usr/local/bin/matrix-premid``. The systemd service is placed in ``/etc/systemd/system/``. | ||
|
|
There was a problem hiding this comment.
The docs describe global install into /opt and /etc/systemd/system, but the Makefile install target now performs a user-level pip install and sets up a systemd user unit. Update this section so it matches the new installation flow and paths.
| @@ -39,6 +25,13 @@ jobs: | |||
| make deps | |||
There was a problem hiding this comment.
The repo is now a package with a matrix-premid console script, but this workflow only installs requirements*.txt via make deps and never installs the package itself. If the integration test is updated to run matrix-premid/python -m matrix_premid, ensure the workflow also does pip install ./pip install -e . (so runtime deps like keyring are present) or otherwise sets up PYTHONPATH=src explicitly.
| make deps | |
| make deps | |
| . .venv/bin/activate | |
| pip install . |
|
|
||
| .. code-block:: bash | ||
|
|
||
| make install-user |
There was a problem hiding this comment.
README references a make install-user target, but the Makefile in this PR only defines install (and no install-user). Either add the missing target or update the README to point to the correct Make target/command.
| make install-user | |
| pip install --user . |
| lines = stdout.decode("utf-8").strip().splitlines() if stdout else [] | ||
| if updaters and updaters[0].verbose: # pragma: no cover | ||
| print(f"DEBUG: raw playerctl lines: {lines}", flush=True) | ||
| activity, title = _get_best_mpris_activity(lines) |
There was a problem hiding this comment.
When _get_best_mpris_activity() returns an empty activity (e.g., no players / only unparseable lines), MatrixStatusUpdater.update() currently treats that as a no-op, so the idle debounce logic never runs and the last non-idle status can remain stuck indefinitely after media stops/closes. Consider mapping the “no activity” case to an explicit "Idle" update (either in _get_best_mpris_activity() or here in monitor_mpris) so the existing idle_strikes / idle_timeout mechanism can work.
| activity, title = _get_best_mpris_activity(lines) | |
| activity, title = _get_best_mpris_activity(lines) | |
| # If no meaningful activity is found, treat it as explicit Idle | |
| if not activity: | |
| activity = "Idle" |
| if "YT Music" in activity: | ||
| quality += 1 | ||
| elif activity.startswith("Paused:"): | ||
| quality = 6 if " - " in activity else 4 | ||
| if "YT Music" in activity: |
There was a problem hiding this comment.
The quality boost checks for the substring "YT Music", but activities are tagged with the provider name "YouTube Music" (e.g., "| YouTube Music"). As written, this bonus will never apply, which can affect metadata quality comparisons and update suppression. Align this check with the actual provider label you emit.
| if "YT Music" in activity: | |
| quality += 1 | |
| elif activity.startswith("Paused:"): | |
| quality = 6 if " - " in activity else 4 | |
| if "YT Music" in activity: | |
| if "YouTube Music" in activity: | |
| quality += 1 | |
| elif activity.startswith("Paused:"): | |
| quality = 6 if " - " in activity else 4 | |
| if "YouTube Music" in activity: |
| if is_exit or activity == "Idle" or not activity: | ||
| payload_s = {} | ||
| else: | ||
| payload_s = {"status": activity} |
There was a problem hiding this comment.
update() treats any activity starting with "Idle" as idle (for debouncing), but send_update() only clears Element status when activity == "Idle". This means idle-like states such as "Idle (YouTube Music)" can be sent as a visible custom status instead of being cleared. Consider using a consistent idle predicate here (e.g., activity.startswith("Idle")).
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="tests/test_updater.py">
<violation number="1" location="tests/test_updater.py:544">
P2: Avoid re-patching `open` inside the test body; call `install_service()` directly and assert the existing `_mock_open` decorator mock was used.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 15 out of 18 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| env: | ||
| CONDUWUIT_CONTAINER_ID: ${{ job.services.conduwuit.id }} | ||
| run: | | ||
| python docker/test_integration.py |
There was a problem hiding this comment.
The workflow runs the integration test without installing the package. With the new src/ layout and removal of matrix_premid.py, the integration test needs the module installed (e.g. pip install -e .) or an explicit PYTHONPATH=src when invoking the CLI. As written, the job is likely to fail once docker/test_integration.py is updated to python -m matrix_premid/matrix-premid.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 16 out of 19 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="tests/test_parser.py">
<violation number="1" location="tests/test_parser.py:192">
P3: This test bypasses provider detection by hardcoding `global_provider`, so it doesn’t actually verify the stated Spotify detection behavior.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 16 out of 19 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| .. code-block:: bash | ||
|
|
||
| make install | ||
| sudo make install |
There was a problem hiding this comment.
README now instructs sudo make install, but the Makefile install target already uses sudo internally and then runs matrix-premid install-service without sudo to create a user systemd unit. Running the whole make target under sudo will create the user service/config under root’s home instead. Update the docs to run make install (no sudo) or adjust the install target so only the privileged steps run as root.
| sudo make install | |
| make install |
| if "YT Music" in activity: | ||
| quality += 1 | ||
| elif activity.startswith("Paused:"): | ||
| quality = 6 if " - " in activity else 4 | ||
| if "YT Music" in activity: |
There was a problem hiding this comment.
MatrixStatusUpdater.update() boosts quality when activity contains "YT Music", but the provider strings produced elsewhere are "YouTube Music". This makes the quality boost ineffective and can change debouncing/override behavior. Consider checking for "YouTube Music" (or a more general provider marker like "| YouTube Music").
| if "YT Music" in activity: | |
| quality += 1 | |
| elif activity.startswith("Paused:"): | |
| quality = 6 if " - " in activity else 4 | |
| if "YT Music" in activity: | |
| if "YT Music" in activity or "YouTube Music" in activity: | |
| quality += 1 | |
| elif activity.startswith("Paused:"): | |
| quality = 6 if " - " in activity else 4 | |
| if "YT Music" in activity or "YouTube Music" in activity: |
| payload_p = { | ||
| "presence": self.current_presence, | ||
| } | ||
| if activity and activity != "Idle": | ||
| payload_p["status_msg"] = activity | ||
|
|
||
| # 2. Element Status Payload | ||
| url_s = ( | ||
| f"{self.homeserver}/_matrix/client/v3/user/{self.username}/" | ||
| "account_data/im.vector.user_status" | ||
| ) | ||
|
|
||
| if is_exit or activity == "Idle" or not activity: | ||
| payload_s = {} | ||
| else: | ||
| payload_s = {"status": activity} | ||
|
|
There was a problem hiding this comment.
send_update() treats only the exact string "Idle" as idle when deciding whether to omit status_msg / clear im.vector.user_status, but other code emits idle variants like "Idle (YouTube Music)". This will incorrectly publish an idle status message instead of clearing it. Use the same idle predicate as update() (e.g., activity.startswith("Idle") or a shared helper) when building both payloads.
There was a problem hiding this comment.
3 issues found across 4 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/matrix_premid/__main__.py">
<violation number="1" location="src/matrix_premid/__main__.py:696">
P2: Disabled accounts still require an access token, so `enabled: false` accounts can abort startup before they are skipped.</violation>
<violation number="2" location="src/matrix_premid/__main__.py:867">
P3: Close the temporary `/dev/null` FD after `dup2` to avoid leaking a file descriptor in daemon mode.</violation>
</file>
<file name="Makefile">
<violation number="1" location="Makefile:29">
P1: `install-service` is executed as the current Make user, which becomes root under `sudo make install`; this sets up user-service/config for the wrong account.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| sudo /opt/matrix-premid/venv/bin/pip install . | ||
| sudo ln -sf /opt/matrix-premid/venv/bin/matrix-premid /usr/local/bin/matrix-premid | ||
| @echo "Setting up systemd service..." | ||
| /usr/local/bin/matrix-premid install-service |
There was a problem hiding this comment.
P1: install-service is executed as the current Make user, which becomes root under sudo make install; this sets up user-service/config for the wrong account.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Makefile, line 29:
<comment>`install-service` is executed as the current Make user, which becomes root under `sudo make install`; this sets up user-service/config for the wrong account.</comment>
<file context>
@@ -21,9 +21,12 @@ deps: ##H Install standard and dev dependencies
+ sudo ln -sf /opt/matrix-premid/venv/bin/matrix-premid /usr/local/bin/matrix-premid
@echo "Setting up systemd service..."
- ~/.local/bin/matrix-premid install-service
+ /usr/local/bin/matrix-premid install-service
# ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
</file context>
| /usr/local/bin/matrix-premid install-service | |
| if [ -n "$$SUDO_USER" ]; then sudo -u "$$SUDO_USER" /usr/local/bin/matrix-premid install-service; else /usr/local/bin/matrix-premid install-service; fi |
| account["username"], | ||
| account["access_token"], | ||
| device_id=account.get("device_id", ""), | ||
| enabled=account.get("enabled", True), |
There was a problem hiding this comment.
P2: Disabled accounts still require an access token, so enabled: false accounts can abort startup before they are skipped.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/matrix_premid/__main__.py, line 696:
<comment>Disabled accounts still require an access token, so `enabled: false` accounts can abort startup before they are skipped.</comment>
<file context>
@@ -685,6 +693,7 @@ async def main(args=None):
account["username"],
account["access_token"],
device_id=account.get("device_id", ""),
+ enabled=account.get("enabled", True),
idle_timeout=idle_timeout,
poll_interval=poll_interval,
</file context>
There was a problem hiding this comment.
1 issue found across 6 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="tests/test_updater.py">
<violation number="1" location="tests/test_updater.py:187">
P2: This test now uses a loose substring check and no longer validates the expected `title` kwarg, which can let real regressions pass unnoticed.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/matrix_premid/__main__.py">
<violation number="1" location="src/matrix_premid/__main__.py:643">
P2: Time-quality bonus is granted based on raw non-empty fields, so zero/invalid durations can be misclassified as valid timestamps.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| has_real_artist = bool( | ||
| clean_raw_artist and clean_raw_artist.lower() not in banned_artists | ||
| ) | ||
| has_time = bool(raw_pos and raw_len) |
There was a problem hiding this comment.
P2: Time-quality bonus is granted based on raw non-empty fields, so zero/invalid durations can be misclassified as valid timestamps.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/matrix_premid/__main__.py, line 643:
<comment>Time-quality bonus is granted based on raw non-empty fields, so zero/invalid durations can be misclassified as valid timestamps.</comment>
<file context>
@@ -629,13 +629,32 @@ def _get_best_mpris_activity(lines: list[str]) -> tuple[str, str]:
+ has_real_artist = bool(
+ clean_raw_artist and clean_raw_artist.lower() not in banned_artists
+ )
+ has_time = bool(raw_pos and raw_len)
+
quality = 0
</file context>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 19 out of 22 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| 1. **Install dependencies**: ``pip install .`` (or use installation methods above). | ||
| 2. **Setup environment**: Place your ``.env`` file in the current directory or at ``~/.config/matrix-premid/.env``. | ||
| 3. **Run the script**: ``matrix-premid`` | ||
|
|
There was a problem hiding this comment.
The README now instructs users to place a .env file for configuration, but the new CLI no longer loads dotenv/env-based configuration; it requires ~/.config/matrix-premid/config.json and uses keyring for missing access tokens. Update the “Basic Usage” steps to describe creating/editing config.json (or using the provided template) and storing tokens in keyring (or placing access_token in config for non-interactive setups like CI).
| # If no activity detected, we don't force 'Idle' immediately. | ||
| # We preserve the last activity unless it's explicitly 'Idle' | ||
| # or we are exiting. | ||
| return |
There was a problem hiding this comment.
MatrixStatusUpdater.update() returns immediately when activity is empty (unless is_exit), but the idle debounce/idle_timeout logic only runs when activity is "Idle"/"Idle...". Combined with _get_best_mpris_activity() returning "" when nothing is playing, this means the updater may never transition to Idle when playback stops and idle_timeout becomes ineffective. Consider treating empty activity as an idle signal (possibly debounced via idle_strikes) or have _get_best_mpris_activity() return "Idle" when no active players are detected.
| # If no activity detected, we don't force 'Idle' immediately. | |
| # We preserve the last activity unless it's explicitly 'Idle' | |
| # or we are exiting. | |
| return | |
| # Treat lack of activity as a potential idle state so that | |
| # idle_timeout / idle_strikes logic can take effect. | |
| activity = "Idle" | |
| title = "" |
| # Clean up lock file so next start works cleanly | ||
| try: | ||
| os.unlink(LOCK_FILE) | ||
| except OSError: | ||
| pass |
There was a problem hiding this comment.
The lock file is unlinked during shutdown, but the lock file descriptor returned by acquire_lock() is never closed. Unlinking while still holding the lock releases the name while the process is still alive, allowing a second instance to create a new lock file and acquire a lock before this process exits. Keep a reference to the lock FD and close it before unlinking (or just leave the lock file in place and rely on lockf + truncation).
| # Clean up lock file so next start works cleanly | |
| try: | |
| os.unlink(LOCK_FILE) | |
| except OSError: | |
| pass | |
| # Do not unlink the lock file here. Locking is enforced via the file | |
| # descriptor and POSIX locking (lockf), not by the mere presence of | |
| # the file. Unlinking while still holding the lock can allow another | |
| # process to recreate the file and acquire a separate lock before this | |
| # process fully exits. |
| sudo /opt/matrix-premid/venv/bin/pip install . | ||
| sudo ln -sf /opt/matrix-premid/venv/bin/matrix-premid /usr/local/bin/matrix-premid | ||
| @echo "Setting up systemd service..." | ||
| /usr/local/bin/matrix-premid install-service |
There was a problem hiding this comment.
make install is intended to be run via sudo (per README), but it then runs matrix-premid install-service without dropping privileges. That will create the user systemd unit under /root/.config/systemd/user (and generate root-owned config), not the invoking user’s home directory. Either avoid sudo make install for the user-service flow, or run install-service as the original user (e.g., via sudo -u $SUDO_USER ...), or change the install to create a system-level service instead of a user service.
| /usr/local/bin/matrix-premid install-service | |
| @if [ -n "$$SUDO_USER" ]; then \ | |
| echo "Running matrix-premid install-service as $$SUDO_USER"; \ | |
| sudo -u "$$SUDO_USER" /usr/local/bin/matrix-premid install-service; \ | |
| else \ | |
| /usr/local/bin/matrix-premid install-service; \ | |
| fi |
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/matrix_premid/__main__.py">
<violation number="1" location="src/matrix_premid/__main__.py:707">
P2: Validate `poll_interval`/`idle_timeout` from hot-reloaded config before assigning; invalid types can crash the monitor loop at runtime.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 19 out of 22 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # Clean up lock file so next start works cleanly | ||
| try: | ||
| os.unlink(LOCK_FILE) | ||
| except OSError: | ||
| pass |
There was a problem hiding this comment.
The lock file is unlinked at shutdown, but the file descriptor returned from acquire_lock() is never closed before os.unlink(LOCK_FILE). On POSIX, unlinking while still holding the lock can allow a second process to create a new lock file and acquire a lock before the first process fully exits, defeating the single-instance guarantee. Fix by closing the lock fd before unlinking, or by leaving the lock file in place and relying on advisory locking + PID contents for cleanup.
| # Clean up lock file so next start works cleanly | |
| try: | |
| os.unlink(LOCK_FILE) | |
| except OSError: | |
| pass | |
| # Intentionally keep the lock file; rely on advisory locking and PID | |
| # contents for cleanup to avoid unlink-while-locked races on POSIX. |
|
|
||
| if quality > best_quality: | ||
| best_activity, best_title, best_quality = activity, title, quality | ||
|
|
There was a problem hiding this comment.
When no valid playerctl lines are found, _get_best_mpris_activity() returns empty strings. monitor_mpris() then passes an empty activity into MatrixStatusUpdater.update(), which returns early and never increments idle_strikes, so the last activity can be refreshed indefinitely (keep_alive sends last_activity). Consider returning an explicit idle sentinel (e.g., "Idle") in the no-activity case (or translating empty->Idle in monitor_mpris()), and update tests accordingly so debounced idle can work.
| # If no suitable activity was found, explicitly return Idle so that | |
| # downstream idle debouncing logic can trigger correctly. | |
| if not best_activity: | |
| return "Idle", "" |
|
You're iterating quickly on this pull request. To help protect your rate limits, cubic has paused automatic reviews on new pushes for now—when you're ready for another review, comment |
- Removed matrix-nio dependency in favor of lightweight aiohttp requests - Added multi-account support with secure keyring token storage - Refactored into a distributable Python package with dynamic git versioning (hatch-vcs) - Introduced daemon and shutdown CLI commands with robust lockfile management - Improved provider parsing (media time tracking, watch vs. listen priority) - Implemented exponential backoff for sync loop and fixed idle state transitions - Expanded test coverage and fixed integration test instability
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 19 out of 22 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| Command-line Options | ||
| -------------------- | ||
|
|
||
| * ``--unset`` or ``--clear``: Manually clear status to AFK (unavailable) and exit. |
There was a problem hiding this comment.
--unset/--clear is documented as clearing status to "AFK (unavailable)", but the implementation sets Matrix presence to offline and clears the custom status (see MatrixStatusUpdater.send_update(..., is_exit=True)). Suggest updating the wording to match actual behavior (offline/clear) or changing the code to set the intended presence state.
| * ``--unset`` or ``--clear``: Manually clear status to AFK (unavailable) and exit. | |
| * ``--unset`` or ``--clear``: Set presence to ``offline``, clear the custom status, and exit. |
Summary by cubic
Ships a v2 rewrite that packages
matrix-premidas a proper CLI/daemon with a systemd user service, keyring-backed multi-account config, and dynamic versioning viahatch-vcs; it also stabilizes CI by running a configured Conduwuit and parsing its registration token. Presence is more accurate with a config-driven, URL-aware provider system (watch vs listen, timestamps), debounced idle, and clean shutdown to offline using direct REST viaaiohttp.New Features
matrix-premidentry point, tab completion, and git-backed versioning.config.template.json.daemon/shutdown,--debug/--version, installers for user or/opt, and secure tokens via OS keyring.Bug Fixes
Written for commit 1eb1a10. Summary will update on new commits.