Synthesize SarifLogger invocation only when the run has none - #3136
Open
Sukanth Gunda (sukanth) wants to merge 1 commit into
Open
Conversation
SarifLogger.EnhanceRun always created and appended a synthesized Invocation, even when the caller supplied a Run that already carried one or more. That duplicated the caller's invocation in the emitted run and left downstream consumers reading a phantom trailing invocation. Guard the synthesis behind a check for an empty invocation collection so an invocation is created only when the caller supplied none. The null-coalescing initialization of the invocation list is kept ahead of the guard because Dispose, AnalysisStopped, and the notification writers index the first invocation and read its count, and would throw on a null list. Add tests covering the single caller-supplied invocation and multiple caller-supplied invocations, extracting a shared helper reused by the existing empty-run test. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Sukanth Gunda (sukanth)
requested review from
Aasim Malladi (aasim),
cfaucon and
Michael C. Fanning (michaelcfanning)
as code owners
July 15, 2026 20:46
| // by a dedicated rewriting visitor or some other approach. | ||
|
|
||
| if (invocation.EnvironmentVariables != null) | ||
| if (invocationTokensToRedact != null) |
Contributor
There was a problem hiding this comment.
You've created some API confusion here as now arguments such as 'invocationsToRedact' are only used in cases where an invocation is created by this code. So, if you provide a non-null Invocations list and also invocationTokensToRedact, this latter argument goes silently unused. There are two obvious possibilities, attempt to apply invocationsToRedact over all invocations, whether passed in or created or enforce that arguments such as invocationsToRedact are only provided when the incoming invocation list is null or empty.
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
2 participants
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
SarifLogger.EnhanceRununconditionally created a newInvocationand appended it torun.Invocationsevery time the logger enhanced a run. When a caller supplied aRunthat already carried one or moreInvocationobjects, the logger appended a second, machine-synthesized invocation — duplicating invocation data the caller had already provided and producing a SARIF run with a spurious trailing invocation.This change makes the synthesis conditional:
SarifLoggernow synthesizes and appends anInvocationonly whenrun.Invocationsisnullor empty. When the caller already supplied at least one invocation, the logger leaves the caller's invocations untouched.Fixes #2519.
Details
EnhanceRunstill normalizes_run.Invocationsto a non-null list (??= new List<Invocation>()) because downstream code paths —Dispose/AnalysisStoppedend-time stamping, the notification writers, SARIF v1 conversion, andResultProvenance.InvocationIndex— readInvocations[0]/Invocations.Countand wouldNullReferenceExceptionagainst a null list. Only the creation of a new invocation (plus its redaction and append) is now guarded byif (_run.Invocations.Count == 0).Tests
SarifLogger_DoesNotAddInvocationWhenRunAlreadyHasInvocation— asserts no second invocation is appended when the caller supplies one.SarifLogger_RetainsAllCallerSuppliedInvocations— asserts multiple caller-supplied invocations are all retained and none are added.SarifLogger_EnhancesRunWithInvocation— refactored to share a private helper (RunSarifLoggerOverRun) with the new tests; still asserts the synthesized-invocation path for a run that starts with none.Per repo convention, the new coverage is expressed as multiple
[Fact]methods delegating to a shared helper (no[Theory]/[InlineData]). AllSarifLoggerTestspass locally (33/33); the solution builds clean.Validation
The change was validated with a design review before implementation and an independent diff review afterward across two model families (OpenAI GPT-5.6 and Anthropic Claude Opus 4.8); both reached approve with no blocking findings.