Skip to content

feat: add reviewedBy and reviewedOn to suppression details [IGNR-2204] - #660

Open
adisatop wants to merge 11 commits into
mainfrom
fix/IGNR-2204-bump-code-client-go
Open

feat: add reviewedBy and reviewedOn to suppression details [IGNR-2204]#660
adisatop wants to merge 11 commits into
mainfrom
fix/IGNR-2204-bump-code-client-go

Conversation

@adisatop

@adisatop adisatop commented Jul 20, 2026

Copy link
Copy Markdown

Description

Adds optional reviewedBy and reviewedOn fields to the suppression details of
the local finding model, so consumers (CLI, IDE, MCP) can see who reviewed an
ignore and when
— not just who requested it (ignoredBy / ignoredOn).

Snyk ignores go through a review lifecycle (pending / approved / rejected).
Upstream SARIF (code-client-go SuppressionProperties) already exposes
ReviewedOn and ReviewedBy, but GAF was dropping them during transformation.

Changes:

  • Extend types.SuppressionDetails in the REST test spec with optional
    reviewedOn and reviewedBy (both string) and regenerate local_models.
  • Map the new fields in mapSuppressions: reviewedOn passes through, and
    reviewedBy is flattened from the SARIF Reviewer.Name (nil-safe).
  • Bump code-client-go to expose the reviewed suppression properties.
  • Add unit-test coverage for the reviewed fields in transform_test.go.

The fields are optional pointers (omitempty), so unreviewed ignores leave them
unset. This keeps the change additive and backwards compatible on the public API.

Checklist

  • Tests added and all succeed (make test)
  • Regenerated mocks, etc. (make generate)
  • Linted (make lint)
  • Test your changes work for the CLI
    1. Clone / pull the latest CLI main.
    2. Run go get github.com/snyk/go-application-framework@YOUR_LATEST_GAF_COMMIT in the cliv2 directory.
      • Tip: for local testing, you can uncomment the line near the bottom of the CLI's go.mod to point to your local GAF code.
    3. Run go mod tidy in the cliv2 directory.
    4. Run the CLI tests and do any required manual testing.
    5. Open a PR in the CLI repo now with the go.mod and go.sum changes.
    • Once this PR is merged, repeat these steps, but pointing to the latest GAF commit on main and update your CLI PR.

Note

Low Risk
Additive optional fields on suppression metadata and presentation only; no auth or scan logic changes. Dependency bumps are routine.

Overview
Adds optional reviewedOn and reviewedBy on suppression details so CLI, IDE, and MCP outputs can show who reviewed an ignore and when, alongside existing ignoredBy / ignoredOn request metadata.

The OpenAPI test spec and regenerated local_models gain TypesReviewer and the new suppression fields; SARIF → local finding mapping in mapSuppressions now preserves ReviewedOn and ReviewedBy from code-client-go (bumped to v1.29.0) instead of dropping them. Text and SARIF presenters emit the fields when present.

Tests cover transform mapping and presenter output for component text and SARIF JSON. Fields are optional (omitempty), so behavior stays backward compatible when review data is absent.

Reviewed by Cursor Bugbot for commit 09af3d4. Bugbot is set up for automated code reviews on this repo. Configure here.

@adisatop
adisatop requested review from a team as code owners July 20, 2026 10:17
@adisatop
adisatop marked this pull request as draft July 20, 2026 10:17
@snyk-io

snyk-io Bot commented Jul 20, 2026

Copy link
Copy Markdown

Snyk checks have passed. No issues have been found so far.

Status Scan Engine Critical High Medium Low Total (0)
Open Source Security 0 0 0 0 0 issues
Licenses 0 0 0 0 0 issues
Code Security 0 0 0 0 0 issues

💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse.

@snyk-io

snyk-io Bot commented Jul 20, 2026

Copy link
Copy Markdown

Snyk checks have passed. No issues have been found so far.

Status Scan Engine Critical High Medium Low Total (0)
Open Source Security 0 0 0 0 0 issues
Licenses 0 0 0 0 0 issues
Code Security 0 0 0 0 0 issues
Secrets 0 0 0 0 0 issues

💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse.

@snyk-pr-review-bot

