feat: add aggregation user detail stripping hook#224
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR adds configurable user-detail stripping to playbook aggregation. It defines stripping contracts and placeholder helpers, threads an optional stripper through generation and service entry points, sanitizes prompts, model outputs, and logs, and updates the playbook aggregation prompt and tests. ChangesUser Detail Stripping Pipeline
Sequence Diagram(s)sequenceDiagram
participant PlaybookGenerationService
participant BaseConfigurator
participant PlaybookAggregator
participant UserDetailStripper
participant llm_client
participant record_usage_event
PlaybookGenerationService->>BaseConfigurator: create_user_detail_stripper()
BaseConfigurator-->>PlaybookGenerationService: UserDetailStripper | None
PlaybookGenerationService->>PlaybookAggregator: __init__(..., user_detail_stripper)
PlaybookAggregator->>UserDetailStripper: strip_user_details(prompt fields, shared_mapping)
PlaybookAggregator->>llm_client: send sanitized aggregation prompt
llm_client-->>PlaybookAggregator: aggregation output
PlaybookAggregator->>record_usage_event: record placeholder leakage count
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@reflexio/server/services/playbook/playbook_aggregator.py`:
- Around line 1355-1359: The fallback logging path in playbook aggregation still
emits unsanitized raw responses when structured parsing fails, so placeholder
tokens can leak in logs. Update the response handling around the
PlaybookAggregationOutput branch in playbook_aggregator.py to apply the same
placeholder sanitization logic to string and dict responses before
log_model_response() is called, using the existing
_sanitize_aggregation_response helper and preserving the placeholder leak
recording behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: b367dfb0-967c-4af7-aaa6-789bf9087585
📒 Files selected for processing (8)
reflexio/lib/_generation.pyreflexio/models/config_schema.pyreflexio/server/services/configurator/base_configurator.pyreflexio/server/services/playbook/playbook_aggregator.pyreflexio/server/services/playbook/playbook_generation_service.pyreflexio/server/services/playbook/user_detail_stripping.pytests/models/test_config_f1_fields.pytests/server/services/playbook/test_playbook_aggregator.py
There was a problem hiding this comment.
🧹 Nitpick comments (1)
reflexio/lib/_generation.py (1)
47-66: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider extracting the aggregator-stripping kwargs assembly into a shared helper.
This block (fetch
user_detail_stripper+aggregation_prompt_extra_instructionsfrom the configurator, then conditionally buildaggregator_kwargs) is duplicated verbatim inPlaybookGenerationService._trigger_playbook_aggregation(Lines 511-525). If the inclusion contract changes (e.g. a third hook, or always-pass semantics), both sites must be kept in sync. A single helper inuser_detail_stripping.pyreturning the kwargs dict would centralize it.♻️ Sketch of a shared helper
# user_detail_stripping.py def build_aggregator_stripping_kwargs(configurator: object) -> dict[str, Any]: kwargs: dict[str, Any] = {} stripper = create_configured_user_detail_stripper(configurator) if stripper is not None: kwargs["user_detail_stripper"] = stripper extra = get_configured_playbook_aggregation_prompt_extra_instructions(configurator) if extra: kwargs["aggregation_prompt_extra_instructions"] = extra return kwargs🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@reflexio/lib/_generation.py` around lines 47 - 66, The aggregator kwargs assembly is duplicated between this generation path and PlaybookGenerationService._trigger_playbook_aggregation, so centralize it in a shared helper in user_detail_stripping.py. Add a helper that takes the configurator, builds the conditional kwargs for user_detail_stripper and aggregation_prompt_extra_instructions, and returns the dict, then replace the local assembly here and in _trigger_playbook_aggregation with calls to that helper to keep the inclusion contract in one place.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@reflexio/lib/_generation.py`:
- Around line 47-66: The aggregator kwargs assembly is duplicated between this
generation path and PlaybookGenerationService._trigger_playbook_aggregation, so
centralize it in a shared helper in user_detail_stripping.py. Add a helper that
takes the configurator, builds the conditional kwargs for user_detail_stripper
and aggregation_prompt_extra_instructions, and returns the dict, then replace
the local assembly here and in _trigger_playbook_aggregation with calls to that
helper to keep the inclusion contract in one place.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 96ba040e-0e1d-4baa-9a0a-3de1300060e5
📒 Files selected for processing (9)
reflexio/lib/_generation.pyreflexio/server/prompt/prompt_bank/playbook_aggregation/v2.3.0.prompt.mdreflexio/server/services/configurator/base_configurator.pyreflexio/server/services/playbook/playbook_aggregator.pyreflexio/server/services/playbook/playbook_generation_service.pyreflexio/server/services/playbook/user_detail_stripping.pytests/lib/test_generation_unit.pytests/server/services/playbook/test_playbook_aggregator.pytests/server/services/playbook/test_playbook_generation_service.py
🚧 Files skipped from review as they are similar to previous changes (3)
- reflexio/server/services/playbook/user_detail_stripping.py
- reflexio/server/prompt/prompt_bank/playbook_aggregation/v2.3.0.prompt.md
- reflexio/server/services/playbook/playbook_aggregator.py
44c0761 to
6e8f8f7
Compare
|
one more not sure if applied to the change |
f0f2802 to
b0a1065
Compare
c6cc155 to
cabb9a3
Compare
cabb9a3 to
dc8e1c0
Compare
Summary
BaseConfiguratorowns the no-op defaults.Changes
count_stripping_placeholders()andreplace_stripping_placeholders()with generic replacements for email, phone, and person placeholders.create_user_detail_stripper()andget_playbook_aggregation_prompt_extra_instructions()configurator calls.Test Plan
uv run ruff check reflexio/lib/_generation.py reflexio/server/services/playbook/playbook_aggregator.py reflexio/server/services/playbook/playbook_generation_service.py reflexio/server/services/playbook/user_detail_stripping.py tests/lib/test_generation_unit.py tests/server/services/playbook/test_playbook_aggregator.py tests/server/services/playbook/test_playbook_generation_service.pyuv run pyright reflexio/server/services/playbook/user_detail_stripping.py reflexio/server/services/playbook/playbook_aggregator.py reflexio/lib/_generation.py reflexio/server/services/playbook/playbook_generation_service.pyuv run pytest tests/server/services/playbook/test_playbook_aggregator.py tests/lib/test_generation_unit.py tests/server/services/playbook/test_playbook_generation_service.py -q -o 'addopts='(101 passed)uv run pytest tests/lib/test_generation_unit.py tests/server/services/playbook/test_playbook_generation_service.py -q -o 'addopts='(31 passed after final wrap cleanup)uv run pytest tests/ --ignore=tests/e2e_tests/ --ignore=tests/benchmarks -q -o 'addopts='(3615 passed, 70 skipped, 6 subtests passed)uv run pytest tests/e2e_tests/ -q -o 'addopts='(41 passed, 47 skipped)