Skip to content

feat(xtest): Enables mlkem tests on java-sdk; skips hpqt skew failures#538

Merged
dmihalcik-virtru merged 5 commits into
mainfrom
fix-mlkem-check-java
Jun 26, 2026
Merged

feat(xtest): Enables mlkem tests on java-sdk; skips hpqt skew failures#538
dmihalcik-virtru merged 5 commits into
mainfrom
fix-mlkem-check-java

Conversation

@dmihalcik-virtru

@dmihalcik-virtru dmihalcik-virtru commented Jun 25, 2026

Copy link
Copy Markdown
Member
  • New Features
    • Added CLI capability detection for mechanism-mlkem, verifying support via the encryption help output.
  • Tests
    • Improved post-quantum hybrid compatibility handling by adding an additional conditional skip gate to hybrid KEM roundtrip tests, preventing runs when version/format skew is detected.

@dmihalcik-virtru dmihalcik-virtru requested review from a team as code owners June 25, 2026 15:30
@coderabbitai

coderabbitai Bot commented Jun 25, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

The Java CLI wrapper adds a mechanism-mlkem support check. Python test helpers add semver parsing and a hybrid-format cutoff, then use a new skip gate in four PQC roundtrip tests.

Changes

Java CLI support check

Layer / File(s) Summary
mechanism-mlkem support case
xtest/sdk/java/cli.sh
Adds a mechanism-mlkem branch that runs cmdline.jar help encrypt under set -o pipefail and exits with the pipeline status.

PQC hybrid format skip gating

Layer / File(s) Summary
Semver parsing and cutoff
xtest/tdfs.py
Adds semver parsing helpers, reads OTDFCTL_HEADS for an otdfctl version, and defines the hybrid-format cutoff version.
Hybrid format skip gate and test wiring
xtest/tdfs.py, xtest/test_pqc.py
Adds skip_pqc_hybrid_format_skew(...) for hybrid PQ mechanisms and calls it from the four hybrid roundtrip tests before the encrypt/decrypt flow.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

  • opentdf/tests#531: Also updates xtest/sdk/java/cli.sh supports logic for ML-KEM-related mechanism detection using cmdline.jar help encrypt output.
  • opentdf/tests#534: Also involves xtest/tdfs.py and OTDFCTL_HEADS-driven PQ/T skip behavior for hybrid test gating.
  • opentdf/tests#446: Related to the semver/release detection used by the new skip gate for released Go SDK versions.

Suggested reviewers

  • sujankota
  • jakedoublev

Poem

🐰 I nibbled a semver, then hopped through the gate,
ML-KEM and hybrid checks now know their fate.
A skip when the versions are old or too near,
And tests keep on hopping with a cleaner path here.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 77.78% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Title check ✅ Passed The title matches the main change: enabling mlkem tests for the Java SDK and adding skew-failure skips for hybrid PQ tests.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix-mlkem-check-java

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request adds support for checking the mechanism-mlkem capability in the Java CLI test script. However, a copy-paste error was identified where the script greps for xwing instead of ml-kem or mlkem, which would cause incorrect capability detection.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment thread xtest/sdk/java/cli.sh Outdated
…rm skew

otdfctl v0.33.0 and older were built against a pre-ocrypto-0.13.0 that
produces non-conformant X-Wing and secp+ML-KEM key material.  When
paired with a platform that has hybrid PQC support enabled (new-format
KAS), both key creation and decapsulation fail with opaque errors.

Add skip_pqc_hybrid_format_skew() + _otdfctl_semver() to tdfs.py and
call the guard in the four hybrid test functions (xwing roundtrip,
xwing+ec roundtrip, secpmlkem-3 roundtrip, secpmlkem-5 roundtrip).
Pure ML-KEM tests are unaffected.
sujankota
sujankota previously approved these changes Jun 26, 2026
Two independent sources of format incompatibility exist for hybrid PQC tests
(X-Wing and secp+ML-KEM):
- otdfctl ≤ 0.33.0: registers hybrid KAS keys in non-conformant format
- go encrypt SDK ≤ 0.33.0: produces hybrid KEM ciphertexts in old format

Both are built against lib/ocrypto < v0.13.0 which changed the key-material
format. Either causes opaque crypto failures when paired with a platform that
has hybrid PQC support enabled.

Extend skip_pqc_hybrid_format_skew() to accept an encrypt_sdk parameter and
check its version (when it is a released go SDK ≤ v0.33.0) in addition to the
existing otdfctl version check. Pass encrypt_sdk at all four hybrid test call
sites.
The grep command was missing, leaving a dangling pipe into exit.
Add the missing grep for 'mlkem:768' to match the help output.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 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 `@xtest/tdfs.py`:
- Around line 748-754: Update the version parsing used by the Go SDK safety gate
so _parse_semver handles released version strings with an sdk/ prefix as well as
a leading v. The current check in the encrypt_sdk.version flow can return None
for values like sdk/v0.12.0, which bypasses the cutoff comparison in the gate.
Adjust _parse_semver to normalize the prefix before matching, and keep the
existing call site in the encrypt_sdk / is_released path working without
changing the gate logic.
🪄 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: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 24c2d0f0-37be-418d-b5c0-f9f0cbb4c9e1

📥 Commits

Reviewing files that changed from the base of the PR and between a5198a5 and d937e09.

📒 Files selected for processing (3)
  • xtest/sdk/java/cli.sh
  • xtest/tdfs.py
  • xtest/test_pqc.py
🚧 Files skipped from review as they are similar to previous changes (1)
  • xtest/sdk/java/cli.sh

Comment thread xtest/tdfs.py
@dmihalcik-virtru dmihalcik-virtru changed the title feat(xtest): Enables mlkem tests on java-sdk feat(xtest): Enables mlkem tests on java-sdk; skips hpqt skew failures Jun 26, 2026
@dmihalcik-virtru dmihalcik-virtru merged commit 9ce9f4e into main Jun 26, 2026
22 checks passed
@dmihalcik-virtru dmihalcik-virtru deleted the fix-mlkem-check-java branch June 26, 2026 17:33
@sonarqubecloud

Copy link
Copy Markdown

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.

2 participants