Wire dead JPEG segment parsers into read path (COM, SPIFF, DQT quality, multi-chunk ICC, APP6/GoPro) - #22
Merged
Conversation
…multi-chunk ICC) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
parse_app6 existed but was never dispatched (stale TODO claimed it was unimplemented). Wiring it surfaced parity defects, all fixed: - GoPro payloads start with "GoPro\0" (old FourCC heuristic never matched real files); GPMF FourCCs now map to ExifTool tag names under the GoPro: family (GoPro:Model, GoPro:CameraSerialNumber, ...) - unknown GPMF FourCCs and unknown APP6 formats extract nothing, matching ExifTool without -u (no more APP6:Unknown binary blobs) - NITF gate corrected to "NTIF\0", TDHD gate to "TDHD\x01\0\0\0" - print conversions ported: OREN/PRTN/VFOV/VERS + noYes tags Verified against exiftool 13.55 on a synthetic GoPro APP6 fixture. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…dy exist Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… hardening Fixes from a whole-branch code review of the JPEG dead-parser wiring work: - NITF identifier inversion (app6.rs): parse_app6 dispatched NITF on "NTIF\0", backwards from ExifTool's actual READ path (ExifTool.pm:8140 matches /^NITF\0/, DirStart=5; JPEG.pm's table Condition citing "NTIF\0" never governs reads). Flipped the dispatch and fixed all comments that repeated the wrong identifier. Verified empirically against exiftool 13.55: a "NITF\0" APP6 payload yields NITF:* tags, "NTIF\0" yields only an "Unknown APP6 'NTIF' segment" warning. - Duplicate 1-of-1 ICC profiles (jpeg_helpers.rs): a file with two APP2 segments each marked chunk 1-of-1 used to hit a duplicate-chunk error in IccChunkAssembler and drop every ICC tag. Now falls back to parsing the first "chunk 1 of 1" segment on assembler error, approximating ExifTool's warn-and-keep-first behavior (the previous oxidex release kept the last). - GPMF recursion depth cap (app6.rs): parse_gpmf_records recursed into container records (format 0) with no depth limit. Added a depth parameter capped at 16; containers beyond the cap are skipped but siblings at the current level still parse. - TDHD length gate (app6.rs): ExifTool's TDHD condition also requires segment length > 12 (ExifTool.pm:8146); an 8-byte bare identifier now extracts nothing. - KNOWN_DISCREPANCIES.md: moved the JPEG COM/DQT wiring section to sit with the other discrepancy sections ahead of References, and documented two more divergences: non-UTF-8 COM comments store as a binary blob instead of ExifTool's Latin-1 decode, and duplicate 1-of-1 ICC profiles keep the first (matching ExifTool; previous oxidex kept the last). - Documented that process_spiff_segments' 32-byte/"SPIFF\0" gate is intentionally duplicated in parse_spiff_segment as defense-in-depth. Each behavior-changing fix followed TDD: test added/adjusted first, watched to fail, then the fix applied. Full verification: cargo fmt clean, no new clippy warnings in touched files, cargo test -p oxidex --lib (2671 passed) and cargo test --test integration (587 passed) both green. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Pulls in PR #18 (ExifTool-compatible date shifting for JPEG via in-place EXIF patching) ahead of pushing the JPEG dead-parser wiring branch; merge-tree reported no conflicts.
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | -10 |
| Duplication | 0 |
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
swackhamer
added a commit
that referenced
this pull request
Jul 19, 2026
PR #19 merged mid-session (by swackhamer) before the classification- pipeline refinements and the CI runner.temp fix landed; those commits were stranded on this branch behind the now-closed PR. #21 (pin remaining workflow actions) and #22 (wire dead JPEG segment parsers -- COM, SPIFF, DQT quality, multi-chunk ICC, APP6/GoPro -- into the read path) merged to main after #19. Merged origin/main (resolved 7 add/add conflicts: this branch's fixed versions of the JPEG tag matrix pipeline/docs/workflow superseded PR #19's pre-refinement originals; #21/#22's files merged cleanly with no overlap). Verified: full workspace test suite passes (3,615 tests, 0 failures, 37 binaries), release binary rebuilds cleanly. Re-ran the full 4,812-tag matrix against the fully current binary. Result: only one tag changed status -- File:Comment flipped from unsupported to readable, confirming #22's COM-comment read-path fix. The rest of #22's fixes (SPIFF, DQT-derived quality, multi-chunk ICC, APP6/GoPro) aren't independently visible to this harness: they're either not ExifTool-writable tags at all, or fall outside the EXIF/XMP/IPTC/JFIF/Photoshop/ICC_Profile groups this matrix synthesizes samples for -- noted explicitly in the updated R8 entry rather than left to look unconfirmed. Zero write regressions verified: the exact same 122 tags remain CLI-writable in every re-run this session. Baseline ratcheted 2028 -> 2029 readable to match. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
swackhamer
added a commit
that referenced
this pull request
Jul 19, 2026
…#22 re-verification (#23) * feat(docs+ci): empirical JPEG tag matrix with exiftool comparison in CI Adds an empirical, per-tag exiftool<->oxidex JPEG support matrix: - scripts/generate_exiftool_manifest.py: dumps ExifTool's tag DB (-f -listx) for JPEG-relevant groups and synthesizes a type-appropriate sample value per writable tag (4,812 tags, 74 XMP namespaces) - scripts/jpeg_tag_matrix.py: for every tag, exiftool-writes a sample into a clean JPEG and verifies oxidex reads it; then oxidex-writes it and verifies both oxidex and exiftool read it back (with per-tag isolation retests so one poison tag can't contaminate a group batch) - scripts/jpeg_tag_report.py: renders docs/reference/jpeg-tag-support.md (readable-tag mapping with working write keys + example values) and docs/reference/jpeg-tag-matrix.md (full 4,812-tag classification and the confirmed W1-W11/R1-R8 bug inventory with root causes), plus a ratcheting baseline gate (docs/reference/jpeg-tag-baseline.json) - tests/fixtures/jpeg/tag_matrix_base.jpg: minimal 8x8 base image - .github/workflows/jpeg-tag-matrix.yml: rebuilds oxidex, runs the matrix against pinned ExifTool 13.55 on main pushes / weekly / on demand, auto-commits refreshed docs, and fails on support regressions - VitePress sidebar entries so both reports publish to oxidex.net Current baseline: 1,621/4,812 tags readable, 122 CLI-writable (56 clean), 48 read-broken, 240 read-OK-write-broken. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * docs: integrate JPEG tag matrix into site structure Fit the empirical JPEG reports into the right places on oxidex.net: - Move the two pages into the Compatibility sidebar section (next to the generated ExifTool comparison reports) instead of the top-level Reference list - Add an ExifTool Compatibility entry to the Reference overview page - Style both generated pages like the site's other auto-generated docs: VitePress info callout with last-updated date and pinned ExifTool version, outline depth for the large matrix page, and cross-links between JPEG Tag Support <-> JPEG Tag Matrix <-> ExifTool Coverage <-> Compatibility overview - Cross-link from the auto-generated tag-coverage-analysis page (via its generator, so the link survives regeneration) and from the deploy-time comparison overview (src/bin/tag-comparison markdown generator) Verified: vitepress build succeeds; tag-comparison binary compiles. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(security): address Codacy findings on the JPEG tag matrix pipeline Resolves all 23 new issues flagged by Codacy on PR #19: - Pin the two third-party GitHub Actions in the new workflow (dtolnay/rust-toolchain, Swatinem/rust-cache, astral-sh/setup-uv) to full commit SHAs; dtolnay/rust-toolchain keeps its "stable" channel behavior via an explicit `toolchain: stable` input (2 High) - Require defusedxml unconditionally instead of falling back to stdlib xml.etree.ElementTree, closing the XXE gap for real rather than just preferring the hardened parser when available (4 Error/Warning) - Replace the hardcoded "/tmp/oxidex-tagmap" default work directory with tempfile.gettempdir()-based construction (Bandit B108 x3); CI now pins TAGMATRIX_WORK to ${runner.temp}/oxidex-tagmap explicitly - Add narrowly-scoped, justified nosec/nosemgrep suppressions on the subprocess.run() call sites: all use list-argv with no shell=True, and every argument is either a local tool path from an env var or a value this same tooling synthesized moments earlier from exiftool's own local -listx XML dump -- never externally-controlled input Also converts the three scripts to PEP 723 inline-script format (matching the existing scripts/generate_tag_coverage.py convention) and switches the CI workflow from pip+setup-python to uv, declaring defusedxml as a proper inline dependency instead of a manual CI install step. Verified: uv run on all three scripts reproduces identical manifest/matrix output; YAML validated; git status confirms no incidental changes to the generated docs pages (smoke-testing against partial datasets was reverted before commit). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(security): correct nosemgrep suppression placement for tainted-env-args The previous commit's suppression comments for python.lang.security.audit.dangerous-subprocess-use-tainted-env-args were placed on the `subprocess.run(` line, but Semgrep anchors that rule on the line containing the actual argument list -- a different physical line in these multi-line calls (confirmed by re-querying Codacy's PR analysis, which still reported 4 new issues after the first fix commit). Centralizes generate_exiftool_manifest.py's three subprocess call sites into a single _run_exiftool() helper (also fixing a DRY violation) and collapses jpeg_tag_matrix.py's existing run() wrapper call to one physical line, so both the audit and tainted-env-args rule anchors are covered by one trailing suppression comment per call site. Verified: uv run on both scripts reproduces identical manifest/matrix output as before; docs pages confirmed untouched by smoke testing. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix: address code-quality review findings on PR #19 Resolves 5 unresolved review threads from automated code review: - Two empty "except: pass" blocks (generate_exiftool_manifest.py's non-numeric tag `count` handling, jpeg_tag_report.py's malformed- manifest fallback in autogen_callout()) now carry a one-line comment explaining why the exception is intentionally swallowed, preserving identical runtime behavior - Three paragraphs in jpeg_tag_report.py's generated Markdown relied on Python's implicit adjacent-string-literal concatenation inside a list literal -- a pattern that silently merges list items if a comma is ever dropped. Made each paragraph an explicit `+`-joined expression so a missing separator would be a visible syntax/runtime error instead of a silent merge. Verified byte-for-byte: reran scripts/jpeg_tag_report.py against the original full 4,812-tag results.json and diffed the regenerated docs/reference/jpeg-tag-{support,matrix}.md against the committed versions -- zero differences, confirming the refactor is a pure readability/maintainability change with no output impact. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix: merge origin/main, fold classification logic into committed pipeline Merges origin/main (adds src/writers/exif_inplace.rs and rewrites date_shift.rs for ExifTool-compatible JPEG date shifting, PR #18). Verified: full workspace test suite passes (598 tests), release binary rebuilds cleanly. Also closes a real gap found while re-verifying the JPEG tag matrix against the merged binary: the read-bug pattern classification, write-bug-cluster annotation, and non-standard-encoding validation that back the published docs were only ever run via throwaway /tmp scripts, never committed. The CI workflow as originally written would have regenerated a materially cruder report on its first run (e.g. every raw read mismatch showing as "read broken" instead of the refined 48-bug / formatting-only split). Ports all of it into scripts/jpeg_tag_matrix.py: - classify_read_mismatch() + apply_bug_classification(): root-causes read=MISMATCH into a tagged bug or MISMATCH_FORMAT (value-equivalent, formatting differs), and labels write=INTEROP_BROKEN with its root-caused bug cluster (I1-I5) -- ported as empirically-verified data, not guesses - Generic no-op detection: every write result is compared against the tag's pristine pre-write value in the base fixture, so a write that silently changed nothing is classified NOT_WRITTEN regardless of whether the stale value happens to coincidentally match or mismatch the sample (replaces a hardcoded false-positive tag list) - Generic same-group key-fallback scan: catches registry asymmetries (value written/read correctly but under an unexpected raw/hex key) without hardcoding tag names -- applied to both read and write paths, and found one previously-undetected case (TargetPrinter) was incorrectly counted as unsupported on the read side too - Tightened write-verification lookups to require an exact group match, fixing two false READBACK_BROKEN results caused by matching an unrelated pre-existing tag of the same bare name in a different group - Integrated exiftool -validate diffing directly into the write-test phase (was a separate, uncommitted script) - Fixed a real field-naming collision: read and write phases both wrote "ox_val"/"et_val" into the same per-tag result dict, so a successful write silently clobbered the read phase's observed value for that tag -- corrupting the bug classifier's input on any tag where read failed but an independent write-then-readback happened to succeed. Write's fields are now namespaced (write_ox_val/write_et_val/write_ox_key) - Fixed the sample generator picking a literal "None"/"Unknown" enum label as a tag's test value when a more distinctive option existed, which made a genuine write indistinguishable from a no-op for any tag whose pristine default happens to equal that label (found via JFIF:ResolutionUnit) Re-ran the full 4,812-tag matrix against the merged binary end-to-end through the now-fully-committed pipeline. Verified zero write regressions: the exact same 122 tags remain CLI-writable, no losses. Readable-tag detection improved 1621 -> 2028 (registry-asymmetry and field-collision fixes surfacing previously-miscounted tags); the "write broken" bucket's rise (240 -> 252) is entirely tags whose already- broken write status was simply invisible before because their read was wrongly counted as unsupported (verified precisely: it only fires where read crosses from MISSING into a readable status while write is unchanged). Baseline updated to reflect the corrected, more accurate counts. Also updates the W2 known-bug description: the merged date-shift rewrite fixed updating an existing DateTimeOriginal-style tag in place (verified manually); only fresh-creation on a tag with no prior value still fails, which is what the harness's dateless base fixture measures. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(ci): move runner.temp reference out of unsupported env: scope The `runner` context is only available inside individual step contexts (steps[*].run/with/if/env), never in a workflow- or job-level `env:` block -- confirmed by actionlint and by a real CI run on this branch (29682285560), which silently scheduled zero jobs with the generic "This run likely failed because of a workflow file issue" error. This went undetected since d14d772 because no prior push touched a path matching the workflow's trigger filter until the previous commit did. Route TAGMATRIX_WORK through $GITHUB_ENV from a plain shell step instead (uses $RUNNER_TEMP, not the `${{ runner.temp }}` expression), which is the standard idiom for computing a workflow-wide env var from step-only context. `actionlint` now reports zero issues for this file. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * docs: re-verify JPEG tag matrix against #21/#22, update R8 status PR #19 merged mid-session (by swackhamer) before the classification- pipeline refinements and the CI runner.temp fix landed; those commits were stranded on this branch behind the now-closed PR. #21 (pin remaining workflow actions) and #22 (wire dead JPEG segment parsers -- COM, SPIFF, DQT quality, multi-chunk ICC, APP6/GoPro -- into the read path) merged to main after #19. Merged origin/main (resolved 7 add/add conflicts: this branch's fixed versions of the JPEG tag matrix pipeline/docs/workflow superseded PR #19's pre-refinement originals; #21/#22's files merged cleanly with no overlap). Verified: full workspace test suite passes (3,615 tests, 0 failures, 37 binaries), release binary rebuilds cleanly. Re-ran the full 4,812-tag matrix against the fully current binary. Result: only one tag changed status -- File:Comment flipped from unsupported to readable, confirming #22's COM-comment read-path fix. The rest of #22's fixes (SPIFF, DQT-derived quality, multi-chunk ICC, APP6/GoPro) aren't independently visible to this harness: they're either not ExifTool-writable tags at all, or fall outside the EXIF/XMP/IPTC/JFIF/Photoshop/ICC_Profile groups this matrix synthesizes samples for -- noted explicitly in the updated R8 entry rather than left to look unconfirmed. Zero write regressions verified: the exact same 122 tags remain CLI-writable in every re-run this session. Baseline ratcheted 2028 -> 2029 readable to match. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
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.
Summary
Several JPEG segment parsers existed fully implemented but were never invoked from
parse_jpeg_metadata— verified dead via call-site analysis and an empirical read of a synthetic fixture. This wires them in with ExifTool 13.55 parity, validated against a local exiftool install throughout.File:Comment(trailing NULs stripped, matching ExifTool's COM handler)SPIFF:*tags, using ExifTool's real 32-byte-payload gate and offsets (not the SPIFF spec's, which don't match real-world samples ExifTool follows)File:JPEGQualityEstimate, a verbatim port of ExifTool'sEstimateQualityalgorithm (JPEGDigest.pm), replacing a heuristic that diverged from ExifTool's actual outputIccChunkAssemblerinstead of being dropped with a warningparse_icc_profile_segment,parse_adobe_segment,parse_activephoto_segment,parse_jpeg_ls_segment)A whole-branch review caught two real regressions before merge, both fixed and re-verified independently:
NTIF\0vs ExifTool's actualNITF\0read-path condition) — a test had locked in the wrong directionSpec and implementation plan are included under
docs/superpowers/.Test plan
cargo test --workspacegreen post-merge withmain(2689 lib + 587 integration + all other suites, 0 failures)cargo clippy --workspace --all-targetsshows no new warnings in any file this branch touchesexiftool 13.55on synthetic fixtures:File:Comment,File:JPEGQualityEstimate(87), all 11SPIFF:*tags, andGoPro:*tags match one-for-one