-
Notifications
You must be signed in to change notification settings - Fork 295
fix: reply_to_pull_request_review_comment missing from config.json #20525
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
Changes from all commits
b2b20c4
407d5c4
2bac94a
b96bf1d
7af9252
580fc6d
e594710
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -546,3 +546,65 @@ func TestGenerateSafeOutputsConfigEmptyRepoMemory(t *testing.T) { | |
| _, hasPushRepoMemory := parsed["push_repo_memory"] | ||
| assert.False(t, hasPushRepoMemory, "push_repo_memory should not be present when Memories slice is empty") | ||
| } | ||
|
|
||
| // TestGenerateSafeOutputsConfigReplyToPullRequestReviewComment verifies that | ||
| // reply_to_pull_request_review_comment appears in config.json when configured. | ||
| // Previously this key was missing from generateSafeOutputsConfig, causing the | ||
| // safe-outputs MCP server to skip the tool at runtime. | ||
| func TestGenerateSafeOutputsConfigReplyToPullRequestReviewComment(t *testing.T) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Great test coverage! The comment explains exactly why this test exists (the tool was previously missing from |
||
| data := &WorkflowData{ | ||
| SafeOutputs: &SafeOutputsConfig{ | ||
| ReplyToPullRequestReviewComment: &ReplyToPullRequestReviewCommentConfig{ | ||
| BaseSafeOutputConfig: BaseSafeOutputConfig{Max: strPtr("25")}, | ||
| }, | ||
| }, | ||
| } | ||
|
|
||
| result := generateSafeOutputsConfig(data) | ||
| require.NotEmpty(t, result, "Expected non-empty config") | ||
|
|
||
| var parsed map[string]any | ||
| require.NoError(t, json.Unmarshal([]byte(result), &parsed), "Result must be valid JSON") | ||
|
|
||
| replyConfig, ok := parsed["reply_to_pull_request_review_comment"].(map[string]any) | ||
| require.True(t, ok, "Expected reply_to_pull_request_review_comment key in config.json") | ||
| assert.InDelta(t, float64(25), replyConfig["max"], 0.0001, "max should be 25") | ||
pelikhan marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| } | ||
|
|
||
| // TestGenerateSafeOutputsConfigReplyToPullRequestReviewCommentWithTarget verifies that | ||
| // target, target-repo, allowed_repos, and footer are forwarded to config.json. | ||
| func TestGenerateSafeOutputsConfigReplyToPullRequestReviewCommentWithTarget(t *testing.T) { | ||
| footerTrue := "true" | ||
| data := &WorkflowData{ | ||
| SafeOutputs: &SafeOutputsConfig{ | ||
| ReplyToPullRequestReviewComment: &ReplyToPullRequestReviewCommentConfig{ | ||
| BaseSafeOutputConfig: BaseSafeOutputConfig{Max: strPtr("10")}, | ||
| SafeOutputTargetConfig: SafeOutputTargetConfig{ | ||
| Target: "pull_request", | ||
| TargetRepoSlug: "org/other-repo", | ||
| AllowedRepos: []string{"org/other-repo"}, | ||
| }, | ||
| Footer: &footerTrue, | ||
| }, | ||
| }, | ||
| } | ||
|
|
||
| result := generateSafeOutputsConfig(data) | ||
| require.NotEmpty(t, result, "Expected non-empty config") | ||
|
|
||
| var parsed map[string]any | ||
| require.NoError(t, json.Unmarshal([]byte(result), &parsed), "Result must be valid JSON") | ||
|
|
||
| replyConfig, ok := parsed["reply_to_pull_request_review_comment"].(map[string]any) | ||
| require.True(t, ok, "Expected reply_to_pull_request_review_comment key in config.json") | ||
| assert.InDelta(t, float64(10), replyConfig["max"], 0.0001, "max should be 10") | ||
| assert.Equal(t, "pull_request", replyConfig["target"], "target should be set") | ||
| assert.Equal(t, "org/other-repo", replyConfig["target-repo"], "target-repo should be set") | ||
|
|
||
| allowedRepos, ok := replyConfig["allowed_repos"].([]any) | ||
| require.True(t, ok, "allowed_repos should be an array") | ||
| assert.Len(t, allowedRepos, 1, "Should have 1 allowed repo") | ||
| assert.Equal(t, "org/other-repo", allowedRepos[0], "allowed_repos entry should match") | ||
|
|
||
| assert.Equal(t, true, replyConfig["footer"], "footer should be true") | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good addition! This correctly follows the same pattern used for other safe-output tools like
ResolvePullRequestReviewThread. The nil-guard ensures the config key is only emitted when the tool is explicitly configured.