Skip to content

Add Go rewrite of autosolve actions#8

Closed
fantapop wants to merge 17 commits intoCNSL-1944-generic-autosolve-git-hub-workflow-for-automated-issue-resolutionfrom
autosolve-go-rewrite
Closed

Add Go rewrite of autosolve actions#8
fantapop wants to merge 17 commits intoCNSL-1944-generic-autosolve-git-hub-workflow-for-automated-issue-resolutionfrom
autosolve-go-rewrite

Conversation

@fantapop
Copy link
Copy Markdown
Contributor

Single Go binary (autosolve) replaces the bash script chain with typed config, mockable interfaces for Claude CLI and GitHub API, embedded prompt templates, and 45 unit tests. Composite action YAMLs reduced to two steps each (build + run).

Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR introduces a Go-based autosolve CLI to replace the prior bash-script chain for the autosolve GitHub Actions, aiming for typed configuration, testable interfaces, and embedded prompt templates.

Changes:

  • Add a new Go module (autosolve-go) implementing assess/implement/security/prompt subcommands with Claude CLI + GitHub API wrappers.
  • Embed prompt templates (security preamble + assessment/implementation footers) and generate prompts on the fly.
  • Update composite GitHub Actions to build and run the new Go binary in two steps (build + run).

Reviewed changes

Copilot reviewed 23 out of 24 changed files in this pull request and generated 7 comments.

Show a summary per file
File Description
autosolve-go/go.mod Introduces Go module definition and dependencies for the autosolve CLI.
autosolve-go/go.sum Adds dependency checksums.
autosolve-go/Makefile Adds basic build/test/clean targets for the Go implementation.
autosolve-go/cmd/autosolve/main.go Adds the main CLI entrypoint and subcommand wiring for GitHub Actions usage.
autosolve-go/internal/action/action.go Adds helpers for GitHub Actions outputs, summaries, and log annotations.
autosolve-go/internal/action/action_test.go Adds unit tests for GitHub Actions I/O helpers.
autosolve-go/internal/assess/assess.go Implements the assessment phase orchestration.
autosolve-go/internal/assess/assess_test.go Adds unit tests for assessment phase behavior and outputs.
autosolve-go/internal/claude/claude.go Implements Claude CLI runner and output parsing helpers.
autosolve-go/internal/claude/claude_test.go Adds unit tests for Claude JSON parsing and marker detection.
autosolve-go/internal/config/config.go Adds typed config loading/validation from GitHub Actions env inputs.
autosolve-go/internal/config/config_test.go Adds unit tests for config validation and parsing behavior.
autosolve-go/internal/github/github.go Adds a gh-CLI-backed GitHub client wrapper interface/implementation.
autosolve-go/internal/implement/implement.go Implements retry orchestration, security checks, and PR creation flow.
autosolve-go/internal/implement/implement_test.go Adds unit tests for retry behavior, output writing, and orchestration outcomes.
autosolve-go/internal/prompt/prompt.go Implements prompt assembly from embedded templates and inputs.
autosolve-go/internal/prompt/prompt_test.go Adds unit tests for prompt assembly variants and error cases.
autosolve-go/internal/prompt/templates/security-preamble.md Adds embedded security preamble template for the Claude prompt.
autosolve-go/internal/prompt/templates/assessment-footer.md Adds embedded assessment footer template and output marker requirement.
autosolve-go/internal/prompt/templates/implementation-footer.md Adds embedded implementation footer template and output marker requirement.
autosolve-go/internal/security/security.go Adds working-tree blocked-path checks and symlink detection.
autosolve-go/internal/security/security_test.go Adds unit tests for security blocked-path detection and staging behavior.
autosolve-go/assess/action.yml Updates assess composite action to build/run the Go binary.
autosolve-go/implement/action.yml Updates implement composite action to build/run the Go binary.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +237 to +245
p := cfg.Prompt
if p == "" {
p = "automated change"
}
if len(p) > 72 {
p = p[:72]
}
subject = "autosolve: " + p
}
Copy link

Copilot AI Mar 19, 2026

Choose a reason for hiding this comment

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

When falling back to deriving the commit subject from cfg.Prompt, the prompt can contain newlines or leading/trailing whitespace, which can produce a malformed multi-line subject (and an odd PR title derived from it). Consider normalizing to the first line, trimming whitespace, and then enforcing the 72-character limit.

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done. Now normalizes to first line, trims whitespace, and enforces the 72-char limit (named const maxCommitSubjectLen). </claude>

Comment on lines +30 to +45
// Check symlinks pointing into blocked paths
info, err := os.Lstat(file)
if err != nil {
continue
}
if info.Mode()&os.ModeSymlink != 0 {
target, err := filepath.EvalSymlinks(file)
if err != nil {
continue
}
for _, blocked := range blockedPaths {
if strings.Contains(target, "/"+blocked) {
violations = append(violations, fmt.Sprintf("symlink to blocked path: %s -> %s", file, target))
}
}
}
Copy link

