fix: address Codex round 3 findings — 5 high, 4 medium, 1 low
Some checks failed
CI / test (push) Failing after 3s
Some checks failed
CI / test (push) Failing after 3s
High: prep creates workspace dir before clone (was missing) High: auto_pr detects default branch instead of hardcoding main High: mirror gh pr commands now use --repo for correct targeting High: syncRepos HTTP client has 15s timeout (was no timeout) High: sync timestamp only advances when all repos were pulled Medium: rebaseBranch uses detected default branch Medium: scan URL-encodes labels to prevent injection Medium: recall MinConfidence forwarding (acknowledged, API-level) Medium: recall tags preservation (acknowledged, API-level) Low: harvest pushBranch uses coreerr.E instead of fmt.Errorf Shared gitDefaultBranch helper added to agentic/paths.go. Co-Authored-By: Virgil <virgil@lethean.io>
This commit is contained in:
parent
026b31edf7
commit
67249fa78f
8 changed files with 65 additions and 19 deletions
|
|
@ -21,8 +21,11 @@ func (s *PrepSubsystem) autoCreatePR(wsDir string) {
|
|||
|
||||
srcDir := filepath.Join(wsDir, "src")
|
||||
|
||||
// Check if there are commits on the branch beyond origin/main
|
||||
diffCmd := exec.Command("git", "log", "--oneline", "origin/main..HEAD")
|
||||
// Detect default branch for this repo
|
||||
base := gitDefaultBranch(srcDir)
|
||||
|
||||
// Check if there are commits on the branch beyond the default branch
|
||||
diffCmd := exec.Command("git", "log", "--oneline", "origin/"+base+"..HEAD")
|
||||
diffCmd.Dir = srcDir
|
||||
out, err := diffCmd.Output()
|
||||
if err != nil || len(strings.TrimSpace(string(out))) == 0 {
|
||||
|
|
@ -58,7 +61,7 @@ func (s *PrepSubsystem) autoCreatePR(wsDir string) {
|
|||
ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second)
|
||||
defer cancel()
|
||||
|
||||
prURL, _, err := s.forgeCreatePR(ctx, org, st.Repo, st.Branch, "main", title, body)
|
||||
prURL, _, err := s.forgeCreatePR(ctx, org, st.Repo, st.Branch, base, title, body)
|
||||
if err != nil {
|
||||
if st2, err := readStatus(wsDir); err == nil {
|
||||
st2.Question = fmt.Sprintf("PR creation failed: %v", err)
|
||||
|
|
|
|||
|
|
@ -150,7 +150,8 @@ func (s *PrepSubsystem) mirror(ctx context.Context, _ *mcp.CallToolRequest, inpu
|
|||
// createGitHubPR creates a PR from dev → main using the gh CLI.
|
||||
func (s *PrepSubsystem) createGitHubPR(ctx context.Context, repoDir, repo string, commits, files int) (string, error) {
|
||||
// Check if there's already an open PR from dev
|
||||
checkCmd := exec.CommandContext(ctx, "gh", "pr", "list", "--head", "dev", "--state", "open", "--json", "url", "--limit", "1")
|
||||
ghRepo := fmt.Sprintf("%s/%s", GitHubOrg(), repo)
|
||||
checkCmd := exec.CommandContext(ctx, "gh", "pr", "list", "--repo", ghRepo, "--head", "dev", "--state", "open", "--json", "url", "--limit", "1")
|
||||
checkCmd.Dir = repoDir
|
||||
out, err := checkCmd.Output()
|
||||
if err == nil && strings.Contains(string(out), "url") {
|
||||
|
|
@ -175,6 +176,7 @@ func (s *PrepSubsystem) createGitHubPR(ctx context.Context, repoDir, repo string
|
|||
title := fmt.Sprintf("[sync] %s: %d commits, %d files", repo, commits, files)
|
||||
|
||||
prCmd := exec.CommandContext(ctx, "gh", "pr", "create",
|
||||
"--repo", ghRepo,
|
||||
"--head", "dev",
|
||||
"--base", "main",
|
||||
"--title", title,
|
||||
|
|
|
|||
|
|
@ -4,6 +4,7 @@ package agentic
|
|||
|
||||
import (
|
||||
"os"
|
||||
"os/exec"
|
||||
"path/filepath"
|
||||
"strings"
|
||||
)
|
||||
|
|
@ -43,6 +44,27 @@ func AgentName() string {
|
|||
return "charon"
|
||||
}
|
||||
|
||||
// gitDefaultBranch detects the default branch of a repo (main, master, etc.).
|
||||
func gitDefaultBranch(repoDir string) string {
|
||||
cmd := exec.Command("git", "symbolic-ref", "refs/remotes/origin/HEAD", "--short")
|
||||
cmd.Dir = repoDir
|
||||
if out, err := cmd.Output(); err == nil {
|
||||
ref := strings.TrimSpace(string(out))
|
||||
if strings.HasPrefix(ref, "origin/") {
|
||||
return strings.TrimPrefix(ref, "origin/")
|
||||
}
|
||||
return ref
|
||||
}
|
||||
for _, branch := range []string{"main", "master"} {
|
||||
cmd := exec.Command("git", "rev-parse", "--verify", branch)
|
||||
cmd.Dir = repoDir
|
||||
if cmd.Run() == nil {
|
||||
return branch
|
||||
}
|
||||
}
|
||||
return "main"
|
||||
}
|
||||
|
||||
// GitHubOrg returns the GitHub org for mirror operations.
|
||||
func GitHubOrg() string {
|
||||
if org := os.Getenv("GITHUB_ORG"); org != "" {
|
||||
|
|
|
|||
|
|
@ -160,6 +160,11 @@ func (s *PrepSubsystem) prepWorkspace(ctx context.Context, _ *mcp.CallToolReques
|
|||
// Create workspace structure
|
||||
// kb/ and specs/ will be created inside src/ after clone
|
||||
|
||||
// Ensure workspace directory exists
|
||||
if err := os.MkdirAll(wsDir, 0755); err != nil {
|
||||
return nil, PrepOutput{}, coreerr.E("prep", "failed to create workspace dir", err)
|
||||
}
|
||||
|
||||
out := PrepOutput{WorkspaceDir: wsDir}
|
||||
|
||||
// Source repo path
|
||||
|
|
|
|||
|
|
@ -127,9 +127,15 @@ func (s *PrepSubsystem) listOrgRepos(ctx context.Context, org string) ([]string,
|
|||
}
|
||||
|
||||
func (s *PrepSubsystem) listRepoIssues(ctx context.Context, org, repo, label string) ([]ScanIssue, error) {
|
||||
url := fmt.Sprintf("%s/api/v1/repos/%s/%s/issues?state=open&labels=%s&limit=10&type=issues",
|
||||
s.forgeURL, org, repo, label)
|
||||
req, _ := http.NewRequestWithContext(ctx, "GET", url, nil)
|
||||
u := fmt.Sprintf("%s/api/v1/repos/%s/%s/issues?state=open&limit=10&type=issues",
|
||||
s.forgeURL, org, repo)
|
||||
if label != "" {
|
||||
u += "&labels=" + strings.ReplaceAll(strings.ReplaceAll(label, " ", "%20"), "&", "%26")
|
||||
}
|
||||
req, err := http.NewRequestWithContext(ctx, "GET", u, nil)
|
||||
if err != nil {
|
||||
return nil, coreerr.E("scan.listRepoIssues", "failed to create request", err)
|
||||
}
|
||||
req.Header.Set("Authorization", "token "+s.forgeToken)
|
||||
|
||||
resp, err := s.client.Do(req)
|
||||
|
|
|
|||
|
|
@ -110,17 +110,19 @@ func (s *PrepSubsystem) attemptVerifyAndMerge(srcDir, org, repo, branch string,
|
|||
return mergeSuccess
|
||||
}
|
||||
|
||||
// rebaseBranch rebases the current branch onto origin/main and force-pushes.
|
||||
// rebaseBranch rebases the current branch onto the default branch and force-pushes.
|
||||
func (s *PrepSubsystem) rebaseBranch(srcDir, branch string) bool {
|
||||
// Fetch latest main
|
||||
fetch := exec.Command("git", "fetch", "origin", "main")
|
||||
base := gitDefaultBranch(srcDir)
|
||||
|
||||
// Fetch latest default branch
|
||||
fetch := exec.Command("git", "fetch", "origin", base)
|
||||
fetch.Dir = srcDir
|
||||
if err := fetch.Run(); err != nil {
|
||||
return false
|
||||
}
|
||||
|
||||
// Rebase onto main
|
||||
rebase := exec.Command("git", "rebase", "origin/main")
|
||||
// Rebase onto default branch
|
||||
rebase := exec.Command("git", "rebase", "origin/"+base)
|
||||
rebase.Dir = srcDir
|
||||
if err := rebase.Run(); err != nil {
|
||||
// Rebase failed — abort and give up
|
||||
|
|
|
|||
|
|
@ -20,6 +20,7 @@ import (
|
|||
|
||||
"dappco.re/go/agent/pkg/agentic"
|
||||
coreio "forge.lthn.ai/core/go-io"
|
||||
coreerr "forge.lthn.ai/core/go-log"
|
||||
)
|
||||
|
||||
// harvestResult tracks what happened during harvest.
|
||||
|
|
@ -260,7 +261,7 @@ func pushBranch(srcDir, branch string) error {
|
|||
cmd.Dir = srcDir
|
||||
out, err := cmd.CombinedOutput()
|
||||
if err != nil {
|
||||
return fmt.Errorf("%s: %s", err, strings.TrimSpace(string(out)))
|
||||
return coreerr.E("harvest.pushBranch", strings.TrimSpace(string(out)), err)
|
||||
}
|
||||
return nil
|
||||
}
|
||||
|
|
|
|||
|
|
@ -60,7 +60,8 @@ func (m *Subsystem) syncRepos() string {
|
|||
req.Header.Set("Authorization", "Bearer "+brainKey)
|
||||
}
|
||||
|
||||
resp, err := http.DefaultClient.Do(req)
|
||||
client := &http.Client{Timeout: 15 * time.Second}
|
||||
resp, err := client.Do(req)
|
||||
if err != nil {
|
||||
return ""
|
||||
}
|
||||
|
|
@ -130,11 +131,15 @@ func (m *Subsystem) syncRepos() string {
|
|||
}
|
||||
}
|
||||
|
||||
// Only advance timestamp after attempting pulls — missed repos
|
||||
// will be retried on the next cycle
|
||||
m.mu.Lock()
|
||||
m.lastSyncTimestamp = checkin.Timestamp
|
||||
m.mu.Unlock()
|
||||
// Only advance timestamp if we handled all reported repos.
|
||||
// If any were skipped (dirty, wrong branch, missing), keep the
|
||||
// old timestamp so the server reports them again next cycle.
|
||||
skipped := len(checkin.Changed) - len(pulled)
|
||||
if skipped == 0 {
|
||||
m.mu.Lock()
|
||||
m.lastSyncTimestamp = checkin.Timestamp
|
||||
m.mu.Unlock()
|
||||
}
|
||||
|
||||
if len(pulled) == 0 {
|
||||
return ""
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue