feat(examples): S3, Redis, Postgres SessionStore reference adapters#842
feat(examples): S3, Redis, Postgres SessionStore reference adapters#842
Conversation
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
No bugs found, but this is a ~2,800-line PR introducing three new reference adapters (including a brand-new Postgres design with no TS equivalent) plus production guidance docs that users will copy verbatim — worth a human pass on the key schemes, schema, and README before it lands.
Extended reasoning...
Overview
This PR adds three reference SessionStore adapters under examples/session_stores/ (S3, Redis, Postgres), a 400-line README with production guidance, five new test modules (~1,500 lines), an optional [examples] dependency group in pyproject.toml, and a .gitignore tweak for .env files. None of this ships in the wheel — it's copy-in reference code — but it's ~2,800 net new lines and establishes patterns users are explicitly told to copy into production.
Security risks
Low and well-handled. The Postgres adapter interpolates the table name into SQL but validates it against ^[A-Za-z_][A-Za-z0-9_]*$ in __init__ and rejects anything else. The S3 adapter filters .././empty segments out of list_subkeys() as defense-in-depth against traversal. Live-test credentials come from env vars only; .env is now gitignored. I didn't spot injection, auth, or data-exposure issues.
Level of scrutiny
Medium-high despite being "just examples." The README explicitly positions these as starting points users will run in production, and the Postgres adapter is net-new design (no TS equivalent to mirror). Decisions like the jsonb-vs-json column choice, the bigserial ordering guarantee under concurrent inserts, the S3 part-file monotonic-ms scheme, and the Redis key layout are all things a maintainer should sign off on once, since changing them later is a migration for anyone who copied the adapter.
Other factors
Test coverage is thorough (conformance harness + unit tests + env-gated live suites, all importorskip-guarded so default CI is unaffected). The PR is stacked on #837. No prior reviews or comments on the timeline. The bug-hunting system found nothing. This is well-crafted code, but the size and the number of one-way-door design choices put it outside what I'd approve without a human look.
qing-ant
force-pushed
the
qing/example-session-stores
branch
from
b0065d2 to
4677449
Compare
qing-ant
force-pushed
the
qing/session-store-types
branch
from
e4b487f to
1bdc8c4
Compare
There was a problem hiding this comment.
No issues found, but this adds three substantial reference adapters (~2.8k lines across S3/Redis/Postgres + tests) with non-trivial key-scheme and schema design — worth a human pass to confirm parity with the TS reference implementations and the SessionStore contract from #837.
Extended reasoning...
Overview
This PR adds three copy-in SessionStore reference adapters under examples/session_stores/ (S3 via boto3, Redis via redis-py, Postgres via asyncpg), a 400-line README, five test modules (moto/fakeredis unit tests + env-gated live e2e suites), an optional [examples] dependency group in pyproject.toml, and .env* entries in .gitignore. Roughly 2,800 lines of net-new code. None of it ships in the wheel (hatch only packages src/claude_agent_sdk), and all test modules importorskip on their backend clients so default CI is unaffected.
Security risks
Low. The Postgres adapter f-string-interpolates the table name into SQL but guards it with a strict ^[A-Za-z_][A-Za-z0-9_]*$ regex in __init__, which is the right pattern for non-parameterizable identifiers. The S3 adapter filters .././'' segments from list_subkeys as defense-in-depth against traversal. .gitignore now excludes .env/.env.*. No credentials or internal references in the diff. Live tests read endpoints/secrets only from env vars and skip otherwise.
Level of scrutiny
Medium. Although this is example code (not shipped, explicitly labeled "reference, not production"), it is documentation-by-implementation that users will copy verbatim into production systems. The key schemes (S3 part-file naming with monotonic ms ordering, Redis __sessions/__subkeys sentinels, Postgres bigserial + jsonb), cascade-delete semantics, and the JSONB key-reordering caveat all encode design decisions that should be sanity-checked by someone familiar with the SessionStore contract introduced in #837 and the TS adapters this claims to mirror.
Other factors
The PR is stacked on #837 (the protocol it implements), so correctness depends on that base. Test coverage looks thorough — each adapter runs the 13-contract conformance harness plus a full mirror→resume round-trip — but the size and the number of independent design choices (three different storage models) make this unsuitable for bot-only approval.
qing-ant
force-pushed
the
qing/example-session-stores
branch
from
4677449 to
ec37075
Compare
There was a problem hiding this comment.
No issues found, but this is a large addition (~2.8k lines across three backend adapters with non-trivial design choices around key schemes, SQL identifier handling, and ordering guarantees) — worth a human pass even though it's example code that isn't shipped in the wheel.
Extended reasoning...
Overview
This PR adds three reference SessionStore adapters (S3, Redis, Postgres) under examples/session_stores/, plus a 400-line README, five test modules (~1.5k lines), an optional [examples] dependency group in pyproject.toml, and .env* entries in .gitignore. The adapters are copy-in references not packaged in the wheel; tests importorskip on backend clients and live suites are env-gated.
Security risks
The Postgres adapter interpolates the table name directly into SQL (asyncpg can't parameterize identifiers) but guards it with a strict [A-Za-z_][A-Za-z0-9_]* regex in __init__, which looks correct. The S3 adapter filters .././```` segments from list_subkeys as defense-in-depth against path traversal. The `.gitignore` addition for `.env`/`.env.*` is appropriate given the new live-test credential pattern. I didn't spot injection, traversal, or credential-leak issues, but the SQL-interpolation pattern and the fact that users will copy these into production warrant a second set of eyes.
Level of scrutiny
Medium-high. While the code lives in examples/ and isn't shipped, these are explicitly positioned as reference implementations users will copy into production. Design choices (S3 part-file naming and clock-based ordering, Redis key scheme with reserved sentinels, Postgres JSONB key-reordering semantics, bigserial ordering via unnest WITH ORDINALITY) are documented but non-trivial and merit human review for correctness and API ergonomics.
Other factors
The PR is stacked on #837 and replaces a mis-merged #841. Test coverage is thorough (conformance harness, unit tests, round-trip integration, env-gated live suites). The bug-hunting system found no issues. The sheer size (~850 lines of adapter code, ~1.5k lines of tests, 400-line README) and the number of independent design decisions put this well outside the simple/mechanical bar for auto-approval.
qing-ant
force-pushed
the
qing/example-session-stores
branch
from
ec37075 to
d04ca6a
Compare
There was a problem hiding this comment.
I didn't find any bugs, but this is ~2.8k lines of new reference adapters across three backends (S3/Redis/Postgres) with non-trivial design choices (key schemes, schema, ordering semantics) plus a new optional dep group — worth a human pass before it lands as the canonical reference.
Extended reasoning...
Overview
This PR adds three reference SessionStore adapters under examples/session_stores/ (S3, Redis, Postgres), a 400-line README, five test modules (~1.5k lines), an optional [examples] dependency group in pyproject.toml, and .env* entries in .gitignore. Everything is additive — no existing SDK code is modified — and the adapters are explicitly excluded from the wheel.
Security risks
Low. The Postgres adapter interpolates the table name into SQL but guards it with a strict [A-Za-z_][A-Za-z0-9_]* regex; all values use asyncpg parameter binding. The S3 list_subkeys filters .././empty path segments as defense-in-depth. .gitignore now excludes .env files. No secrets or credentials in the diff. Tests that touch the home directory monkeypatch Path.home and unset auth env vars.
Level of scrutiny
Moderate-to-high. While these are "just examples" not shipped in the package, they're explicitly positioned as copy-in reference implementations that users will run against production S3/Redis/Postgres. The key schemes, part-file ordering guarantees, JSONB caveats, and cascade-delete semantics are design decisions a maintainer should sign off on, since they'll be hard to change once users have copied them. The PR is also stacked on #837, so review depends on that base landing in its current shape.
Other factors
The bug-hunting system found no issues. Test coverage is thorough (conformance harness + unit + round-trip + env-gated live), and all new test modules importorskip so default CI is unaffected. Code quality looks high. The size alone (~2.8k added lines, three distinct backend integrations) and the user-facing reference nature put this outside the "obvious/mechanical" bar for auto-approval.
qing-ant
force-pushed
the
qing/example-session-stores
branch
from
d04ca6a to
01f7ecd
Compare
There was a problem hiding this comment.
No bugs found, but this is a ~2,800-line addition with three new reference implementations (including a novel Postgres adapter with no TS equivalent) that users will copy into production code, so it warrants a human pass on the design and key/schema choices.
Extended reasoning...
Overview
This PR adds three reference SessionStore adapters under examples/session_stores/ (S3 via boto3, Redis via redis.asyncio, Postgres via asyncpg), a 400-line README, five test modules (moto/fakeredis-backed unit tests plus env-gated live e2e suites), an optional [examples] dependency group in pyproject.toml, and .env* entries in .gitignore. Roughly 2,800 net new lines. It is stacked on #837.
Security risks
Low-to-moderate and apparently well-handled: the Postgres adapter f-string-interpolates the table name into SQL but validates it against ^[A-Za-z_][A-Za-z0-9_]*$ in __init__; all values go through asyncpg parameter binding. The S3 list_subkeys filters .././empty path segments as defense-in-depth against traversal when subkeys are later materialized to disk. Live-test credentials are read from env only, and .env* is now gitignored. I didn't spot injection, secret leakage, or auth issues.
Level of scrutiny
Medium-high. Although this is example code that is not shipped in the wheel and the tests importorskip so default CI is unaffected, these adapters are explicitly intended to be copied into users' production projects. The S3 and Redis adapters mirror existing TypeScript references, but the Postgres adapter is brand-new design work (schema, bigserial ordering, jsonb key-reordering caveat, partial index). Key-scheme and schema choices here will propagate into user codebases, so a maintainer should sign off on them.
Other factors
The PR is well-tested (conformance harness + round-trip + live e2e for all three backends), thoroughly documented, and the bug-hunting system found nothing. The blockers for auto-approval are purely size and design scope: ~2,800 lines of new code, a stacked dependency on #837, and novel architectural decisions in the Postgres adapter. This is well beyond the "simple, mechanical, or obvious" bar for shadow approval.
qing-ant
force-pushed
the
qing/example-session-stores
branch
from
01f7ecd to
b5cae13
Compare
There was a problem hiding this comment.
No issues found, but this is a substantial feature PR (~2k lines of new adapter code, tests, and docs across three backends) with design decisions around key schemes, schema layout, and ordering semantics — worth a human pass, especially since it's stacked on #837.
Extended reasoning...
Overview
This PR adds three reference SessionStore adapters (S3, Redis, Postgres) under examples/session_stores/, each with extensive docstrings and a 400-line README. It also adds ~1,500 lines of tests (moto/fakeredis-backed unit tests plus env-gated live e2e suites), an empty __init__.py, a new [examples] optional dependency group in pyproject.toml, and .env/.env.* entries in .gitignore. The adapters are not packaged in the wheel — they're copy-in reference code — but the tests live in tests/ and run in CI when the deps are installed.
Security risks
I looked specifically at the SQL-identifier interpolation in the Postgres adapter (table name is f-string'd into DDL/DML) — it's guarded by a strict ^[A-Za-z_][A-Za-z0-9_]*$ regex in __init__, which is a sound pattern. The S3 adapter filters .. / . / empty segments out of list_subkeys as defense-in-depth against path traversal. Adding .env* to .gitignore is a positive change. No credentials or internal references in the diff. I don't see meaningful security risk here.
Level of scrutiny
Although this is example code (not shipped in the wheel), it's explicitly positioned as the reference users will copy into production, and it encodes a number of non-trivial design choices: S3 part-file naming/monotonic-ms ordering, Redis key scheme with reserved __sessions/__subkeys sentinels, Postgres jsonb + bigserial schema, cascade-vs-targeted delete semantics, and prefix normalization rules. These deserve a maintainer's eye to confirm they match the TS reference implementations and the SessionStore contract intent.
Other factors
The PR is stacked on #837, so the base branch itself is unmerged. Test coverage is thorough (conformance harness + unit + round-trip + live), and the bug-hunting pass found nothing. The .gitignore and pyproject.toml changes are minimal and safe. Net: this looks well-crafted, but its size and the number of embedded design decisions put it outside the "simple/mechanical" bar for auto-approval.
qing-ant
force-pushed
the
qing/example-session-stores
branch
from
b5cae13 to
02ab224
Compare
There was a problem hiding this comment.
I didn't find any bugs, but this is a large PR (~2k lines) introducing three new storage-backend reference implementations with non-trivial design choices (S3 part-file ordering, Redis key scheming, Postgres jsonb/identifier handling) that users will copy into production — worth a human pass on the overall design and the stacked dependency on #837.
Extended reasoning...
Overview
This PR adds three copy-in SessionStore reference adapters under examples/session_stores/ (S3 via boto3, Redis via redis.asyncio, Postgres via asyncpg), each with a full conformance + round-trip test module, env-gated live e2e suites, a detailed README with production checklist, an optional [examples] dependency group in pyproject.toml, and .env* entries in .gitignore. It is stacked on #837 and is roughly 2000 lines of net-new code.
Security risks
I see no concrete vulnerabilities. The Postgres adapter interpolates the table name into DDL/DML but guards it with a strict ^[A-Za-z_][A-Za-z0-9_]*$ regex check in __init__, which is the correct mitigation since asyncpg can't parameterize identifiers. The S3 list_subkeys filters .././empty path segments as defense-in-depth against traversal. The .gitignore addition for .env/.env.* is a positive hardening step. No secrets or credentials appear in the diff.
Level of scrutiny
Although this lives in examples/ and is excluded from the wheel, the README explicitly positions these as reference implementations that users will copy into production systems. That raises the bar above typical example-script scrutiny — the S3 monotonic-ms part naming, Redis reserved-sentinel key scheme, Postgres jsonb-vs-json key-ordering tradeoff, and cascade-delete semantics are all real design decisions with operational consequences that a maintainer should sign off on.
Other factors
Test coverage is thorough (moto/fakeredis in-process mocks plus env-gated live suites, all importorskip-guarded so default CI is unaffected). The bug-hunting system found nothing. However, the sheer size, the three distinct backend integrations, and the fact that this is stacked on an unmerged base (#837) all argue for a human review rather than auto-approval.
There was a problem hiding this comment.
I didn't find any bugs, but this is a ~2.8k-line PR adding three distinct backend reference adapters (S3, Redis, Postgres) with non-trivial design decisions around key schemes, ordering semantics, and SQL handling — worth a human pass on the overall design, especially the new Postgres adapter which has no TS counterpart.
Extended reasoning...
Overview
This PR adds three reference SessionStore adapters under examples/session_stores/ (S3, Redis, Postgres), a detailed README, five test modules (unit + env-gated live e2e), an optional [examples] dependency group in pyproject.toml, and .env/.env.* entries in .gitignore. ~2,800 lines added across 12 files. The adapters are not shipped in the wheel — they are copy-in reference code — but they are intended for users to drop into production projects.
Security risks
Low but non-zero. The Postgres adapter interpolates the table name into SQL (guarded by a regex [A-Za-z_][A-Za-z0-9_]*), the S3 adapter has a defensive .././`` segment filter in list_subkeys, and the Redis key scheme relies on reserved sentinel names (`__sessions`, `__subkeys`) not colliding with SDK-generated identifiers. None of these look exploitable as written, but they are exactly the kind of surface a human should sanity-check. No secrets in the diff; `.gitignore` correctly adds `.env*`.
Level of scrutiny
Medium-high. Although this lives in examples/ and is excluded from the wheel, the explicit purpose is for users to copy these files into production. The S3 and Redis adapters mirror existing TypeScript references (lower risk), but the Postgres adapter is brand-new with its own schema, ordering semantics (bigserial + unnest ... WITH ORDINALITY), and JSONB key-reordering caveats. The README also makes operational claims (IAM actions, clock-skew warnings, Redis Cluster hash-tag advice) that someone with backend familiarity should validate.
Other factors
Test coverage is thorough — all three adapters run the shipped 13-contract conformance harness, plus targeted unit tests and full mirror→resume round-trips; tests importorskip so default CI is unaffected. The PR is stacked on #837. No prior reviews or comments on the timeline. Given the size, the number of independent design decisions (part-file naming/ordering, Redis key layout, Postgres schema/index choices), and the new-from-scratch Postgres adapter, this exceeds my threshold for auto-approval even with clean bug-hunter results.
There was a problem hiding this comment.
No bugs found, but this is a substantial feature (~850 LoC across three new backend adapters + ~1500 LoC of tests, stacked on #837) with non-trivial design choices around key schemes, SQL schema, and ordering semantics — worth a human pass.
Extended reasoning...
Overview
This PR adds three reference SessionStore adapters under examples/session_stores/ (S3 via boto3, Redis via redis.asyncio, Postgres via asyncpg), a detailed README, an optional [examples] dependency group in pyproject.toml, /examples in the sdist include list, .env* in .gitignore, and five new test modules (moto/fakeredis-backed unit suites plus env-gated live e2e suites). Roughly 2,400 lines added, all new files except the three small config tweaks. It is stacked on #837, which presumably introduces the SessionStore protocol, conformance harness, and the _internal.session_resume / transcript_mirror_batcher machinery these tests exercise.
Security risks
Low but non-zero. The Postgres adapter interpolates the table name into DDL/DML; it's guarded by a strict ^[A-Za-z_][A-Za-z0-9_]*$ regex in __init__, which looks correct. The S3 adapter filters .././'' segments out of list_subkeys() as defense-in-depth against path traversal during resume materialization. The Redis adapter's : -delimited key scheme is documented as safe for SDK-generated keys but could collide if users supply arbitrary IDs containing : — this is called out in the README. No secrets in the diff; .env* is added to .gitignore. None of this ships in the wheel (sdist only), so the SDK runtime surface is unchanged.
Level of scrutiny
Medium-high. Although these are "reference" examples that users copy rather than import, they will realistically be pasted into production code paths handling conversation transcripts, so correctness matters. Each adapter encodes meaningful design decisions: S3 part-file naming and monotonic-ms ordering, Redis key scheme and index sentinels, Postgres schema (bigserial seq, '' subpath sentinel, jsonb vs json, partial index). The README also makes operational claims (IAM actions, eviction policy, pool sizing, schema divergence vs the TS SDK) that a maintainer should sanity-check.
Other factors
Test coverage is thorough — each adapter runs the 13-contract conformance harness plus targeted regression tests and a full mirror→resume round-trip, and all new test modules importorskip so default CI is unaffected. The bug-hunting pass found nothing. That said, this is far from a mechanical change: it's three independent storage implementations with subtle ordering/cascade/pagination semantics, plus packaging and docs changes, and it depends on an unmerged base PR. A human reviewer should sign off on the overall design and the public-facing README guidance.
qing-ant
force-pushed
the
qing/example-session-stores
branch
from
3947200 to
b48dabf
Compare
There was a problem hiding this comment.
No issues found, but this is a substantial PR (~2800 lines across three new reference adapters with non-trivial ordering/key-scheme/schema design choices) stacked on #837, so it warrants a human pass.
Extended reasoning...
Overview
This PR adds three copy-in SessionStore reference adapters under examples/session_stores/ (S3 via boto3, Redis via redis.asyncio, Postgres via asyncpg), a comprehensive README, an empty __init__.py, a new [examples] optional-dependency group in pyproject.toml, .env* entries in .gitignore, and five new test modules (~1500 lines) covering conformance, unit, and env-gated live e2e suites. Nothing touches src/ — the adapters are not packaged in the wheel.
Security risks
Low. The Postgres adapter interpolates the table name into SQL but guards it with a strict [A-Za-z_][A-Za-z0-9_]* regex in __init__, which is a sound injection guard for an identifier. The S3 adapter filters .././empty segments from list_subkeys as defense-in-depth against path traversal. No credentials, endpoints, or internal references appear in the diff; live tests read connection info exclusively from env vars and the .gitignore addition prevents accidental .env commits.
Level of scrutiny
Moderate-to-high despite being example code. These are explicitly positioned as reference implementations that users will copy into production projects, so correctness of the ordering guarantees (S3 monotonic-ms part naming, Redis MULTI pipelines, Postgres unnest ... WITH ORDINALITY insert), the cascade-vs-targeted delete semantics, and the key/schema designs matter. The PR is also stacked on #837, so the underlying SessionStore protocol and conformance harness it exercises are themselves still in flight.
Other factors
Test coverage is thorough (full 13-contract conformance harness against moto/fakeredis, regression tests for same-ms ordering, subpath isolation, delete cascade scope, prefix normalization, and full mirror→resume round-trips), and all new test modules importorskip so default CI is unaffected. The bug-hunting system found nothing. Still, ~850 lines of new adapter logic plus several durable design decisions (Redis sentinel key names, Postgres schema/PK shape, S3 part-file layout) are the kind of thing a maintainer should sign off on rather than a bot.
There was a problem hiding this comment.
Previous issue is fixed and no new bugs found, but this is ~2000 lines of new reference adapters with schema/key-scheme design decisions and user-facing production guidance — worth a human pass on the design choices before merge.
Extended reasoning...
Overview
This PR adds three copy-in SessionStore reference adapters under examples/session_stores/ (S3 via boto3, Redis via redis-py, Postgres via asyncpg), a 400-line README with production guidance, five test modules (~1400 lines: moto/fakeredis-backed unit tests + env-gated live e2e suites), an optional [examples] dependency group in pyproject.toml, sdist inclusion of /examples, and .env* entries in .gitignore. Nothing touches src/ — the wheel artifact is unchanged.
Security risks
Low. The adapters are example code users copy into their own projects; they are not imported by the SDK at runtime. The Postgres adapter f-string-interpolates the table name into SQL but guards it with a strict ^[A-Za-z_][A-Za-z0-9_]*$ regex in __init__ (with a test asserting rejection of "bad; DROP TABLE x"). The S3 adapter filters .././empty path segments out of list_subkeys() as defense-in-depth. The new .gitignore entries reduce risk of committing live-backend credentials. No secrets in the diff.
Level of scrutiny
Medium. Blast radius is contained (examples-only, not in the wheel, all new tests importorskip/env-skip so default CI is unaffected). However, this is ~2000 net-new lines establishing reference patterns that users are explicitly told to copy into production, including a Postgres schema, a Redis key scheme, an S3 part-file layout, and a README "Production checklist" with operational guidance (IAM actions, eviction policies, pool sizing, clock-skew caveats). These are design decisions that set precedent and deserve a human sign-off rather than bot auto-approval.
Other factors
My one prior inline finding (the isinstance(store, SessionStore) TypeError in the Postgres test) was fixed in 73406c4 and verified by the author against live Postgres; the thread is resolved. The bug-hunting pass on the current revision found nothing. Test coverage is thorough (full 13-contract conformance harness for each backend, plus mirror→resume round-trips and targeted regression tests). The PR is also stacked on #837, so merge order/base-PR review status is another reason for a human to coordinate.
qing-ant
force-pushed
the
qing/example-session-stores
branch
from
73406c4 to
d28e6c7
Compare
There was a problem hiding this comment.
Thanks for the fix in 73406c4 — the Postgres test now matches the Redis pattern and no further issues turned up. Given the scope (three new backend adapters with key/schema design choices that users will copy into production), this still warrants a human sign-off.
Extended reasoning...
Overview
This PR adds three reference SessionStore adapters under examples/session_stores/ — S3 (~410 LoC), Redis (~180 LoC), and Postgres (~260 LoC) — plus a 412-line README and five test modules (~1580 LoC). It also adds an [examples] optional-dependency group to pyproject.toml, includes /examples in the sdist, and adds .env* to .gitignore. The adapters are not packaged in the wheel; they're copy-in reference code.
Security risks
Low but non-zero. The Postgres adapter interpolates the table name into SQL and guards it with a strict [A-Za-z_][A-Za-z0-9_]* regex (validated in __init__), which is the right approach since asyncpg can't parameterize identifiers. The S3 adapter's list_subkeys filters .././empty path segments as defense-in-depth against traversal. The Redis README documents the : key-separator collision caveat. No secrets in the diff; .env* is now gitignored. Nothing here touches the SDK's auth or permission paths.
Level of scrutiny
Medium. While this is example code excluded from the wheel, the README explicitly positions these as reference implementations users will copy into production and run against real S3/Redis/Postgres. The S3 part-file naming/ordering scheme, Redis key layout, and Postgres schema (bigserial seq, jsonb, '' subpath sentinel) are all design decisions with operational implications that a human maintainer should sign off on — especially the noted divergence from the TS SDK's Postgres schema.
Other factors
The one issue I raised earlier (the isinstance(SessionStore) TypeError in the Postgres test) was fixed in 73406c4 and verified against a live container. Test coverage is thorough: all three adapters run the 13-contract conformance harness, plus targeted unit tests and mirror→resume round-trips; S3/Redis use in-process mocks (moto/fakeredis) so they exercise in default CI. The PR is stacked on #837. Overall the code looks carefully done, but at ~2800 added lines with externally-facing schema/key-layout choices, it exceeds what I'd auto-approve.
|
Verified against the
Total: 182 passed, 0 failed.
GitHub reports |
…ssage check - postgres_session_store.py + README.md: the type-hoist lives in the *_from_store read helpers (_entries_to_jsonl) protecting the SDK's lite-parse tag scan, not in the resume materializer protecting a CLI prefix-scan. Reword so adapter authors debugging the read path aren't pointed at the wrong function. - README.md usage examples (S3/Redis/Postgres): query() yields dataclass instances with no .type attribute; message.type == 'result' raises AttributeError on the first iteration. Switch to isinstance(message, ResultMessage) like every other example in the repo.
… not delete_session delete_session() is the pre-existing sync disk-path helper that os.remove()s the local JSONL and never touches a SessionStore. The async store-backed counterpart that actually calls store.delete() is delete_session_via_store(). A user following the old wording would call delete_session() expecting their S3/Redis/Postgres data to be removed and only the local file would go. Fixes 4 locations: README.md S3/Redis/Postgres Retention sections and the s3_session_store.py module docstring. The corresponding SessionStore Protocol docstring fix is in the base PR.
…r + Postgres schema divergence
qing-ant
force-pushed
the
qing/example-session-stores
branch
from
d28e6c7 to
0876247
Compare
There was a problem hiding this comment.
Thanks for the fix in 73406c4 — the Postgres test now matches the Redis pattern and the bug hunter found nothing new. I'm not auto-approving only because this is ~2000 lines of new reference adapter code (S3/Redis/Postgres key schemes, SQL DDL, part-file ordering) that's worth one human pass before merge; no outstanding correctness concerns from my side.
Extended reasoning...
Overview
This PR adds three copy-in SessionStore reference adapters under examples/session_stores/ (S3 via boto3, Redis via redis.asyncio, Postgres via asyncpg), a 400-line README, five new test modules (moto/fakeredis-backed unit suites + env-gated live e2e suites), an optional [examples] dependency group in pyproject.toml, the /examples sdist include, and .env* entries in .gitignore. Nothing under src/ is touched — the wheel is byte-identical.
Security risks
The Postgres adapter interpolates the table name into SQL via f-string, but it's guarded by a strict ^[A-Za-z_][A-Za-z0-9_]*$ regex in __init__ and the value is developer-supplied config, not user input — this is the standard pattern for non-parameterizable identifiers and is explicitly tested (test_rejects_unsafe_table_name). The S3 adapter's list_subkeys() filters .././empty path segments as defense-in-depth. The .gitignore addition for .env* is a security improvement. No secrets, no auth/crypto code, no changes to the SDK runtime path.
Level of scrutiny
Medium. The blast radius is low (example code, not shipped in the wheel; tests importorskip so default CI is unaffected), but this is ~2000 net-new lines that users are explicitly told to copy into production. The key-scheme designs (S3 part-file naming with monotonic-ms ordering, Redis :-separated keys with __sessions/__subkeys sentinels, Postgres bigserial+jsonb schema) are durable API-adjacent decisions that a human maintainer should sign off on once.
Other factors
My one prior finding (the isinstance(store, SessionStore) TypeError in the Postgres test) was fixed in 73406c4 and verified live by the author; the current diff confirms test_store_implements_required_methods now uses _store_implements(). The author posted a thorough verification table (182 passed across core + all three adapters, ruff/mypy clean). The bug-hunting system found nothing this run. This is a well-tested, well-documented PR — I'm deferring purely on size/design-review grounds, not correctness.
2 participants
Stacked on #837. (Replaces #841, which was merged into #837's branch by mistake — this re-splits it.)
Three copy-in reference
SessionStoreadapters underexamples/session_stores/:s3_session_store.py) — JSONL part files per append; mirrors@anthropic-ai/claude-agent-sdk-s3(TS)redis_session_store.py) — RPUSH/LRANGE lists + zset session index; mirrors the TS Redis referencepostgres_session_store.py) —asyncpg+ jsonb rows with bigserial seq; new (no TS equivalent)Not shipped in the wheel — these live in
examples/, users copy the file they need into their project. The sdist does includeexamples/so the references travel with source releases. Backend deps are in the optional[examples]group only.Tests — 678 passed, 3 skipped (env-gated live suites)
importorskipon their backend client so default CI is unaffected.motoin-process; Redis usesfakeredis; both pass the full conformance suite.SESSION_STORE_S3_*,SESSION_STORE_REDIS_URL,SESSION_STORE_POSTGRES_URL) and skip otherwise.postgres:16-alpine.Zero secrets / internal refs in the diff (scanned).
Review follow-ups (9 commits total)
delete_session_via_store, scrubbed internal TS path refs, S3 ref now points at the public TS example path, README notes Redis key separator + Postgres schema divergence.examples/.isinstance(store, SessionStore)→_store_implements()after feat: SessionStore adapter — TS parity (protocol, mirror, resume, helpers) #837 dropped@runtime_checkablefrom the base Protocol.