Skip to content

fix: resolve the eight-item review audit (13 issues, 2 breaking changes) - #36

Merged
baraline merged 21 commits into
mainfrom
fix/audit-findings
Aug 13, 2026
Merged

fix: resolve the eight-item review audit (13 issues, 2 breaking changes)#36
baraline merged 21 commits into
mainfrom
fix/audit-findings

Conversation

@baraline

@baralinebaraline commented Aug 13, 2026

Copy link
Copy Markdown
Owner

Resolves an eight-item review of the package by verifying each claim against the code
before acting on it. Every one turned out partly founded — none was fully correct as
written, and none was baseless. Two of the proposed remedies would have broken the
package.

Closes#22, #23, #24, #25, #26, #27, #28, #29, #30, #31, #32, #33, #34.
Leaves #35 open pending a decision.

Every change was test-first: a failing test, verified failing for the right reason, then
the fix. 1084 passing (was 941), 97.26% coverage, mypy strict clean, codegen in sync,
zero-warning docs build.


⚠️ Two breaking changes

1. server_timezone is now a required client argument (GLPI_SERVER_TIMEZONE for
from_env), taking an IANA name or a tzinfo.

There is deliberately no default. Against a Europe/Paris instance, assuming UTC shifts the
affected timestamps by two hours and stops them raising — a loud failure becomes a quiet
wrong answer. A name rather than a fixed offset because a name follows DST; the same
instance emits both +01:00 and +02:00. Adds tzdata on Windows, which ships no system
tz database.

It governs both directions. On read it interprets the timestamps GLPI sends bare; on
write it converts an aware datetime onto the server's clock, because GLPI discards the
offset it is sent — see the measurement below.

2. Search endpoints raise GlpiStatusError on a 4xx instead of returning [].

This reverses decision D2 of the 0.4.0 error work, which chose tolerance deliberately.
Callers relying on [] after a permission error must now catch GlpiStatusError.


Bugs fixed

#Defect
#22Every datetime write raised TypeError.model_to_payload dumped in python mode, leaving live datetime objects for json.dumps. Invisible to the suite because TransportRecorder stubs above the JSON encoder — the new test asserts encodability, not shape.
#23get_ticket_statistics silently truncated at 200 tickets on an instance whose own docstring records 59,690. One call, no loop. The two entity and one user name-resolution sites had the same shape.
#24Fenced code blocks degraded to inline <code>, tables to literal pipes, and snake_case accumulated backslashes on every read.
#25_MAX_DATETIME was naive, so to_markdown() raised on mixed-awareness timelines. Reachable with no code change — and the live probe later confirmed the mixed population is real.
#26require_response_int refused numeric-string ids, data-nested ids and Location-header creates, raising a protocol error over a usable identifier.
#27from_transport silently deleted text."use the <Enter> key""use the key". Now decided by HTML element name.
#28A 403 made a batch iterator end having yielded nothing, so the caller saw a successful empty walk.
sort="date_mod desc" — the library's own documented example — is HTTP 400. Found while running the probe. Correct syntax is field:direction; bare date_mod is accepted but sorts ascending, and order= is ignored.
GLPI discards the offset on every datetime write. Measured: 12:30:00 bare, as ...Z, and with +02:00/+09:00/-08:00/+14:00 all store 12:30 Paris. 12:30-08:00 is 21:30 there, so that write lands nine hours early with a 200. Aware values are now converted before sending.
Three construction examples had server_timezone inserted twice. A repeated keyword is a SyntaxError, so they could not be copied at all. Plus two older doc defects: stranded output inside a code-block:: python, and an import indented four spaces in a three-space block.

Features


Three findings worth reading

The obvious fix for #27 would have broken the package.from_transport is wired as a
Pydantic BeforeValidator, so it also runs on caller-authored Markdown on the way out.
Deleting the guard escapes **bold** into literal asterisks. Measured: 938 passing → 9
failing, on both the bare deletion and the proposed _looks_like_html() probe — which is
additionally a no-op on both reported symptoms.

#31's premise was largely false, and measuring said so. Rather than build the proposed
machinery, a live probe found 19 of 20 datetime fields already arrive aware. The one
straggler (KBArticle.revisions[].date) genuinely breaks — comparing it with its own
parent article raises — so the fix shipped, but scoped to what the data justified.

#35's cheap option was ruled out by measurement. The POST body is {"id", "href"}, so
obtaining a created record is a real second round trip. Had it returned the full record,
the fix would have been "stop discarding it" — no new API.

integration_tests/probe_wire_format.py is included so these are reproducible.


Reviewer notes

  • All nine skills and both guides are reconciled with this branch. Every client
    construction example — 7 across the skills, 5 across docs/user_guide.rst and
    README.md — gained the now-required server_timezone; as written they would all have
    raised TypeError. The five skills that documented the 4xx-swallowing contract at length
    now say what happens instead, while keeping the warning about the fail-open path that has
    not changed. The knowledge-base skill also carried sort="date_mod desc" — the same
    HTTP 400 the docstrings had.
  • test_every_public_method_is_named_by_some_skill caught stream_document_content
    after rebasing, and was right to. Worth knowing it exists: adding a public method without
    naming it in a SKILL.md fails the suite, and _UNDOCUMENTED is deliberately empty.
  • The offset-on-write question is now measured, and the answer was the dangerous one.
    GLPI accepts an offset and then ignores it — seven spellings of one moment, from -08:00
    to +14:00, all stored the same wall clock. It is not skipping the parse either: +99:99
    answers HTTP 500. So server_timezone did gain a second job, and mode="json" alone
    would have shipped writes that are silently wrong by up to twelve hours. Probe 3 in
    integration_tests/probe_wire_format.py reproduces it.
  • Two new structural guards, both verified by breaking them: no library module may call
    model_to_payload without the timezone, and all 77 documented Python snippets must
    compile. The second uses compile rather than ast.parse because ast.parse accepts a
    repeated keyword argument — parsing alone would have passed the file it was written to
    catch.

🤖 Generated with Claude Code