Copilot AI Mar 19, 2026

Choose a reason for hiding this comment

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

The symlink protection is incomplete and can be bypassed:

  • It only checks EvalSymlinks when the changed file itself is a symlink, so changes under a symlinked directory (e.g. allowed_dir/file where allowed_dir -> secrets/) won't be detected.
  • The strings.Contains(target, "/"+blocked) check is not a reliable path-boundary check and can miss cases like a symlink resolving exactly to a blocked directory path.
    Consider resolving the real path for every changed file (or its parent dir if needed), then compare against cleaned absolute blocked prefixes (repoRoot + blocked) using filepath.Clean + prefix checks with path separators.

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done. Now resolves real path for every changed file (not just symlinks), and compares against cleaned absolute blocked path prefixes using filepath.Clean + separator-aware prefix checks. </claude>

Comment on lines +19 to +21
delim := fmt.Sprintf("GHEOF_%d", os.Getpid())
content := fmt.Sprintf("%s<<%s\n%s\n%s", key, delim, value, delim)
appendToFile(os.Getenv("GITHUB_OUTPUT"), content)
Copy link

Copilot AI Mar 19, 2026

Choose a reason for hiding this comment

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

The multiline output delimiter is derived only from the PID (GHEOF_<pid>). If the output value happens to contain that delimiter (or if multiple processes write to the same output file), the GitHub Actions output file can become malformed. Prefer a cryptographically-random delimiter (and/or ensure the delimiter does not appear in value) to make this robust.

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done. Now uses crypto/rand to generate a 16-byte hex suffix for the delimiter, with PID-based fallback only if rand fails. </claude>

Comment on lines +121 to +122
_ = filepath.Base(dir) // use filepath to avoid unused import
}
Copy link

Copilot AI Mar 19, 2026

Choose a reason for hiding this comment

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

The filepath import is only kept alive via _ = filepath.Base(dir); this makes the test harder to read and is easy to forget about later. Remove the unused import and the dummy reference instead of using a no-op line.

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done. Removed both the dummy _ = filepath.Base(dir) line and the path/filepath import. </claude>

Comment on lines +109 to +112
}
_ = out // TODO: parse JSON response
return nil, nil
}
Copy link

Copilot AI Mar 19, 2026

Choose a reason for hiding this comment

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

ListPRs currently discards the gh pr list JSON output and returns (nil, nil) even on success. This makes it very easy for callers to mis-handle the result (nil slice with no error) and hides incomplete functionality behind a seemingly valid API. Either parse and return the PRs, or return a clear "not implemented" error until it's needed.

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done. Removed ListPRs entirely along with the ListPRsOptions and PR types — they weren't needed. </claude>

Comment on lines +211 to +212
// Stage tracked modifications
_ = exec.Command("git", "add", "--update").Run()
Copy link

Copilot AI Mar 19, 2026

Choose a reason for hiding this comment

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

git add --update only stages modifications/deletions of already-tracked files. If Claude creates new files (a common outcome), they will not be included in the commit and this can also cause a false "no changes to commit" error. Use a staging command that includes new files as well (e.g. git add -A) after removing the autosolve metadata files.

Suggested change
// Stage tracked modifications
_ = exec.Command("git", "add", "--update").Run()
// Stage all changes (including new files and deletions)
_ = exec.Command("git", "add", "-A").Run()

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done. Changed to git add -A to include new files created by Claude. </claude>

Comment on lines +66 to +67
opts.Resume = sessionID
opts.RetryPrompt = retryPrompt
Copy link

Copilot AI Mar 19, 2026

Choose a reason for hiding this comment

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

Retry attempts always set opts.RetryPrompt, but if sessionID is empty then CLIRunner.Run won't provide any stdin (it only uses RetryPrompt when Resume is set). This can cause the retry invocation to hang or fail unexpectedly. Handle the empty-session case explicitly (e.g., re-run with the original prompt file, or treat it as a hard error before retrying).

Suggested change
opts.Resume = sessionID
opts.RetryPrompt = retryPrompt
if sessionID == "" {
action.LogWarning("No session ID from previous attempt; restarting conversation with original prompt")
opts.PromptFile = promptFile
} else {
opts.Resume = sessionID
opts.RetryPrompt = retryPrompt
}

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done. Now explicitly handles empty session: logs a warning and restarts with the original prompt file instead of hanging. </claude>

@fantapop fantapop force-pushed the autosolve-go-rewrite branch from 7f74273 to 3b9b49a Compare March 19, 2026 21:26
}

// PROptions configures PR creation.
type PROptions struct {
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This type name is hard to read. Let's change it to PullRequestOptions

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done. Renamed to PullRequestOptions. </claude>

}

