Skip to content

Fix resolution comment length bug when migrating Secret Scanning alerts #1349

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

Copilot
Copy link
Contributor

@Copilot Copilot AI commented Jun 7, 2025

Problem

When migrating secret scanning alerts with gh gei migrate-secret-alerts, if the new comment format [@resolverName] originalComment exceeds 270 characters, the GitHub API call fails with an error about comment length limits.

This was introduced in PR #1337 which added the resolver name prefix to resolution comments, but didn't account for the 270 character limit.

Solution

Added length validation before using the prefixed comment format:

  • If [@resolverName] originalComment is < 270 characters: use the prefixed format (preserves existing behavior)
  • If [@resolverName] originalComment is >= 270 characters: truncate the original comment to make it fit

This ensures migration completes successfully while preserving the most important content when length limits are exceeded.

Changes

  • SecretScanningAlertService.cs: Added comment length check with fallback logic
  • SecretScanningAlertServiceTests.cs: Added test cases for both scenarios:
    • Update_With_Long_Comment_Uses_Original_Comment_Only - verifies fallback behavior
    • Update_With_Short_Comment_Uses_Prefixed_Comment - ensures existing behavior preserved

Example

// Before: Always used prefixed format, could fail on long comments
var targetResolutionComment = $"[@{resolverName}] {originalComment}";

// After: Length-aware with fallback
var prefixedComment = $"[@{resolverName}] {originalComment}";
var targetResolutionComment = prefixedComment.Length < 270 
    ? prefixedComment 
    : originalComment ?? string.Empty;

All existing tests pass. The fix is minimal and preserves all existing functionality while gracefully handling the edge case.

Fixes #1348.

Warning

Firewall rules blocked me from connecting to one or more addresses

I tried to connect to the following addresses, but was blocked by firewall rules:

  • http://www.microsoft.com:80/pkiops/crl/Microsoft%20Identity%20Verification%20Root%20Certificate%20Authority%202020.crl
    • Triggering command: dotnet build src/OctoshiftCLI.sln (http block)
  • http://www.microsoft.com:80/pkiops/crl/Microsoft%20Public%20RSA%20Timestamping%20CA%202020.crl
    • Triggering command: dotnet build src/OctoshiftCLI.sln (http block)

If you need me to access, download, or install something from one of these locations, you can either:


💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.

Copilot AI and others added 2 commits June 7, 2025 04:26
Co-authored-by: dylan-smith <1508559+dylan-smith@users.noreply.github.com>
Co-authored-by: dylan-smith <1508559+dylan-smith@users.noreply.github.com>
@Copilot Copilot AI changed the title [WIP] Resolution comment length bug when Secret Scanning alerts Fix resolution comment length bug when migrating Secret Scanning alerts Jun 7, 2025
@Copilot Copilot AI requested a review from dylan-smith June 7, 2025 04:39
Copilot finished work on behalf of dylan-smith June 7, 2025 04:39
@theztefan
Copy link
Contributor

@dylan-smith I suggest we implement the other option. If new comment = concat original author + original comment has length > 270, trim the new content to the acceptable length (<=270). In practice these migrations are commonly provided as service and executed by a central team therefore preserving the original author (and therefore trail) gives better confidence to customers.

…riginal comment

Co-authored-by: dylan-smith <1508559+dylan-smith@users.noreply.github.com>
@dylan-smith
Copy link
Collaborator

The build gave this error:
/Users/runner/work/gh-gei/gh-gei/src/Octoshift/Services/SecretScanningAlertService.cs(84,66): warning IDE0057: Substring can be simplified

Make sure you run dotnet format src/OctoshiftCLI.sln before every commit.

Co-authored-by: dylan-smith <1508559+dylan-smith@users.noreply.github.com>
Copy link
Contributor Author

Copilot AI commented Jun 26, 2025

The build gave this error:
/Users/runner/work/gh-gei/gh-gei/src/Octoshift/Services/SecretScanningAlertService.cs(84,66): warning IDE0057: Substring can be simplified

...

Fixed the IDE0057 warning by running dotnet format which simplified the Substring call to use modern C# range syntax. The build now completes without warnings. Commit: 8f06e3a

@Copilot Copilot AI requested a review from dylan-smith June 26, 2025 14:10
Copilot finished work on behalf of dylan-smith June 26, 2025 14:10
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.

Resolution comment length bug when Secret Scanning alerts
3 participants