baralineand others added 18 commits August 13, 2026 09:46
model_to_payload dumped in pydantic's python mode, leaving datetime
fields as live objects in the body mapping. _execute_request hands that
mapping to httpx as json=, whose encoder is json.dumps, so every write of
a date field raised "Object of type datetime is not JSON serializable"
before the request left the process.
The suite could not see it: TransportRecorder stubs at
client._session.request, above the JSON encoder, so no unit test ever
encoded a payload. The new tests assert encodability rather than shape,
which is the property that actually holds the bug down.
Affects PostTicketTask/PatchTicketTask (planned_begin, planned_end,
date), PostFollowup, PostSolution, PostUser/PatchUser (begin_date,
end_date, substitution_*) and the KB post models.
Closes#22
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
_MAX_DATETIME was datetime.max, a naive value, and _event_sort_key
returned either it or a date_creation straight off the model. Awareness
is not uniform across a timeline -- GLPI omits the offset on some
resources and sends it on others -- so one response can carry both
spellings. Sorting then raised "can't compare offset-naive and
offset-aware datetimes", surfacing as a crash in to_markdown() rather
than as a mis-ordering.
The sentinel is now aware and the key normalises a naive date_creation
to UTC, so all three populations sort: all-naive, all-aware, and mixed.
The assumption is confined to ordering; the rendered output still prints
the value the server sent.
Closes#25
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
get_ticket_statistics fetched tickets with a single search_tickets call
at limit=200. _resource_list issues exactly one GET per call, so any
corpus larger than the page size was silently truncated and the helper
reported a plausible number that was really just the page size. The
module's own docstring records 59,690 live tickets on the target
instance, so aggregations there were wrong by three orders of magnitude.
The two entity-name resolutions and the user-name resolution had the
same shape and are paged too: a name prefix shared by more than 200
entities dropped the remainder from the OR group, and the tickets
belonging to them vanished from the aggregate with no error.
get_task_durations and get_user_activity already iterated
iter_search_tickets; this brings the rest of the module to that pattern.
The pre-existing fake_search stubs gained **kwargs because the iterator
forwards sort and fields, which a direct search_tickets call did not.
Closes#23
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
require_response_int accepted only a top-level key holding a native int,
so three shapes the server considers successful raised
GlpiProtocolError over an identifier that was right there: a numeric
string, an id nested one level under "data", and an empty 201 whose
Location header names the new resource. Each cost the caller the id of a
record that now exists, with no way to recover it.
GLPI is PHP-backed and PHP-backed APIs routinely render integers as
strings, which makes the first shape the likely one. Live runs indicate
GLPI 11 currently returns a native int, so this is hardening rather than
a break -- but it is the single point through which all 12 create_*
methods and link_ticket_timeline_document pass.
bool stays rejected (it is an int subclass in Python) and floats stay
rejected: a fractional id is a misread payload, not a value to round.
Closes#26
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
from_transport routed content to markdownify whenever it contained both
"<" and ">". That guard is not an HTML test, and it deleted text: "use
the <Enter> key" became "use the key", "cmd </dev/null > out" became
"cmd out", "if x<y then z>0" became "if x0". An unknown tag's markup is
dropped while its empty body is kept, so the token vanished from the
middle of a sentence with nothing left to show it existed.
_looks_like_html now decides on the tag *name*, against the HTML5
element set, and requires "<" to abut the name the way a parser does.
Arithmetic ("2 < 3 > 1", "x <= y") never reaches the HTML path at all.
The guard could not simply be removed. from_transport is wired as a
BeforeValidator on the content fields, so it also runs on caller-authored
Markdown on the way out; routing that through markdownify escapes it and
GLPI receives literal asterisks. Both regression directions now have a
test.
a<b>c is still read as markup, and always will be: "b" is both a real
element and a plausible variable, and no probe reading the text alone can
resolve that. Recorded in the module docstring rather than papered over.
Closes#27
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
to_transport rendered with only nl2br and sane_lists, so a fenced code
block became inline <code>: the GLPI web UI showed a pasted log as one
run-on line, and the next read wrote it back as inline code, degrading a
little more on every edit. A Markdown table rendered as literal pipes.
fenced_code and tables fix both.
markdownify also escaped underscores and asterisks in prose, so every
read turned snake_case into snake\_case and the backslash accumulated
across read-modify-write cycles. Both escapes are off now.
A language tag is still lost -- markdownify drops the
class="language-python" that fenced_code emits -- which is a limitation
of the library pair rather than of the extension list. Recorded next to
the list instead of left to be rediscovered.
Closes#24
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The only round-trip assertion was one hand-picked single-line string,
which structurally could not exercise any of the module's losses. This
adds a 21-case corpus.
It is an inventory, not a property test, because
from_transport(to_transport(m)) == m does not hold universally and
cannot: markdownify and python-markdown disagree about nested-list
indentation and about the language class on a fence, and neither has an
option that resolves it.
The four known losses carry xfail(strict=True) so fixing one turns into
an XPASS and fails the suite. That is the point -- the inventory has to
be updated deliberately rather than drifting out of date, and a
regression in any of the 17 passing cases fails immediately.
Refs #32
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
search_tickets and its six siblings pass no failure_message to
_resource_list, so a 400/401/403/404 is logged and returns []. That is a
deliberate decision (0.4.0 plan-1, D2), not an oversight, and reversing
it would flip seven endpoints from "return []" to "raise" -- so this
records the contract rather than changing it.
Verified against a mock transport: 400, 401, 403 and 404 all return [];
500 raises GlpiServerError. The issue's claim that 5xx is swallowed too
was wrong.
The consequence worth documenting is that it does not compose with the
batch iterators: they stop on a page shorter than batch_size, so a 403 on
page one ends the walk having yielded nothing and the caller sees a
successful empty result. Noted on the three iterators and on the shared
helper, which is where someone reading an empty result would look.
Refs #28
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ations
Three of the seven searchable resources had a batch iterator; four did
not, so callers of those four drove the start cursor by hand. The four
new helpers copy the existing loop and forward their own resource's
options (sort and language for the two knowledge base resources, nothing
extra for documents and locations).
Duplicating the eight-line loop is deliberate. Factoring it onto
TransportMixin was tried: the Callable[..., Awaitable[...]] spelling
generates type-broken sync code, and the Protocol alternative only
type-checks with an invariant TypeVar. The loop was already written
three times; four more copies beat a generic indirection here.
Also corrects a skill claim that was already false before this change:
glpi-ticket-workflow said "There is no batch iterator" while
iter_search_tickets has existed for some time.
The knowledge base and plugin-fields skills live only on
docs/glpi-skills-refresh, which is not merged, so the two new KB
iterators still need a mention there when that branch lands.
Closes#29
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
download_document_content returns response.content after a
non-streaming dispatch, so httpx materialises the entire body first: a
500 MB attachment costs 500 MB of process memory even when the caller
only writes it straight to disk. stream_document_content yields it in
chunks instead.
Three things this needed that are worth recording, because each fails
quietly rather than loudly:
- "aiter_bytes": "iter_bytes" in TOKEN_REPLACEMENTS. async with, async
for and AsyncIterator are all handled by unasync already; aiter_bytes
is not, and httpx defines both readers on one Response class, so the
un-rewritten sync twin fails at *iteration* with "'async_generator'
object is not iterable" rather than at the call.
- The matching _INTENTIONAL_RENAMES entry, in the same commit, or the
codegen collision guard flags it.
- A .stream stub in test_method_invocation's _install_stub. Streaming
does not route through session.request on either surface, so the stub
seam missed it entirely: the public-surface test failed "made no HTTP
call" while trying to open a real socket to the fake host. Written by
hand for each surface since that file is not unasync-generated.
The status is checked inside the context manager, after reading the
body: the error helpers format response.text, and reading text off an
unread stream raises instead of reporting the status. No @Retry here --
tenacity does not wrap an async generator and would degrade to the sync
path silently.
Upload still buffers; that is a separate change.
Closes#30
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The date_creation window was concatenated by hand at three sites in
_statistics.py. glpi_python_client.rsql now owns the grammar:
created_between, date_window and changed_since, exported from the package
root.
Retargeted from the originally proposed changed_since(date_mod): this
codebase never filters on date_mod -- it appears only as a sort value --
so that builder would have deduplicated nothing. All three real sites
build a date_creation window, so that is the one that pays for itself
today. changed_since ships too, for incremental sync, but as a
forward-looking helper with no current in-repo caller.
The module is tree-neutral rather than living beside the six internal
composition helpers, which are under _async/ and therefore duplicated
into the generated tree -- a public API there could not be exported
without picking a tree. GlpiEnum.rsql_equals is the precedent.
The builders validate rather than concatenate, because GLPI v2 fails open
on both counts: an unparsable bound produces an expression the server
ignores, and an ignored filter returns the whole table with a 200. A
reversed window is rejected for the same reason -- GLPI answers it with
zero rows, which reads as "nothing matched" rather than "your dates are
backwards".
_filters.py's module docstring now records both fail-open behaviours and
the object-vs-array join asymmetry, which were documented only in
_statistics.py comments -- nowhere a filter author would look.
Closes#34
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Every consumer resolving a person by e-mail was inventing its own
approach. This gives them one implementation whose cost and limits are
stated rather than discovered.
It scans, and that is not a shortcut taken for expedience. GLPI exposes
addresses as User.emails, a nested array, and the v2 filter engine cannot
join a nested array: the structurally identical Ticket.team answers HTTP
500 for its declared subfields and is silently ignored for every other
spelling. So there is no server-side e-mail filter to use.
A guessed server-side fast path is deliberately not attempted, because
the failure mode is invisible. v2 ignores a filter field it does not
recognise and answers 200 with the whole unfiltered table, so a wrong
e-mail filter returns a plausible non-empty page whose first row is
somebody else. The documented "empty is not proof of absence" guard does
not help -- that failure is not empty. Adding an RSQL fast path needs a
live instance and a check that it returns FEWER rows than the unfiltered
baseline, not merely that it returns rows.
skip_entity defaults to True: a user outside the client's configured
entity is invisible otherwise, and the helper would answer None for
somebody who exists.
Closes#33
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
BREAKING: the seven search_* helpers passed no failure_message to
_resource_list, which skipped the status check entirely, so a 400, 401,
403 or 404 came back as an empty list -- indistinguishable from a filter
that legitimately matched nothing. 5xx already raised.
The failure mode that decides it is the batch iterators. They stop on a
page shorter than batch_size, so a 403 on page one ended the walk having
yielded nothing and the caller saw a *successful* empty result. Combined
with v2's other fail-open behaviour -- an unknown filter field is ignored
and the whole table is returned -- the library had two silent wrong
answers in opposite directions and neither raised.
This reverses decision D2 of the 0.4.0 error work, which chose tolerance
deliberately. The test that pinned the old behaviour is replaced by its
inverse, plus a guard that a 200 carrying an empty list is still an
ordinary empty result, and one that an iterator surfaces the error rather
than ending quietly.
Callers relying on [] after a permission error must now catch
GlpiStatusError.
Closes#28
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Both #31 and #35 turn on what GLPI 11 actually puts on the wire, which
this repository cannot answer from inside.
#31 assumes datetimes arrive without an offset. That is unproven, and the
project's own fixtures disagree with each other: the knowledge base tests
use +00:00 (already parsing aware) while the timeline, management and
administration tests use bare timestamps. If the live server sends an
offset, #31 closes unbuilt.
#35 assumes a POST returns only an id. _resource_create parses the whole
body and keeps one integer, so if the body carries the full record the
fix is to stop discarding it rather than to add a second request behind a
create_*_and_fetch helper.
The probe is read-mostly: it creates one ticket to observe a create
response and deletes it in a finally. Credentials load exactly as the
integration suite loads them.
Refs #31, #35
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The instance lives on an internal .local name, so running the probe off
the corporate VPN failed during DNS with a forty-line httpx traceback
ending in "getaddrinfo failed" -- which reads as a bug in the probe
rather than a missing network.
It now resolves the host first and, when that fails, distinguishes the
two cases: public DNS also down (no connectivity) versus public DNS fine
but the internal domain unresolvable (not on the VPN). The second prints
one line naming the fix.
The same check would help anyone running the integration suite, which
fails identically for the same reason.
Refs #31, #35
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Three docstrings gave "date_mod desc" / "name asc" as the example sort
value. Measured against live GLPI 11, that is HTTP 400 "Invalid property
for sorting: date_mod desc" -- the documented example never worked.
The correct spelling is field:direction. Also measured, because both are
easy to get wrong in the quiet direction: a bare `field` is accepted but
sorts *ascending*, and a separate `order` parameter is ignored, so
`sort=date_mod&order=desc` silently returns oldest-first.
sort='date_mod desc' -> 400
sort='-date_mod' -> 400
sort='date_mod,desc' -> 400
sort='date_mod' -> 206, ASCENDING (2018 first)
sort='date_mod:desc' -> 206, correct (2026 first)
The unit tests asserted the broken spelling was forwarded verbatim. What
they are really pinning is that `sort` reaches the query string, so they
now pin a spelling the server accepts.
Note this got worse before it got better: until the 4xx flip in d4095de
a caller following the docs got a silent [], not an error.
Also fixes the probe's own two bugs found running it live -- it sent
Content-Type: application/json on GETs, which makes GLPI parse the absent
body and answer 400 "Contenu du JSON invalide", and it used
client.delete(json=...), which httpx does not accept, so its cleanup
crashed and left the test ticket behind.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… bare
BREAKING: `server_timezone` is now a required client argument, read from
GLPI_SERVER_TIMEZONE by from_env. It takes an IANA name ("Europe/Paris")
or a tzinfo.
Measured against a live GLPI 11 instance, 19 of the 20 datetime fields
across every resource arrive with the correct historical offset. One does
not: KBArticle.revisions[].date. So a single response carries both kinds,
and comparing them raises "can't compare offset-naive and offset-aware
datetimes" -- sorting an article's revision history against the article's
own dates was enough to hit it.
There is deliberately no default. Every candidate is wrong somewhere:
against this instance, assuming UTC shifts the affected values by two
hours AND stops them raising, which converts a loud failure into a quiet
wrong answer. Requiring the operator to declare it is the only option
that cannot be silently wrong, and GLPI does not advertise it anywhere.
An IANA name rather than a fixed offset because a name follows DST -- the
same instance emits +01:00 and +02:00 depending on the date, so a fixed
offset would be wrong for half the year.
Two rules keep it safe: an offset already on the wire always wins over
the configured zone, and a model validated without a context keeps its
naive values instead of being stamped with a guess.
The zone is threaded through model_from_payload as a pydantic validation
context, which reaches nested submodels -- necessary, since the naive
field is nested. The three model_validate calls in plugins/_fields.py
that bypassed the helper now go through it, so the choke point is real
rather than nearly-real.
Adds tzdata on Windows, which ships no system tz database; without it
zoneinfo resolves in Linux CI and raises on a developer machine.
Closes#31
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The knowledge-base and plugin-fields skills landed on main after this
branch was cut, so they never saw its changes. Reconciled, along with
three claims elsewhere that the branch had made false:
- Every client construction example -- 7 across the skills, 5 across
docs/user_guide.rst and README.md -- gained the now-required
server_timezone. They would all have raised TypeError as written.
- The knowledge-base skill's search example used sort="date_mod desc",
which is HTTP 400 against a live instance. The same bug the three
docstrings carried.
- The 4xx-swallowing contract is documented in five skills, at length,
because it was a genuine trap. It is now reversed, so each says what
happens instead -- while keeping the warning about the fail-open path
that has NOT changed: v2 still ignores an unknown filter field and
answers 200 with the whole table, which no status check can catch.
- The knowledge-base skill gained iter_search_kb_articles and
iter_search_kb_categories; document-workflow gained
stream_document_content and iter_search_documents.
stream_document_content is what test_every_public_method_is_named_by_some_skill
flagged after the rebase -- a guard that landed on main in 08fb8df and
that I had wrongly reported as not existing, having read it from a stale
checkout.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@codecov-commenter

