Migrate generic and AI-detected secret scanning alerts (fixes #1595) - #1613
Open
SAIKARTHIKGOTURI wants to merge 2 commits into
Open
SAIKARTHIKGOTURI wants to merge 2 commits into
SAIKARTHIKGOTURI wants to merge 2 commits into
Conversation
Fixes github#1595. The secret-scanning list-alerts endpoint returns only default (provider) and custom pattern alerts unless non-default secret types are requested explicitly. GetSecretScanningAlertsForRepository now performs a second call filtered by the generic and AI-detected secret type slugs and merges the results (de-duplicated by alert number) so those alerts are migrated too. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Unit Test Results 1 files 1 suites 23s ⏱️ Results for commit 1a25503. ♻️ This comment has been updated with latest results. |
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Unsupported secret types can cause migrations against older GHES versions to fail.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Extends secret-alert migration to include generic and AI-detected patterns.
Changes:
- Fetches and deduplicates non-default alerts.
- Adds unit coverage and release notes.
File summaries
| File | Description |
|---|---|
| src/OctoshiftCLI.Tests/Octoshift/Services/GithubApiTests.cs | Tests non-default alert retrieval. |
| src/Octoshift/Services/GithubApi.cs | Fetches generic and AI-detected alerts. |
| RELEASENOTES.md | Documents the migration fix. |
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 2
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Address Copilot review feedback on PR github#1613: - GetSecretScanningAlertsForRepository now wraps only the non-default (secret_type-filtered) call and degrades gracefully on the HTTP 422 'Validation Failed' (unsupported) response older GHES targets return when a requested generic/AI-detected secret type is not available. The default/custom call still runs first and its result always survives; all other failures continue to propagate. A warning is logged via an optional OctoLogger now passed through GithubApiFactory. - Corrected the comments that wrongly claimed unsupported slugs return no rows (they cause a 422, handled above). - Reworked the merge test so the same alert number appears in both responses, proving DistinctBy(Number) actually dedups. - Added tests for the 422 graceful-degradation path (default alerts kept, warning logged) and for a non-422 error still propagating. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The compatibility path can suppress unrelated validation errors and repeatedly retries an expected non-transient response.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (1)
src/Octoshift/Services/GithubApi.cs:1051
- This method queries both source and target repositories (
SecretScanningAlertService.cs:31-32), so an older source can also reach this handler. Calling it “the target” then misidentifies which side skipped alerts; use role-neutral wording unless the caller supplies the repository role.
_log?.LogWarning(
"The target does not support one or more of the generic or AI-detected secret scanning types, " +
"so alerts of those types will not be migrated. This is expected on older GitHub Enterprise Server " +
"versions where some of these secret types are not available.");
- Files reviewed: 4/4 changed files
- Comments generated: 2
- Review effort level: Balanced
| { | ||
| nonDefaultAlerts = await _client.GetAllAsync(nonDefaultUrl).ToListAsync(); | ||
| } | ||
| catch (HttpRequestException ex) when (ex.StatusCode == HttpStatusCode.UnprocessableEntity && ex.Message.Contains("secret_type")) |
| var nonDefaultAlerts = new List<JToken>(); | ||
| try | ||
| { | ||
| nonDefaultAlerts = await _client.GetAllAsync(nonDefaultUrl).ToListAsync(); |
Author
|
@brianaj, can you please look into this ? this problem is being faced by many users and fix to this will be really helpful. |
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.
What
migrate-secret-alertsonly migrated default (provider) + custom secret scanning alerts. Generic patterns (e.g.rsa_private_key, connection strings) and the AI-detectedpasswordpattern were silently skipped, so those alerts stayed open on the target even when resolved on the source.Why
GET /repos/{org}/{repo}/secret-scanning/alertsreturns only default/custom patterns unless non-default secret types are requested explicitly viasecret_type.exclude_secret_typesdoes not surface them either, so the non-default types must be enumerated.How
NonDefaultSecretTypeslist (10 generic slugs +password) with doc links and a "last verified" date.GetSecretScanningAlertsForRepositorynow makes a second call with&secret_type=<joined list>and merges results with aDistinctBy(Number)safety net.unsupported) for the whole request rather than omitting that slug. Only the non-default call is wrapped: on that specific 422 it logs a warning and continues migrating the default/custom alerts. The default/custom call always runs first and its result is never lost, and all other failures (auth, 5xx, unrelated 422s, network) still propagate.Supersedes #1596 (adds the missing AI-detected
passwordtype, the 422 compatibility handling, tests, dedup guard, and docs).Testing
dotnet build src/OctoshiftCLI.sln— succeeds.dotnet test --filter FullyQualifiedName~SecretScanning— 38/38 pass.Fixes #1595
migrate-secret-alertsis unchanged from the user's perspective (it now simply migrates more of the existing alert types)ThirdPartyNotices.txt(if applicable) — N/A, no new dependencies