fix(sdk)!: name the TDF manifest entry manifest.json per spec - #405
Open
pflynn-virtru wants to merge 1 commit into
Open
pflynn-virtru wants to merge 1 commit into
pflynn-virtru wants to merge 1 commit into
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (6)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe SDK now writes ChangesManifest compatibility
Priority: ➖ Normal Estimated code review effort: 2 (Simple) | ~10 minutes Change: Bug fix Suggested reviewers: Merge Risk: ⚪ Minimal · up to The manifest-name compatibility change preserves legacy reads and accepts spec-compliant archives without an identified actionable regression. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
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. A rabbit packs the manifest bright Comment |
pflynn-virtru
marked this pull request as ready for review
September 15, 2026 19:04
The OpenTDF spec puts the manifest at the archive root under `manifest.json`. This SDK wrote and read `0.manifest.json`, an exact lookup with no fallback, so a TDF produced by any implementation written against the published spec was rejected outright. Write: `TDFWriter` emits `manifest.json`. Read: `TDFReader` accepts either name, preferring `manifest.json` when an archive carries both, so archives from earlier releases keep working. `SDK.isTDF` accepts either name too, and no longer requires the archive to hold exactly two entries -- the spec fixes where the manifest lives, not what else the archive may hold, and the count check made isTDF stricter than the reader it screens for. `0.payload` is unchanged: it is recorded in the manifest's payload URL field, so renaming it would alter manifest contents rather than just archive layout. Refs opentdf/platform#3513 Follows opentdf/platform#4049 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Paul Flynn <pflynn-virtru@users.noreply.github.com>
pflynn-virtru
force-pushed
the
fix/manifest-json-entry-name
branch
from
September 15, 2026 19:06
446d8ec to
9503fa7
Compare
pflynn-virtru
changed the title
Contributor
X-Test Failure Report |
|
Contributor
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.
Refs opentdf/platform#3513. Follows opentdf/platform#4049 (go) and unblocks the java cells of opentdf/tests#600.
Warning
Breaking file-format change. Archives written by this SDK now name the manifest entry
manifest.json. Readers that look it up by exact name cannot read them until they accept the spec name — including this SDK's own earlier releases.Problem
The OpenTDF spec names the manifest entry in a
.tdfarchivemanifest.json:This SDK writes and reads
0.manifest.json. On the read sideTDFReaderdoes an exactcontainsKeywith no fallback, so a TDF produced by any implementation written against the published spec is rejected withtdf doesn't contain a manifestbefore any schema check runs.SDK.isTDFcarries its own copy of the literal.Every SDK agreed on the same wrong name, so nothing caught it — which is exactly the third-party incompatibility opentdf/platform#3513 reports.
The
0.prefix is a holdover from an early design that anticipated several payload/manifest pairs per archive. That design never shipped.Change
Write —
TDFWriter.TDF_MANIFEST_FILE_NAMEis nowmanifest.json; the off-spec name moves to a newTDF_MANIFEST_FILE_NAME_OFFSPECconstant that only readers consult.Read —
TDFReaderprefersmanifest.jsonand falls back to0.manifest.json, so archives written by earlier releases keep working. Preference matters rather than first-match: an archive carrying both must not have its conformant entry passed over for the superseded one.Sniffing —
SDK.isTDFaccepts either name. It also no longer requires the archive to hold exactly two entries. That check was flagged in opentdf/tests#600 and is worth dropping on its own terms: the spec fixes where the manifest lives, not what else the archive may hold, and the count madeisTDFstricter than theTDFReaderit screens for — a three-entry archive would be rejected by the sniffer and then read fine by the reader.0.payloadis untouched. It is recorded in the manifest'spayload.url, so renaming it would alter manifest contents rather than just archive layout, and neither the spec nor opentdf/platform#4049 fixes it.scripts/test-mlkem.shandscripts/test-hybrid-pqc.shalready match the entry by themanifest\.json$suffix and read back whatever name they find, so they need no change.Downstream impact
TDFReader.javaexactcontainsKey;SDK.isTDFgetManifest(cd, '0.manifest.json')intdf3/src/tdf.ts,tdf3/src/client/index.tssdk_constants.h: kTDFManifestFileNameunzip -p example.tdf 0.manifest.jsongo (opentdf/platform#4049) and xtest (opentdf/tests#600) are in flight.
Testing
Written test-first. Each behavioral test was watched failing against unmodified production code, and the failure was the expected one:
TDFReaderTest.readsManifestUnderTheSpecNameIllegalArgumentException: tdf doesn't contain a manifestTDFReaderTest.readsThePayloadAlongsideASpecNamedManifestTDFReaderTest.prefersTheSpecNameWhenAnArchiveCarriesBothTDFWriterTest.writesTheManifestUnderTheSpecEntryName0.manifest.jsonSDKTest.testExaminingTDFWithSpecManifestNamefalseSDKTest.testExaminingTDFWithAnExtraEntryfalseprefersTheSpecNameWhenAnArchiveCarriesBothfiles a different manifest under each name, so it cannot pass by reading whichever entry the reader happened to pick.Four tests guard behavior that must not change and pass both before and after:
TDFReaderTest.readsManifestUnderTheOffspecName,TDFReaderTest.rejectsAnArchiveWithNoManifestUnderEitherName,SDKTest.testExaminingZipWithNoManifest,SDKTest.testExaminingZipWithNoPayload. The pre-existingSDKTest.testExaminingValidZTDFandtestExaminingManifestrun against the checked-insample.txt.tdf, which is an off-spec-named archive from before this change, so backward compatibility stays under test with a real fixture rather than a synthetic one.CI
mavenverifyis green — the full suite, both the fips and non-fips runs, includingTDFRootSignatureTestand the zip/fuzzing suites.Platform IntegrationandPlatform Integration (FIPS)also pass.I could not run any of that locally: codegen needs
bufagainst the BSR, and this machine is unauthenticated and hard rate-limited (resource_exhausted: too many requests), somvn installnever got pastgenerate-sources. Only the three targeted test classes above ran locally.xtest is red, as expected
Both
java@pull-405cells fail: 93 failed, 5 passed — the same tally, from the same four causes, that opentdf/platform#4049 reported for its own xct cells.KeyError: There is no item named '0.manifest.json' in the archivemanifest(),validate_manifest_schema()FileNotFoundError: .../unzipped/0.manifest.jsonupdate_manifest()CalledProcessErroron decryptassert b'segment' in ...test_tdf_altered_payload_endgets a manifest-lookup failure instead of a segment-integrity message78 of 93 are xtest's own hardcoded
0.manifest.json, which opentdf/tests#600 fixes.Of the decrypt failures, 8 are
sdk/java/dist/v0.18.0/cli.shraisingtdf doesn't contain a manifestagainst an archive written by this branch. That is this PR's documented breaking change observed directly, not a defect: v0.18.0's reader predates the fallback added here. Every current reader in the matrix handles it.xct (main, go@main)passes, confirming the failures are confined to cells that build java from this branch.This PR should stay draft until opentdf/tests#600 lands; merging sooner leaves xtest red.
🤖 Generated with Claude Code