-
Notifications
You must be signed in to change notification settings - Fork 18
feat: deployment plan dry runs comment back to associated version creation pull request #999
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
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,267 @@ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| package deploymentplanresult | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import ( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "context" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "fmt" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "strings" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "github.com/google/go-github/v66/github" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "github.com/pmezard/go-difflib/difflib" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| gh "workspace-engine/pkg/github" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "workspace-engine/pkg/oapi" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const ( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| metaGitHubOwner = "github/owner" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| metaGitHubRepo = "github/repo" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| metaGitSHA = "git/sha" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| type prCommentResult struct { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| AgentID string | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| AgentName string | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| AgentType string | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Status string | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| HasChanges bool | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Current string | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Proposed string | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Message string | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| func commentMarker(targetID string) string { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return fmt.Sprintf("<!-- ctrlplane-plan-target:%s -->", targetID) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| func agentSectionStart(agentID string) string { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return fmt.Sprintf("<!-- agent:%s:start -->", agentID) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| func agentSectionEnd(agentID string) string { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return fmt.Sprintf("<!-- agent:%s:end -->", agentID) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| func wrapAgentSection(agentID, content string) string { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return agentSectionStart(agentID) + "\n" + content + agentSectionEnd(agentID) + "\n" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| func replaceOrAppendAgentSection(body, agentID, section string) string { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| start := agentSectionStart(agentID) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| end := agentSectionEnd(agentID) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| startIdx := strings.Index(body, start) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| endIdx := strings.Index(body, end) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if startIdx >= 0 && endIdx >= 0 && endIdx > startIdx { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return body[:startIdx] + wrapAgentSection(agentID, section) + body[endIdx+len(end):] | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return body + "\n" + wrapAgentSection(agentID, section) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| func formatResultSection(r prCommentResult) string { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| var sb strings.Builder | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| fmt.Fprintf(&sb, "**%s** · `%s`\n", r.AgentName, r.AgentType) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| switch r.Status { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| case "errored": | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| fmt.Fprintf(&sb, "> ❌ Error: %s\n", r.Message) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return sb.String() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| case "unsupported": | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| fmt.Fprintf(&sb, "> ⚠️ Agent does not support plan operations\n") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return sb.String() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if !r.HasChanges { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| sb.WriteString("> No changes\n") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return sb.String() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| diff, err := difflib.GetUnifiedDiffString(difflib.UnifiedDiff{ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| A: difflib.SplitLines(r.Current), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| B: difflib.SplitLines(r.Proposed), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| FromFile: "current", | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ToFile: "proposed", | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Context: 3, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if err != nil { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| diff = "Failed to compute diff" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| sb.WriteString("<details>\n<summary>Changes detected</summary>\n\n") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| sb.WriteString("```diff\n") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| sb.WriteString(diff) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| sb.WriteString("```\n") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| sb.WriteString("</details>\n") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+78
to
+93
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| sb.WriteString("<details>\n<summary>Changes detected</summary>\n\n") | |
| sb.WriteString("```diff\n") | |
| sb.WriteString(diff) | |
| sb.WriteString("```\n") | |
| sb.WriteString("</details>\n") | |
| const maxDiffChars = 50000 | |
| func formatResultSection(r prCommentResult) string { | |
| var sb strings.Builder | |
| fmt.Fprintf(&sb, "**%s** · `%s`\n", r.AgentName, r.AgentType) | |
| if err != nil { | |
| diff = "Failed to compute diff" | |
| } | |
| if len(diff) > maxDiffChars { | |
| diff = diff[:maxDiffChars] + "\n...diff truncated..." | |
| } | |
| sb.WriteString("<details>\n<summary>Changes detected</summary>\n\n") | |
| sb.WriteString(" |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/workspace-engine/svc/controllers/deploymentplanresult/github_comment.go`
around lines 89 - 93, formatResultSection currently writes the full unified diff
into the GitHub comment body which can exceed the API 65,536-character limit;
before writing the diff (sb.WriteString(diff)) truncate it to a safe threshold
(e.g., 50,000 characters) and append a clear marker such as "\n...[truncated]"
so the comment remains valid. Update the same truncation logic where you call
EditComment (the EditComment invocation referenced in the review) so both
CreateComment and EditComment use the same truncatedDiff variable; implement the
truncation as a small helper (e.g., truncateDiff(diff string, max int) string)
and replace direct uses of diff with truncateDiff(diff, 50000) in
formatResultSection and the EditComment call.
Copilot
AI
Apr 15, 2026
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.
MaybeCommentOnPR depends on specific DeploymentVersion.Metadata keys (github/owner, github/repo, git/sha) but the function comment only mentions “GitHub info” generically. Please document the exact required keys (and expected formats) so callers/workflows know what must be set for PR commenting to work.
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.
🧩 Analysis chain
🌐 Web query:
For the GitHub REST endpoint "List pull requests associated with a commit" (and go-github v66 PullRequests.ListPullRequestsWithCommit), what ordering guarantees exist? Can per_page=1 miss an open PR when multiple PRs reference the same commit SHA?
💡 Result:
No explicit ordering guarantees are documented for the GitHub REST API endpoint GET /repos/{owner}/{repo}/commits/{commit_sha}/pulls or the go-github v66 PullRequests.ListPullRequestsWithCommit method. The official documentation does not specify sort order, direction, or stability of results. It supports standard pagination with per_page (default 30, max 100) and page parameters. The endpoint returns merged PRs that introduced the commit if present in the default branch, or both merged and open PRs otherwise. Multiple PRs can reference the same commit SHA (e.g., cherry-picked commits), so lists can exceed length 1. With per_page=1, only the first page (1 result) is returned. Since ordering is unspecified, yes, it can miss an open PR if >1 PRs exist and the open PR is not first in the undocumented order. To detect all PRs including open ones reliably, fetch all pages (check Link headers or iterate until empty response).
Citations:
- 1: https://github.com/google/go-github/blob/master/github/pulls.go
- 2: https://docs.github.com/en/rest/commits/commits
- 3: https://docs.github.com/v3/repos/commits
- 4: https://docs.github.com/rest/commits/commits
🏁 Script executed:
cd apps/workspace-engine && find . -name "github_comment.go" -type fRepository: ctrlplanedev/ctrlplane
Length of output: 122
🏁 Script executed:
head -220 apps/workspace-engine/svc/controllers/deploymentplanresult/github_comment.go | tail -50Repository: ctrlplanedev/ctrlplane
Length of output: 1200
PR lookup can select the wrong PR because only one result is fetched.
Using PerPage: 1 risks missing an open PR associated with the commit and then selecting a closed PR fallback. GitHub's REST API does not document ordering guarantees for the list pull requests endpoint, so an open PR may not be the first result returned. Fetch a larger page size and iterate through all results to find an open PR before falling back to the first result.
Suggested fix
func findPRForSHA(
ctx context.Context,
client *github.Client,
owner, repo, sha string,
) (int, error) {
- prs, _, err := client.PullRequests.ListPullRequestsWithCommit(
- ctx, owner, repo, sha, &github.ListOptions{PerPage: 1},
- )
- if err != nil {
- return 0, fmt.Errorf("list PRs for commit %s: %w", sha, err)
- }
- for _, pr := range prs {
- if pr.GetState() == "open" {
- return pr.GetNumber(), nil
- }
- }
- if len(prs) > 0 {
- return prs[0].GetNumber(), nil
- }
- return 0, nil
+ opts := &github.ListOptions{PerPage: 100}
+ firstPR := 0
+ for {
+ prs, resp, err := client.PullRequests.ListPullRequestsWithCommit(
+ ctx, owner, repo, sha, opts,
+ )
+ if err != nil {
+ return 0, fmt.Errorf("list PRs for commit %s: %w", sha, err)
+ }
+ for _, pr := range prs {
+ if firstPR == 0 {
+ firstPR = pr.GetNumber()
+ }
+ if pr.GetState() == "open" {
+ return pr.GetNumber(), nil
+ }
+ }
+ if resp.NextPage == 0 {
+ break
+ }
+ opts.Page = resp.NextPage
+ }
+ return firstPR, nil
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| prs, _, err := client.PullRequests.ListPullRequestsWithCommit( | |
| ctx, owner, repo, sha, &github.ListOptions{PerPage: 1}, | |
| ) | |
| func findPRForSHA( | |
| ctx context.Context, | |
| client *github.Client, | |
| owner, repo, sha string, | |
| ) (int, error) { | |
| opts := &github.ListOptions{PerPage: 100} | |
| firstPR := 0 | |
| for { | |
| prs, resp, err := client.PullRequests.ListPullRequestsWithCommit( | |
| ctx, owner, repo, sha, opts, | |
| ) | |
| if err != nil { | |
| return 0, fmt.Errorf("list PRs for commit %s: %w", sha, err) | |
| } | |
| for _, pr := range prs { | |
| if firstPR == 0 { | |
| firstPR = pr.GetNumber() | |
| } | |
| if pr.GetState() == "open" { | |
| return pr.GetNumber(), nil | |
| } | |
| } | |
| if resp.NextPage == 0 { | |
| break | |
| } | |
| opts.Page = resp.NextPage | |
| } | |
| return firstPR, nil | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/workspace-engine/svc/controllers/deploymentplanresult/github_comment.go`
around lines 184 - 186, The current call to
client.PullRequests.ListPullRequestsWithCommit uses github.ListOptions{PerPage:
1}, which can return a closed PR first and cause wrong selection; change the
logic in github_comment.go to fetch a larger page size (e.g., PerPage: 100) and
paginate through results from ListPullRequestsWithCommit, iterating all returned
PRs (the prs slice) to prefer a PR with State == "open" before falling back to
the first result; ensure you loop over pages (incrementing ListOptions.Page)
until you find an open PR or exhaust results, then use that chosen PR.
Copilot
AI
Apr 15, 2026
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.
findPRForSHA requests only 1 PR (PerPage: 1), which makes the subsequent loop that prefers an open PR ineffective and can easily select the wrong PR (or none) if the first returned PR isn’t the desired open one. Increase PerPage and/or paginate until an open PR is found (falling back only after exhausting results).
| prs, _, err := client.PullRequests.ListPullRequestsWithCommit( | |
| ctx, owner, repo, sha, &github.ListOptions{PerPage: 1}, | |
| ) | |
| if err != nil { | |
| return 0, fmt.Errorf("list PRs for commit %s: %w", sha, err) | |
| } | |
| for _, pr := range prs { | |
| if pr.GetState() == "open" { | |
| return pr.GetNumber(), nil | |
| } | |
| } | |
| if len(prs) > 0 { | |
| return prs[0].GetNumber(), nil | |
| } | |
| return 0, nil | |
| opts := &github.ListOptions{PerPage: 100, Page: 1} | |
| fallbackPRNumber := 0 | |
| for { | |
| prs, resp, err := client.PullRequests.ListPullRequestsWithCommit( | |
| ctx, owner, repo, sha, opts, | |
| ) | |
| if err != nil { | |
| return 0, fmt.Errorf("list PRs for commit %s: %w", sha, err) | |
| } | |
| for _, pr := range prs { | |
| if fallbackPRNumber == 0 { | |
| fallbackPRNumber = pr.GetNumber() | |
| } | |
| if pr.GetState() == "open" { | |
| return pr.GetNumber(), nil | |
| } | |
| } | |
| if resp == nil || resp.NextPage == 0 { | |
| break | |
| } | |
| opts.Page = resp.NextPage | |
| } | |
| return fallbackPRNumber, nil |
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.
Comment posting failures are non-recoverable right now.
If
MaybeCommentOnPRfails (rate limit, transient GitHub outage), the job still completes and no retry path exists, so PR diff visibility can be permanently lost for that run.Also applies to: 136-149, 201-212