From a6e6c1bbd844acb193b0224261e714c38f85cc8a Mon Sep 17 00:00:00 2001 From: Vuong Hoang Date: Wed, 20 May 2026 21:52:32 -0700 Subject: [PATCH] =?UTF-8?q?contract(issue#2):=20amend=20per=20Volva=20para?= =?UTF-8?q?phrase=20=E2=80=94=20defaults,=20query,=20metadata?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Volva's contract paraphrase round (thread 01KS4DTCW8CV) surfaced five ambiguities; three are real contract-text gaps and addressed here. 1) tags/archived/name defaulting was inconsistent across prose, INV-002, and STEP 6. open_questions said "defaulting to sensible None/empty", INV-002 said "populated from the response item shape", STEP 6 said `item.get("tags", []) if "tags" in item else None` (which collapses absent and explicit-null into the same None branch while letting an explicit [] pass through). Tightened to: tags is always list[str] defaulting to [] for absent/null/empty in list items; archived is always bool defaulting to False; name remains str | None (the only field where None is a meaningful value). create_session always sets list-only fields to their fixed defaults (name=None, archived=False, tags=[]) instead of None to keep the dataclass shape uniform. 2) The include_archived_query test said "URL has no include_archived param OR explicit false". STEP 2 prescribes "ADD include_archived='true' iff include_archived" — the OR-clause weakened the test against the prescribed behavior. Tightened to: default (include_archived=False) asserts NO include_archived param at all, not an explicit false. 5) metadata's populated semantics: INV-001 said "populated from the 201 response", STEP 5 said body.get("metadata", {}) — two valid readings (trust the spec vs defensive default). Aligned to the defensive shape: INV-001 + INV-002 now explicitly state "defaults to {} when absent" as spec-drift tolerance. Volva flags #3 (exception .body sensitivity — truncation reduces size not sensitivity) and #4 (assert for runtime validation — Python -O disables) reviewed and kept as-is. Both are intentional carryovers from issue #1's precedent: exception .body is for caller debugging bound to 1024 bytes (caller's responsibility to not log raw); assert chosen for fast-path validation, trading -O robustness for normal-mode speed. Drift check still clean — amendments don't touch the pinned issue body, so prd: hashes remain valid. --- docs/contracts/issues/2.contract.md | 34 +++++++++++++++-------------- persistent-memory.md | 1 + 2 files changed, 19 insertions(+), 16 deletions(-) diff --git a/docs/contracts/issues/2.contract.md b/docs/contracts/issues/2.contract.md index 7f2bae4..17d0762 100644 --- a/docs/contracts/issues/2.contract.md +++ b/docs/contracts/issues/2.contract.md @@ -17,7 +17,7 @@ assumptions: - "GET /sessions cursor pagination uses the `v1.` envelope (§Pagination); the consumer treats cursors as opaque strings (does not parse or construct them)." - "Bifrost binding (Worldtree issue #160) is NOT used. create_session does not accept a `bifrost` parameter and never sends one in the request body." open_questions: - - "Should SessionInfo split into two dataclasses (CreatedSessionInfo with message_count vs ListedSessionInfo with archived/tags/name) since the two endpoints return different field sets? Draft uses one SessionInfo with optional fields keyed by origin; consumers can rely on `metadata` being present in both, and `message_count`/`archived`/`tags`/`name` defaulting to sensible None/empty when not present." + - "Should SessionInfo split into two dataclasses (CreatedSessionInfo with message_count vs ListedSessionInfo with archived/tags/name)? Draft uses one SessionInfo with origin-conditional fields whose defaults are codified in INV-001 (create) and INV-002 (list). Splitting would force callers to handle two types where they currently handle one; collapsing felt right for v1 but reconsider if presenters end up branching by origin." - "Should list_sessions transparently paginate (iterate all pages) or surface one page at a time? Draft surfaces one page (SessionPage with next_cursor). Caller decides whether to iterate. Matches Worldtree's pagination idiom and lets the TUI render lazily." prd: issue: 2 @@ -56,9 +56,11 @@ Convention-aligned with issue #1: caller owns the `httpx.AsyncClient` and Author - `agent_id: str` - `created_at: str` (ISO 8601 with offset) - `last_active: str` - - `metadata: dict[str, Any]` - - `message_count: int | None` (present from POST response; None when SessionInfo was sourced from a list item) - - `name: str | None`, `archived: bool | None`, `tags: list[str] | None` (present from list items; None when sourced from POST response) + - `metadata: dict[str, Any]` (defaults to `{}` if the response omits the field — see INV-001) + - `message_count: int | None` (present from POST response; `None` when SessionInfo was sourced from a list item per spec §GET /sessions) + - `name: str | None` (always `None` when sourced from POST response; `None` if absent from list item; otherwise the list item's value) + - `archived: bool` (always `False` when sourced from POST response; defaults to `False` if absent or null in a list item; otherwise the list item's value) + - `tags: list[str]` (always `[]` when sourced from POST response; defaults to `[]` if absent or null in a list item; otherwise the list item's value) - `list_sessions` → `SessionPage`: - `items: list[SessionInfo]` - `next_cursor: str | None` (None on the last page; opaque string otherwise) @@ -67,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` fields are populated from the 201 response. `message_count` is populated from the response; list-only fields (`name`, `archived`, `tags`) are `None`. -- **INV-002 [hard]**: `list_sessions` returns a `SessionPage` where every `SessionInfo` has `session_id`, `agent_id`, `created_at`, `last_active`, `metadata`, `name`, `archived`, and `tags` populated from the response item shape. `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"]` (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-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`. @@ -124,11 +126,11 @@ STEPS: agent_id=body["agent_id"], created_at=body["created_at"], last_active=body["last_active"], - metadata=body.get("metadata", {}), + metadata=body.get("metadata", {}), # INV-001 defensive default message_count=body.get("message_count"), - name=None, - archived=None, - tags=None, + name=None, # INV-001 fixed for create-origin + archived=False, # INV-001 fixed for create-origin + tags=[], # INV-001 fixed for create-origin ) TESTS: happy_create [happy,tracer]: mock returns 201 with full body → returns SessionInfo with all create-side fields populated; list-only fields are None @@ -178,18 +180,18 @@ STEPS: agent_id=item["agent_id"], created_at=item["created_at"], last_active=item["last_active"], - metadata=item.get("metadata", {}), - message_count=None, # not in list response per spec - name=item.get("name"), - archived=item.get("archived"), - tags=item.get("tags", []) if "tags" in item else None, + 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 + tags=item.get("tags") or [], # INV-002: absent/null/[] → [] ) 7. [cleanup] RETURN SessionPage(items=infos, next_cursor=body.get("next_cursor")) TESTS: happy_first_page [happy,tracer]: GET /sessions, mock returns {items: [one full session shape], next_cursor: "v1.abc..."} → SessionPage(items=[1], next_cursor="v1.abc...") happy_last_page [happy]: mock returns {items: [...], next_cursor: null} → SessionPage with next_cursor=None empty_results [happy]: mock returns {items: [], next_cursor: null} → SessionPage([], None) - include_archived_query [trace]: include_archived=True → URL has include_archived=true; default → URL has no include_archived param OR explicit false (asserts default behavior) + include_archived_query [trace]: include_archived=True → URL has include_archived=true; default (include_archived=False) → URL has NO include_archived param at all (STEP 2 prescribes "ADD include_archived='true' iff include_archived" — the test asserts absence on default, not an explicit false) cursor_threaded [trace]: cursor="opaque-from-prev-page" → URL has cursor=opaque-from-prev-page limit_query [trace]: limit=10 → URL has limit=10 invalid_cursor_server [error]: mock returns 422 with body {"error_code":"cursor_invalid","message":"..."} → raises InvalidCursor(raw=) diff --git a/persistent-memory.md b/persistent-memory.md index 62a40b2..a2b7f78 100644 --- a/persistent-memory.md +++ b/persistent-memory.md @@ -80,6 +80,7 @@ decision. Captures rationale that won't be obvious from code alone. - `[2026-05-21]` **Default issue-tracker labels seeded** (17 total). Sleipnir gating (`ready-for-agent`, `blocked-needs-contract`, `blocked-needs-dependency`), triage (`needs-triage`, `needs-architect-decision`, `needs-info`), type (`bug`, `enhancement`, `task`, `documentation`), resolution (`duplicate`, `wontfix`, `invalid`), Ratatoskr-specific area (`sse-client`, `tui`, `cli`, `observability`). - `[2026-05-21]` **`ratatoskr.sse_client` implemented via TDD against issue #1's contract.** 37 contract-listed tests authored + GREEN per the tracer-bullet vertical-slice ordering (`_parse_sse_id` → `stream_turn` → `reconnect_turn` → `cancel_turn`). Refactor pass extracted `_iter_events` helper to dedupe INV-002 + INV-003 + terminal-break logic across `stream_turn` and `reconnect_turn`; `expected_turn_id=None` vs `expected_turn_id=N` distinguishes the two entry-point semantics Volva surfaced. Notable choices made during implementation: (a) regex `^-?\d+$` pre-check in `_parse_sse_id` to reject whitespace before `int()` (Python's `int(" 3 ")` would silently strip — this kept the strict-no-whitespace test honest); (b) `_DropAfter` AsyncByteStream subclass in tests to simulate mid-stream `RemoteProtocolError`; (c) ToolResult.result and ToolStart.arguments typed as `Any` (server JSON varies); (d) ruff line-length=100 (per pyproject) forced some test docstrings to be tighter than v0 draft. - `[2026-05-21]` **Issue #2 + contract: `ratatoskr.sessions`.** Scope is narrow — `create_session` (POST /sessions) + `list_sessions` (GET /sessions, cursor-paginated) + shared `SessionInfo` and `SessionPage` frozen dataclasses. Bundles two endpoints in one contract because they share the response envelope shape; splitting would duplicate the dataclass. Bifrost binding (Worldtree issue #160), ephemeral sessions, `GET /sessions/{id}`, `PATCH`, `DELETE`, and `GET /sessions/{id}/messages` (history) are explicitly out of scope (codified in the contract's `## Out of scope` H2 — first contract in this repo to carry that section, so future Volva consults resolve cleanly via the default path instead of needing `--out-of-scope` overrides). `prd:` pinned to issue #2 body SHA `01fbbd52b6d90eb0` at `2026-05-21T04:45:06+00:00`; drift check clean. `dependencies:` lists issue #1 as a convention-dependency (no code import; same API-consumption posture). +- `[2026-05-21]` **Volva paraphrase round on `docs/contracts/issues/2.contract.md`.** Volva flagged 5 ambiguities; operator approved amendments to 3 of them. (1) `tags`/`archived`/`name` defaulting semantics now explicit: `tags: list[str]` (default `[]`), `archived: bool` (default `False`), `name: str | None` (default `None`); INV-001/INV-002 + STEPS aligned. (2) `include_archived_query` test tightened: default → URL has NO `include_archived` param at all (was "no param OR explicit false" — softened the assertion against STEP 2's prescriptive behavior). (5) `metadata` populated-vs-defaulted slippage resolved: INV-001 + INV-002 now spell out the defensive `body.get("metadata", {})` default for spec drift tolerance. Volva flags #3 (exception `.body` sensitivity) and #4 (`assert` for runtime validation) reviewed and kept as-is — both intentional and consistent with issue #1's precedent. Drift check still clean (amendments don't touch the pinned issue body). - `[2026-05-21]` **Volva code-vs-contract review round on `ratatoskr.sse_client`.** Volva flagged 4 findings (3 drifts + 1 test-gap), all code-side "fix it" recommendations: (1) `_iter_events` fell off cleanly on EOF before terminal, violating INV-001 ("MUST NOT raise StopAsyncIteration before a terminal event arrives unless connection drops"); fix tracks `terminal_seen` flag and raises `SseConnectionDropped` on clean-EOF-without-terminal. (2) Both `SseConnectFailed.body` and `CancelFailed.body` stored full response bytes; ERROR_ROUTING specified truncation to `[:1024]`; fix truncates in `__init__` before storing. (3) `_parse_sse_id` PRE-001 specified `assert isinstance(raw, str)`, but code called `.split(":")` directly (incidental `AttributeError` on non-str); fix adds the assert. (4) Test-gap on cancel_turn's "other status → CancelFailed" branch; fix adds a 503 test with >1024-byte body that double-covers finding #2. Meta-note: Volva said TDD caught the main happy/adversarial shape; the misses were "negative space" cases (clean EOF, exception payload truncation, untested generic cancel branch) — calibration evidence that cross-model review pulls weight on the same-model author's blind spots. 43 tests GREEN post-fix (42 sse_client + 1 boundary), ruff clean. - `[2026-05-21]` **Volva paraphrase round on `docs/contracts/issues/1.contract.md`.** Volva flagged 5 ambiguities; operator approved amendments to 3 of them. (1) `reconnect_turn` STEP 2 punt resolved: signature now carries `content: str`; STEP 2 body is `json={"content": content}` matching spec §Reconnect flow example verbatim. Spec line 732 makes the agent's tools+LLM run "exactly once regardless of disconnects/reconnects" — the `content` is a wire-schema requirement, not re-processed server-side. (2) `_parse_sse_id` tightened: `turn_id ≥ 1` AND `seq ≥ 1` (was `≥ 0`); spec §SSE id format line 705 explicitly states `seq` starts at 1, and `turn_id` is SQLite autoincrement (≥1). Test `happy_zero_seq` flipped to `zero_seq [adversarial]`; new `zero_turn_id` + `negative_seq` adversarial tests added. (3) INV-003 clarified to spell out the two-entry-point semantics: `stream_turn` establishes `turn_id` from the first event (first event always yields); `reconnect_turn` parses the expected `turn_id` FROM `last_event_id` BEFORE the connection opens, so the first server event is already a flip-candidate and is NOT yielded on mismatch. Volva flags #3 (MalformedSseId-vs-ValueError split) and #5 (exactly-one-terminal as server-assumed) noted but kept as-is — deliberate distinctions. Drift check still clean against issue #1 (amending the contract doesn't touch the pinned issue body).