Skip to content

feat(cli): add supports subcommand and xtest env auth fallbacks#167

Merged
b-long merged 3 commits into
b-long:mainfrom
arkavo-org:feat/xtest-supports-env
Jul 14, 2026
Merged

feat(cli): add supports subcommand and xtest env auth fallbacks#167
b-long merged 3 commits into
b-long:mainfrom
arkavo-org:feat/xtest-supports-env

Conversation

@arkavo-com

Copy link
Copy Markdown
Collaborator

Summary

Adds community xtest integration surface for OpenTDF cross-SDK conformance:

  1. supports <feature> — exit codes 0 supported / 1 unsupported / 2 unknown (matches official go/java/js CLI contract)
  2. Env fallbacksCLIENTID/CLIENTSECRET/PLATFORMURL/KASURL/KCFULLURL when CLI flags are omitted (so shims need not put secrets on argv)
  3. Conservative advertised features: autoconfigure, connectrpc, hexless, kasallowlist

Context

Used by arkavo-org/opentdf-tests community Stage-1 CI (pythongo@main ztdf roundtrips). See community-conformance.md.

Test plan

  • Unit tests: tests/test_cli_supports.py
  • Manual: python -m otdf_python supports autoconfigure → 0; ecwrap → 1; unknown → 2
  • CI green on this PR

Enable community xtest Stage-1 integration:

- supports <feature> exits 0/1/2 for supported/unsupported/unknown
- Read CLIENTID/CLIENTSECRET/PLATFORMURL/KASURL/KCFULLURL when flags omit
- Keep advertised features conservative (autoconfigure, connectrpc, hexless, kasallowlist)

@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 introduces environment variable fallbacks for SDK configuration (such as CLIENTID, CLIENTSECRET, PLATFORMURL, KCFULLURL, and KASURL) and adds a new 'supports' CLI subcommand to query supported features. The review feedback suggests improving robustness by using getattr and setattr on the arguments object to prevent potential AttributeErrors, and refactoring cmd_supports to return exit codes instead of calling sys.exit() directly to improve testability.

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 packages/otdf-python/src/otdf_python/cli.py Outdated
Comment thread packages/otdf-python/src/otdf_python/cli.py Outdated
Comment thread packages/otdf-python/src/otdf_python/cli.py Outdated
- Use getattr/setattr in build_sdk for robust args handling (mocks/tests)
- cmd_supports returns int (0/1/2); main() calls sys.exit with the result
- Add in-process unit test for cmd_supports return codes
@arkavo-com

Copy link
Copy Markdown
Collaborator Author

Addressed Gemini Code Assist feedback in b93ea22:

  1. build_sdk: use getattr / setattr for platform URL, OIDC, KAS endpoint, plaintext, and insecure so mocks/custom args do not raise AttributeError.
  2. cmd_supports: returns 0 / 1 / 2 instead of calling sys.exit internally.
  3. main: sys.exit(cmd_supports(args)).
  4. Added in-process unit test for return codes in tests/test_cli_supports.py.

…ail-fast auth

Fixes the three failures observed in community conformance run
arkavo-org/opentdf-tests 29210974140:

- encrypt: an explicit --kas-endpoint now replaces the platform-derived
  default KAS instead of extending it, and _validate_kas_infos dedupes
  same-URL entries — the manifest carried two key access objects for the
  same KAS (expected one).
- OIDC discovery: an --oidc-endpoint that already names a realm is used
  verbatim; the realm segment was being appended twice
  (…/auth/realms/opentdf/realms/opentdf/…) and 404'd.
- rewrap: token-acquisition failure now raises instead of sending an
  unauthenticated request that KAS rejects with a misleading
  "missing authorization header".

Also clears ruff findings (B010 setattr, D413, C901 main complexity)
introduced by the earlier review-feedback commit so CI lint passes.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Dvm7uKaNssz7ztu3nAmf7u
@arkavo-com

Copy link
Copy Markdown
Collaborator Author