// PR represents a pull request.
type PR struct {
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

lets' change to PullRequest

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Removed this type entirely along with ListPRs — it wasn't used. </claude>

}

// GHClient implements Client by shelling out to the gh CLI.
type GHClient struct {
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Let's change to GithubClient

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done. Renamed to GithubClient. </claude>

if err != nil {
return nil, fmt.Errorf("listing PRs: %w", err)
}
_ = out // TODO: parse JSON response
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Let's not leave TODO comments. Is there something to be done here?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Removed — the TODO was for implementing ListPRs which we removed entirely. </claude>

if positive {
action.LogNotice(fmt.Sprintf("Implementation succeeded on attempt %d", attempt))
implStatus = "SUCCESS"
_ = os.WriteFile(resultFile, []byte(resultText), 0644)
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Is this skipping an error check? I see some later in this file as well. Ideally we would check those

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done. All os.WriteFile calls now have error checks — warnings are logged if writes fail. </claude>

if commitBody != "" {
commitMsg += "\n\n" + commitBody
}
commitMsg += "\n\nCo-Authored-By: Claude <noreply@anthropic.com>"
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The signature should be pulled out to a configuration value. It can default to this.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done. Added CommitSignature config field (defaults to Co-Authored-By: Claude <noreply@anthropic.com>, configurable via INPUT_COMMIT_SIGNATURE). </claude>

if err != nil {
return
}
_ = os.WriteFile(filepath.Join(tmpDir, "autosolve-pr-body"), data, 0644)
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

this seems like an error we should check

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done. Error checked — logs a warning if the write fails. </claude>

}
}

body += "\n\n---\n\n*This PR was auto-generated by [claude-autosolve-action](https://github.com/cockroachdb/actions) using Claude Code.*\n*Please review carefully before approving.*"
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Let's make this controllable via configuration

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done. Added PRFooter config field (configurable via INPUT_PR_FOOTER), with a sensible default. </claude>

if prompt != "" {
return prompt
}
return fmt.Sprintf("Fix GitHub issue #%s.\nTitle: %s\nBody: %s", issueNumber, issueTitle, issueBody)
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It would be nice if we exposed this as a configurable template somehow...

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done. BuildIssuePrompt now accepts an optional template string with {{ISSUE_NUMBER}}, {{ISSUE_TITLE}}, and {{ISSUE_BODY}} placeholders (configurable via INPUT_ISSUE_PROMPT_TEMPLATE). Falls back to a sensible default. </claude>

