fix(report): preserve occurrence columns in JSON and SARIF - #584
Merged
Merged
Conversation
Signed-off-by: Chandrashekar Ramachandran <cramachandra@nvidia.com>
rng1995
approved these changes
Sep 21, 2026
rng1995
left a comment
Collaborator
There was a problem hiding this comment.
[SkillSpector Review]
Approved after reviewing the complete three-file diff and relevant surrounding code at commit 47d6275af8368dc927f8a87b47487de96047b87f, with independent checks of SARIF compatibility and column producer/consumer consistency. No required correctness, security, compatibility, or test changes found.
Review details:
_expand_occurrences()now takes columns from each occurrence. Missing or explicitly unknown columns no longer inherit the representative finding's span; a finding without an occurrence list retains its own optional columns.- Both active and baseline-suppressed SARIF results apply the same conversion. The internal zero-based, end-exclusive convention correctly becomes one-based, end-exclusive SARIF coordinates.
columnKind: "unicodeCodePoints"matches static character offsets and the existing conversion of Python AST byte offsets into character columns. - Deduplication, immutable source provenance, and match fingerprints remain intact. The JSON expansion agrees with the compact findings returned by MCP; this change does not alter risk scoring or suppression decisions.
- Reviewed all 11 added regression cases: repeated same-line occurrences, multiple files, optional/unknown columns, multiline ranges, a non-BMP Unicode prefix, normalized/projected source text, suppressed SARIF results, and MCP embedded JSON. The assertions directly exercise the original incorrect-column behavior.
Validation: static source and test inspection only; contributor code was not executed locally. GitHub reports five completed checks passing (changes, lint, OpenCode TypeScript Tests, DCO, and Docker smoke); test-unit is still running for this commit. Wait for that job to pass before merging. There were no prior review threads to resolve.
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.
Repeated findings could inherit the first occurrence's columns in JSON reports, while SARIF reports omitted columns. Preserve each occurrence's source span, emit one-based Unicode code-point columns in SARIF, and add regression coverage for JSON, SARIF, and MCP reports.