Pushed 825b041 extending this PR with fixes for the three Stage-1 conformance failures observed in arkavo-org/opentdf-tests run 29210974140:

  1. Duplicate key access object on encryptcreate_tdf_config extended the platform-derived default KAS with the explicit --kas-endpoint, so the manifest carried two KAOs for the same KAS (assert 2 == 1 in xtest). Explicit endpoints now replace the default, and _validate_kas_infos dedupes same-URL entries as a backstop.
  2. OIDC realm path doubled — an --oidc-endpoint already naming a realm got /realms/opentdf appended again (…/auth/realms/opentdf/realms/opentdf/… → 404). Issuer URLs containing /realms/ are now used verbatim; bare Keycloak base URLs still get the default realm appended.
  3. Rewrap sent without auth — after token acquisition failed, unwrap proceeded tokenless and KAS rejected it with a misleading missing authorization header. It now fails fast with the real cause.

Also cleared the ruff findings (B010/D413/C901) introduced by the previous review-feedback commit so uv run ruff check passes again.

Regression tests: tests/test_xtest_roundtrip_fixes.py (7 tests). Full unit suite: 236 passed (the 3 test_validate_otdf_python.py failures are pre-existing on this branch — pyqwest can't disable TLS verification for the local-HTTPS validate tests).

@arkavo-com

Copy link
Copy Markdown
Collaborator Author

Local end-to-end verification of 825b041 against a live platform (opentdf/platform tip + go v0.35.0 peer, full Keycloak/KAS stack):

uv run pytest test_tdfs.py -m stage1 \
  --sdks-encrypt "python@main go@v0.35.0" --sdks-decrypt "python@main go@v0.35.0" \
  --containers tdf --focus python -v
Roundtrip Result
python → python ✅ passed
python → go@v0.35.0 ✅ passed
go@v0.35.0 → python ✅ passed

3 passed, 1 skipped (out-of-focus go→go). Inspected the python-encrypted manifest directly: keyAccess count is 1 (was 2). The go→python decrypt no longer emits missing authorization header, and KAS audit logs show result: success for the rewraps.

(One local-env note, unrelated to this SDK: the EC-rewrap re-decrypt step needs services.kas.preview.ec_tdf_enabled: true on the platform, as the xtest docs describe — with that set, everything above is green.)

These are the same three cases that failed in community conformance run 29210974140. The Test Suite / Build Wheel workflows on this PR are waiting on maintainer approval to run.

@b-long

b-long commented Jul 14, 2026

Copy link
Copy Markdown
Owner

/gemini review

@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 introduces several fixes and enhancements to align with the community xtest contract. Key changes include adding environment variable fallbacks for authentication and endpoint configuration, implementing a supports CLI subcommand to probe features, and improving OIDC discovery to handle realm paths correctly without doubling them. Additionally, it ensures KAS endpoints are deduplicated and that KAS rewrap fails fast if token acquisition fails. Unit and regression tests are added to cover these scenarios. The reviewer suggested normalizing KAS URLs to lowercase during deduplication to prevent duplicates caused by casing differences.

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.

seen_urls = set()
unique_kas_infos = []
for kas in kas_infos:
url_key = (getattr(kas, "url", "") or "").rstrip("/")

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.

medium

When deduplicating KAS URLs, comparing them with case sensitivity can lead to duplicate entries if there are minor casing differences in the scheme or hostname (e.g., http:// vs HTTP:// or localhost vs Localhost). Normalizing the URL key to lowercase ensures robust deduplication.

Suggested change
url_key = (getattr(kas, "url", "") or "").rstrip("/")
url_key = (getattr(kas, "url", "") or "").rstrip("/").lower()

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Deferred to separate issue, see: #168

@b-long

b-long commented Jul 14, 2026

Copy link
Copy Markdown
Owner

This looks great, thank you for the contribution @arkavo-com ! 🙇 🎉

@b-long b-long merged commit 39a5c8f into b-long:main Jul 14, 2026
12 checks passed
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