Comment on lines +64 to +69
for _, args := range [][]string{
{"git", "diff", "--name-only"},
{"git", "diff", "--name-only", "--cached"},
{"git", "ls-files", "--others", "--exclude-standard"},
} {
out, err := exec.Command(args[0], args[1:]...).Output()
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Ideally this could use a git client hidden behind an interface. discussed earlier.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done. security.Check now takes a git.Client parameter. Tests updated to pass &git.CLIClient{}. </claude>

@fantapop fantapop force-pushed the autosolve-go-rewrite branch 9 times, most recently from a898747 to c102f72 Compare March 20, 2026 22:38
- name: Move credentials out of workspace
if: inputs.auth_mode == 'vertex' && inputs.vertex_workload_identity_provider != ''
shell: bash
run: |
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🚫 [actionlint] reported by reviewdog 🐶
shellcheck reported issue in this script: SC2129:style:6:3: Consider using { cmd1; cmd2; } >> file instead of individual redirects [shellcheck]

@fantapop fantapop force-pushed the autosolve-go-rewrite branch from c102f72 to 0202471 Compare March 20, 2026 22:46
fantapop and others added 3 commits March 20, 2026 15:51
Co-Authored-By: roachdev-claude <roachdev-claude-bot@cockroachlabs.com>
Co-Authored-By: roachdev-claude <roachdev-claude-bot@cockroachlabs.com>
Co-Authored-By: roachdev-claude <roachdev-claude-bot@cockroachlabs.com>
@fantapop fantapop force-pushed the autosolve-go-rewrite branch from 0202471 to 3f51ac4 Compare March 20, 2026 22:51
fantapop and others added 9 commits March 24, 2026 12:31
…ge tracking

- Add precompiled linux/amd64 binary with build.sh and CI staleness check
- Refactor workflow: inline simple ops (comments, labels, prompt, PR check)
  as gh/bash, side-by-side checkout layout, remove unused AUTOSOLVE_BIN env
- Remove ensureDefaultBranch (checkout layout makes it unnecessary)
- Remove DefaultBranch from GitHub client interface
- Simplify main.go to just assess and implement commands
- AI security review: per-file diffs with batching instead of truncation,
  skip generated files, fail on errors instead of silently skipping
- Add token usage and cost tracking per section with markdown summary table

Co-Authored-By: roachdev-claude <roachdev-claude-bot@cockroachlabs.com>
Remove quotes around binary path expression — YAML parser treats
the quoted ${{ }} expression as a string value ending at the closing
quote, leaving the subcommand as an unexpected token.

Co-Authored-By: roachdev-claude <roachdev-claude-bot@cockroachlabs.com>
Assess saves its usage to a shared file ($RUNNER_TEMP/autosolve-usage.json).
Implement loads it and combines with its own usage so the step summary
shows a single table covering all phases.

Co-Authored-By: roachdev-claude <roachdev-claude-bot@cockroachlabs.com>
Bash double quotes allow backtick command substitution, so untrusted
content (issue body, Claude summaries) containing markdown backticks
like `config/credential.go` gets executed as shell commands. Use printf
with %s format to treat values as literal strings.

This is a security fix: a malicious issue body could contain backticks
with arbitrary commands.

Co-Authored-By: roachdev-claude <roachdev-claude-bot@cockroachlabs.com>
When the workflow short-circuits because a PR already exists or the
assessment result is SKIP, write a step summary explaining why and
what to do next.

Co-Authored-By: roachdev-claude <roachdev-claude-bot@cockroachlabs.com>
The run() and output() helpers were discarding git's stderr, so when
git commands failed (e.g., exit code 129/SIGTERM) we lost the actual
error message. Now stderr flows to the action log.

Co-Authored-By: roachdev-claude <roachdev-claude-bot@cockroachlabs.com>
defaults.run.working-directory does not propagate into composite action
steps. Add a working_directory input to both actions (defaults to ".")
and pass REPO_DIR from the reusable workflow. Extract "repo" into a
REPO_DIR env var for single-source-of-truth.

Co-Authored-By: roachdev-claude <roachdev-claude-bot@cockroachlabs.com>
Remove the usage table from the assess and implement step summaries
so it isn't duplicated. Add a usage-summary command to the binary
that reads the shared usage file and writes the combined table. The
reusable workflow calls it in an always() step at the end, producing
a single token usage section after all other summaries.

Co-Authored-By: roachdev-claude <roachdev-claude-bot@cockroachlabs.com>
The previous approach checked out the actions repo separately to find the
binary, but it defaulted to the main branch which doesn't have the binary
yet. Composite actions use github.action_path which resolves correctly
via the _actions cache, so convert usage-summary to a composite action
and remove the now-unnecessary separate checkout.

Co-Authored-By: roachdev-claude <roachdev-claude-bot@cockroachlabs.com>
- name: Comment that issue was skipped
if: steps.check.outputs.exists != 'true' && steps.assess.outputs.assessment == 'SKIP'
shell: bash
run: |
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

📝 [actionlint] reported by reviewdog 🐶
shellcheck reported issue in this script: SC2016:info:1:16: Expressions don't expand in single quotes, use double quotes for that [shellcheck]

fantapop and others added 5 commits March 24, 2026 15:35
- Use full model ID (claude-haiku-4-5-20251001) for security review
  instead of shorthand "haiku" which may not work on Vertex
- Only pass --allowedTools when non-empty to avoid passing an empty
  string which may cause unexpected CLI behavior

Co-Authored-By: roachdev-claude <roachdev-claude-bot@cockroachlabs.com>
Replace the usage-summary composite action with a simpler approach:
- UsageTracker.Save() now writes both the JSON data file and a
  rendered markdown table to $RUNNER_TEMP/autosolve-usage.md
- Each action overwrites the markdown with the full accumulated table
- The workflow step just cats the file into GITHUB_STEP_SUMMARY

This keeps the table format defined in one place (FormatSummary in
claude.go) and eliminates the need for a separate action or Go command.

Co-Authored-By: roachdev-claude <roachdev-claude-bot@cockroachlabs.com>
claude-haiku-4-5-20251001 is not available on Vertex AI, causing the
security review to fail with exit code 1 and 0 tokens. Use sonnet
which is confirmed available on Vertex.

Co-Authored-By: roachdev-claude <roachdev-claude-bot@cockroachlabs.com>
Load() now parses the markdown table produced by FormatSummary(),
eliminating the separate JSON file. The table format is defined in
one place (FormatSummary) and the parser (ParseSummary) lives next
to it in the same file.

Adds round-trip, load/save, and empty-input tests to verify the
parser can reconstruct what the formatter writes.

Co-Authored-By: roachdev-claude <roachdev-claude-bot@cockroachlabs.com>
Fixes Node.js 20 deprecation warning.

Co-Authored-By: roachdev-claude <roachdev-claude-bot@cockroachlabs.com>
@fantapop
Copy link
Copy Markdown
Contributor Author

closed in favor of:
#13
#14
#15

@fantapop fantapop closed this Mar 25, 2026
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.

2 participants