codecov-commenter commented Aug 13, 2026

Copy link
Copy Markdown

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

❌ Patch coverage is 97.33333% with 6 lines in your changes missing coverage. Please review.
✅ Project coverage is 97.24%. Comparing base (713c540) to head (997b0c3).

Files with missing linesPatch %Lines
glpi_python_client/models/_base.py92.59%2 Missing ⚠️
...pi_python_client/_async/clients/commons/_config.py93.75%1 Missing ⚠️
glpi_python_client/_async/clients/commons/_http.py96.42%1 Missing ⚠️
...python_client/_async/clients/commons/_transport.py95.83%1 Missing ⚠️
glpi_python_client/rsql.py96.55%1 Missing ⚠️
❗ Your organization needs to install the Codecov GitHub app to enable full functionality.
Additional details and impacted files
@@ Coverage Diff @@## main #36 +/- ##
==========================================
+ Coverage 97.10% 97.24% +0.13% 
==========================================
Files 79 80 +1 Lines 2422 2609 +187 ==========================================
+ Hits 2352 2537 +185 - Misses 70 72 +2 

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

baralineand others added 3 commits August 13, 2026 10:31
GLPI 11 does not read the offset it sends. Measured on preprod against one
ticket field, `12:30:00` written bare, as `...Z`, and with `+02:00`,
`+09:00`, `-08:00` and `+14:00` all store the same moment -- 12:30 Paris.
The server takes the naive prefix, interprets it in its own timezone, and
discards the rest.
It is not ignoring the offset unparsed: `+99:99` answers HTTP 500. The value
is read and then thrown away, which is the bad half of both worlds -- a
malformed offset crashes, and a well-formed one is silently wrong. Writing
`12:30-08:00` (21:30 in Paris) stores 12:30: nine hours early, with a 200 and
nothing in the response that looks off.
This is the second job the earlier `mode="json"` change implied but did not
do. That fix stopped every datetime write raising TypeError, but rendering an
aware value *with* its offset is only correct if someone reads it. So the
offset is now spent converting the value onto the server's clock and then
dropped, via a serialisation context mirroring the validation context already
threaded for the inbound half. Naive values are untouched -- they already mean
the server's clock -- and no context means no conversion, so a model dumped
outside the client is unchanged.
`test_model_to_payload_preserves_aware_datetime_offset` asserted the
behaviour this replaces. It was reasonable when written; the measurement is
what makes preserving an offset the wrong goal, so it is replaced by its
inverse rather than deleted.
Writes now go through `TransportMixin._body`, which binds the client's
timezone once, and an audit test fails if any library module calls
`model_to_payload` directly. That guard exists because the argument is
optional in the signature and omitting it produces no error at all -- just a
timestamp GLPI reinterprets. Both halves of the audit were verified by
breaking them.
Probe 3 in probe_wire_format.py records the measurement.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The snippets are the package's primary teaching surface -- nine SKILL.md
files an agent reads before writing any GLPI code -- and nothing checked
them. This adds a guard that compiles all 77.
It was written after finding that the previous commit's own sweep, which
added `server_timezone` to twelve construction examples, had left three of
them with the argument inserted twice and misindented. `GlpiClient(...)` with
a keyword repeated is a SyntaxError, so those three examples could not be
copied at all, and reviewing the diff by eye did not catch it.
Two further defects the guard found, both predating that sweep:
- user_guide.rst had three lines of expected output stranded inside a
`code-block:: python`. They are the last three prints of the example
further up, whose "Example output" block kept only the first; they are
moved back to it.
- One import in a three-space block was indented four, so copying that
snippet raised IndentationError.
It compiles rather than parses because `ast.parse` accepts a call with the
same keyword twice -- the duplicate is only rejected when the tree is
compiled. Parsing alone would have passed the exact file this was written to
catch, which is worth stating in the module docstring since `ast.parse` is
the obvious first reach.
`PyCF_ALLOW_TOP_LEVEL_AWAIT` is set: most async examples are fragments with
no surrounding `async def`, which is how they are meant to be read. Without
it the guard rejects 28 good snippets and teaches the next author to wrap
examples in scaffolding no reader needs. A block that is illustrative rather
than runnable can opt out with `# doc: no-parse`.
`server_timezone` now governs both directions, so its parameter docstring
says what it does on write as well as on read.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@baraline
baraline merged commit f35f443 into mainAug 13, 2026
8 checks passed
@baraline
baraline deleted the fix/audit-findings branch August 13, 2026 09:15
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Writing any datetime field raises TypeError: model_to_payload dumps in python mode

2 participants

@baraline@codecov-commenter