Repository navigation
Conversation
…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.
Owner
Author
|
Landed on master in the v0.1.0 chain: the branch was fast-forward merged as part of |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
fix(server,sdk): archiving a cell is one request, and created_at is not writable
PATCH {"lifecycle": {"status": "archived"}}was rejected.created_atwasrequired by the
MemoryLifecyclemodel shared with the create path, so a clientcould not change a status without first reading the cell and echoing the timestamp
back. Both SDKs did exactly that, on every
forget. Meanwhiledocs/api-reference.mddocumentedcreated_atas unpatchable - so the field wasmandatory and mutable, and the documentation was wrong rather than the code.
MemoryLifecycleUpdatereplaces it on the update path:created_atis absent from the model, not optional. A generated client willnot 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.
records.IMMUTABLE_PATHS), becausethe 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.
forgetis now two requests instead of three, and the lifecycle engine sendsonly the status it means to change rather than a snapshot of the timestamps it
read - which could write a stale
last_accessed_atover a newer one a GET hadjust 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, whichthe protocol permits only from
archived, so it was refused with409for everycell 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.
a side effect of
forget's round-trip; once that went, the contract test showedthe 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.pyreached the app through whatever storage aprevious 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 --strictclean, and the committed OpenAPIcontract regenerated so
MemoryLifecycleUpdateis what a generated client reads.