Repository navigation
[Debugger] Add Dynamic Instrumentation rules to /sdk-review - #9566
Merged
Merged
Conversation
Add SEC-3.2 to SEC-3.6 and PERF-5.1 to PERF-5.3 for debugger redaction, runtime-type checks, expression-tree null checks, per-hit locks, failure output and evaluation budget checks, and extend SEC-3.1. Reword COR-6.4 to say which time bounds are fine in tests.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
dudikeleti
marked this pull request as draft
October 30, 2026 13:28
![datadog-prod-us1-3[bot]](https://avatars.githubusercontent.com/u/365230?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
More details
No actionable inconsistency was identified between the new Dynamic Instrumentation guardrails, the referenced debugger APIs, and the review workflow.
🤖 Bits Code Review · Commit e22159d
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e22159d834
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
andrewlock
approved these changes
Oct 30, 2026
andrewlock
left a comment
andrewlock
left a comment
Member
There was a problem hiding this comment.
Thanks! Just a couple of nits
dudikeleti
marked this pull request as ready for review
October 30, 2026 13:56
Contributor
There was a problem hiding this comment.
More details
The new Dynamic Instrumentation rules are consistent with the referenced debugger helpers. COR-6.4 explicitly handles parallel workers and legitimate timeout checks; no actionable defect remained in the reviewed documentation.
🤖 Bits Code Review · Commit 68a3644
BenchmarksBenchmark execution time: 2026-10-30 14:45:40 Comparing candidate commit 68a3644 in PR branch Found 0 performance improvements and 0 performance regressions! Performance is the same for 72 metrics, 0 unstable metrics, 6 known flaky benchmarks, 119 flaky benchmarks without significant changes.
|
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
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 of changes
Adds Dynamic Instrumentation rules to the
/sdk-reviewlens files, so the guardrails inDebuggerSafetyBoundaries.mdare checked before every push:Security-And-Safety.md: section 3 is now "Dynamic Instrumentation". SEC-3.1 also covers expression-tree property access and static property getters. New rules SEC-3.2 to SEC-3.6 cover runtime-type checks, dictionary keys and compiler-generated names, values computed from redacted values, customer data in evaluation errors and logs, andExpression.Equalnull checks.Performance.md: new section 5 with PERF-5.1 and PERF-5.2 for per-hit locks and cache hits, and failure output on every hit.Correctness.md: COR-6.4 now says which time bounds are acceptable in tests, flags raising a timeout to fix a flaky test, and limits queued markers to single-consumer queues. This replaces [Docs] Add event-based test synchronization guideline to AGENTS.md #9414.Reason for change
The guardrails added to
DebuggerSafetyBoundaries.mdin #9562 are rules we want enforced, and/sdk-reviewis what runs before every push. Keeping them in the lens files instead ofAGENTS.mdkeepsAGENTS.mdsmall.Each new Dynamic Instrumentation rule comes from a bug fixed in a recent PR, and the COR-6.4 change comes from #9414:
Expression.ReferenceEqualImplementation details
SEC-3.1 keeps its ID; the section heading changes from "Customer-process safety" to "Dynamic Instrumentation". The Performance "Don't flag" entry about uncontended per-request locks now points to PERF-5.1, so the two don't conflict for locks shared by every thread that hits a probe.
SEC-3.1 still links to
DebuggerSafetyBoundaries.mdfor the allowed execution policy. None of these rules depend on #9562, but #9562 points the guide's review checklist to them, so merge this PR first.Test coverage
Docs only. As in #9472, I ran the rules against commits that introduced the issues. Each run was one independent lens reviewer reading the frozen commit and only its lens file. No reviewer was told which rule was being tested.
ShouldRedactDumpValue(null, expression.Type)inProbeExpressionParser.Dump.csdecides redaction from the declared typelen(ref password)andeq(ref password, "x")capture values computed from the redaction placeholdere.MessageinevaluationErrorsExpression.Equal(expression, Expression.Constant(null))null checkReaderWriterLockSlimread lock and the LFU policy's write lock, and allocatesEvaluation result should not be null), not theLog.Errorin the catchTaskCompletionSourcewith a 30s guard)Not tested: SEC-3.1's new expression-tree clause and SEC-3.3. Reviewers did point out SEC-3.3's literal-only dictionary key check in code that predates the commits under review.
Other details
I kept only rules a reviewer can match in a diff and that a PR shows were needed. An earlier version had a PERF-5.3 rule for
EvaluationBudgetchecks. I dropped it because no PR shows a missed check, and it mostly explained how the budget works; that guidance stays inDebuggerSafetyBoundaries.md(#9562). SEC-3.3 was also narrowed to the two shapes a diff shows: dictionary keys checked only when literal, and keywords matched against compiler-generated names.