fix(sessions): address Volva code-vs-contract drift (issue #2)
Volva's code-spec review (thread 01KS4EKVKKGF) surfaced three findings
on the TDD-passing sessions module. All three addressed; one carries
a collateral contract amendment to keep INV-002 truthful.
1) drift: archived=item.get("archived", False) returned None for an
explicit "archived": null in the response. dict.get(k, default) only
fires the default when the key is absent — it does NOT default for
explicit-null values. The dataclass type is `bool` (not `bool | None`)
and INV-002 says explicit-null → False; the .get() form silently
violated both. Fixed: archived=item.get("archived") or False
(handles absent, null, false, and true cleanly).
INV-002 wording was the source of the bug — I introduced the
mis-spelled form during the Volva amendment round. Updated to spell
out the .get(default) foot-gun explicitly so future readers (and
future paraphrase rounds) don't fall back to the broken pattern.
2) test-gap: no test exercised explicit-null archived/tags. The
_list_item() helper had its own defaulting layer (tags=None →
["work"]) so a happy path test couldn't catch the underlying drift.
Added test_explicit_null_list_defaults using a raw dict to bypass
the helper. Catches the drift directly.
3) precision: message_count=body.get("message_count") could silently
default to None while POST-003 required it non-None. INV-001 prose
literally said "body['message_count']" (bracket access) so the
STEP 5 .get() was the contract's own internal inconsistency.
Aligned the code to bracket access (matches sibling required
fields like session_id) and amended STEP 5 + INV-001 to spell out
the strict semantics explicitly.
Volva's meta-note: "modest weight" — TDD caught the main surface;
this round caught a narrow Python .get() semantics edge that no
human reading would have spotted without explicit-null priors.
Still pulls real weight: that's the kind of bug that ships and
shows up months later when a server starts emitting null where
it used to omit a field.
63 tests GREEN (42 sse_client + 20 sessions + 1 boundary).
Ruff clean. Drift check still GREEN against the pinned issue body.
This commit is contained in:
@@ -69,8 +69,8 @@ Convention-aligned with issue #1: caller owns the `httpx.AsyncClient` and Author
|
||||
|
||||
## Invariants
|
||||
|
||||
- **INV-001 [hard]**: `create_session` returns a `SessionInfo` whose `session_id`, `agent_id`, `created_at`, `last_active`, and `metadata` are sourced from the 201 response body. `metadata` is taken from `body["metadata"]` when present and defaults to `{}` when absent (defensive against minor server-side spec drift; spec example always shows it present). `message_count` is taken from `body["message_count"]` (typically 0 for a fresh session). List-only fields are fixed: `name=None`, `archived=False`, `tags=[]`.
|
||||
- **INV-002 [hard]**: `list_sessions` returns a `SessionPage` where every `SessionInfo` has `session_id`, `agent_id`, `created_at`, `last_active`, and `metadata` from the response item (same defensive `metadata` default as INV-001). `name` is `item.get("name")` (may be `None`). `archived` is `item.get("archived", False)` (absent or explicit-null both yield `False`). `tags` is `item.get("tags") or []` (absent, explicit-null, or empty list all yield `[]`; a populated list passes through). `message_count` is `None` (the list endpoint does not include it — spec §GET /sessions: "`message_count` is not included in list items").
|
||||
- **INV-001 [hard]**: `create_session` returns a `SessionInfo` whose `session_id`, `agent_id`, `created_at`, `last_active`, and `metadata` are sourced from the 201 response body. `metadata` is taken from `body["metadata"]` when present and defaults to `{}` when absent (defensive against minor server-side spec drift; spec example always shows it present). `message_count` is taken from `body["message_count"]` (strict — bracket access, not `.get()`; the spec lists it as a response field and absent should surface as KeyError rather than silently default to None). List-only fields are fixed: `name=None`, `archived=False`, `tags=[]`.
|
||||
- **INV-002 [hard]**: `list_sessions` returns a `SessionPage` where every `SessionInfo` has `session_id`, `agent_id`, `created_at`, `last_active`, and `metadata` from the response item (same defensive `metadata` default as INV-001). `name` is `item.get("name")` (may be `None`). `archived` is `item.get("archived") or False` — absent, explicit-null, or explicit-false all yield `False`; explicit-true passes through. (Note: `item.get(key, default)` only fires `default` for absent keys, NOT for explicit-null values, so the `or False` form is load-bearing here.) `tags` is `item.get("tags") or []` (absent, explicit-null, or empty list all yield `[]`; a populated list passes through). `message_count` is `None` (the list endpoint does not include it — spec §GET /sessions: "`message_count` is not included in list items").
|
||||
- **INV-003 [hard]**: `list_sessions` treats cursors as opaque strings. The module never parses, base64-decodes, or constructs a cursor — it threads the server-provided `next_cursor` back verbatim on the next call. Per spec §Pagination ("Cursors are opaque to clients — do not parse or construct them.").
|
||||
- **INV-004 [hard]**: Both functions truncate exception `.body` payloads to `[:1024]` at construction. Matches the issue #1 precedent (`SseConnectFailed`, `CancelFailed`).
|
||||
- **INV-005 [hard]**: No `core.*` or `worldtree.*` imports. Boundary verified by `tests/test_no_worldtree_imports.py`.
|
||||
@@ -127,7 +127,7 @@ STEPS:
|
||||
created_at=body["created_at"],
|
||||
last_active=body["last_active"],
|
||||
metadata=body.get("metadata", {}), # INV-001 defensive default
|
||||
message_count=body.get("message_count"),
|
||||
message_count=body["message_count"], # INV-001/POST-003: required, never defaulted
|
||||
name=None, # INV-001 fixed for create-origin
|
||||
archived=False, # INV-001 fixed for create-origin
|
||||
tags=[], # INV-001 fixed for create-origin
|
||||
@@ -183,7 +183,7 @@ STEPS:
|
||||
metadata=item.get("metadata", {}), # INV-002 defensive default
|
||||
message_count=None, # not in list response per spec
|
||||
name=item.get("name"), # INV-002: may be None
|
||||
archived=item.get("archived", False), # INV-002: absent/null → False
|
||||
archived=item.get("archived") or False, # INV-002: absent/null/false → False (the `or` form is load-bearing — .get(k, default) does not fire default on explicit null)
|
||||
tags=item.get("tags") or [], # INV-002: absent/null/[] → []
|
||||
)
|
||||
7. [cleanup] RETURN SessionPage(items=infos, next_cursor=body.get("next_cursor"))
|
||||
|
||||
Reference in New Issue
Block a user