Skip to content

fix(server,sdk): archiving a cell is one request, and created_at is not writable - #17

Closed
glatinone wants to merge 1 commit into
docs/launch-material-refreshfrom
fix/lifecycle-update-model
Closed

glatinone wants to merge 1 commit into
docs/launch-material-refreshfrom
fix/lifecycle-update-model

Conversation

@glatinone

Copy link
Copy Markdown
Owner

fix(server,sdk): archiving a cell is one request, and created_at is not writable

PATCH {"lifecycle": {"status": "archived"}} was rejected. created_at was
required by the MemoryLifecycle model shared with the create path, so a client
could not change a status without first reading the cell and echoing the timestamp
back. Both SDKs did exactly that, on every forget. Meanwhile
docs/api-reference.md documented created_at as unpatchable - so the field was
mandatory and mutable, and the documentation was wrong rather than the code.

MemoryLifecycleUpdate replaces it on the update path:

  • created_at is absent from the model, not optional. A generated client will
    not offer it, and an extra field in a request body is ignored rather than
    applied. It is the anchor the decay formula measures a cell's age from, so a
    client able to rewrite it could reset that age and defeat the decay the whole
    lifecycle runs on - which is what the documentation claimed all along.
  • The same rule lives in the storage layer (records.IMMUTABLE_PATHS), because
    the adapters accept a raw dict as well as a model and the dict path never sees a
    model. Paths rather than top-level names: the rule is about a field, not its
    depth. The test that proves it exercises the dict path, which is where the
    protection was missing.
  • forget is now two requests instead of three, and the lifecycle engine sends
    only the status it means to change rather than a snapshot of the timestamps it
    read - which could write a stale last_accessed_at over a newer one a GET had
    just set.

Two real bugs surfaced while removing the workaround, both of which the tests were
hiding rather than catching:

  • AsyncAMPClient.forget() archived nothing. It went straight to DELETE, which
    the protocol permits only from archived, so it was refused with 409 for every
    cell a caller would want to forget. Its test mocked the DELETE and never modelled
    the precondition. The test now asserts the order (PATCH, then DELETE) and the
    archiving body, because that order is the requirement.
  • Neither SDK could read a single cell. The GET route was only ever reached as
    a side effect of forget's round-trip; once that went, the contract test showed
    the client's coverage of the API was accidental. Reading is what resets a cell's
    decay clock server-side, so both SDKs gained get_memory / getMemory.

Also: tests/test_memory_crud.py reached the app through whatever storage a
previous test file had left on the module - it passed as part of the whole suite
and failed the moment it ran alone, which is how a single file is run while working
on it. An autouse fixture gives every test in the file its own state.

Verified: 244 server tests, 34 Python SDK tests, 24 Node tests, ruff/format/mypy
clean on every package, mkdocs build --strict clean, and the committed OpenAPI
contract regenerated so MemoryLifecycleUpdate is what a generated client reads.

…ot writable

`PATCH {"lifecycle": {"status": "archived"}}` was rejected. `created_at` was
required by the `MemoryLifecycle` model shared with the create path, so a client
could not change a status without first reading the cell and echoing the timestamp
back. Both SDKs did exactly that, on every `forget`. Meanwhile
`docs/api-reference.md` documented `created_at` as unpatchable - so the field was
mandatory *and* mutable, and the documentation was wrong rather than the code.

`MemoryLifecycleUpdate` replaces it on the update path:

- **`created_at` is absent from the model, not optional.** A generated client will
  not offer it, and an extra field in a request body is ignored rather than
  applied. It is the anchor the decay formula measures a cell's age from, so a
  client able to rewrite it could reset that age and defeat the decay the whole
  lifecycle runs on - which is what the documentation claimed all along.
- **The same rule lives in the storage layer** (`records.IMMUTABLE_PATHS`), because
  the adapters accept a raw dict as well as a model and the dict path never sees a
  model. Paths rather than top-level names: the rule is about a field, not its
  depth. The test that proves it exercises the dict path, which is where the
  protection was missing.
- **`forget` is now two requests instead of three**, and the lifecycle engine sends
  only the status it means to change rather than a snapshot of the timestamps it
  read - which could write a stale `last_accessed_at` over a newer one a GET had
  just set.

Two real bugs surfaced while removing the workaround, both of which the tests were
hiding rather than catching:

- **`AsyncAMPClient.forget()` archived nothing.** It went straight to DELETE, which
  the protocol permits only from `archived`, so it was refused with `409` for every
  cell a caller would want to forget. Its test mocked the DELETE and never modelled
  the precondition. The test now asserts the order (PATCH, then DELETE) and the
  archiving body, because that order *is* the requirement.
- **Neither SDK could read a single cell.** The GET route was only ever reached as
  a side effect of `forget`'s round-trip; once that went, the contract test showed
  the client's coverage of the API was accidental. Reading is what resets a cell's
  decay clock server-side, so both SDKs gained `get_memory` / `getMemory`.

Also: `tests/test_memory_crud.py` reached the app through whatever storage a
*previous test file* had left on the module - it passed as part of the whole suite
and failed the moment it ran alone, which is how a single file is run while working
on it. An autouse fixture gives every test in the file its own state.

Verified: 244 server tests, 34 Python SDK tests, 24 Node tests, ruff/format/mypy
clean on every package, `mkdocs build --strict` clean, and the committed OpenAPI
contract regenerated so `MemoryLifecycleUpdate` is what a generated client reads.
@glatinone

Copy link
Copy Markdown
Owner Author

Landed on master in the v0.1.0 chain: the branch was fast-forward merged as part of b940905..92b88ee and released as v0.1.0. Closing so the open list matches reality - the commits are in master, and the tag points at them.

@glatinone glatinone closed this Oct 4, 2026
Sign up for free to 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.

1 participant