fix(xtest)!: read the manifest entry as manifest.json, per spec by pflynn-virtru · Pull Request #600 · opentdf/tests

pflynn-virtru · 2026-09-15T17:04:06Z

Refs opentdf/platform#3513. Unblocks opentdf/platform#4049.

Warning

Breaking for every SDK shipping today. xtest now requires the spec's
manifest.json and rejects 0.manifest.json. go (before platform#4049),
java-sdk and web-sdk all still write the legacy name, so they fail. That is
the intended report — see Expected failures.

Problem

The spec puts the manifest at the archive root under manifest.json:

The manifest.json file MUST be in JSON format and reside within the root of
the OpenTDF Zip archive.

— spec/schema/OpenTDF/manifest.md

xtest hardcoded 0.manifest.json in its four manifest lookups, so the suite
asserted a container the spec does not describe. Every SDK agreed on the same
wrong name, so nothing caught it — test_manifest_validity checks the
manifest's contents against manifest.schema.json and never looks at what
the archive calls it.

platform#4049 aligns the go writer. Its xct cells came back 93 failed, 5
passed, identical across all three cells building go from that branch.
Classifying all 93:

Cause	Count	Whose bug
`KeyError: There is no item named '0.manifest.json' in the archive`	46	xtest — `manifest()`, `validate_manifest_schema()`
`FileNotFoundError: .../unzipped/0.manifest.json`	32	xtest — `update_manifest()`
`CalledProcessError` on decrypt	12	the reader: `zip: file not found` (go@main), `tdf doesn't contain a manifest` (java@main), `Unable to retrieve CD manifest` (js@main)
`assert b'segment' in ...`	3	same — `test_tdf_altered_payload_end` expects a segment-integrity message, gets a manifest-lookup failure

78 of 93 are this repo. The other 15 are 5 tests × 3 decrypting SDKs, and are
the downstream reader gaps platform#4049's own impact table predicts.

Change

Every manifest lookup goes through manifest_entry_name(), which returns
manifest.json or raises. No fallback to the legacy name — a tolerant
reader would report a conformant archive and a non-conformant one identically,
which is how the divergence went unnoticed. The unit tests pin the rejection,
not just the acceptance.

Adds one ungated conformance test asserting the writer's entry name.

0.payload is untouched: the spec does not fix it (it is whatever the
manifest's payload.url says), and platform#4049 does not change it.

Expected failures

This is the test half of platform#3513 and cannot be green until the writers
move:

SDK	Blocker
go	platform#4049, open
java-sdk	writes legacy; `TDFReader.java` exact `containsKey`, `SDK.isTDF` asserts an exactly 2-entry set. Not started
web-sdk	writes legacy; `getManifest(cd, '0.manifest.json')` in `tdf3/src/tdf.ts`. Not started

Draft until at least go lands — merging sooner would leave this repo's CI red
on main.

This PR's own xct run bears that out: all 12 cells report 116 failed, 30
passed, 1 skipped, and the failures are two kinds only — 115 KeyError from
the manifest lookup and 1 AssertionError from the conformance test. No
decrypt failures, because each SDK writes and reads the legacy name
consistently. The 30 passes are the five tests in test_tdfs.py that never
parse a manifest.

116 is a floor, not a ceiling: the failing step aborts the job, so the later
step covering test_abac.py, test_pqc.py and test_dpop.py never runs, and
those parse manifests too.

Testing

$ uv run pytest test_tdfs_units.py test_zip64_units.py test_sizes_units.py \
    test_encryption_units.py test_bench_stats.py -q --no-audit-logs
124 passed
$ uv run ruff check .    # All checks passed
$ uv run ruff format .   # 49 files unchanged
$ uv run pyright         # 0 errors (1 pre-existing warning: requests stubs, test_dpop.py)

The four rejection cases are mutation-tested: re-adding a legacy-name fallback
to manifest_entry_name() fails exactly those four and leaves the other 24
green, so they cannot pass vacuously.

Not run: the integration matrix, which needs a live platform and the three
SDK CLIs. The 78 cells this fixes are characterized from the platform#4049 CI
logs, not from a local run.

🤖 Generated with Claude Code

The OpenTDF spec puts `manifest.json` at the root of the archive ("The manifest.json file MUST be in JSON format and reside within the root of the OpenTDF Zip archive" -- spec/schema/OpenTDF/manifest.md). xtest hardcoded `0.manifest.json` in four places, so the suite was asserting a container the spec does not describe. Every SDK agreed on the same wrong name, so nothing ever caught it. platform#4049 aligns the go writer. Its xct cells came back 93 failed, 5 passed, and 78 of those failures were xtest's own Python rather than anything wrong with the SDK: 46 `KeyError: There is no item named '0.manifest.json'` from `manifest()`/`validate_manifest_schema()`, and 32 `FileNotFoundError` from `update_manifest()` reaching into the extracted tree by name. Reads the spec name and no other. No fallback to the legacy name: a tolerant reader is what let the divergence survive unreported, and accepting both here would hide exactly the defect the suite exists to find. `manifest_entry_name()` centralizes the lookup and lists the archive's actual members on failure -- zipfile's own KeyError names only the entry it wanted, which made a renamed manifest read as an empty archive. `0.payload` is unchanged. It is not fixed by the spec: it is whatever the manifest's `payload.url` says, so renaming it would change manifest contents rather than archive layout. Adds `test_manifest_entry_name_is_spec_compliant`, ungated. A writer still emitting `0.manifest.json` fails it, and that red cell is the finding. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

coderabbitai · 2026-09-15T17:04:14Z

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Advanced

Run ID: 3526e312-9204-4f91-9eb8-0233a6465f80

📥 Commits

Reviewing files that changed from the base of the PR and between 9cf66a9 and 337ca07.

📒 Files selected for processing (5)

xtest/sizes.py
xtest/tdfs.py
xtest/test_tdfs.py
xtest/test_tdfs_units.py
xtest/test_zip64_units.py

Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review.

📝 Walkthrough

Walkthrough

The change standardizes TDF manifest archives on manifest.json, adds lookup validation and error reporting, updates manifest-related tests, and replaces hard-coded manifest names in ZIP and ZIP64 fixtures.

Changes

Manifest entry compliance

Layer / File(s)	Summary
Manifest lookup and validation `xtest/tdfs.py`, `xtest/sizes.py`	Adds `MANIFEST_ENTRY` and `manifest_entry_name`. Manifest reading, updating, and schema validation now use the spec-required `manifest.json` entry.
Manifest compliance tests `xtest/test_tdfs.py`, `xtest/test_tdfs_units.py`	Adds archive compliance coverage and tests rejection of off-spec manifest names.
ZIP and ZIP64 fixture alignment `xtest/test_zip64_units.py`	Updates ZIP and ZIP64 fixtures to use `tdfs.MANIFEST_ENTRY`.

Priority: ⬇️ Low

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Bug fix

Suggested reviewers: dmihalcik-virtru

Merge Risk: ⚪ Minimal · up to 337ca

The manifest conformance test follows the repository’s normal focused-test behavior and covers all configured encryption SDKs by default. No actionable merge-blocking risk remains.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 52.78% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 36 functions across 5 files.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
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.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and concisely describes the main change: reading the manifest from the spec-compliant `manifest.json` entry instead of the legacy entry.

Fix all pre-merge checks with AI

✨ Finishing Touches 💡 1

📝 Generate docstrings 💡

Create stacked PR
Commit on current branch

🧪 Generate unit tests (beta)

Create PR with unit tests
Commit unit tests in branch fix/spec-manifest-entry-name

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

_{A rabbit checks the archive door
manifest.json is named once more
Old names hop out of sight
ZIP64 tests align just right
The signed TDF rests secure}

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

Keep the normative rule and the reason a fallback is refused; the platform#4049 timeline and failure counts belong in the PR description, not in comments that outlive it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

github-actions · 2026-09-15T17:10:00Z

X-Test Failure Report

Out of scope. The payload entry name is not what platform#4049 changes, so update_payload and the fixtures keep their 0.payload literals. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

github-actions · 2026-09-15T17:15:55Z

X-Test Failure Report

…4 fixtures These fixtures model TDF layout deliberately -- payload first, manifest after it at the ZIP64 boundary -- so the entry should come from the same constant the readers use rather than a repeated literal. Also drops a byte count from the overrun test's comment. It said the name was 15 bytes, which stopped being true when the entry was renamed, and zipinspect never reads the name anyway. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

sonarqubecloud · 2026-09-15T17:25:06Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

github-actions · 2026-09-15T17:26:11Z

X-Test Failure Report

server-logs-java@v0.18.0-v0.26.0
❌ java@v0.18.0-v0.26.0

github-actions · 2026-09-15T17:39:07Z

The OpenTDF spec puts the manifest at the archive root under `manifest.json`. This SDK writes and reads `0.manifest.json`. The `0.` prefix is a holdover from an early design that anticipated several payload/manifest pairs per archive; that design never shipped. Write: `writeStream` emits `manifest.json`. Read: `loadTDFStream` and `getPolicyId` resolve the entry through `manifestEntryName()`, which prefers the spec name and falls back to the legacy name, so archives written by earlier releases keep opening. The dual-name handling lives at the TDF layer rather than in `ZipReader`, whose signature is unchanged — its other lookup is a payload path that must stay exact. Resolution runs against the central directory, not against a failed read, so an oversized manifest under the spec name stays a size error instead of silently yielding the superseded legacy entry. `0.payload` is unchanged: it is recorded in the manifest's `payload.url`, so renaming it would alter manifest contents rather than just archive layout. BREAKING CHANGE: archives written by this SDK name the manifest entry `manifest.json`. Readers that look it up by exact name — web-sdk <= 0.20, java-sdk, client-cpp — cannot read them until they accept the spec name. Refs opentdf/platform#3513, opentdf/platform#4049, opentdf/tests#600. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Paul Flynn <pflynn-virtru@users.noreply.github.com>

The OpenTDF spec puts the manifest at the archive root under `manifest.json`. This SDK writes and reads `0.manifest.json`. The `0.` prefix is a holdover from an early design that anticipated several payload/manifest pairs per archive; that design never shipped. Write: `writeStream` emits `manifest.json`. Read: `loadTDFStream` and `getPolicyId` resolve the entry through `manifestEntryName()`, which prefers the spec name and falls back to the off-spec name, so archives written by earlier releases keep opening. The dual-name handling lives at the TDF layer rather than in `ZipReader`, whose signature is unchanged — its other lookup is a payload path that must stay exact. Resolution runs against the central directory, not against a failed read, so an oversized manifest under the spec name stays a size error instead of silently yielding the superseded off-spec entry. `0.payload` is unchanged: it is recorded in the manifest's `payload.url`, so renaming it would alter manifest contents rather than just archive layout. BREAKING CHANGE: archives written by this SDK name the manifest entry `manifest.json`. Readers that look it up by exact name — web-sdk <= 0.20, java-sdk, client-cpp — cannot read them until they accept the spec name. Refs opentdf/platform#3513, opentdf/platform#4049, opentdf/tests#600. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Paul Flynn <pflynn-virtru@users.noreply.github.com>

revert(xtest): drop the PAYLOAD_ENTRY constant

3bf4546

Out of scope. The payload entry name is not what platform#4049 changes, so update_payload and the fixtures keep their 0.payload literals. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

pflynn-virtru changed the title ~~test(xtest)!: read the manifest entry as manifest.json, per spec~~ fix(xtest)!: read the manifest entry as manifest.json, per spec Sep 15, 2026

pflynn-virtru marked this pull request as ready for review September 15, 2026 17:25

pflynn-virtru requested review from a team as code owners September 15, 2026 17:25

pflynn-virtru mentioned this pull request Sep 15, 2026

fix(sdk)!: name the TDF manifest entry manifest.json per spec opentdf/web-sdk#1040

Open

pflynn-virtru mentioned this pull request Sep 15, 2026

fix(sdk)!: name the TDF manifest entry manifest.json per spec opentdf/java-sdk#405

Open

Navigation Menu

fix(xtest)!: read the manifest entry as manifest.json, per spec - #600

fix(xtest)!: read the manifest entry as manifest.json, per spec#600
pflynn-virtru wants to merge 4 commits into
mainfrom
fix/spec-manifest-entry-name

pflynn-virtru commented Sep 15, 2026 •

edited

Loading

Uh oh!

coderabbitai Bot commented Sep 15, 2026 •

edited

Loading

Walkthrough

Changes

❌ Failed checks (1 warning)

Uh oh!

github-actions Bot commented Sep 15, 2026

Uh oh!

github-actions Bot commented Sep 15, 2026

Uh oh!

sonarqubecloud Bot commented Sep 15, 2026

Uh oh!

github-actions Bot commented Sep 15, 2026

Uh oh!

github-actions Bot commented Sep 15, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

pflynn-virtru commented Sep 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Problem

Change

Expected failures

Testing

Uh oh!

coderabbitai Bot commented Sep 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

❌ Failed checks (1 warning)

Uh oh!

github-actions Bot commented Sep 15, 2026

X-Test Failure Report

Uh oh!

github-actions Bot commented Sep 15, 2026

X-Test Failure Report

Uh oh!

sonarqubecloud Bot commented Sep 15, 2026

Quality Gate passed

Uh oh!

github-actions Bot commented Sep 15, 2026

X-Test Failure Report

Uh oh!

github-actions Bot commented Sep 15, 2026

X-Test Failure Report

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

pflynn-virtru commented Sep 15, 2026 •

edited

Loading

coderabbitai Bot commented Sep 15, 2026 •

edited

Loading