Skip to content

Tell the state detector which reader doctor is on - #16

Merged
mmlado merged 1 commit into
mainfrom
fix/doctor-reader-name
Sep 29, 2026
Merged

mmlado merged 1 commit into
mainfrom
fix/doctor-reader-name

Conversation

@mmlado

@mmlado mmlado commented Sep 25, 2026

Copy link
Copy Markdown
Collaborator

Tell the state detector which reader doctor is on

doctor built its card session without the resolved reader name, so the DESFire diagnosis
ran on the ATR alone and could recommend a contactless reader to someone already using one.

Changes

  • doctor passes reader_name= to make_session.
  • tests/unit/test_doctor_reader_name.py: three tests driving the real command.
  • CHANGELOG entry under Fixed.

Constraints

Both signals are needed. is_contactless_interface accepts either the reader name or a
PC/SC composed contactless ATR (3B 8x 80 01 ...). Readers synthesise that ATR for ISO 14443
cards, but a card answering with its own wired-style ATR leaves the reader name as the only
evidence. With the name dropped, such a session looks like contact and
DesfireState.NEEDS_CONTACTLESS_READER wins, which is the inverted advice. The comment at
state/detector.py:248-252 already describes this failure mode; the session simply was not
carrying what it needed.

The existing detector tests cannot catch this. test_state_detector.py constructs
CardSession directly and passes reader_name itself, so it never exercises how doctor
builds one. Its contactless case also uses the composed 3B 80 80 01 ... ATR, which
satisfies the ATR signal alone and would pass with or without the name. The new tests use a
wired-style ATR so the reader name is the only thing under test, and patch reader_states,
pick_reader and connect in the doctor module namespace rather than anything inside
doctor itself.

A control test guards the other direction. On a genuine contact interface the
contactless advice is correct, and a test asserts it still appears. It passes both before and
after this change, so the pair discriminates rather than merely failing.

Verification

ruff check, ruff format --check, mypy, sphinx-build -W --keep-going pass. The suite
goes 347 to 350 tests. Before the fix, two of the three new tests fail and the control passes.
Coverage of cli/commands/doctor.py rises from 11% to 55%.

Note for whoever merges second

Touches CHANGELOG.md under Unreleased, as do #13 and #14. The entries are independent and
any conflict is textual.

🤖 Generated with Claude Code

doctor built its card session without the resolved reader name, so
is_contactless_interface saw only the ATR. Readers synthesise the PC/SC
composed contactless ATR for ISO 14443 cards, but a card answering with its
own wired-style ATR leaves the reader name as the only evidence - and
without it the DESFire probe recommends a contactless reader to an operator
already on one, the exact misdirection state/detector.py:248-252 describes.

The detector tests could not catch this: they build CardSession themselves
and pass reader_name, and the contactless case uses the composed ATR, which
satisfies the ATR signal alone. The new tests drive the command, patch PC/SC
at the module boundary, and use a wired-style ATR so the name is the only
signal under test. A contact-reader control asserts the other direction is
unchanged.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@mmlado
mmlado requested a review from embarquech September 25, 2026 15:06
@mmlado
mmlado merged commit 4150daa into main Sep 29, 2026
12 checks passed
@mmlado
mmlado deleted the fix/doctor-reader-name branch September 29, 2026 12:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant