Uh oh!
There was an error while loading. Please reload this page.
test(e2e): stop the SLS masking spec asserting over its own warm-up traffic - #890
Conversation
…raffic `sls-content-capture-masked-e2e` waits for config propagation by sending warm-up chats until the masked form appears on the wire. Those warm-up requests are exported too, and the ones served before the guardrail went live carry the UNMASKED reply — correct behaviour for a gateway that has no masking rule yet. `decodedTextFor` joins every PutLogs body the mock ever received, so `expect(fullText).not.toContain(EMAIL)` was also asserting over those pre-propagation exports. On a fast machine the guardrail is live by the first warm-up and nothing unmasked is ever exported; on slower CI it is not, and the spec fails with the raw address present. Snapshot `sls.requests.length` once the config is confirmed live and scope the assertions to what was exported after it, via a `fromIndex` argument on `decodedTextFor` / `waitForToken` (default 0, so other callers are unchanged). Verified by forcing the condition: a request served after the model is live but before the guardrail exists reproduces the CI failure exactly, and passes with this change.
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in:45 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe SLS mock now supports request-index filtering for decoded text and token polling. The masked content capture test records the post-warm-up index and applies it to chat and bridge export assertions. ChangesMasked SLS capture assertions
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related issues
🚥 Pre-merge checks | ✅ 6✅ Passed checks (6 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 `@tests/e2e/src/cases/sls-content-capture-masked-e2e.test.ts`:
- Around line 194-200: Ensure the warm-up boundary in the test aligns with
exporter completion before capturing afterWarmup: after waitConfigPropagation,
explicitly drain or flush the SLS exporter, or wait long enough for its 5-second
buffer interval to elapse before snapshotting sls.requests.length.
Alternatively, add and use a correlation marker or request ID so decodedTextFor
can distinguish warm-up records from post-mask traffic.
🪄 Autofix
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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1f37a747-62d5-46d1-83e5-f4ba9db55b65
📒 Files selected for processing (2)
tests/e2e/src/cases/sls-content-capture-masked-e2e.test.tstests/e2e/src/harness/sls-mock.ts
Uh oh!
There was an error while loading. Please reload this page.
…boundary
An index boundary alone is not deterministic: the sink buffers records and
flushes on a 5s tick (`PipelineConfig { max_batch: 100, flush_interval: 5s }`),
so a warm-up record still in that buffer ships in the SAME PutLogs body as
the requests under test, and slicing by request index cannot separate them.
Send one marker request after the guardrail is live — masked, so it carries
no raw PII — and wait for it to arrive before snapshotting. The sink flushes
its buffer in order, so the marker being visible proves every warm-up record
has already shipped.Uh oh!
There was an error while loading. Please reload this page.
…ne copy of it Two findings from a cross-plane review of the previous commits. The previous commit said a rename "splits the counter into a before and an after series". A rename does not reach the data plane at all on its own: `user_name` is stamped when cp-api PROJECTS the api_keys row, and nothing reprojects a key because its member was renamed — SCIM's `UpdateMember` writes `auth."user".name` and only cascades to the member's keys on an `Active` flip, the dashboard's self-service rename goes to better-auth without touching cp-api, and none of the eight `reprojectAPIKey` call sites is a rename. So the label holds the name as of that key's last write, and a rename shows up only once the key is written again for some other reason. Both label structs now say so, and the budget-gauge note points at it rather than implying renames strand samples on their own. `apply_caller_identity` also ran `user_name` through `sanitize_tag` while the four #890 families stamp the same value raw off the same row. That call protects the cp-api wire and the provider-call log line, and `user_name` reaches neither — it is `#[serde(skip)]` and `log_provider_call` does not render it. Its only destination is the metric label, so sanitising this one copy could make a long or odd name read differently on `aisix_usage_event*` than on `aisix_llm_*` and drop that member out of a cross-family join on the pair. Removed; `user_id` keeps it, because that one does travel to cp-api. Also corrects a type name in the new `CallerIdentity` doc: the allowlist is `HEADER_TEMPLATE_VARS`, resolved by `HeaderVars::resolve`.
Fixes#889
Fixes a CI-only failure in
sls-content-capture-masked-e2e, seen on #888:Cause
The spec waits for config propagation by sending warm-up chats until the masked form shows up on the wire. Those warm-up requests are exported to SLS too, and the ones served before the guardrail went live carry the unmasked reply — which is correct behaviour for a gateway that has no masking rule yet.
decodedTextForjoins every PutLogs body the mock has ever received, soexpect(fullText).not.toContain(EMAIL)was also asserting over those pre-propagation exports. On a fast machine the guardrail is live by the first warm-up, nothing unmasked is ever exported, and the spec passes; on slower CI it is not, and the raw address is there.The product behaviour under test is fine — the assertion population was wrong.
Fix
Snapshot
sls.requests.lengthonce the config is confirmed live, and scope the assertions to what was exported after it.decodedTextForandwaitForTokentake afromIndex(default0, so the other SLS specs are unchanged).Verification
The condition is timing-dependent, so I forced it rather than guessing: inserting one request that is served after the model is live but before the guardrail exists reproduces the CI failure exactly with the old assertion scope, and passes with this change. All four SLS specs pass locally.
Summary by CodeRabbit