epic-4/story-3: Support Cross-SDK Debugging - #40
Conversation
There was a problem hiding this comment.
Code Review
This pull request implements Story 4.3 to support cross-SDK debugging by adding a partial cross-SDK-comparable field set (including environment, bucket value, variation key, and hashed visitor reference) to diagnostics and log records. It also includes a comprehensive suite of unit tests to verify these additions. The review feedback suggests two main improvements: first, using modern Python 3.9+ built-in generic types (like dict) instead of importing Dict from typing to maintain codebase consistency; second, precomputing and caching the hashed visitor reference during Context initialization to avoid redundant SHA-256 hash computations on every log emission and diagnostic call.
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.
| import logging | ||
| from types import MappingProxyType | ||
| from typing import TYPE_CHECKING, Any, List, Mapping, Optional | ||
| from typing import TYPE_CHECKING, Any, Dict, List, Mapping, Optional |
There was a problem hiding this comment.
The codebase consistently uses modern Python 3.9+ built-in generic types (like dict[str, Any] and list[str]) instead of typing.Dict or typing.List. We should avoid importing Dict from typing and use the built-in dict instead to maintain consistency.
| from typing import TYPE_CHECKING, Any, Dict, List, Mapping, Optional | |
| from typing import TYPE_CHECKING, Any, List, Mapping, Optional |
| # Story 4.3: the SDK config environment (or None for a directly | ||
| # constructed context). It is an allowlist-safe operational field | ||
| # (NFR6) included in the cross-SDK-comparable diagnostic field set so | ||
| # diagnostics captured in a mixed Python/JS deployment carry the same | ||
| # environment qualifier. Optional + defaulting to None keeps the | ||
| # constructor backward compatible (Critical Warning #1). | ||
| self._environment = environment |
There was a problem hiding this comment.
To avoid repeatedly recomputing the SHA-256 hash of the static visitor_id on every log emission and diagnostic call, we can precompute and cache the hashed visitor reference (self._visitor_ref) once during Context initialization.
| # Story 4.3: the SDK config environment (or None for a directly | |
| # constructed context). It is an allowlist-safe operational field | |
| # (NFR6) included in the cross-SDK-comparable diagnostic field set so | |
| # diagnostics captured in a mixed Python/JS deployment carry the same | |
| # environment qualifier. Optional + defaulting to None keeps the | |
| # constructor backward compatible (Critical Warning #1). | |
| self._environment = environment | |
| # Story 4.3: the SDK config environment (or None for a directly | |
| # constructed context). It is an allowlist-safe operational field | |
| # (NFR6) included in the cross-SDK-comparable diagnostic field set so | |
| # diagnostics captured in a mixed Python/JS deployment carry the same | |
| # environment qualifier. Optional + defaulting to None keeps the | |
| # constructor backward compatible (Critical Warning #1). | |
| self._environment = environment | |
| self._visitor_ref = fingerprint_visitor(visitor_id) |
| optional: Dict[str, Any] = {} | ||
| if environment is not None: | ||
| optional["environment"] = environment | ||
| if bucket_value is not None: | ||
| optional["bucket_value"] = bucket_value | ||
| if variation_key is not None: | ||
| optional["variation_key"] = variation_key | ||
| log_safe( | ||
| LifecycleEvent.DIAGNOSTIC, | ||
| level=logging.DEBUG, | ||
| context=SafeContext(entity_key=entity_key), | ||
| visitor=fingerprint_visitor(self._state.visitor_id), | ||
| reason=reason.value, | ||
| **optional, | ||
| ) |
There was a problem hiding this comment.
Use the built-in dict generic instead of Dict from typing for consistency with the rest of the codebase. Additionally, leverage the cached self._visitor_ref to avoid recomputing the SHA-256 hash of the visitor ID.
| optional: Dict[str, Any] = {} | |
| if environment is not None: | |
| optional["environment"] = environment | |
| if bucket_value is not None: | |
| optional["bucket_value"] = bucket_value | |
| if variation_key is not None: | |
| optional["variation_key"] = variation_key | |
| log_safe( | |
| LifecycleEvent.DIAGNOSTIC, | |
| level=logging.DEBUG, | |
| context=SafeContext(entity_key=entity_key), | |
| visitor=fingerprint_visitor(self._state.visitor_id), | |
| reason=reason.value, | |
| **optional, | |
| ) | |
| optional: dict[str, Any] = {} | |
| if environment is not None: | |
| optional["environment"] = environment | |
| if bucket_value is not None: | |
| optional["bucket_value"] = bucket_value | |
| if variation_key is not None: | |
| optional["variation_key"] = variation_key | |
| log_safe( | |
| LifecycleEvent.DIAGNOSTIC, | |
| level=logging.DEBUG, | |
| context=SafeContext(entity_key=entity_key), | |
| visitor=self._visitor_ref, | |
| reason=reason.value, | |
| **optional, | |
| ) |
| comparable: Dict[str, Any] = dict(details) | ||
| comparable["reason"] = reason.value | ||
| comparable["environment"] = self._environment | ||
| comparable["visitor_ref"] = fingerprint_visitor(self._state.visitor_id) | ||
| comparable["bucket_value"] = bucket_value | ||
| comparable["variation_key"] = variation_key | ||
| diagnostic = cls(reason=reason, message=message, details=comparable) |
There was a problem hiding this comment.
Use the built-in dict generic instead of Dict from typing for consistency with the rest of the codebase. Additionally, leverage the cached self._visitor_ref to avoid recomputing the SHA-256 hash of the visitor ID.
| comparable: Dict[str, Any] = dict(details) | |
| comparable["reason"] = reason.value | |
| comparable["environment"] = self._environment | |
| comparable["visitor_ref"] = fingerprint_visitor(self._state.visitor_id) | |
| comparable["bucket_value"] = bucket_value | |
| comparable["variation_key"] = variation_key | |
| diagnostic = cls(reason=reason, message=message, details=comparable) | |
| comparable: dict[str, Any] = dict(details) | |
| comparable["reason"] = reason.value | |
| comparable["environment"] = self._environment | |
| comparable["visitor_ref"] = self._visitor_ref | |
| comparable["bucket_value"] = bucket_value | |
| comparable["variation_key"] = variation_key | |
| diagnostic = cls(reason=reason, message=message, details=comparable) |
60ad205 to
bf3c16d
Compare
f723894 to
17b6548
Compare
bf3c16d to
9370d58
Compare
Beads: ai-driven-product-dev-klm5 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Extend the existing diagnostic surface (4-1 log_safe / 4-2 _Diagnostic.details) so each diagnostic carries the partial cross-SDK-comparable field set: reason, environment, bucket_value, variation_key, hashed visitor_ref (fingerprint_visitor). Additive: optional environment kwarg on Context.__init__ wired from Core.create_context. Frozen ExperienceResult shape and None-returning callers preserved. Deferred AC-1 fields (config_version/bucketing_inputs/experience_key) and AC#2/#3 honored (4.5/5.1). Beads: ai-driven-product-dev-klm5 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
17b6548 to
05162b0
Compare
F-066 propagation (rebase onto remediated 3-3)Rebased onto the remediated stack. Does not modify
|
|
Superseded — all commits already in main (bc76b64). Closing without merge as part of post-sprint cleanup. |
Story 4.3 — Support Cross-SDK Debugging
Part of sprint `sprint/2026-04-06-convert-python-sdk`. Stacked on #39 (epic-4/story-2) → #38 (epic-4/story-1) → epic-3 chain.
What was built (net in-scope — most ACs deferred by audit)
Extends the existing diagnostic surface (4-1 `log_safe` seam + 4-2 `_Diagnostic.details`) so a diagnostic carries the partial cross-SDK-comparable field set: `reason`, `environment`, `bucket_value`, `variation_key`, and a hashed `visitor_ref` (via `_internal.redaction.fingerprint_visitor`). Visitor id appears only as a hash — never raw. Additive/non-breaking: frozen `ExperienceResult` shape and `None`-returning evaluation callers preserved.
Tests: 628 → 635 (+7), zero regressions.
Audit findings
Beads
Epic `ai-driven-product-dev-9x4m`; tasks `-klm5` (extend diagnostic surface), `-v1c1` (Task 4.1 test) — both closed.
Provenance note
The story file was retro-marked `done` with a stale GPT-5-Codex Dev Agent Record claiming `diagnostics.py`, `events.visitor_reference`, `tests/test_integration_customization.py` already shipped — none existed on the stack. This PR is the real build against the actual 4-1/4-2 seams; no Codex-lineage artifacts were recreated.
Readiness
Gate passed 9/10 round 1, no blocking questions. Two design decisions delegated "your call" against ground truth (environment source; bucket_value recomputation).
🤖 Generated with Claude Code