epic-5/story-3: Add Planned Expansion Surfaces for Async and Framework Integrations - #45
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces documentation outlining the Phase 3 design intent for async and framework integrations, including a detailed roadmap and a sync-async coexistence model. The review feedback suggests improving the consistency of the forward-compatibility audit table in docs/async.md by fully qualifying all listed file paths with their respective directory prefixes.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
|
|
||
| | Module | Verdict | Rationale | | ||
| |--------|---------|-----------| | ||
| | `evaluation/bucketing.py`, `rules.py`, `segments.py`, `experiences.py`, `features.py`, `entity_lookup.py` | **async-ready** | Pure synchronous computation, no I/O, no shared mutable state. The async surface calls these directly (coexistence rule #2). | |
There was a problem hiding this comment.
To avoid confusion and maintain consistency with other rows (such as the tracking/ row), all file paths in this list should be fully qualified with their directory prefix (evaluation/). Currently, only the first file has the prefix, which might lead readers to assume the subsequent files reside in the root directory.
| | `evaluation/bucketing.py`, `rules.py`, `segments.py`, `experiences.py`, `features.py`, `entity_lookup.py` | **async-ready** | Pure synchronous computation, no I/O, no shared mutable state. The async surface calls these directly (coexistence rule #2). | | |
| | `evaluation/bucketing.py`, `evaluation/rules.py`, `evaluation/segments.py`, `evaluation/experiences.py`, `evaluation/features.py`, `evaluation/entity_lookup.py` | **async-ready** | Pure synchronous computation, no I/O, no shared mutable state. The async surface calls these directly (coexistence rule #2). | |
| | Module | Verdict | Rationale | | ||
| |--------|---------|-----------| | ||
| | `evaluation/bucketing.py`, `rules.py`, `segments.py`, `experiences.py`, `features.py`, `entity_lookup.py` | **async-ready** | Pure synchronous computation, no I/O, no shared mutable state. The async surface calls these directly (coexistence rule #2). | | ||
| | `domain/config_snapshot.py`, `context_state.py`, `results.py` | **async-ready** | Immutable dataclasses / value objects. Safe to read from any thread or coroutine. | |
There was a problem hiding this comment.
To avoid confusion and maintain consistency with other rows, all file paths in this list should be fully qualified with their directory prefix (domain/). Currently, only the first file has the prefix, which might lead readers to assume the subsequent files reside in the root directory.
| | `domain/config_snapshot.py`, `context_state.py`, `results.py` | **async-ready** | Immutable dataclasses / value objects. Safe to read from any thread or coroutine. | | |
| | `domain/config_snapshot.py`, `domain/context_state.py`, `domain/results.py` | **async-ready** | Immutable dataclasses / value objects. Safe to read from any thread or coroutine. | |
| | `adapters/events/in_process.py` | **async-ready** | Synchronous fan-out to handlers; isolates and swallows handler errors. An async event bus is an optional sibling, not a precondition. | | ||
| | `errors.py`, `events.py`, `logging.py`, `config.py`, `version.py` | **async-ready** | Type/constant/exception definitions and a stdlib `logging` seam — no runtime behavior to make async. | | ||
| | `_internal/redaction.py` | **async-ready** | Pure string/fingerprint helpers. | | ||
| | `config_loader/normalizer.py`, `validators.py` | **async-ready** | Pure transformation/validation over decoded payloads; no I/O. | |
There was a problem hiding this comment.
To avoid confusion and maintain consistency with other rows, all file paths in this list should be fully qualified with their directory prefix (config_loader/). Currently, only the first file has the prefix, which might lead readers to assume the subsequent files reside in the root directory.
| | `config_loader/normalizer.py`, `validators.py` | **async-ready** | Pure transformation/validation over decoded payloads; no I/O. | | |
| | `config_loader/normalizer.py`, `config_loader/validators.py` | **async-ready** | Pure transformation/validation over decoded payloads; no I/O. | |
b99ceaa to
65349c2
Compare
2605a59 to
f09ee36
Compare
65349c2 to
8a6badc
Compare
Beads: ai-driven-product-dev-1pug Agent: fullstack-sdk-dev Status: completed Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…-compat audit Beads: ai-driven-product-dev-gfy3 Agent: fullstack-sdk-dev Status: completed Forward-compat audit grounded in actual post-5.2 module tree; no Phase-3 code. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Beads: ai-driven-product-dev-ugkj Agent: fullstack-sdk-dev Status: completed Looking-ahead in index; async/framework forward-refs in migration + extending docs; changes/+story-5-3-async-framework-roadmap.internal.md fragment. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…py + flush.py rows) Beads: ai-driven-product-dev-gfy3 Review finding: audit table claimed to cover every MVP module but omitted tracking/tracker.py and tracking/flush.py. Both added with extend-with-sibling verdicts. No production code; gates green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… parity The rules engine implemented only 8 of the 10 comparison operators present in the JS reference and PHP SDK. isIn and regexMatches were missing, so any config using "value in list" or regex audience rules silently never matched (unknown match_type resolves to None -> rule item returns False). - Add _is_in and _regex_matches, registered under "isIn"/"regexMatches". Semantics mirror the JS source of truth (regex catches re.error -> False). - 20 unit tests (10 per operator) matching PHP ComparisonsTest granularity. - 17 cross-SDK parity vectors (9 isIn, 8 regexMatches). Full suite: 799 passed; ruff + mypy --strict clean; rule parity 44 vectors pass. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
f09ee36 to
c934e6b
Compare
F-066 propagation (rebase onto remediated 3-3)Rebased
This completes the F-066 propagation across the full stack (3-3 → 5-3). |
abbaseya
left a comment
There was a problem hiding this comment.
Round-3 implementation review — PR #45 is the canonical branch
Reviewed epic-5/story-3 (the patch-sprint-propagated last-story branch). Verified on disk and by execution.
✅ Parity fixes — correct
- Bucketing UTF-8 (F-063):
bucketing.py:64hashesvalue.encode("utf-8")+ UTF-8 byte length. Executed:用户123→3859151469(exact match to the real npmmurmurhashv3 the JS SDK imports); ASCII controltest_visitor→2397688595. ✅ - Real parity oracle:
tests/parity/fixtures/bucketing_vectors.jsonpins用户123→3859151469(correct), andscripts/js_reference/bucketing.jsnowrequire('murmurhash')/murmurhash.v3(...)— the real npm package, not the hand port. The self-referential co-buggy loop is gone. ✅ - Operators (F-064): full 13-name JS set. ✅
- Segment latch (F-066):
segments.pylatches, mirrors JSsegments-manager.ts:100-121. ✅
This is solid work and clears the round-2 flagship blocker.
🔴 Blocker — this branch is missing PR #46's fixes
This branch diverges from PR #46 and lacks three fixes that PR #46 has:
- Config
/api/v1route:httpx_transport.py:77still builds/config/{sdk_key}against a pure-host base → 404 in sdk_key mode (the original calibration defect). - Tracking delivery:
send_trackingPOSTs/track/{sdk_key}to the config CDN host with noConvertAgentUA → 403 / silently dropped. Nometrics.convertexperimentsreference anywhere on this branch. - Demo harness (DoD #5): absent (no
demo/; only the oldexamples/with invented keys).
Required before merge
Reconcile with PR #46: bring its config /api/v1 fix, tracking→metrics-endpoint + UA fix, and demo onto this branch (or cherry-pick this branch's F-063 commits onto #46). The single merged branch must carry all of: bucketing UTF-8, config /api/v1, tracking→metrics+UA, demo, segment latch, operators. Then run the demo live against staging 10035569/10034190 and confirm a conversion appears in reports. See the PR #46 review re: closing that branch to avoid two authoritative lines.
…fetch
HttpxTransport._build_route built /config/{sdk_key}, but the real
config-serving CDN only serves that resource under /api/v1, so every
sdk_key-mode initialization 404'd against the production and staging
endpoints. Confirmed against the php-sdk generated ProjectConfigApi
(server base https://cdn-4.convertexperiments.com/api/v1 + resource
/config/{sdk_key}) and a live staging fetch (404 without the prefix,
200 with it).
The /api/v1 prefix lives in _build_route so DEFAULT_CONFIG_BASE_URL and
any user-supplied base_url stay pure hosts. Query-param variants
(environment, _conv_low_cache) are preserved. Direct-config (data=) is
unaffected.
Adds 5 regression tests asserting the prefix across query variants and
updates the integration mock endpoint and docstrings/comments.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…t UA and report status
Tracking events were POSTed to the config CDN host (returning 403) instead of
the separate metrics endpoint. Adds TransportConfig.track_base_url (default
https://[project_id].metrics.convertexperiments.com/v1, JS/PHP parity),
substitutes [project_id] at request time, and POSTs to {track_base}/track/{key}.
Also sets User-Agent: ConvertAgent/1.0 so the metrics bot filter does not
silently drop events (200-but-discarded), and propagates the HTTP status code
on the success path so QueueReleasedPayload.status_code carries the real 2xx
(send_tracking now returns int).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Adds demo/run_demo.py — an OFFLINE (committed fixture, deterministic, no network) and LIVE (sdk_key against staging) manual-verification harness that runs the full FullStack arc: experience -> feature-rollout experience -> segments -> conversion + delivery report. Mirrors the JS and PHP demos: buckets the feature rollout via run_experience (key test-experience-ab-fullstack-4, per php-sdk config; the JS demo's test-feature-rollout-1 is stale), passes location=events and the mobile=true audience attribute, and sets the country=US default segment. Self-diagnoses any non-bucketing via diagnose_experience. Excludes demo/ from the sdist, gitignores demo/.env, and lints demo/ in CI. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ollout-1 typed vars The committed demo harness (49de4e9) had unilaterally overridden the frozen qs-12 spec's feature-rollout decision: it used PHP's run_experience on test-experience-ab-fullstack-4 and printed raw string variables_data, instead of the spec's JS-authoritative run_feature('test-feature-rollout-1') resolving typed variables (bool/string/integer). This left two gaps: the OFFLINE output did not match the README contract, and the typed-variables acceptance criterion was unmet. Align the implementation to the spec: - config_fixture.json: re-key the feature to test-feature-rollout-1; declare the 4 typed vars the README shows; drop the redundant rollout-as-experience block; correct the _comment (JS-authoritative; LIVE override via env var). - run_demo.py: step [2] now calls run_feature(test-feature-rollout-1) printing typed vars + status + via experience/variation; move the audience attrs + country default-segments LIVE-only so OFFLINE output is byte-identical to the README; correct the misleading comments. - test_demo_harness.py: flip the feature-key assertions/comments back to test-feature-rollout-1; the /api/v1/config route guard is unchanged. OFFLINE output now byte-matches demo/README.md verbatim. Full suite 828 passed, ruff + mypy green, demo/ excluded from sdist + wheel, zero src/ changes. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… py3.10 match under py39 gate) anyio 4.x uses `match` statements (Python 3.10+) in its internals. When mypy descends into anyio transitively via `import httpx` in src/convert_sdk/adapters/transport/httpx_transport.py, it fails under our `python_version = "3.9"` target with: anyio/_core/_tasks.py:361: error: Pattern matching is only supported in Python 3.10 and greater [syntax] This is dependency drift (anyio version churn), not a code defect. The fix adds "anyio.*" to the existing [[tool.mypy.overrides]] block that already skips click.* and towncrier.* for the same reason — third-party packages that use newer syntax than our minimum-Python target. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
abbaseya
left a comment
There was a problem hiding this comment.
Round-4 verification — APPROVE
Verified the reconciliation on epic-5/story-3 (== integration/python-sdk-round3-final, tip 9f603b4) on a clean checkout. The round-3 split-branch problem is resolved: PR #46's three fixes were brought onto this branch and PR #46 was closed.
All round-3 items confirmed by execution, not by reading docstrings:
- Bucketing UTF-8 (the round-3 blocker): executed shipped
murmurhash3_32("用户123", 9999)→ 3859151469 (real npm value);test_visitor→ 2397688595._utf16_code_units/charCodeAtgone fromsrc/;scripts/js_reference/bucketing.jsnowrequire('murmurhash'); fixture pins the correct value. - Config route
/api/v1/config/{sdkKey}: verified live against the staging CDN (GET → 200,is_ready=True) — the round-2 404 is gone. - Tracking transport: POSTs to
{[project_id].metrics…}/track/{sdkKey}withUser-Agent: ConvertAgent/1.0(the 403 fix),[project_id]substituted at request time. Verified by code + httpx/respx tests. (The live conversion write was not exercised this pass — only the config GET was.) - Operators:
isIn+regexMatchespresent (13 comparators). - Segment latch mirrors JS
segments-manager.ts; demo harness is dual-mode with real staging keys; OFFLINE run is deterministic. - Quality gate: 836 tests pass · ruff clean · mypy
--strictclean · coverage 97% (evaluation package 97.2%, floor 95%).
Approving the SDK code.
Two org-level follow-ups are out of this repo and are tracked on the Asana task (reassigned to Usman):
- Backend serving-config codegen for Python — JS/PHP/Ruby each get their Serving API config types regenerated from the backend serving schema and auto-PR'd into their repo; Python has no such wiring, so its config model can silently drift from the serving contract.
- python-sdk wiki is a 2026-06-08 snapshot that predates these 06-15/06-17 fixes — it still documents the pre-fix config route (
/config/without/api/v1), the pre-fix tracking endpoint (no[project_id], noConvertAgentUA), and omits the UTF-8 bucketing parity contract.
Merge mechanics: PR #45 is the top of the stacked chain (base epic-5/story-2, not main). Land the stack bottom-up, or merge the integration snapshot to main.
|
Superseded — all commits already in main (bc76b64). Closing without merge as part of post-sprint cleanup. |
Story 5.3 — Add Planned Expansion Surfaces for Async and Framework Integrations
Part of sprint
sprint/2026-04-06-convert-python-sdk. Stacked on epic-5/story-2 (PR #44 → #43 → … → epic-3 chain). Final story of the sprint.This is a design-intent / documentation story — it ships NO production code (per the story's Critical Warning #1: async + framework implementation is gated on explicit Phase 3 sign-off). Implements FR45/FR46 at the planning level so MVP code does not foreclose async or framework integration surfaces.
What was delivered (docs-only)
docs/roadmap.md(new) — Phase 1 (MVP, shipped) / Phase 2 (auto-config-refresh ✅ shipped via Story 5.2; durable storage + retry tuning planned) / Phase 3 (async public API, framework integrations, OpenTelemetry — all planned, gated).docs/async.md(new) — Phase 3 design intent: sync↔async coexistence model; a forward-compatibility audit table (Task 4) covering every current MVP module with anasync-ready/extend-with-sibling-never-rewriteverdict (built fresh against the real post-5.2 tree, includingconfig_loader/refresh.py,tracking/tracker.py,tracking/flush.py); public method-name reservation; framework-integration plan (separate distributions, framework-free core per NFR13, deprecation policy); open questions flagged as Phase-3-sign-off decisions.docs/index.md("Looking ahead"),docs/migration-from-javascript.md,docs/migration-from-rest.md(notes today's async-Python users can call the sync SDK viaasyncio.to_thread()),docs/extending.md.changes/+story-5-3-async-framework-roadmap.internal.md(typeinternal, matching the live+story-*convention).Audit finding addressed
docs/async.mdstates the parity discipline is a hard requirement when async code lands.Forward-compat audit bottom line
No MVP module requires a rewrite to enable async — async lands as siblings/extensions. Phase-3-gated code (
src/convert_sdk/asyncio/,httpx_async_transport.py, framework distributions) remains absent by design.Tests / gates
762 tests pass (unchanged — docs-only diff).
scripts/verify_release.pyall gates green (ruff, mypy --strict, coverage ≥85% / evaluation ≥95%, parity suite, towncrier draft including the new fragment, uv build).Beads
epic
ai-driven-product-dev-ezhe; tasks-1pug/-gfy3/-ugkj(all closed).Sprint-mode notes
tracker.py+flush.pyrows).🤖 Generated with Claude Code