Repository navigation
Conversation
Wraps `c2pa_builder_sign_ladder` (castlabs/c2pa-rs#9): sign an ABR ladder of single-file fragmented MP4s, one file per rendition, into one manifest with one Merkle tree per rendition. Sources and dests are parallel lists, positionally matched; lengths are checked and an empty ladder refused before the FFI runs; the builder is marked consumed and the manifest bytes released on every path. There is no read-back step: the writer returns the manifest it embedded, so the glob read-back defect fixed in castlabs/c2pa-rs#6 cannot recur here. Requires a native library built from contentauth#9. Against an older libc2pa_c the registration table fails at import, so this ships with the native bump. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
`c2pa_builder_sign_ladder` was listed in `_REQUIRED_FUNCTIONS`, so every importer failed at import against the pinned native library, which does not carry it yet. The symbol is now optional: `_HAS_SIGN_LADDER` records whether the loaded library has it, the ctypes signature is installed only then, and `Builder.sign_ladder` raises a C2paError naming the missing capability. The stock 0.31.0+stardustproof.5 library imports again and the unit suite passes against it. `sign_ladder` also called `_mark_consumed()` on success and on failure, although the FFI borrows the builder and never frees it, so the native object leaked. The handle is kept and released by `close()` or collection; verified that the builder closes cleanly after a successful and a failed ladder signing on a library that has the symbol. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
mstattma
left a comment
There was a problem hiding this comment.
Request changes on 298919cb852e402df427d4cdbfbfbb6352ed85b5.
Reviewed the two PR-authored commits from merge base 385cb2d, not the unrelated consolidation changes subsequently added to the base branch. Fable 5.1 reviewed statically without fallback; the findings below include independent runtime probes of the unchanged method.
Findings
- [P2] Reject embedded NULs before marshalling filesystem paths.
src/c2pa/c2pa.py:4021-4026: encoded strings containing NUL are accepted byc_char_p, so C receives only the prefix. A source/destination such asinput.mp4\0ignored/output.mp4\0ignoredbecomesinput.mp4/output.mp4. This changes which file is read or written instead of rejecting an invalid path. A mocked-FFI probe of the actual source method reproduced[(b'input.mp4', b'output.mp4')]. Validate each path before FFI, and translate UTF-8 encoding failures consistently. - [P2] Do not turn a manifest-copy failure into successful empty bytes.
src/c2pa/c2pa.py:4050-4057: allocation/copy exceptions are swallowed andb''is returned even though the method failed to deliver the manifest. Injecting a copyMemoryErrorreproduced successfulb''. Propagate the error while retaining the allocation release infinally;ctypes.string_atcan simplify the copy. - [P2] Commit regression coverage for this optional FFI API and its ownership contract. The PR changes only the binding file. Add tests for missing-symbol import/call-time failure, preflight failures without native calls, ordered path arrays and encoding, native negative results, copy failure with exactly-once freeing, and success/failure builder lifetime. Include a real native ladder roundtrip; feature qualification must fail rather than skip if the intended candidate library lacks the symbol.
Documentation And Contract
The final optional-symbol and borrowed-builder changes are sensible. The PR description still claims import failure against older native libraries and unconditional builder consumption; please update it and the stable fork's required patch/release documentation. Prefer C2paError.NotSupported for missing capability. A new public capability API is optional, not a prerequisite imposed by this review.
Do not restore _mark_consumed() for this borrowed native handle: that would leak it. Any deliberate adoption of modern upstream single-sign/close semantics belongs to the modern port, not an assertion that this stable method already consumes the handle.
Verification
- Independently reproduced both NUL truncation and swallowed copy failure against this exact source method.
- Original Python #2 with original Rust contentauth#9 and signer contentauth#21 passed real single-file ladder signing/validation.
- The stock
.5compatibility lane remains usable for ordinary paths; single-file ladder execution is unavailable there, as expected. - This does not qualify release-wheel packaging or live keystore interaction.
The modern-base binding port will carry tests and the corrected behavior, but please fix the original stable contribution independently before re-review. No Contentauth artifacts or existing upstream heads were changed.
|
The modern binding port is now published as mstattma#2 and #3, sharing head Your exact source head Final Fable 5.1 re-review found no confirmed blockers. The final combined stock CI selection passed 475 tests plus 44 subtests, and 31 focused tests plus real stock/candidate native lanes passed. The original Changes Requested findings still need corresponding stable fixes/tests here. Native pins, wheel releases, Contentauth artifacts, and existing published branches were not changed. |
…dd tests Review findings on Builder.sign_ladder. A path with an embedded NUL was accepted by `c_char_p` and silently truncated at the NUL, so `output.mp4\0ignored` wrote `output.mp4`. Every source and destination is now encoded by `_ladder_path_bytes`, which accepts str, bytes and PathLike, refuses a NUL, and turns an unencodable or non-path value into a C2paError, all before the native call. A failure while copying the returned manifest was swallowed and reported as successful empty bytes. The copy now uses `ctypes.string_at` and any failure propagates; the native buffer is released exactly once either way; a positive length with no buffer is an error; a zero length is b"". The missing-symbol case raises `C2paError.NotSupported`. tests/test_builder_sign_ladder.py covers the optional symbol at import and call time, every preflight refusal (no native call made), path order and encoding through a CFUNCTYPE stand-in for the native function, negative results, copy failure with exactly-once freeing, and the builder surviving success and failure. Its last test signs two real renditions and validates each output; it skips when the loaded library lacks the symbol unless C2PA_REQUIRE_SIGN_LADDER=1, in which case the absence is a failure -- the switch a release lane that is supposed to carry the symbol turns on. The single-file fragmented fixture is the c2pa-rs one, with its generation command. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
The README's stable fMP4 section and the AGENTS.md change table now record `Builder.sign_ladder`: what it signs, that the native symbol is optional (`_HAS_SIGN_LADDER`, `C2paError.NotSupported` when absent, the .5 library does not carry it), that the builder is borrowed, that outputs must not exist and are removed on failure, and how `C2PA_REQUIRE_SIGN_LADDER=1` turns the real roundtrip into a hard requirement for a lane that should have the symbol. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
mstattma
left a comment
There was a problem hiding this comment.
Approve 641750f1dff5a9e3e8b9a8e246e4b934b30e4487, with nonblocking follow-ups.
The prior blockers are resolved: NUL/path validation occurs before FFI, copy failures propagate with cleanup, positive-length/null-buffer results fail, missing capability is typed, and committed ctypes/native regression coverage is present. Retaining the borrowed builder matches this stable binding contract; no modern close-on-attempt behavior is required here.
Verification
- Updated binding tests against the assembled current source native stack: 18 passed, including native roundtrip with
C2PA_REQUIRE_SIGN_LADDER=1. - Against stock native: 17 passed, one expected missing-capability skip. Setting REQUIRE turns that absence into the expected failure.
- Native stack combines current Rust contentauth#6/contentauth#7/contentauth#9; SHA-256
4c5d8aa58e2d18e2a9001569e78c35ada70869afde3ed361f23e4e72058888f2, SDK 0.80.0. - The test-process TSA pointer was explicitly set to NULL for offline qualification. This is real signing/readback, not validation of the external timestamp service configured in the source test fixture.
Nonblocking Follow-Ups
- README's promise that every output is removed if the Python call fails is too strong: native cleanup is best-effort, and Python manifest-copy failure happens after native signing has succeeded, leaving signed outputs. Distinguish those cases and advise callers not to infer output absence from an exception.
- Strengthen the native test to assert a non-null active manifest, overall validation success, and identical embedded manifests/two rendition maps, rather than only an active-manifest key and absence of one failure code.
Independent source review used the configured GPT-6 Astra fallback after Fable failed; the completed review was retained as requested. New candidate reviews use Opus 5.5. The modern port remains intentionally different in lifecycle and no-overwrite/partial-output policy. This approval does not release or merge the patch set, and no Contentauth changes were made.
… test Non-blocking review follow-ups. The README distinguished no cases: native cleanup after a failed call is best effort, and a Python-side failure while copying the returned manifest happens after native signing has succeeded, so signed outputs are then in place. The native roundtrip test now asserts a non-null active manifest, a Valid/Trusted state with only the untrusted test certificate reported, a BMFF hash match and validated signature per output, exactly one BMFF assertion carrying two Merkle maps, and identical manifests across the two renditions. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
mstattma
left a comment
There was a problem hiding this comment.
Approve b13e8a3cfb75c032bae4954beef30193313c8017.
Opus 5.5 reviewed the new README/test delta from 641750f1, without fallback. Binding implementation, native pin, build scripts, and the intentional stable borrowed-builder contract are unchanged. The README now distinguishes best-effort native cleanup from Python-side failure after successful native signing, and the native test checks binding/signature success, two maps, and matching active manifests.
All 18 source binding tests passed against the fresh R3 combined library with C2PA_REQUIRE_SIGN_LADDER=1. Against stock native, 17 pass and the native test skips as expected; REQUIRE turns missing capability into an expected failure. The source fixture's TSA was externally disabled for offline execution, so this is actual signing/readback qualification, not network TSA qualification.
Nonblocking improvements: compare extracted embedded bytes directly with the returned manifest, not only parsed JSON; avoid relying on a particular Reader JSON presentation of hash assertions when future SDK versions differ. The current source/native combination does pass the stronger test. Update the PR description's remaining unconditional cleanup wording to match the README.
Approval is not release or merge authorization. No source edits or Contentauth changes were made.
… from CBOR Follow-ups from the review of b13e8a3. The real-ladder test now checks the strong property from the bytes: the manifest embedded in each output, read straight from the C2PA uuid box after its merkle_offset, must equal the bytes sign_ladder returned, and the one bmff hash assertion must carry one Merkle map per rendition -- decoded from the manifest's own CBOR, so the check no longer depends on how a given Reader version renders hash assertions as JSON. Reader is kept only for the validation result, and the JSON-presentation assertions on the hash assertion are gone. cbor2 is imported inside the one helper that needs it: the seventeen stand-in tests must keep running in a lane without it, and they do -- verified with the module hidden. Against a library built from the contentauth#7+contentauth#9 stable tree with C2PA_REQUIRE_SIGN_LADDER=1: 18 passed. Against the stock library: 17 passed, the native test skipped as designed. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
Follow-ups pushed as The real-ladder test now checks the strong property from the bytes. The manifest embedded in each output — read straight from the C2PA One detail worth having on record: the
Verification: against a library built from the contentauth#7+contentauth#9 stable tree with Still holding the merge until a native library from the merged stable head is pinned, as you asked. |
An adversarial pass over fc26ec2 found two things. The jumd label check tested toggle bit 0x01, which is Requestable; Label Present is 0x02. c2pa-rs's reader requires both (togs & 0x03 == 0x03) and its writer always emits 3, so the test passed for the right files by accident. It now tests 0x02, with a comment saying why. The README's "inspect or discard the destinations" was the indiscriminate wording the maintainer rejected on castlabs/c2pa-rs#9, and this binding's docs must give the same guidance as the native ones: a destination that already existed is refused before anything is written and never touched; on a native failure the writer removes what is then at the output paths it reserved, by path and best effort; discard only the leftovers at the destinations you passed, never a pre-existing file such as a source or a link to one. The two new assert messages that ran past the file's line length are wrapped. Against a library built from the contentauth#7+contentauth#9 stable tree with C2PA_REQUIRE_SIGN_LADDER=1: 18 passed. Stock library with cbor2 hidden: 17 passed, 1 skipped. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
Second follow-up pushed as
Against a library built from contentauth#7+contentauth#9 with |
mstattma
left a comment
There was a problem hiding this comment.
Fable 5.1 delta review of b13e8a3..1fa2278: no blocking findings. The embedded-manifest byte comparison and CBOR Merkle-map checks correctly handle the eight-byte offset and Label Present toggle bit. Local paired ladder/fragmented tests passed against the integrated stable SDK 0.80 native library with ladder support required. We are taking over the stable native/Python integration and preparing a new unreleased .6 identity with coherent native provenance pins and mandatory platform wheel ladder gates. Missing-cbor2 optional/required handling and the smoke-parser toggle follow-up are being addressed in that integration. No release publication is authorized by this approval.
4ec85a2
into
castlabs:fix/stable-single-file-fmp4
Adds
Builder.sign_ladder(signer, sources, dests) -> bytes, wrapping thec2pa_builder_sign_ladderFFI added in castlabs/c2pa-rs#9.Signs an ABR ladder of single-file fragmented MP4s — one file per rendition —
into one manifest: one Merkle tree per rendition in the shared assertion, the
identical manifest embedded in every output. Sources and dests are parallel
lists, positionally matched.
Contract
c2pa_builder_sign_ladderis not in_REQUIRED_FUNCTIONS;c2pa.c2pa._HAS_SIGN_LADDERrecords whether theloaded library has it, and
sign_ladderraisesC2paError.NotSupportedwhen it does not. The stock
0.31.0+stardustproof.5library imports andworks as before; only the new method is unavailable on it.
handle is kept and released by
close()as usual — on success and onfailure. (Marking it consumed here would leak the native object.)
str,bytesandPathLikeare accepted; an embedded NUL, anunencodable value or a non-path is a
C2paError—c_char_pwouldotherwise silently truncate a path at the NUL.
becoming successful empty bytes; the native buffer is released exactly
once; a positive length with no buffer is an error.
called. The native writer refuses a destination that already exists before anything
is written and never touches it. If it fails after that it removes what is
then at the output paths it reserved -- by path and best effort, as the
README says: a removal that fails is not reported, so a failure does not mean
no output exists, and a Python-side failure while copying the returned
manifest happens after native signing succeeded, with the signed outputs in
place. Discard only the leftovers at the destinations you passed, never a
pre-existing file such as a source or a link to one (see feat(sdk): sign an ABR ladder of single-file fragmented assets into one claim c2pa-rs#9).
Unlike
sign_fragmentedthere is no read-back: the ladder writer returns themanifest it embedded, so the glob read-back defect fixed in castlabs/c2pa-rs#6
cannot recur here.
Tests
tests/test_builder_sign_ladder.py(18): the symbol optional at import andNotSupportedat call time with the builder untouched; every preflightrefusal made with no native call; path order and encoding through a
CFUNCTYPEstand-in installed as the native function; negative results; acopy failure raising with exactly-once freeing; the builder surviving
success and failure. The last test signs two real renditions and checks them
from the bytes first: the manifest embedded in each output (read straight from
the C2PA
uuidbox, after itsmerkle_offset, not through a Reader) must equal the returned bytes, andthe assertion's Merkle map count is decoded from the manifest's own CBOR rather
than from any Reader JSON presentation, so the test does not depend on how a
given SDK renders hash assertions; Reader is then used for the validation result and to confirm the renditions
carry the identical manifest. It skips when the loaded library lacks the symbol unless
C2PA_REQUIRE_SIGN_LADDER=1, which makes the absence a failure — the switchfor a release lane whose library is supposed to carry it. The fixture is the
c2pa-rs single-file fragmented fixture, with its generation command.
Exercised on a real nine-rendition customer ladder through the signer: one
claim,
uniqueId1..9, each rendition validating individually. README andAGENTS.md document the binding in the stable-release context.
🤖 Generated with Claude Code