Skip to content

test(node): Assert absence of PII attributes in OpenAI integration tests - #24031

Open
itz-puneet wants to merge 3 commits into
getsentry:developfrom
itz-puneet:test/ai-pii-absence-assertions
Open

itz-puneet wants to merge 3 commits into
getsentry:developfrom
itz-puneet:test/ai-pii-absence-assertions

Conversation

@itz-puneet

@itz-puneet itz-puneet commented Sep 3, 2026

Copy link
Copy Markdown

The OpenAI integration tests assert which attributes are present when GenAI recording is disabled, but never assert that the gated attributes are absent. As #19801 notes, a regression that leaked prompt or response content onto spans would pass the existing suite.

#19801 was filed against sendDefaultPii, which was removed in v11. The gap still exists under its replacement: the disabled scenarios now opt out with dataCollection: { genAI: { inputs: false, outputs: false } }, which resolveAIRecordingOptions turns into recordInputs / recordOutputs. Since v11 defaults to collecting GenAI inputs and outputs, the explicit opt-out is the path that most needs a guard.

This adds an explicit absence check to both instrument.mjs (recording disabled) scenarios — chat and embeddings — covering the four content-bearing attributes that the paired instrument-with-pii.mjs tests assert are present:

  • gen_ai.input.messages (gated by inputs)
  • gen_ai.system_instructions (gated by inputs)
  • gen_ai.embeddings.input (gated by inputs)
  • gen_ai.response.text (gated by outputs)

The check iterates every span in the envelope rather than only the spans asserted individually above, so content leaking onto an uninspected span is also caught. for (const span of container.items) already appears 4 times in this file, so the shape matches the surrounding style.

Scope

Only the openai suite here, to keep the diff reviewable. The same gap exists in anthropic, langchain and google-genai — happy to follow up in a separate PR if you'd like the same treatment there.

I did not use the flare-redact approach suggested in the issue comments, since AGENTS.md says not to add dependencies unless explicitly asked. These assertions use vitest built-ins only.

Testing note

I was unable to run yarn install, yarn lint, yarn test or yarn format locally — my environment enforces an npm registry publish-date cutoff that blocks resolving this repo's dependency tree. The change is additive test assertions with no production code touched, and I verified the constants used are still imported by this file on develop, but CI is the first real execution of these assertions. Please flag anything it turns up and I'll fix promptly.

Closes #19801

The OpenAI integration tests assert which attributes are present when
`sendDefaultPii` is disabled, but never assert that the PII-gated
attributes are absent. A regression which leaked prompt or response
content onto spans would therefore pass the existing suite.

Adds an explicit check over every span in the envelope, for both
`instrument.mjs` (recording disabled) scenarios, covering the four
content-bearing attributes which the paired `instrument-with-pii.mjs`
tests assert are present:

-   `gen_ai.input_messages`
-   `gen_ai.system_instructions`
-   `gen_ai.response.text`
-   `gen_ai.embeddings.input`

Iterating the whole envelope rather than the individually inspected
spans also catches content leaking onto spans the tests do not assert
against directly.

The same gap exists in the anthropic, langchain and google-genai
suites; happy to follow up in a separate PR.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@itz-puneet
itz-puneet requested a review from a team as a code owner September 3, 2026 21:30
@itz-puneet
itz-puneet requested review from isaacs and mydea and a lite review from Copilot and removed request for a team September 3, 2026 21:30

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Approval recommended

The change is a targeted, consistent test hardening that directly addresses the documented gap without introducing behavioral or structural risk.

Pull request overview

Strengthens the OpenAI Node integration test suite to prevent regressions where PII-bearing GenAI content attributes are accidentally attached to spans while sendDefaultPii is disabled, aligning coverage with the concerns raised in #19801.

Changes:

  • Adds explicit assertions that content-bearing GenAI attributes are absent when sendDefaultPii is disabled.
  • Applies the absence checks across all spans in the captured envelope for both chat and embeddings scenarios to catch leaks on uninspected spans.
File summaries
File Description
dev-packages/node-integration-tests/suites/tracing/openai/test.ts Adds envelope-wide assertions ensuring PII-gated GenAI attributes are undefined when sendDefaultPii is disabled (chat + embeddings).
Review details
  • Files reviewed: 1/1 changed files
  • Comments generated: 0
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

@RulaKhaled
RulaKhaled self-requested a review September 4, 2026 16:45
@github-actions

github-actions Bot commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

👋 @isaacs, @mydea — Please review this PR when you get a chance!

1 similar comment
@github-actions

Copy link
Copy Markdown
Contributor

👋 @isaacs, @mydea — Please review this PR when you get a chance!

@isaacs

isaacs commented Sep 15, 2026

Copy link
Copy Markdown
Member

Oh, wait a second, I think this might be outdated, since we removed that field in v11.

…i in OpenAI assertions

sendDefaultPii was removed in v11 in favour of dataCollection. The disabled
scenarios now opt out via dataCollection.genAI, so the comment describing the
absence assertions is updated to match.
@itz-puneet

Copy link
Copy Markdown
Author

Thanks for catching that, @isaacs — you're right that sendDefaultPii is gone in v11, and my wording was out of date.

The assertions themselves still apply though. On develop, the two scenarios they sit in (scenario-chat.mjs and scenario-embeddings.mjs with instrument.mjs) now opt out via dataCollection: { genAI: { inputs: false, outputs: false } }. That flows through resolveAIRecordingOptions into recordInputs / recordOutputs, which gate exactly these four attributes — gen_ai.input.messages, gen_ai.system_instructions and gen_ai.embeddings.input on inputs, gen_ai.response.text on outputs. Since v11 defaults to collecting, the explicit opt-out is arguably the path most worth guarding now.

I've pushed a987ded, which updates the code comment to reference dataCollection.genAI, and updated the PR description to match. No change to the assertions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

AI integration tests don't assert absence of PII when sendDefaultPii is false

3 participants