Repository navigation
[Debugger] Update safety guide and add redaction guardrails - #9562
Merged
Merged
Conversation
dudikeleti
changed the title
dudikeleti
marked this pull request as ready for review
October 30, 2026 09:36
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. |
![datadog-prod-us1-6[bot]](https://avatars.githubusercontent.com/u/365230?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
More details
The guide’s descriptions of safe member access, static-member checks, and evaluation budgets match the implementations examined by the specialist. Its new guardrails explicitly distinguish requirements for new and changed code from acknowledged existing gaps.
🤖 Bits Code Review · Commit 1189085
dudikeleti
enabled auto-merge (squash)
October 30, 2026 09:46
LLM ValidationLLM Validation Gate — dotnet-tracer-agent✅ PASS
AnalysisChanged instruction file(s): Ran 5 of 6 selected case(s) — skipped 1 not tied to the changed file(s): No safety or blocking-case regressions across 5 of 6 selected case(s). Overall pairwise win-rate 49% [48%–50%], quality 0.0 — see the verdict above for whether that clears the noise band. Results
Cases
Per-dimension scores, token usage, latency, and estimated cost are in the CI job logs. |
BenchmarksBenchmark execution time: 2026-10-30 15:10:55 Comparing candidate commit 7e02baf 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.
|
dudikeleti
requested review from
andrewlock
and removed request for
andrewlock
October 30, 2026 13:36
dudikeleti
added a commit
that referenced
this pull request
Oct 30, 2026
## Summary of changes Adds Dynamic Instrumentation rules to the `/sdk-review` lens files, so the guardrails in `DebuggerSafetyBoundaries.md` are 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, and `Expression.Equal` null 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 #9414. ## Reason for change The guardrails added to `DebuggerSafetyBoundaries.md` in #9562 are rules we want enforced, and `/sdk-review` is what runs before every push. Keeping them in the lens files instead of `AGENTS.md` keeps `AGENTS.md` small. Each new Dynamic Instrumentation rule comes from a bug fixed in a recent PR, and the COR-6.4 change comes from #9414: | Rule | Fixed in | | --- | --- | | SEC-3.1 (update) | #8878 | | SEC-3.2 runtime type | #9483, #9541, #9547 | | SEC-3.3 dictionary keys and source names | #9536, #9539, #9548 | | SEC-3.4 computed from redacted values | #9485, #9537, #9547 | | SEC-3.5 errors and logs | #9559 | | SEC-3.6 `Expression.ReferenceEqual` | #9541 | | PERF-5.1 per-hit locks, cache hits | #9538 | | PERF-5.2 failure output on every hit | #9536, #9537, #9559 | | COR-6.4 (update) | from #9414, not a bug fix | ## Implementation 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.md` for 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. | Rule | Commit reviewed | Result | | --- | --- | --- | | SEC-3.2 | #8879 | Caught: `ShouldRedactDumpValue(null, expression.Type)` in `ProbeExpressionParser.Dump.cs` decides redaction from the declared type | | SEC-3.4 | #8621 (product code) | Caught: `len(ref password)` and `eq(ref password, "x")` capture values computed from the redaction placeholder | | SEC-3.5 | #8621 (product code) | Caught: capture expression errors send `e.Message` in `evaluationErrors` | | SEC-3.6 | #5543 (product code) | Caught: the `Expression.Equal(expression, Expression.Constant(null))` null check | | PERF-5.1 | #6292 | Caught: every redaction cache hit takes a `ReaderWriterLockSlim` read lock and the LFU policy's write lock, and allocates | | PERF-5.1 | #9538 | Correctly not flagged (the lock-free fix) | | PERF-5.2 | #8725 | Missed: the uncapped metric probe path isn't in that diff | | PERF-5.2 | #8621 (product code) | Flagged a different per-hit failure log in the same block (`Evaluation result should not be null`), not the `Log.Error` in the catch | | COR-6.4 | #9396 | Caught: the flush timeout raised from 5s to 1 minute to fix a flaky test | | COR-6.4 | #9074 | Correctly not flagged (`TaskCompletionSource` with 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 `EvaluationBudget` checks. I dropped it because no PR shows a missed check, and it mostly explained how the budget works; that guidance stays in `DebuggerSafetyBoundaries.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.
andrewlock
approved these changes
Oct 30, 2026
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
Update
docs/development/DebuggerSafetyBoundaries.md:getmember,ref) reads only fields and compiler-generated auto-property backing fields, throughTryResolveSafeMemberExpression([Debugger] Prevent expression member access from invoking getters #8878). The guide still said expressions may read properties.EvaluationBudget,DD_DYNAMIC_INSTRUMENTATION_EVALUATION_TIMEOUT_MS) from [Debugger] Bound Dynamic Instrumentation probe expression evaluation with a time budget #8806, and when new code should add budget checkpoints.StaticMemberSafetyfor static member reads ([Debugger] Guard static member capture #8814), and state that static property getters also fall under the customer-code execution policy./sdk-reviewlens files, which check these guardrails before every push ([Debugger] Add Dynamic Instrumentation rules to/sdk-review#9566). The checklist itself is unchanged from master, so the rules aren't written in two places.AGENTS.mdpointers to the guide.Reason for change
Agents and the review rules (
Security-And-Safety.md, SEC-3.1) use this guide as the contract for debugger safety. It had fallen behind #8878 and #8806, and it covered only customer-code execution and type loading, not leaking sensitive values out of the process. Recent redaction and expression-evaluation work showed which rules are worth stating up front.Implementation details
Docs only. The guide states the new rules as requirements for new and changed code and says that some existing paths don't meet the redaction and runtime-type rules yet. It names current symbols and points to
supported-configurations.yamlfor the budget's default and range instead of repeating them.Merge after #9566: the review checklist now says
/sdk-reviewchecks these guardrails, and that PR adds the rules that do.Test coverage
N/A (docs only). Statements about current behavior were checked against the code on master.
Other details