mirror of
https://github.com/domainaware/parsedmarc.git
synced 2026-08-01 05:02:18 +00:00
4e80047e685607ba291f4618e6eeb65ca19afd83
9
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
  
|
23c5ea9ad6 |
Fix DKIM/SPF and SMTP TLS detail-table cross-products in dashboards (#169) (#839)
* Fix DKIM/SPF alignment detail cross-product in dashboards (#169) Elasticsearch and OpenSearch dynamic-map the dkim_results/spf_results object arrays as `object` (create_indexes never registers the DSL document mappings), so Lucene flattens each array into independent multi-valued fields and stacked terms aggregations on dkim_results.selector/.domain/.result return every combination of values across a report's signatures — each phantom row repeating the full message count. Aggregate documents now also carry dkim_results_combined and spf_results_combined: one "selector / domain / result" ("scope / domain / result") string per auth result, composed in add_dkim_result/add_spf_result. The Kibana/OpenSearch Dashboards and Grafana (Elasticsearch) alignment-detail tables aggregate those instead, and the Splunk detail panels pair the values with mvzip/mvexpand. A documented idempotent _update_by_query backfills documents saved by older versions; the query matches only documents that have auth results and lack the combined fields, because an `exists` query cannot see an empty array. Also corrects the dead _SPFResult.results (plural) declaration to `result` (the save path always wrote the singular key), fixes the result parameter annotations on add_dkim_result/add_spf_result, and removes the Grafana dmarcian.com DKIM-checker data link, which required the separate domain/selector columns. The SMTP TLS visualizations have the same class of defect and are tracked separately. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Address Copilot review findings on #839 Reword the combined-field regression test docstrings: the DKIM/SPF auth results are dynamic-mapped as plain `object`, not the `nested` mapping type the previous wording implied — the distinction is the crux of the fix. Also drop the inert renameByName entries Copilot flagged on the Grafana Overview and DKIM Alignment Details panels, which referenced fields those panels' queries no longer (or never) produced. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Add PR #839 review lessons to AGENTS.md Extend the "Review passes cover prose" section with two rules from the #839 Copilot findings: docstrings/comments get the same text-level review pass as docs and dashboard labels (with suspicion for dual-use terms like "nested" near Elasticsearch code), and inert config entries inside hunks a PR already rewrites should be cleaned rather than preserved to minimize the diff. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Reflow create_indexes comments flagged by Copilot The line wrap placed "#169" directly after the comment marker, so the raw source read "# #169;". Reword so the issue reference stays on one line. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Extend the hunk-proofreading rule with rendered-text wraps Fold the PR #839 second-round Copilot lesson into the existing rule: proofread how wrapped lines render (comment markers, punctuation at wrap points), not just the wording itself. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Rename Overview combined-result column labels (Copilot round 3) The Overview table's "DKIM Auth Result" / "SPF Auth Result" labels were kept when the columns switched to the combined "selector / domain / result" values, leaving the headers misleading. Rename them to match the detail panels' convention and retarget the byName width overrides that matched the old labels, widening them for the longer values. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Document per-signature row semantics in the alignment tables A message carrying multiple DKIM signatures appears once per signature in the details tables, so summing the messages column across rows can exceed the total message count. State that explicitly rather than leaving readers to infer it. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Address Copilot round-4 findings on dashboards Fix three pre-existing saved-object title typos in the OpenSearch ndjson (leading space on "Aggregate DMARC passed DMARC", trailing space on "Aggregate DMARC reporting organizations", double space in "map of message sources by country"), in both the top-level title and the embedded visState title. Normalize the Splunk DKIM details placeholders: the base search's fillnull renders wholly-missing DKIM fields as the literal string "null", so unsigned mail showed "null / null / null" while the SPF panel shows "none". Rewrite the values to "none" after the signature split, where the fields are single-valued and the mvzip pairing cannot be disturbed. Verified against the dev Splunk that no truncation or mis-pairing occurs either way, since fillnull guarantees the fields are never actually null. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Backfill combined DKIM/SPF fields automatically at startup migrate_indexes() now backfills dkim_results_combined and spf_results_combined on aggregate documents saved by older versions, so ES/OS users get historical data in the reworked alignment tables without running the documented _update_by_query by hand. The backfill is submitted as a non-blocking background task (wait_for_completion=false, conflicts=proceed) guarded by a cheap count query, making repeated startups a fast no-op once an index is backfilled; any cluster error is logged as a warning and retried at the next startup rather than raised. The manual command remains documented for users who upgrade dashboards without pointing the new parsedmarc at the cluster or who want to control write-load timing. The legacy published_policy.fo long-to-text reindex migration in the OpenSearch module is kept ahead of the new backfill, for clusters upgraded from very old data. Verified end-to-end against the live dev environment: a real CLI startup backfilled 9 stripped OpenSearch documents (logged with task ID) while the already-backfilled Elasticsearch side stayed silent, and a second startup was silent on both engines. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Match backfill guard on either domain or result subfield End-to-end upgrade testing (real parsedmarc 10.2.4 ingest, then a branch startup) surfaced that an exists query cannot see an empty string: a text field with no tokens is invisible to exists. The parsers we audited never store an auth result with an empty or missing domain — they drop such entries entirely, so the previous domain-only guard was sufficient for their data — but the storage shape of every historical parsedmarc version can't be audited, so the guard (and the documented manual command) now matches either the domain or the result subfield per protocol. Matching either costs nothing and cannot skip a document that has something to backfill. Verified by recomputing expected combined values from _source for all 2,299 documents on both engines: every document with stored auth results has exactly the recomputed pairs, and the legacy fo migration correctly did not fire on typeless indexes. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Add auth-result filter controls to the Kibana/OSD aggregate dashboard The combined per-signature columns fixed the #169 cross-product but left no way to click-filter by an individual selector, domain, or result. Add an "Aggregate DMARC auth result filters" input_control_vis panel above the SPF/DKIM details tables with six option-list dropdowns (DKIM selector/domain/result, SPF scope/domain/result) that emit ordinary dashboard-wide filter pills. Works on both Kibana 8.19 and OpenSearch Dashboards 3, verified by driving the controls in both UIs against the issue's two-signature repro report. Documented in kibana.md, including the flat-mapping caveat: combining two component filters matches documents where any signature satisfies each condition individually. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Scale Grafana source-country map markers with message volume The "Map of Message Source Countries" panel drew fixed 5 px dark-green markers at 50% opacity — nearly invisible on the dark basemap, so the panel read as empty even when data was flowing (verified via the query API). Markers now scale with Sum(message_count) (min 4, max 30 px) at 0.8 opacity in a higher-contrast green. Pre-existing issue; the identically-styled failure-dashboard map panel is intentionally left untouched. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Correct the nested-mapping rationale in the create_indexes comments The comments claimed Kibana/OSD/Grafana "cannot terms-aggregate fields inside a nested mapping" — too absolute. Fact-checked empirically and against primary docs: Kibana/OSD visual editors (Lens and classic Visualize) do not support nested fields, but Vega panels can run nested aggregations (they just cannot render tables, per Elastic's docs), and Grafana >= 9.4 has a nested bucket aggregation (grafana/grafana#62301) but no reverse_nested, so parent-level metrics like Sum(message_count) return 0 inside per-signature buckets (reproduced live). Conclusion unchanged: the dynamic object mapping stays load-bearing for the shipped dashboards. PR #839's body was updated with the same correction. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Pin ruff exactly, matching the existing pyright pin rationale CI installs the [build] extra fresh on every run, and ruff was the one lint tool left unpinned. ruff 0.16.0 (released this week) began flagging this codebase's str.format() house style, so every PR started failing lint on lines it never touched. Pin to 0.15.21 — the version the codebase is clean under — with the same bump-deliberately comment pyright carries. Upgrading to 0.16 and converting to f-strings can be its own PR. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Address Copilot findings: harden migrate_indexes, normalize panel titles Three unresolved review threads, all verified against cli.py's re-raising init handler before fixing: - elastic.py/opensearch.py: connections.get_connection() sat outside migrate_indexes()'s try/except, so a connection-registration failure would abort startup despite the docstring's promise that migration errors are caught and logged. Now caught, logged, and skipped until the next startup. - opensearch.py: the legacy published_policy.fo migration loop did unguarded network I/O (exists/get_field_mapping/reindex/delete), so a transient cluster error aborted startup on the OpenSearch path while the identical situation on the Elasticsearch path was logged and survived. Each index's migration attempt is now wrapped, warns, and moves on. - opensearch_dashboards.ndjson: normalized two pre-existing panel titles in the aggregate dashboard's panelsJSON ("Reporting organizations " trailing space, "Map of message sources by country" double space). Regression tests assert migrate_indexes never propagates connection or per-index cluster errors on either backend. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Clarify that Nested() on auth-result fields is in-memory shape only Copilot flagged that _AggregateReportDoc declares dkim_results and spf_results with Nested(...) while the create_indexes comment insists the stored mapping must stay dynamic `object`. Both are true: the Nested declaration only shapes the DSL's in-memory document building and is never installed as a mapping, because create_indexes skips Index.document() registration. Say so at both sites, in both backends, so nobody "fixes" the mismatch by registering the mapping — which would install real nested mappings and blank the dashboards. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Refer to the filter panel by its displayed title in docs and CHANGELOG The dashboard convention is a short panel display title backed by a long-form saved-object name ("SPF details" / "Aggregate DMARC SPF details"), and the new controls panel follows it. The docs and CHANGELOG named the panel by its saved-object title, which is not what a user sees on the dashboard; use the displayed "Auth result filters" instead. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Fix misspelled column label in the failure email samples table The "DMARC failure email samples" visualization labeled its authentication_results column "autentication_results". The underlying field reference was already correct; only the user-facing customLabel was misspelled. A sweep of every title and customLabel in the ndjson found no other misspellings. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Extend the combined-field fix to SMTP TLS documents SMTP TLS reports have the same cross-product defect as the DKIM/SPF alignment tables (issue #169), one level deeper: policies is an object array and each policy's failure_details is an object array inside it, so stacked terms aggregations on their subfields fabricate rows. Documents now also carry policies_combined ("domain / type" per policy) and failure_details_combined ("domain / type / result / sending mta / receiving ip / mx" per failure detail), composed at save time with the same "none" fallbacks as the aggregate fields. migrate_indexes() gains smtp_tls_indexes and backfills old documents with the same guarded, non-blocking update_by_query pattern; cli.py wires the index name in on both backends, and the manual _update_by_query command is documented. Also fixes two adjacent dead fields: add_failure_details stored additional_information_uri under the wrong constructor kwarg (additional_information), and receiving_mx_hostname had no declaration despite always being stored. Verified live on ES 8.19 and OpenSearch 3: a two-policy repro report yields exactly 2 policy rows and 2 failure-detail rows via the combined fields where the old stacked aggregations return 4 of each; the startup backfill converted the 4 pre-existing sample documents on both engines with zero recompute mismatches. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Rework the SMTP TLS dashboards onto the combined fields Kibana/OSD: "SMTP TLS domains" replaces its stacked policy_domain × policy_type terms with one terms agg on policies_combined.keyword; "SMTP TLS failure details" replaces six stacked terms spanning both array levels with one on failure_details_combined.keyword; the smtp_tls* index-pattern field cache gains the new fields. The "reporting organizations" table only buckets on doc-level org_name and needed no change. Splunk: the base search now expands policies at the JSON level (spath + mvexpand) so policy fields are scalars per event, and the failure details panel expands the second level the same way — sums are the detail's own failed_session_count, correctly paired. Verified via the search REST API: a two-policy repro returns exactly one row per real failure detail with per-detail counts. kibana.md documents the per-policy/per-detail row semantics and the honest caveat that session-count sums remain per report document. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Persist additional_info_uri from parsed SMTP TLS failure details Copilot caught that the savers read additional_information_uri from the parsed failure-detail dict, but the parser's key is additional_info_uri (SMTPTLSFailureDetailsOptional in types.py, set in parse_smtp_tls_report_json), so the URI was never persisted — the read-side half of the dead-field bug whose write-side half (wrong constructor kwarg) was fixed earlier. Read the parser's key first, keeping the long-form key as a fallback for dicts built by other callers. Regression test proven to fail on the unfixed savers. Also restructured the expected combined-string test values into named locals so no implicit string concatenation sits inside a list literal. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Say "inner doc", not "nested doc", in the singular-key test docstrings Final review sweep: in this codebase "nested" is reserved for the Elasticsearch mapping type, and these InnerDoc-serialization docstrings used it colloquially — the same dual-use-term trap documented in AGENTS.md. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com> |
||
|
|
40509f801b |
Migrate Elasticsearch output to the elasticsearch-py 8.x client (#822)
* Migrate Elasticsearch output to the elasticsearch-py 8.x client (#806) The mandatory elasticsearch<7.14.0 + elasticsearch-dsl==7.4.0 pins transitively forced urllib3<2 (EOL 1.26.x) onto every install. The old <7.14.0 cap only existed to dodge the client product check that broke OpenSearch users (#452, #653) — obsolete now that parsedmarc has a dedicated [opensearch] backend on opensearch-py. - Depend on elasticsearch>=8.18,<9 and drop elasticsearch-dsl entirely (the DSL ships inside the client as elasticsearch.dsl since 8.18.0). The 8.x client's elastic-transport allows urllib3>=1.26.2,<3, so installs can now resolve urllib3 2.x. The 8.x line supports both Elasticsearch 8.x and 9.x servers; ES 7.x servers are no longer supported, and OpenSearch users pointing [elasticsearch] at an OpenSearch cluster must switch to the [opensearch] section. - set_hosts() now builds 8.x connection kwargs (scheme-qualified host URLs, request_timeout, basic_auth) while keeping the function signature and every INI option unchanged. - migrate_indexes() is now a documented no-op kept for API compatibility: its only migration (re-typing published_policy.fo from long to text) applied exclusively to indices carrying the legacy ES 6-era "doc" mapping type, which cannot exist on any server the 8.x client can reach. - The elasticsearch.dsl 8.x stubs use dataclass_transform and don't surface pre-8.x-style bare `name = Text()` fields as constructor parameters; each Document/InnerDoc class now carries a TYPE_CHECKING-only `__init__(*args, **kwargs)` declaration matching the real runtime signature, which also made nine pre-existing pyright ignores unnecessary. Verified with ruff, pyright (0 errors/0 warnings), the full pytest suite (718 passed), and a CLI run over the bundled samples; CI's live elasticsearch:8.19.7 service exercises the new client end-to-end. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Use pass instead of ... in TYPE_CHECKING __init__ stubs CodeQL flags an ellipsis-only body as "Statement has no effect" (12 alerts on PR #822); pass is equivalent at runtime and to the type checker and keeps the alerts from resurfacing on every future scan. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com> |
||
|
|
01b85ab7de |
Fix aggregate-report timestamp skew on non-UTC hosts (#819) (#821)
record["interval_begin"]/["interval_end"] are UTC wall-clock strings
(produced in __init__.py by strftime() on a datetime already converted
via human_timestamp_to_datetime(..., to_utc=True)), but the
Elasticsearch and OpenSearch per-record save loops and the Splunk HEC
aggregate-report event builder re-parsed them without assume_utc=True,
so on non-UTC hosts they were misinterpreted as local time and shifted
by the host's UTC offset -- shifting the stored date_begin/date_end,
the daily/monthly index date, and the Splunk event time. Verified by
parsing a real sample report under TZ=Europe/Warsaw vs TZ=UTC and by
reproducing the exact skew via human_timestamp_to_unix_timestamp().
This is the same class of bug fixed for arrival_date_utc in #811/#812
(commit
|
||
|
|
746da77de5 |
Raise test coverage: utils.py, elastic.py, and opensearch.py to 100% (#816)
* Raise test coverage: utils, elastic, and opensearch to 100%
Coverage of the shipped library rises from 88% to 90%, with
parsedmarc/utils.py 86% -> 100% and elastic.py / opensearch.py
99% -> 100%. All new tests assert on observable behaviour and mock
only at SDK boundaries (dnspython Resolver.resolve, requests.get,
subprocess.check_call, elasticsearch_dsl/opensearchpy Document.save).
New tests cover: query_dns transient-error retries, the load_ip_db
download/cache/bundled fallback chain, the IPinfo API token probe and
per-request MMDB fallbacks, _normalize_ip_record schema handling,
reverse-DNS-map invalid-CSV fallback, caller-provided reverse DNS
maps, Outlook MSG conversion (missing msgconvert and success paths),
parse_email Cc/Bcc/attachment-hash branches, aggregate-XML edge cases
(bytes input, repeated policy_published, unknown RFC 9990 override
types, missing org_name, attribute-only <email>), extract_report on
non-seekable streams, and the _AggregateReportDoc.save() override
that derives passed_dmarc.
Bugs found by the new tests, fixed in the same PR per the testing
standards:
- parse_email() crashed with KeyError: 'Headers' on messages whose
From header is present but unparseable (e.g. a bare "From:" line):
the fallback read parsed_email["Headers"], but the parsed headers
are stored under lowercase "headers" (assigned a few lines up in
the same function), so the key never exists. At the CLI surface
this made any failure report whose embedded sample had an empty
From: header fail to parse ("Missing value: 'Headers'").
- configure_ipinfo_api(probe=True) logged "IPinfo API configured"
when the probe could not reach the API, contradicting its own
docstring ("other errors are logged and the token is still
accepted"): _ipinfo_api_lookup() returns None on network errors
instead of raising, so the probe's exception handler was
unreachable. The probe now checks the lookup result and warns on
failure; 401/403 still raises InvalidIPinfoAPIKey.
Dead code deleted rather than padded with tests:
- _SMTPTLSReportDoc.add_policy() in elastic.py and opensearch.py
(the save paths construct _SMTPTLSPolicyDoc directly).
- The no-op "for failure_index in failure_indexes: pass" loop in
both migrate_indexes() implementations (parameter still accepted).
- The importlib.resources ImportError fallback in utils.py, which
re-imported the same module and is unreachable on Python >= 3.10.
- The "Invalid report content" guard in extract_report(): every
input branch assigns file_object or raises first (confirmed by
pyright narrowing with the guard removed).
Also widens parse_aggregate_report_xml's annotation to str | bytes
to match its existing runtime behaviour (bytes are decoded with
errors ignored).
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* Use assertGreater for the reverse-DNS-map fallback size check
Addresses the github-code-quality bot finding on PR #816: assertTrue
with a comparison inside can't show the operands on failure, while
assertGreater reports both values and the failed relation. No change
to test behavior.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
---------
Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
|
||
 
|
cdda5dae62 |
Fix failure-report timestamp skew on non-UTC hosts in ES/OpenSearch/Splunk outputs (#812)
* Fix failure-report timestamp skew on non-UTC hosts in ES/OS/Splunk sinks arrival_date_utc is a UTC wall-clock string (generated in parse_failure_report via an aware-UTC strftime), but elastic.py, opensearch.py, and splunk.py parsed it back into a naive datetime and called .timestamp(), which per the Python docs interprets naive values as local time (https://docs.python.org/3/library/datetime.html#datetime.datetime.timestamp). On any non-UTC host the epoch stored as the ES/OpenSearch arrival_date field, used in the failure-report dedup match query, and sent as the Splunk HEC event time was therefore off by the host's UTC offset (verified -3600 s under TZ=Europe/Warsaw in January). Add an assume_utc keyword to human_timestamp_to_datetime() / human_timestamp_to_unix_timestamp() that attaches timezone.utc to naive parses, and use it at the three arrival_date_utc call sites. Aware inputs (explicit offsets) are unaffected; all other callers keep the existing local-time semantics, whose round-trip with timestamp_to_human is self-consistent on a single host (the broader local-time output question is tracked separately in issue #811 bug 2). The three new sink regression tests fail on the unfixed code (verified by stashing the source changes) and force TZ=Europe/Warsaw via time.tzset() so they catch the skew even on UTC CI runners. Fixes half of https://github.com/domainaware/parsedmarc/issues/811. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Deduplicate TZ-forcing test boilerplate; fix unix-timestamp docstring Extract the repeated TZ=Europe/Warsaw + time.tzset() setup/cleanup from the four timestamp regression tests into a shared tests/tzutil.py force_tz() helper, and correct human_timestamp_to_unix_timestamp()'s docstring, which said the return type was float while the function returns int. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: MISAPOR LAB <misapor@lab.misapor.pl> Co-authored-by: Claude Fable 5 <noreply@anthropic.com> Co-authored-by: Sean Whalen <44679+seanthegeek@users.noreply.github.com> |
||
|
|
08db305e5a |
test: cover no-display-name Reply-To header flattening (#786)
The 10.0.3 Reply-To header flattening (elastic.py / opensearch.py line 711)
has two branches: display-name present ("Name <addr>") and absent (bare
address). The existing test only exercised the former, leaving the
empty-display-name branch uncovered — the two lines Codecov flagged on the
10.0.3 patch. Add a failure report whose Reply-To has no display name and
assert sample.headers["reply-to"] flattens to the bare address.
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
||
|
|
e104f1118c |
Land 10.0.3 changes on master (#785)
PR #784 was stacked on the #783 branch and its base was never retargeted to master, so it merged into fix/mailsuite-2.2.1-empty-address instead of master. master therefore has 10.0.2 (#783's squash) but is missing the 10.0.3 changes. This re-lands exactly that delta — the Reply-To/Delivered-To parser fix, the ES/OS Reply-To header flattening, and the Splunk/OpenSearch/Grafana failure dashboard fixes, with the version bumped to 10.0.3. No mailsuite re-bump (the >=2.2.1 floor is already on master from 10.0.2). Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com> |
||
 DVBandGitHub
|
bf37ded688 | Add support for Elastic Cloud Serverless projects (#770) | ||
|
|
b7b8383fa4 |
Expand honest test coverage from 59% to 83%; fix two latent bugs (#775)
* Expand honest test coverage from 59% to 83%; fix two latent bugs 271 new tests across the output modules, ES/OS clients, CLI config parsing, and the top-level parsing surface. Coverage measured against shipped code only (see [tool.coverage.run] source = ["parsedmarc"] omit = ["*/parsedmarc/resources/maps/*.py"] in pyproject.toml). Per-module results: s3.py 38% → 100% (also fixes SMTP-TLS-to-S3 bug below) gelf.py 40% → 100% syslog.py 46% → 100% kafkaclient.py 34% → 100% splunk.py 24% → 100% loganalytics.py 56% → 100% webhook.py 78% → 100% (also removes redundant try/except) elastic.py 36% → 99% opensearch.py 40% → 99% cli.py 52% → 69% __init__.py 74% → 76% (also fixes append_json bug below) utils.py 84% (unchanged in this PR) TOTAL 59% → 83% The remaining 17% is honest. The biggest unreached blocks are _main() in cli.py and the watch-mode mailbox iteration in __init__.py, both of which would require either standing up live subsystems (real Elasticsearch, real IMAP) or mocking deep enough that the test would verify the mock rather than the code. The PR-A AGENTS.md guidance — "if 90% requires faking it, ship 85% honestly" — applies here. Bugs fixed while writing tests: 1. parsedmarc/s3.py — SMTP-TLS-to-S3 was completely broken. save_report_to_s3 unconditionally read report["report_metadata"] when building S3 object metadata, but RFC 8460 §4.3 SMTP TLS reports are flat (no report_metadata sub-object). The CLI's surrounding try/except silently swallowed the KeyError, so every SMTP-TLS report quietly failed to upload. Also fixes a related issue: parse_smtp_tls_report_json stores begin_date as the raw ISO-8601 string from the report (per the SMTPTLSReport TypedDict and RFC 8460 §4.3), but the S3 code path assumed a datetime with .year / .month / .day attributes. Both fixed; the broken metadata-extraction branch now uses the flat-report fields, and the date branch normalizes via human_timestamp_to_datetime. 2. parsedmarc/__init__.py — append_json corrupted JSON output files on the second write. The original implementation opened files in "a+" mode, then seek()ed backwards to overwrite the trailing "]" with ",\n" before appending more elements. Python's docs are explicit (https://docs.python.org/3/library/functions.html#open): on POSIX, writes in "a"/"a+" mode always go to EOF regardless of seek() position. The result was that the second call produced [...]\n],\n[...] -style corrupted output instead of a single merged array. Replaced with a read-merge-write pattern: load the existing array (if any), append the new elements, rewrite the whole file. The CSV cousin append_csv was not affected — it doesn't seek backwards. 3. parsedmarc/webhook.py — removed redundant try/except blocks in save_aggregate_report_to_webhook / save_failure_report_to_webhook / save_smtp_tls_report_to_webhook. _send_to_webhook already catches every Exception itself, so the outer except blocks were unreachable dead code (covered nothing, defended against nothing, and inflated the source-line count without testing value). Testing approach: mocks at SDK boundaries (boto3 resource, kafka producer, requests session, opensearch/elasticsearch Document/Search, azure LogsIngestionClient). Tests verify the parsedmarc-side transformation logic — document/event construction, index/topic naming, dedup queries, error wrapping — rather than asserting on mock invocations as a proxy for behaviour. Where a branch is defensive against a caller that doesn't exist in the codebase, the test is omitted (commented in code rather than hidden behind a pragma). 547 tests total (was 276), all passing. ruff check + format clean. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * Document the two bug fixes from this PR in the 10.0.0 changelog Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * Document testing standards in AGENTS.md Adds a "Testing standards" section covering the principles applied in PR-A (split) and PR-B (coverage expansion): - Coverage measures shipped code only — don't reintroduce tests/* to the scope, don't expand omit, don't use # pragma: no cover. - Honest tests assert on observable behaviour, not "the mock was called". Mock at SDK boundaries; parse the payload that gets sent. - "If 90% requires faking it, ship 85% honestly" — coverage is a tool, not a goal. PR-B's deliberate stops at cli.py 69% and __init__.py 76% are the documented precedent for when to halt. - Verify bug claims against the relevant RFC, internal types, installed SDK source, or upstream docs before changing code. Cite the source in the commit message and test docstring (RFC 8460 §4.3 and the Python open() docs for #775's two bug fixes are the pattern to follow). - Bugs found while writing tests are fixed in the same PR; the test doubles as the regression guard. - File layout (tests/test_<module>.py) is non-negotiable; module-level test loggers need fresh-handler setup so test ordering doesn't break assertLogs. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * Cover the corrupt-file fallback in append_json Codecov flagged 2 missing patch-coverage lines on PR #775: the except (json.JSONDecodeError, OSError) branch in append_json, which falls back to overwriting when the existing file isn't a parseable JSON array. Two new tests in tests/test_init.py:TestAppendJson exercise both paths: - test_corrupt_existing_file_is_overwritten_cleanly: existing file contains invalid JSON; append_json overwrites with the new array. - test_existing_file_with_non_list_root_is_overwritten: existing file parses as {"foo": ...} (dict, not list); the isinstance guard rejects it and we overwrite cleanly. Patch coverage now 100% on the bug fix. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com> |