This comment has been minimized.

@adisatop
adisatop force-pushed the fix/IGNR-2204-bump-code-client-go branch from 70faf67 to def56d3 Compare July 20, 2026 13:14
@adisatop adisatop changed the title fix(deps): bump code-client-go to v1.29.0 [IGNR-2204] feat: add reviewedBy and reviewedOn to suppression details [IGNR-2204] Jul 20, 2026

@cbill-s cbill-s left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

We also need to update local_finding.sarif.tmpl for the changes to be written in the Sarif.

Comment thread internal/local_findings/source/openapi/rest/test.spec.yaml Outdated
@adisatop
adisatop requested a review from cbill-s July 20, 2026 14:39
Comment thread internal/local_findings/source/openapi/rest/test.spec.yaml Outdated
Comment thread internal/local_findings/source/openapi/rest/test.spec.yaml Outdated
{{- if and .Attributes.Suppression.Details.ReviewedOn .Attributes.Suppression.Details.ReviewedBy }}
Reviewed on: {{ .Attributes.Suppression.Details.ReviewedOn }}
Reviewed by name : {{ .Attributes.Suppression.Details.ReviewedBy.Name }}
Reviewed by email: {{ .Attributes.Suppression.Details.ReviewedBy.Email }}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

let's follow the same pattern as ignored - can we have something like this?

{{- if .Attributes.Suppression.Details.ReviewedOn }}
Reviewed on: {{ .Attributes.Suppression.Details.ReviewedOn }}
{{- end }}

{{- if and .Attributes.Suppression.Details.ReviewedBy .Attributes.Suppression.Details.ReviewedBy.Name}}
Reviewed by: {{ .Attributes.Suppression.Details.ReviewedBy.Name }}
{{- end }}

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.

I would suggest

{{- if .Attributes.Suppression.Details.ReviewedOn }}
Reviewed on: {{ .Attributes.Suppression.Details.ReviewedOn }}
{{- end }}

{{- if .Attributes.Suppression.Details.ReviewedBy .Attributes.Suppression.Details.ReviewedBy.Name}}
Reviewed by: {{ .Attributes.Suppression.Details.ReviewedBy.Name }}
{{- end }}

the and does not bring anything in this case, if I am not mistaken

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.

Comment thread internal/presenters/templates/local_finding.sarif.tmpl Outdated
@adisatop
adisatop requested a review from cbill-s July 21, 2026 13:06
@adisatop
adisatop marked this pull request as ready for review July 22, 2026 08:42
@snyk-pr-review-bot

This comment has been minimized.

Comment thread internal/presenters/templates/local_finding.sarif.tmpl
Comment thread internal/presenters/templates/local_finding.sarif.tmpl
Ignored on: {{ formatDatetime .Attributes.Suppression.Details.IgnoredOn "2006-01-02T15:04:05.999999999Z07:00" "January 02, 2006" }}
Ignored by: {{ .Attributes.Suppression.Details.IgnoredBy.Name }}
{{- if .Attributes.Suppression.Details.ReviewedOn }}
Reviewed on: {{ .Attributes.Suppression.Details.ReviewedOn }}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

ReviewedOn shown without datetime formatting

Low Severity

Reviewed on prints raw ReviewedOn, while Ignored on in the same block uses formatDatetime for a human-readable date. Review timestamps will appear as ISO strings in CLI output, inconsistent with neighboring suppression fields.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit b977b26. Configure here.

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.

Question: @adisatop I think this is a reasonable comment. or am I missing a reason why the date format should not be presented consistently?

type: object
required:
- name

@cbill-s cbill-s Jul 22, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

please remove the empty lines please

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

done

Comment thread pkg/local_workflows/local_models/transform.go Outdated
Comment thread internal/presenters/templates/local_finding.sarif.tmpl Outdated
Comment thread internal/presenters/templates/local_finding.sarif.tmpl
Comment thread pkg/local_workflows/local_models/models.gen.go
@snyk-pr-review-bot

This comment has been minimized.

Comment thread internal/presenters/templates/finding.component.tmpl Outdated

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Cursor Bugbot has reviewed your changes and found 1 potential issue.

