feat(xtest): typed SDK adapter protocol and a shim-backed implementation - #599
Draft
dmihalcik-virtru wants to merge 5 commits into
Draft
dmihalcik-virtru wants to merge 5 commits into
dmihalcik-virtru wants to merge 5 commits into
Conversation
otdf-adapter protocol
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: trueThanks 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 |
dmihalcik-virtru
force-pushed
the
DSPX-4791-otdf-adapter-protocol
branch
from
September 15, 2026 15:18
46fc125 to
eb6de95
Compare
X-Test Failure Report |
dmihalcik-virtru
force-pushed
the
DSPX-4791-otdf-adapter-protocol
branch
from
September 15, 2026 15:20
eb6de95 to
fcb332d
Compare
dmihalcik-virtru
changed the title
X-Test Failure Report |
dmihalcik-virtru
force-pushed
the
DSPX-4791-otdf-adapter-protocol
branch
from
September 15, 2026 15:23
fcb332d to
617dbd8
Compare
X-Test Failure Report |
dmihalcik-virtru
force-pushed
the
DSPX-4791-otdf-adapter-protocol
branch
from
September 15, 2026 15:35
617dbd8 to
da6d654
Compare
dmihalcik-virtru
force-pushed
the
DSPX-4791-otdf-adapter-protocol
branch
from
September 15, 2026 15:42
da6d654 to
bb542cf
Compare
X-Test Failure Report |
The scaffold was a one-paragraph dump of the ticket description under seven empty template headings. Restructured into the standard shape and every file:line citation checked against the file it names. Corrections to the counts the scaffold carried over: - the supports case statements are 388 lines, not ~360, and they are 46 percent of the 846 lines of shell rather than a minority of it; - there are 46 named feature arms across the three shims answering 20 distinct features, plus three catch-alls; - the Python match in _uncached_supports has nine named arms and a case _, not ten hardcoded answers; - test_sdk_commands.py had 17 tests and 23 assertions, not 18 cases. Also records a latent bug found while reading: tdfs.py:834 writes XT_WITH_KAS_ALLOWLIST and all three shims read XT_WITH_KAS_ALLOW_LIST, so the kasallowlist= option has never reached an SDK.
New package, sibling of otdf-sdk-mgr and otdf-local and built the same way
(uv_build, src/ layout, its own uv.lock and AGENTS.md).
protocol.py SdkAdapter Protocol, frozen EncryptRequest/DecryptRequest,
SdkVersion. Imports nothing else in the package, so a
consumer's adapter can depend on the protocol without
depending on the subprocess machinery.
subprocess_cli.py SubprocessCliAdapter, the only implementation. Builds the
same argv and the same XT_WITH_* env as xtest does today
and execs the existing cli.sh.
gates.py MinVersion, HelpContains, Always, Delegate, Unprobeable,
and one evaluator. The tables are empty on purpose.
registry.py otdf.adapters entry-point discovery.
descriptor.py optional adapter.json, falling back to the cli.sh layout.
Three things worth knowing:
supports() takes a container. cli.sh supports <feature> cannot express a
per-container capability, so today the suite answers per-SDK and re-derives
the rest at the call site. The argument cannot change the answer yet; it is
in the signature so that adding a per-container gate later is a change to
gates.py rather than to every caller a second time.
GATES is {} and supports() falls back to the shim when a feature has no
entry, so an empty table is exactly today's behaviour. That is what lets the
46 case arms be transcribed one row at a time instead of in a flag day.
Delegate and Unprobeable exist because the shims had no way to say either.
A capability with no CLI surface is written as a bare exit 1, which the
suite reports with the same message as a build that is simply too old.
60 tests, none of which need an SDK or a platform: they write a stub cli.sh
into tmp_path. Gates being inert data is what makes that possible, and it is
the only way a transcription of fifty capability rules gets reviewed.
Behaviour-identical by construction. tdfs.SDK keeps its entire public surface and delegates the argv and env construction it used to do inline. What is preserved deliberately, because callers depend on it: - encrypt_command/decrypt_command keep their keyword signatures and their (argv, env) two-tuple return -- fixtures/bench.py:363,374 unpacks them; - the constructor still raises FileNotFoundError with the same message -- conftest.py:362,373 catches it to report an SDK that was named but never installed; - the nine-arm match in _uncached_supports is untouched and still runs before the adapter is consulted. Folding it into the gate tables is the last step of the migration, not this one; - XT_WITH_KAS_ALLOWLIST keeps its misspelling. All three shims read XT_WITH_KAS_ALLOW_LIST, so that option has never reached an SDK; fixing it here would give the first live run two reasons to differ from the last one. Recorded in KNOWN_SHIM_MISMATCH and left for its own change. ztdf-ecwrap is split at the adapter boundary into container="ztdf" plus ecwrap=True, so nothing downstream has to know that one of the container names is not a container. test_sdk_commands.py is rewritten against the new API with every existing assertion reproduced and only the call shape changed, 17 tests to 25. The added cases cover the ecwrap split, target mode surviving it, and tdfs.SDK delegating byte-identically to the adapter. It still asserts literal argv and literal env against a stub cli.sh in tmp_path, and TestDeterminism still pins the builders as pure. The evidence that this is behaviour-identical is not that suite, which was written after the change. It is that the unmodified pre-change test_sdk_commands.py -- 17 tests, 23 assertions -- passes against the refactored tdfs.py unaltered. No shell is deleted.
New otdf-adapter lane in check.yml, mirroring the two existing sibling lanes. It has no path dependencies, so it holds the strong form throughout: uv sync --frozen --no-build and uv run --frozen --no-build. The xtest lanes cannot, in exactly one step each. xtest depends on ../otdf-adapter as an editable path dependency, and --no-build refuses any distribution without a wheel, which an editable install has none of by design. So the three `uv sync` steps that install xtest drop it: check.yml Install dependencies --frozen --extra dev xtest.yml MEASURE / bench --locked xtest.yml RUN / zip64 --locked Every step downstream of those keeps it. They gain --no-sync, which pins them to exactly the environment the install produced, and --no-build then costs nothing because there is nothing left to install. The resolution guarantee the old --no-build carried moves up to the sync, where --frozen and --locked also catch a pyproject.toml edited without a re-lock. The two xtest.yml sites matter more than they look: bench and zip64 are nightly and workflow_dispatch only, so they are skipped on every PR. Left alone they would have failed on the next nightly, not here.
dmihalcik-virtru
force-pushed
the
DSPX-4791-otdf-adapter-protocol
branch
from
September 15, 2026 16:10
bb542cf to
411d494
Compare
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Step A1 of replacing the three
cli.shshims with a typed Python adapterlayer. Behaviour-identical by construction — this lands the abstraction
with a single implementation that still shells out to
cli.sh. No shell isdeleted.
Design:
spec/DSPX-4791.md.Why
xtestreaches every SDK through a hand-written bash shim. Three of them, 846lines, of which 388 — 46% — are a
supportscase statement written out threetimes: 46 named feature arms answering 20 distinct features, each one a
hand-rolled
awksemver comparison, ahelp | grepprobe, or a constant.Rewriting the same version comparison 46 times is where the mistakes are.
sdk/go/cli.sh:83compares$2 >= 2under a comment on the line above thatsays "Schema version 4.3.0 introduced hexless". The gate admits 4.2.
And the migration is already half-done:
tdfs._uncached_supports(
tdfs.py:894) opens with a Pythonmatchcarrying nine hardcoded answersthat override the shell before falling through to it. One concept, two
languages.
xtestis consumed by four repos that pin it atmain—opentdf/platform,java-sdk,web-sdk,otdfctl— each driving its own client, and none ofthem can add a cell to the matrix without a PR here, because
sdk_typeis aclosed
Literalin the suite's own source.What landed
New
otdf-adapter/package, sibling ofotdf-sdk-mgrandotdf-localandbuilt the same way (
uv_build,src/layout, ownuv.lockandAGENTS.md).protocol.pySdkAdapterProtocol, frozenEncryptRequest/DecryptRequest,SdkVersion. Imports nothing else in the package.subprocess_cli.pySubprocessCliAdapter— the only implementation. Same argv, sameXT_WITH_*env, samecli.sh.gates.pyMinVersion,HelpContains,Always,Delegate,Unprobeable, one evaluator. Tables empty.registry.pyotdf.adaptersentry-point discovery.descriptor.pyadapter.json, falling back to thecli.shlayout.tdfs.SDKdelegates to it while keeping its whole public surface:encrypt_command/decrypt_commandkeep their keyword signatures and(argv, env)return (fixtures/bench.py:363,374unpacks them), theconstructor keeps its exact
FileNotFoundErrormessage(
conftest.py:362,373catches it), and the nine-armmatchis untouched.Three decisions worth a second look:
supports()takes a container. It cannot change the answer yet — theshim has nowhere to put it. It is in the signature so that adding a
per-container gate later is a change to
gates.py, not to every caller asecond time.
GATESis{}, deliberately.supports()consults the table and fallsback to the shim when a feature has no entry, so an empty table is exactly
today's behaviour. That is what lets the 46 arms be transcribed one row at a
time, each reviewable against the bash it replaces, rather than in a flag
day.
ztdf-ecwrapis split at the boundary intocontainer="ztdf"plusecwrap=True, so nothing downstream has to know that one of the containernames is not a container.
Evidence that it is behaviour-identical
The rewritten
test_sdk_commands.pyis corroboration, not proof — it waswritten after the change. The actual evidence is that the unmodified
pre-change
test_sdk_commands.py(17 tests, 23 assertions) passes against therefactored
tdfs.py. That file cannot have been written to match the newimplementation.
The rewritten suite reproduces every one of those assertions with only the
call shape changed — including
TestDeterminism::test_builders_are_pure— andgrows to 25. It still asserts literal argv and literal env against a stub
cli.shintmp_path, not against a mocked adapter; asserting thattdfs.SDKreturns whatSubprocessCliAdapterreturns would be a tautology.Ran locally:
test_sdk_commands.py— 25 passedtest_sdk_commands.pyagainst the newtdfs.py— 17 passedotdf-adapterown suite — 60 passedruff check,ruff format --check,pyright— clean in both packagesAnd on CI, the part that needs a platform: all 12
xctcells green — twoplatform versions (
main,v0.26.0) x three SDKs x two versions each.The criterion was never "green", it was no change in pass/skip counts: a
skip that silently became a different skip would still be a behaviour change.
All 36 pytest invocations (12 cells x legacy / standard / attribute-based)
report counts identical to the last
main-based run of this workflow(34851123997):
main,go@mainmain,go@v0.37.0main,java@mainmain,java@v0.18.0main,js@mainmain,js@v0.4.0v0.26.0,go@mainv0.26.0,go@v0.37.0v0.26.0,java@mainv0.26.0,java@v0.18.0v0.26.0,js@mainv0.26.0,js@v0.4.0Not verified: that none of the four repos pinning
opentdf/tests@mainneeds achange. Nothing here removes or renames anything they call, but that is an
argument rather than a run.
Deliberately deferred
reviewable change; this PR lands the vocabulary so it has something to be
reviewed against.
go.py/java.py/js.pydrivingotdfctl,java -jarornpxdirectly.cli.sh. Not one line of shell is removed.Literaltypes or touching thesrc/xtest/layout.One bug found and not fixed here
tdfs.py:834writesXT_WITH_KAS_ALLOWLIST. All three shims readXT_WITH_KAS_ALLOW_LIST(go/cli.sh:247-248,java/cli.sh:237-238,js/cli.sh:294-295). One underscore, and thekasallowlist=option hastherefore never reached any SDK; its only caller is the unit test, which
asserts the Python-side spelling and passes vacuously.
It is preserved verbatim and recorded in
KNOWN_SHIM_MISMATCH. Fixing a realdefect under cover of a refactor would give the first live run two independent
reasons to differ from the last one. It wants its own PR, where the matrix
result is the evidence.
CI
Adds an
otdf-adapterlane tocheck.yml, mirroring the two sibling lanes.It has no path dependencies, so it holds the strong form throughout:
uv sync --frozen --no-buildanduv run --frozen --no-build.The
xtestlanes cannot, in exactly one step each.xtestdepends on../otdf-adapteras an editable path dependency, and--no-buildrefuses anydistribution without a wheel, which an editable install has none of by design.
So the three
uv syncsteps that installxtestdrop it:check.ymluv sync --frozen --extra devxtest.ymlbenchuv sync --lockedxtest.ymlzip64uv sync --lockedEvery step downstream of those keeps
--no-build. They gain--no-sync,which pins them to exactly the environment the install produced, and
--no-buildthen costs nothing because there is nothing left to install. Theresolution guarantee the old
--no-buildcarried moves up to the sync, where--frozen/--lockedalso catch apyproject.tomledited without a re-lock.The two
xtest.ymlsites matter more than they look:benchandzip64arenightly and
workflow_dispatchonly, so they are skipped on every PR. Leftalone they would have failed on the next nightly rather than here.
Known red checks
githubactions:S8541on the threeuv synclines above,"omitting
--no-buildcan lead to the execution of setup scripts". That isthe trade described above and it has no fix short of dropping the editable
path dependency:
uv sync --frozen --extra dev --no-buildfails withDistribution otdf-adapter==0.1.0 @ editable+../otdf-adapter can't be installed because it is marked as --no-build but has no binary distribution. What the rule guards against is a setup script in a packagefrom this repo, built from this repo's lockfile. Each site is annotated in
the workflow. Everything else in the diff, including the whole
otdf-adapterlane, does use--no-build.xctcells failing at "Prepare java cli" — if you see these on are-run, they are not from this PR. The java SDK's maven build shells out to
buf, which intermittently returnsresource_exhausted: too many requestswhen several branches build concurrently. It happened on an earlier push
here and cleared on the next one; the current run is 12/12 green.