Skip to content

fix(dumpkit): write plist dates to whole seconds so plistlib reads them (#1448) - #1450

Merged
JarryShaw merged 1 commit into
mainfrom
fix/1448-plist-timestamp
Oct 9, 2026
Merged

JarryShaw merged 1 commit into
mainfrom
fix/1448-plist-timestamp

Conversation

@JarryShaw

@JarryShaw JarryShaw commented Oct 9, 2026 •

Copy link
Copy Markdown
Owner

Closes #1448

dictdumper's PLIST._append_date writes .%fZ, which plist <date> does not admit. make_dumper now overrides it for PLIST writers, so the xml output (also PLIST) changes too. Aware values are converted to UTC and the fraction of the already microsecond-rounded datetime is dropped, as plistlib.dump does.

Precision: no key is added. Every reported timestamp already has an exact sibling: time_epoch/timestamp_epoch (a Decimal), or the raw ntp_timestamp/pmip_timestamp for the MH options. Checked over all 1660 dates in the 23 captures.

@JarryShaw JarryShaw added fix Pull requests that fix a defect (fix: subject prefix) review: pending No verdict for the current head - never reviewed, or the head moved since the last one labels Oct 9, 2026
@JarryShaw
JarryShaw force-pushed the fix/1448-plist-timestamp branch from 6c20e67 to 4a2d70c Compare October 9, 2026 01:25
@JarryShaw JarryShaw added review: running A cross-review is in flight against the current head - no verdict yet and removed review: pending No verdict for the current head - never reviewed, or the head moved since the last one labels Oct 9, 2026
@JarryShaw
JarryShaw force-pushed the fix/1448-plist-timestamp branch from 4a2d70c to 4029129 Compare October 9, 2026 01:34
@JarryShaw JarryShaw added review: pending No verdict for the current head - never reviewed, or the head moved since the last one and removed review: running A cross-review is in flight against the current head - no verdict yet labels Oct 9, 2026
@JarryShaw
JarryShaw force-pushed the fix/1448-plist-timestamp branch from 4029129 to 5403616 Compare October 9, 2026 01:37
@JarryShaw

Copy link
Copy Markdown
Owner Author

Verdict: GOOD TO GO at 5403616d6 (Sonnet cross-review on 4a2d70c09; the author ran on Opus).

  • plistlib.load now succeeds on every capture's plist and xml dump: 44 of 44, where main fails all 44. Hand-built PCAP-NG files with if_tzone ±5h/+9h, epoch 0, year 5 and year 9999 all load too.
  • Every date keeps its exact value. All 1660 <date>s have an exact sibling (time_epoch, timestamp_epoch, NTP/PMIP), with 0 mismatches under TZ=UTC, Asia/Tokyo and America/New_York.
  • The naive-as-UTC branch is never hit by pcapkit itself. Every datetime it emits is already aware (frame.py:270, pcapng.py:1747, mh.py:4103).
  • json/tree output is byte-identical to main on all captures.
  • Tests fail without the fix: 6 fail with common.py taken from main.
  • Neighbouring suites pass: the dumpkit leg is 232 OK, foundation 528 OK, tests/project 419 passed.
  • What changed since the reviewed head (4a2d70c09 → 5403616d6): only tests and a docstring. plistlib.load(aware_datetime=...), which is Python 3.13+ and was the cause of the red 3.10–3.12 legs, was dropped. I re-ran the three touched modules: 23 passed. The worker also ran them on a real 3.10 interpreter: 23 passed.

Conflict with #1445: both PRs edit tests/integration/test_output_formats.py (the PlistRoundTripTests skip). Whichever lands second must also remove the #1448 KNOWN_FAILURES entry.

@JarryShaw JarryShaw added review: good-to-go Cross-review at the current head says ready; CI state is separate and removed review: pending No verdict for the current head - never reviewed, or the head moved since the last one labels Oct 9, 2026
@github-actions

github-actions Bot commented Oct 9, 2026 •

Copy link
Copy Markdown
Contributor

Coverage: 89.51% (unit tier, Python 3.14, 2ab8bbe83, Unit Tests run success)

Package Statements Missed Branches Partial Cover
pcapkit (top level) 104 4 20 4 93.55%
pcapkit/const 18795 968 2356 869 90.90%
pcapkit/corekit 2043 89 636 25 94.77%
pcapkit/dumpkit 242 2 88 2 98.79%
pcapkit/foundation 2648 98 930 48 94.80%
pcapkit/interface 112 7 40 5 92.11%
pcapkit/protocols 16805 219 4522 183 98.08%
pcapkit/toolkit 539 74 168 3 84.58%
pcapkit/utilities 429 4 122 4 98.55%
pcapkit/vendor 4409 2342 1006 157 43.25%

Per-file detail: the coverage-html artifact of this run.

@JarryShaw

Copy link
Copy Markdown
Owner Author

resolve conflicts

@JarryShaw

Copy link
Copy Markdown
Owner Author

On it. The worker is rebasing onto main (b14d676c5), keeping this PR's PlistRoundTripTests, and dropping the now-stale #1448 row from tests/dumpkit/test_capture_dump_runtime.py in the same commit. I'll re-verify the new head before re-granting the verdict.

@JarryShaw
JarryShaw force-pushed the fix/1448-plist-timestamp branch from 5403616 to 11e7876 Compare October 9, 2026 02:10
…em (#1448)

dictdumper's PLIST._append_date writes '%Y-%m-%dT%H:%M:%S.%fZ', which the
property list <date> grammar does not admit, so plistlib.load failed on the
plist/xml report of every capture.

- Override _append_date in make_dumper for PLIST writers: convert an aware
  value to UTC, take a naive one as UTC, truncate to whole seconds, and
  zero-pad the year, as plistlib.dump does.
- The exact value is kept by the sibling field every reported timestamp
  already has (time_epoch / timestamp_epoch, or the MH option's raw
  ntp_timestamp / pmip_timestamp), so no key is added.
- Un-skip PlistRoundTripTests in tests/integration/test_output_formats.py and
  add tests/dumpkit/test_plist_date_unit.py.
- Read the #772 regression's plist report back with plistlib.load, and
  update the note in docs/source/contributing/pep.rst.
- Drop the #1448 row from KNOWN_FAILURES in
  tests/dumpkit/test_capture_dump_runtime.py, which now passes.

json and tree output is unchanged.
@JarryShaw
JarryShaw force-pushed the fix/1448-plist-timestamp branch from 11e7876 to 2ab8bbe Compare October 9, 2026 02:11
@JarryShaw

Copy link
Copy Markdown
Owner Author

Verdict: GOOD TO GO at 2ab8bbe83, which is 5403616d6 rebased onto main (b14d676c5).

  • Library code is unchanged: the pcapkit/ diff is line-for-line identical to the head the Sonnet review passed.
  • Test changes:
  • Re-run myself on this head: the dump runtime module, test_output_formats, the plist date tests and the escaping-regression tests give 29 passed, 1735 subtests.

@JarryShaw JarryShaw added review: good-to-go Cross-review at the current head says ready; CI state is separate and removed review: good-to-go Cross-review at the current head says ready; CI state is separate labels Oct 9, 2026
@JarryShaw
JarryShaw merged commit ef878e4 into main Oct 9, 2026
38 checks passed
@JarryShaw
JarryShaw deleted the fix/1448-plist-timestamp branch October 9, 2026 02:40
@JarryShaw JarryShaw removed the review: good-to-go Cross-review at the current head says ready; CI state is separate label Oct 9, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

fix Pull requests that fix a defect (fix: subject prefix)

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

fix(dumpkit): plist output is unreadable by plistlib because timestamps carry fractional seconds

1 participant