There are 2 total unresolved issues (including 1 from previous review).

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 0daff2b. Configure here.

Comment thread internal/presenters/templates/local_finding.sarif.tmpl
@snyk-pr-review-bot

This comment has been minimized.

@snyk snyk deleted a comment from snyk-pr-review-bot Bot Jul 23, 2026
Comment thread go.mod
module github.com/snyk/go-application-framework

go 1.26
go 1.26.2

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.

is this an intentional change?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

yes, since bumped the code-client-go dep that using that version.

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.

please add table-driven validity tests for this

Reviewed on: {{ .Attributes.Suppression.Details.ReviewedOn }}
{{- end }}
{{- if .Attributes.Suppression.Details.ReviewedBy }}
Reviewed by name: {{ .Attributes.Suppression.Details.ReviewedBy.Name }}

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.

add test

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

done, thanks.

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.

Question: Should this really be Reviewed by name:? Or should it be something like Reviewed by: <Name> (<EMail>)


Expiration: {{- if .Attributes.Suppression.Details.Expiration }} {{ formatDatetime .Attributes.Suppression.Details.Expiration "2006-01-02T15:04:05.999999999Z07:00" "January 02, 2006" }} {{- else }} never {{- end }}
{{- if .Attributes.Suppression.Details.ReviewedOn }}
Reviewed on: {{ .Attributes.Suppression.Details.ReviewedOn }}

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.

add test

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

done. thanks.

@robertolopezlopez robertolopezlopez 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.

Hi @adisatop , please finish addressing the open threads (ok if you do not like the cursor suggested datetime formatting).

@snyk-pr-review-bot

This comment has been minimized.

{{- end }}
{{- if and $finding.Attributes.Suppression.Details.ReviewedBy }}
"reviewedBy": {
"name": {{ getQuotedString $finding.Attributes.Suppression.Details.ReviewedBy.Name }}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

add indent please on both name, email

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

done, thanks

"properties": {
"category": {{ getQuotedString $finding.Attributes.Suppression.Details.Category }},
{{- if $finding.Attributes.Suppression.Details.Expiration }}"expiration": "{{ $finding.Attributes.Suppression.Details.Expiration }}",{{- end }}
{{- if and $finding.Attributes.Suppression.Details.ReviewedOn }}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

we can remove the and here I believe

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

agree, done

"name": {{ getQuotedString $finding.Attributes.Suppression.Details.IgnoredBy.Name }},
"email": {{ getQuotedString $finding.Attributes.Suppression.Details.IgnoredBy.Email }}
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

please remove empty line

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

done

Remove redundant single-argument `and` conditionals and normalize
indentation/trailing whitespace in the reviewedBy/ignoredBy blocks of
the SARIF template. No change to rendered output.

Co-authored-by: Cursor <cursoragent@cursor.com>
@snyk-pr-review-bot

Copy link
Copy Markdown

PR Reviewer Guide 🔍

🧪 PR contains tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Inconsistent Formatting 🟡 [minor]

The newly added reviewedOn field is rendered raw in the CLI template, whereas existing date fields like ignoredOn and expiration (line 23 and 34) use the formatDatetime helper. This leads to inconsistent output for users where some dates are formatted (e.g., 'January 02, 2006') and others are printed as raw ISO strings.

Reviewed on: {{ .Attributes.Suppression.Details.ReviewedOn }}
📚 Repository Context Analyzed

This review considered 20 relevant code sections from 9 files (average relevance: 0.79)

🤖 Repository instructions applied (from AGENTS.md)

@PeterSchafer

Copy link
Copy Markdown
Contributor

Please open a CLI PR as well, to ensure that the changes integrate well.

Comment thread go.mod
github.com/rs/zerolog v1.34.0
github.com/sabhiram/go-gitignore v0.0.0-20210923224102-525f6e181f06
github.com/snyk/code-client-go v1.24.5
github.com/snyk/code-client-go v1.29.0

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.

Question: This is actually quite a bump. The CLI actually uses 1.27 but still there are some changes in where I wonder if this is tested enough snyk/code-client-go@v1.27.0...v1.29.0

@adisatop
adisatop requested a review from cbill-s July 27, 2026 14:35
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.

4 participants