feat(mpd): optional detectable exclusive-control mode before orphan cleanup (tr-cem) - #173
feat(mpd): optional detectable exclusive-control mode before orphan cleanup (tr-cem)#173jm2 wants to merge 1 commit into
Conversation
…leanup (tr-cem) Add MpdControlMode::Detected plus a bipartite detection probe that the worker restarts on every successful load and advances on each clean status observation. The probe is promoted to Confirmed only after MIN_DETECTION_OBSERVATIONS (3) consecutive clean status responses spanning at least MIN_DETECTION_AGE (250 ms); a foreign current song, any of repeat/random/single/consume flipped away from the enforced defaults, or an observation gap beyond MAX_DETECTION_GAP (2 s) lapses the probe back to Unconfirmed. While Confirmed, automatic orphan cleanup is permitted (matching the user-confirmed Exclusive mode); otherwise the conservative retain-on-cleanup path applies. The legacy exclusive_control: false default and the fail-closed load gate are preserved, and legacy outputs.json entries deserialize with detection_enabled: false. A new opt-in checkbox in the Add Output dialog pairs with a localized warning/confirmation message added to every supported catalog (13 locales). MpdStatus now parses repeat/random/single/consume from the partition-wide status command; observes_options_drift is the single drift gate that the detection probe uses to lapse. Adds detection_phase_state_machine, observed_status_drift_lapses_the_ detection_probe, detected_mode_lapses_on_stale_observation_window, detected_mode_promotes_to_confirmed_after_clean_observations, and detected_unobserved_load_is_rejected_before_any_observed_confirmation regressions in src/audio/mpd_output.rs; output_dialogs tests cover legacy default and detection-only opt-in.
![codacy-production[bot]](https://avatars.githubusercontent.com/in/56611?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull Request Overview
This pull request is fundamentally misaligned with its description. While the title and acceptance criteria focus on implementing MPD exclusive-control mode and orphan cleanup safety logic, the code changes consist entirely of a new GitHub Actions workflow for automated reviews. None of the MPD-related features are present in the diff.
Furthermore, the added workflow is currently non-functional and contains security concerns. It lacks the necessary authentication for the GitHub CLI to operate and requests unnecessary write permissions. Due to the complete absence of the intended features and the broken state of the added CI/CD logic, this PR is not up to standards and should not be merged.
About this PR
- The PR title and description describe a feature for MPD exclusive-control logic in
src/mpd/, but the actual diff contains only a CI/CD workflow file. The intended logic and acceptance criteria are entirely missing from this submission.
Test suggestions
- Missing recommended test scenario: Verify successful ownership confirmation after 3 clean observations over a 250ms window.
- Missing recommended test scenario: Verify probe invalidation and cleanup suppression when a foreign song is detected as 'current'.
- Missing recommended test scenario: Verify probe reset when MPD options (e.g., random, repeat) drift from enforced defaults.
- Missing recommended test scenario: Verify the 2-second timeout correctly lapses the probe and prevents orphan cleanup.
Prompt proposal for missing tests
Consider implementing these tests if applicable:
1. Missing recommended test scenario: Verify successful ownership confirmation after 3 clean observations over a 250ms window.
2. Missing recommended test scenario: Verify probe invalidation and cleanup suppression when a foreign song is detected as 'current'.
3. Missing recommended test scenario: Verify probe reset when MPD options (e.g., random, repeat) drift from enforced defaults.
4. Missing recommended test scenario: Verify the 2-second timeout correctly lapses the probe and prevents orphan cleanup.
TIP Improve review quality by adding custom instructions
TIP How was this review? Give us feedback
| uses: anthropics/claude-code-action@v1 | ||
| with: | ||
| claude_code_oauth_token: ${{ secrets.CLAUDE_CODE_OAUTH_TOKEN }} |
There was a problem hiding this comment.
🔴 HIGH RISK
The gh CLI commands enabled in claude_args will fail because the GH_TOKEN environment variable is not provided to the step. Add the environment variable to allow the action to interact with the GitHub API.
| uses: anthropics/claude-code-action@v1 | |
| with: | |
| claude_code_oauth_token: ${{ secrets.CLAUDE_CODE_OAUTH_TOKEN }} | |
| uses: anthropics/claude-code-action@v1 | |
| env: | |
| GH_TOKEN: ${{ github.token }} | |
| with: | |
| claude_code_oauth_token: ${{ secrets.CLAUDE_CODE_OAUTH_TOKEN }} |
| @@ -0,0 +1,73 @@ | |||
| name: Claude PR Review | |||
There was a problem hiding this comment.
🔴 HIGH RISK
The file added here (a GitHub Actions workflow) does not implement the MPD exclusive-control mode or orphan cleanup safety logic described in the PR summary. The expected changes in src/mpd/ are entirely missing from this diff.
| contents: read | ||
| pull-requests: write | ||
| issues: write | ||
| id-token: write |
There was a problem hiding this comment.
⚪ LOW RISK
Suggestion: The id-token: write permission is not required for this workflow's current configuration. Unless it is specifically needed for OIDC-based authentication, it should be removed to minimize the job's security footprint.
Up to standards ✅🟢 Issues
|
1 participant
Draft — redesign required
This PR investigates the optional MPD exclusive-control/ownership mode requested
by
docs/task.md, P2.10. The current exact head5ffbf0f9b63112adf826dccb60c240c0b9a0707cmust not merge.Exact-head review found that the implementation does not yet establish the
ownership guarantee required before automatic orphan cleanup:
Exclusive, while successful detectionis persisted as
Detected; enabling the feature can therefore make laterpublic loads reject the endpoint.
detection, so the detector can confirm state Tributary created itself rather
than proving the absence of another controller.
resulting confirmation can go stale before cleanup, recreating the
time-of-check/time-of-use race P2.10 and PR Require exclusive control for MPD playback #112 require this work to avoid.
revocation of already-running control paths are not yet modeled coherently.
The branch and PR are retained for a source-compatible redesign. Explicit
user-confirmed
Exclusivestate should remain the only grant of partitionauthority; observation may revoke that grant on positive evidence of
interference, but a quiet observation window must not grant or restore it.
After lapse, require explicit reconfirmation. If automatic authority is a hard
requirement, this bead is infeasible without an enforceable MPD-side lease,
ownership token, or atomic conditional mutation.
The persisted and public state machine must agree. Adversarial tests must cover
quiet competing clients, mutation between status and cleanup, stale authority,
lapse/reconfirmation, post-lapse controls, first-load zero-action, strict
option parsing, restart, and the real public load/persistence paths.
Hosted CI on this stale head is not merge evidence; the redesign must rebase
onto current
main(including the merged Last.fm flake fix) and rerun the fullexact-head matrix plus downstream checksum.
Issue:
tr-cem