fix(unifi): reuse recycled NFC cards (free on deactivate, reclaim from disabled holder)#33
Conversation
Two related fixes for CODE_CREDS_NFC_HAS_BIND_USER (a card CiviCRM assigns to a member is still bound to a different UniFi user, so the force_add=false bind is rejected and the cycle alerts every run): - Deactivation now also deletes the user's NFC card(s), freeing the number so a recycled physical card can be reassigned. Status is set first, so cutting access never depends on the card cleanup succeeding. - When a bind is still rejected because the card is bound elsewhere (a member deactivated before this behavior shipped, or a manually-enrolled admin card), the per-user error now names the current holder — contact id for a sync-managed user, else the UniFi user id, plus display name and status — so the alert is actionable. Card number stays redacted (architecture §11). A token->holder map is captured for every user (sync-managed or not) during fetch_users(). The three card-delete loops and both bind sites are unified into _delete_cards_for_contact() and _bind_nfc_card(). force_add stays false. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Completes the card-reuse story for CODE_CREDS_NFC_HAS_BIND_USER. When a bind is rejected because the card is still bound elsewhere, door-sync inspects the current holder (from the fetch_users snapshot): - Disabled holder: the card is dead state on a deactivated user (e.g. a member deactivated before cards were freed on deactivation). door-sync unbinds it from them and retries the bind, logging a redacted warning. This auto-recovers the backlog of recycled cards that the deactivate-frees-card change can't reach (already-inactive users are never re-processed by the reconciler). - Active holder (or one not in the snapshot): left untouched; the bind fails as a per-user error naming the holder, so an operator resolves it deliberately. force_add stays false — door-sync never force-steals; it only unbinds a card a disabled holder is no longer using. Card numbers stay redacted (architecture §11). The raw bind PUT is factored into _put_bind_card(). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 48 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthrough
ChangesNFC Card Lifecycle and Bind-Conflict Handling
Sequence Diagram(s)sequenceDiagram
participant Caller
participant _bind_nfc_card
participant UniFiAPI
participant _holder_by_token
Caller->>_bind_nfc_card: bind token for contact
_bind_nfc_card->>UniFiAPI: PUT /users/:id (force_add=False)
UniFiAPI-->>_bind_nfc_card: 409 CODE_CREDS_NFC_HAS_BIND_USER
_bind_nfc_card->>_holder_by_token: lookup holder by token
alt holder status is DEACTIVATED
_bind_nfc_card->>UniFiAPI: PUT /users/:id (force_add=True)
UniFiAPI-->>_bind_nfc_card: 200 OK
_bind_nfc_card->>_holder_by_token: drop token from disabled holder cache
_bind_nfc_card-->>Caller: success
else holder is active or unknown
_bind_nfc_card-->>Caller: raise error with redacted holder description
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Code Coverage OverviewLanguages: Python Python / code-coverage/pytestThe overall coverage remains at 93%, unchanged from the branch. Updated |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
This PR updates the UniFi client reconciliation flow to prevent recycled NFC cards from getting “stuck” on prior UniFi users, improving operational recovery and making bind-conflict alerts actionable.
Changes:
- On deactivation, delete the user’s cached NFC card bindings after setting
status=DEACTIVATED. - On bind conflict (
CODE_CREDS_NFC_HAS_BIND_USER), enrich the error with the current holder, and automatically reclaim the card only when the holder is disabled. - Add/adjust tests and update design/architecture documentation to reflect the new behavior and redaction guarantees.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
src/door_sync/unifi/client.py |
Adds card-holder tracking, frees cards on deactivation, and implements disabled-holder reclaim + improved conflict messaging. |
tests/test_unifi_client.py |
Adds coverage for deactivation freeing cards, active-holder conflict naming, and disabled-holder reclaim retry. |
docs/superpowers/specs/2026-05-22-unifi-client-design.md |
Updates the step-by-step apply semantics and force-add rationale to match the new reclaim behavior. |
docs/architecture.md |
Documents card reuse handling (proactive free + reactive reclaim) and holder-identifying bind conflict alerts. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
The reclaim path hit HTTP 404 "CODE_NOT_FOUND / no-man zone" in production: UniFi rejects NFC-card mutations on a *deactivated* user, so the card cannot be DELETEd off a disabled holder. Two corrections, same root cause: - Reclaim: instead of DELETE-then-rebind (impossible on a disabled holder), force-reassign the card to the new member with force_add=true, which moves it. Still gated on the holder being confirmed-disabled; active/admin holders are never displaced. - Deactivate: free the card *before* setting DEACTIVATED (while the account is still active and the delete is permitted), best-effort so a cleanup failure never blocks cutting access. Any leftover is reclaimed on next assignment. force_add stays false for every normal bind; true is used only to reclaim from a disabled holder. Tests + docs (architecture §7, design spec) updated. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Pushed a follow-up correction ( Root cause: UniFi rejects NFC-card mutations on a deactivated user, so a card can't be Corrections (same root cause):
352 tests pass, pyrefly 0 errors, ruff clean. |
…che coherence Review feedback on PR #33 (CodeQL + Copilot): - CodeQL py/clear-text-logging-sensitive-data: _describe_card_holder logged the holder's display_name (PII), which the codebase otherwise never logs (it uses contact ids). Drop the name from _CardHolder entirely; identify holders by contact id (sync-managed) or UniFi user id, plus status. - Null-safety in _record_card_holders: use `or ""` for id/status/token. A JSON null status previously stringified to "None", which _holder_is_disabled read as *disabled* — i.e. door-sync could force-reclaim a card from a holder whose state was actually unknown. Now an unknown status is never treated as disabled. - Cache coherence: after force-reassigning a card off a disabled holder, also drop the moved token from that holder's cached card list (_forget_holder_card), so a later same-cycle op on the holder doesn't DELETE a token it no longer owns. Tests updated/added: holder identified by id not name; null-status holder is not reclaimed; reclaim prunes the holder's cached card. Docs updated. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ntion Round out the documentation for the card-reuse work: - conventions.rst: add the "no member names in logs/alerts" convention alongside card-ID redaction (identify by contact_id; show-diff CLI may still print names as direct operator output). - unifi-client-design spec: note the bind-conflict handling in the apply-flow step; align the force_add note to "identify by id, not name". - manual-test-plan: 4e now verifies the card is freed on deactivation; new 4f covers recycled-card reassignment, reclaim from a disabled holder (the production case), and the active-holder guard. - CLAUDE.md: add the no-member-names hard rule next to card-ID redaction. Docs only; no code change. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
docs/architecture.md (2)
255-261: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd reference to cache coherence update after reclaim.
The architecture describes the reclaim flow well but omits that the implementation also updates internal caches (
_holder_by_token.pop(token, None)and_forget_holder_card) after a successful force-reclaim. This prevents later operations in the same cycle from attempting to delete a token the disabled holder no longer owns. Consider adding a brief note like: "After reclaim, the card is removed from the holder cache so subsequent same-cycle operations see correct state."This documents an important implementation detail that prevents subtle bugs.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/architecture.md` around lines 255 - 261, The reclaim flow description is missing the internal cache update that happens after a successful force-reclaim. Update the architecture note around the bind conflict handling to mention that, after `force_add: true` succeeds, the implementation removes the token from the holder cache via `_holder_by_token.pop(token, None)` and `_forget_holder_card` so later same-cycle operations see the corrected ownership state. Keep the note near the existing `force_add`/reclaim explanation and reference those symbols for easy location.
255-261: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueClarify whose name is protected in the PII note.
The text states "The member's name is never logged" but the context describes logging the current holder's identifiers (contact id or user id, plus status). The member being provisioned is identified by
contact_idin the error, while the holder's name is what's actually kept out of logs. Consider rephrasing to: "The holder's name is never logged" or "PII (names) are never logged; only identifiers are disclosed" for clarity.This is a documentation clarity issue, not a functional bug.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/architecture.md` around lines 255 - 261, Clarify the PII note in the card reuse section so it matches the behavior described by the bind-conflict flow in this docs block. Update the wording around the logging rule in the “On bind conflict” paragraph to state that the current holder’s name is never logged (or that only identifiers/status are logged), since the error message exposes the holder’s contact id or UniFi user id plus status while keeping names out of logs; use the surrounding “force_add”, “current holder”, and “contact id” wording as the anchor for the edit.docs/superpowers/specs/2026-05-22-unifi-client-design.md (1)
152-152: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueGeneralize the reclaim reference beyond "step 4".
The note says "any card left behind is reclaimed on its next assignment (step 4)" but reclaim via
_bind_nfc_cardcan occur in both step 4 (Update credential) and step 6 (Add, including reactivate). Consider rephrasing to "on its next bind attempt" or "when the card is next assigned to any member" to avoid implying reclaim only happens during credential updates.This improves accuracy since the reclaim path is shared across multiple steps.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/superpowers/specs/2026-05-22-unifi-client-design.md` at line 152, Update the deactivate note in the spec so the reclaim wording is not tied only to “step 4”; the current reference under the deactivate flow is too specific because `_bind_nfc_card` can reclaim a leftover card during both the Update credential path and the Add/reactivate path. Rephrase the sentence near `diff.to_deactivate` to say the card is reclaimed on its next bind attempt or next assignment to any member, keeping the delete-before-status-change behavior unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/door_sync/unifi/client.py`:
- Around line 774-782: The delete-failure path in UnifiClient’s contact
deactivation flow leaves _holder_by_token with a stale ACTIVE snapshot after
_delete_cards_for_contact() raises, which can cause later bind conflicts to
mis-handle the holder state. Update the exception handling in the
deactivate/cleanup logic around _delete_cards_for_contact() to drop or refresh
the affected token entries in _holder_by_token to DEACTIVATED so the cache
matches the warning and future binds can force-reclaim correctly.
---
Nitpick comments:
In `@docs/architecture.md`:
- Around line 255-261: The reclaim flow description is missing the internal
cache update that happens after a successful force-reclaim. Update the
architecture note around the bind conflict handling to mention that, after
`force_add: true` succeeds, the implementation removes the token from the holder
cache via `_holder_by_token.pop(token, None)` and `_forget_holder_card` so later
same-cycle operations see the corrected ownership state. Keep the note near the
existing `force_add`/reclaim explanation and reference those symbols for easy
location.
- Around line 255-261: Clarify the PII note in the card reuse section so it
matches the behavior described by the bind-conflict flow in this docs block.
Update the wording around the logging rule in the “On bind conflict” paragraph
to state that the current holder’s name is never logged (or that only
identifiers/status are logged), since the error message exposes the holder’s
contact id or UniFi user id plus status while keeping names out of logs; use the
surrounding “force_add”, “current holder”, and “contact id” wording as the
anchor for the edit.
In `@docs/superpowers/specs/2026-05-22-unifi-client-design.md`:
- Line 152: Update the deactivate note in the spec so the reclaim wording is not
tied only to “step 4”; the current reference under the deactivate flow is too
specific because `_bind_nfc_card` can reclaim a leftover card during both the
Update credential path and the Add/reactivate path. Rephrase the sentence near
`diff.to_deactivate` to say the card is reclaimed on its next bind attempt or
next assignment to any member, keeping the delete-before-status-change behavior
unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: b9bf619f-58b6-4a85-8d69-b59f34645e03
📒 Files selected for processing (4)
docs/architecture.mddocs/superpowers/specs/2026-05-22-unifi-client-design.mdsrc/door_sync/unifi/client.pytests/test_unifi_client.py
| try: | ||
| self._delete_cards_for_contact(user_id, unifi_user.contact_id) | ||
| except UnifiClientError as card_exc: | ||
| logger.warning( | ||
| "contact %d: could not free card before deactivation (%s); " | ||
| "it will be reclaimed when the number is next assigned", | ||
| unifi_user.contact_id, | ||
| card_exc, | ||
| ) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm _holder_by_token is never updated to reflect a same-cycle deactivation.
rg -nP '_holder_by_token' src/door_sync/unifi/client.pyRepository: TechValleyCenterOfGravity/door-sync
Length of output: 411
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the relevant sections around holder caching and deactivation flow.
sed -n '420,720p' src/door_sync/unifi/client.py
printf '\n---\n'
sed -n '748,820p' src/door_sync/unifi/client.pyRepository: TechValleyCenterOfGravity/door-sync
Length of output: 17275
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the helper logic that gates reclaimability and any cache mutation helpers.
sed -n '1,220p' src/door_sync/unifi/client.py
printf '\n---\n'
sed -n '220,420p' src/door_sync/unifi/client.py
printf '\n---\n'
sed -n '820,940p' src/door_sync/unifi/client.pyRepository: TechValleyCenterOfGravity/door-sync
Length of output: 22966
Keep the holder cache in sync on delete failure. If _delete_cards_for_contact() fails here, the warning says the card will be reclaimed on the next assignment, but _holder_by_token still has the pre-fetch ACTIVE snapshot. A later bind conflict in the same cycle will treat the holder as active and re-raise instead of force-reclaiming. Dropping or refreshing the affected token(s) to DEACTIVATED here would match the cache to the warning.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/door_sync/unifi/client.py` around lines 774 - 782, The delete-failure
path in UnifiClient’s contact deactivation flow leaves _holder_by_token with a
stale ACTIVE snapshot after _delete_cards_for_contact() raises, which can cause
later bind conflicts to mis-handle the holder state. Update the exception
handling in the deactivate/cleanup logic around _delete_cards_for_contact() to
drop or refresh the affected token entries in _holder_by_token to DEACTIVATED so
the cache matches the warning and future binds can force-reclaim correctly.
If the pre-deactivation card delete fails, the card stays bound to the user we then deactivate, but _holder_by_token still carries the ACTIVE snapshot taken at fetch time. A same-cycle reassignment of that number would read ACTIVE and refuse to reclaim — a spurious per-user failure. After the status change succeeds, refresh the contact's cached card holders to DEACTIVATED (user_id guarded), so the still-bound card is force-reclaimed correctly. Test: deactivation with a failing card delete, then a same-cycle reassignment of that card is reclaimed (force_add). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Problem
reconcilecrashed/alerted every cycle with:A physical NFC card that CiviCRM assigns to a member was still bound in UniFi Access to a different user, and door-sync binds with
force_add=false. The card got stuck because deactivation never removed it, so a recycled card could never be reassigned. CiviCRM being clean didn't help — the only duplicate-card safety guard checks CiviCRM members, not UniFi state.Key UniFi constraint (found in production): UniFi rejects NFC-card mutations on a deactivated user (HTTP 404 "no-man zone"). A card cannot be unbound from a disabled account — this shapes both fixes below.
Changes
So a recycled card never strands a member:
contact=NNNN (STATUS)for a sync-managed user, otherwise the UniFi user id + status. The member's name is never logged (PII stays out of logs/alerts, matching the rest of door-sync); the contact id resolves to the member in CiviCRM. Card number stays redacted (last-4, architecture §11).statusis set toDEACTIVATED(while the account is still active and the delete is permitted), best-effort so a cleanup failure never blocks cutting access.force_add: true). The card can't be unbound from a deactivated user first, so forcing the new bind is the only way to move it. An active holder (or one not in the fetch snapshot) is left untouched and identified in the error. After a reclaim, the moved token is dropped from the holder's cached card list so a later same-cycle op on that contact doesn't DELETE a token it no longer owns.force_addisfalsefor every normal bind — door-sync never displaces an active user.force_add: trueis used only to reclaim from a confirmed-disabled holder. An unknown/nullholder status is treated as not disabled, so a card is never force-reclaimed from a holder whose state can't be confirmed.Internals: a
token → holdermap (id + status only) captured for every user duringfetch_users(); the three card-delete loops and both bind sites unified into_delete_cards_for_contact()/_bind_nfc_card()/_put_bind_card().Docs (
docs/architecture.md§7, design spec) updated to match.Testing
uv run pytest→ 353 passed (added: deactivate-frees-card-before-status, identify-holder-by-id, reclaim-via-force_add, null-status-not-reclaimed, plus the updated ordering test)uv run pyrefly check→ 0 errors ·ruff check/ruff format --check→ cleanReview
Addressed CodeQL (
py/clear-text-logging-sensitive-data— dropped the holder name) and Copilot (null-safe holder fields; cache coherence after reclaim; description/behavior alignment).🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests