fix(otp): Handle case when last slot is deleted

[DennisDyallo]

This pull request modifies the error handling logic in the OtpErrorTransform pipeline to improve clarity and handle edge cases more effectively. The changes include introducing a helper method for failed APDU responses and refining sequence validation logic.

Fixes: #182

Error handling improvements:

• Yubico.YubiKey/src/Yubico/YubiKey/Pipelines/OtpErrorTransform.cs: Added a FailedApdu helper method to streamline the creation of failed APDU responses. This simplifies the code and avoids redundancy.
• Yubico.YubiKey/src/Yubico/YubiKey/Pipelines/OtpErrorTransform.cs: Updated the sequence validation logic to account for cases where the programming sequence is set to 0 when deleting the last slot, ensuring proper handling of this edge case.

[github-actions]

Test Results: Windows

    2 files      2 suites   10s ⏱️
3 918 tests 3 918 ✅ 0 💤 0 ❌
3 920 runs  3 920 ✅ 0 💤 0 ❌

Results for commit f077510.

♻️ This comment has been updated with latest results.

[github-actions]

Test Results: Ubuntu

    2 files      2 suites   15s ⏱️
3 910 tests 3 910 ✅ 0 💤 0 ❌
3 912 runs  3 912 ✅ 0 💤 0 ❌

Results for commit f077510.

♻️ This comment has been updated with latest results.

[github-actions]

Test Results: MacOS

    2 files      2 suites   12s ⏱️
3 910 tests 3 910 ✅ 0 💤 0 ❌
3 912 runs  3 912 ✅ 0 💤 0 ❌

Results for commit f077510.

♻️ This comment has been updated with latest results.

[Copilot]

Pull Request Overview

This PR enhances the OTP error transform pipeline by centralizing failed-APDU creation and updating sequence validation to correctly handle the “last slot deleted” edge case.

• Added a FailedApdu helper to simplify constructing error responses.
• Introduced IsValidSequenceProgression to encapsulate sequence-number checks, including wrap-around when deleting the final slot.
• Updated the main Invoke method and exception catch block to use the new helper and validation logic.

Comments suppressed due to low confidence (3)

Yubico.YubiKey/src/Yubico/YubiKey/Pipelines/OtpErrorTransform.cs:85

• Add unit tests for IsValidSequenceProgression to cover normal increments, wrap-around cases, and the 'last slot deletion' scenario.

        private static bool IsValidSequenceProgression(int beforeSequence, int nextSequence, OtpStatus afterStatus)

Yubico.YubiKey/src/Yubico/YubiKey/Pipelines/OtpErrorTransform.cs:75

• C# does not support array literal []; use Array.Empty<byte>() or new byte[0] to create an empty byte array.

                return FailedApdu([]);

Yubico.YubiKey/src/Yubico/YubiKey/Pipelines/OtpErrorTransform.cs:89

• This comparison doesn’t handle wrap-around properly. Consider using (beforeSequence + 1) % 0x100 to mirror the original sequence logic.

            return nextSequence == beforeSequence + 1 ||  // Normal increment

[dainnilsson]

Should GetCurrentStatus reuse InvokeCommand and ReadStatus? The InvokeCommand with an out parameter also seems maybe a bit overly complicated? I would probably do something along the lines of:

private OtpStatus GetCurrentStatus() =>
	ReadStatus(InvokeCommand(new ReadStatusCommand().CreateCommandApdu(), typeof(ReadStatusCommand), typeof(ReadStatusResponse));

and

var beforeStatus = ReadStatus(InvokeCommand(new ReadStatusCommand().CreateCommandApdu(), typeof(ReadStatusCommand), typeof(ReadStatusResponse)));

var responseApdu = InvokeCommand(command, commandType, responseType);
var afterStatus = ReadStatus(responseApdu);

[DennisDyallo]

@dainnilsson Thanks, I've simplified the code in the latest commit. I reused the ReadStatus, but I figured there's no need to reuse invokecommand as it doesn't really do much but add another layer in the stack trace.

[github-actions]

Package	Line Rate	Branch Rate	Complexity	Health
Yubico.Core	40%	31%	4371	➖
Yubico.YubiKey	51%	46%	20748	➖
Summary	49% (35571 / 72606)	44% (8727 / 19881)	25119	➖

Minimum allowed line rate is 40%