Skip to content

Commit 8c60a48

Browse files
shivamstaqclaude
andcommitted
Critical architecture fixes: data integrity, process lifecycle, guardrails
Phase A — Data integrity: - WriteBack GraphQL endpoint: pass graphqlEndpoint explicitly instead of constructing wb.baseURL + "/graphql". Prevents double-append for GHES. TrimSuffix on apiURL prevents trailing-slash endpoint bugs. - Pass 2 failure marks items as Pass2Failed. Eligibility checker rejects items with incomplete dependency data — prevents dispatching blocked items when Pass 2 GraphQL fails (was silently dropping all blockers). - Pass2Failed propagated through: WorkItemRaw → NormalizedItem → WorkItem → IsEligible rejection. Phase B — Process lifecycle: - Claude CLI adapter: restructured Prompt() to use cmd.Start() + pipe reading + cmd.Wait() instead of cmd.Output(). Process tracked in c.proc AFTER Start(), so Cancel() can actually kill it during execution. - Workspace FetchOrigin: run git fetch on continuation retries to ensure fresh base refs before commit detection and push. - Template safety: dereference *int to int in workItemToMap to prevent <nil> rendering in gh CLI commands. Phase C — Guardrails: - Continuation retry backoff: increase delay from fixed 1s to exponential (5s → 10s → 20s → 30s cap). Prevents rapid-fire re-invocations that caused 755 sessions in one day. - Max continuation retries (10) still enforced as hard limit. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
1 parent 0d652f6 commit 8c60a48

14 files changed

Lines changed: 81 additions & 31 deletions

File tree

cmd/symphony/main.go

Lines changed: 4 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ import (
99
"os"
1010
"os/signal"
1111
"path/filepath"
12+
"strings"
1213
"syscall"
1314
"time"
1415

@@ -133,17 +134,14 @@ func main() {
133134
os.Exit(1)
134135
}
135136

136-
apiURL := cfg.GitHub.APIURL
137+
apiURL := strings.TrimSuffix(cfg.GitHub.APIURL, "/")
137138
if apiURL == "" {
138139
apiURL = "https://api.github.com"
139140
}
140-
graphqlEndpoint := apiURL
141-
if apiURL == "https://api.github.com" {
142-
graphqlEndpoint = "https://api.github.com/graphql"
143-
}
141+
graphqlEndpoint := apiURL + "/graphql"
144142

145143
gqlClient := ghub.NewGraphQLClient(graphqlEndpoint, token)
146-
writeBack := ghub.NewWriteBack(apiURL, token)
144+
writeBack := ghub.NewWriteBack(apiURL, graphqlEndpoint, token)
147145

148146
// Fetch project field metadata for status updates (best-effort)
149147
var projectMeta *ghub.ProjectFieldMeta

internal/adapter/claude_cli.go

Lines changed: 27 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ import (
44
"context"
55
"encoding/json"
66
"fmt"
7+
"io"
78
"log/slog"
89
"os"
910
"os/exec"
@@ -111,39 +112,56 @@ func (c *ClaudeCLI) Prompt(ctx context.Context, sessionID string, text string) (
111112
cmd.Stdin = strings.NewReader(text)
112113
cmd.Env = os.Environ()
113114

115+
// Capture stdout via pipe so we can track the process for cancellation
116+
stdout, err := cmd.StdoutPipe()
117+
if err != nil {
118+
return &PromptResult{StopReason: StopFailed, Summary: fmt.Sprintf("stdout pipe: %v", err)}, nil
119+
}
120+
114121
slog.Info("claude CLI executing",
115122
"session_id", sessionID,
116123
"model", c.model,
117124
"perm_mode", c.permMode,
118125
"prompt_len", len(text),
119126
)
120127

121-
// Track the process for cancellation
128+
if err := cmd.Start(); err != nil {
129+
return &PromptResult{StopReason: StopFailed, Summary: fmt.Sprintf("start: %v", err)}, nil
130+
}
131+
132+
// Track the process for cancellation AFTER start
122133
c.mu.Lock()
123-
c.proc = nil
134+
c.proc = cmd.Process
124135
c.mu.Unlock()
125136

126-
// Capture stdout and stderr
127-
output, err := cmd.Output()
137+
// Read all output
138+
output, readErr := io.ReadAll(stdout)
139+
140+
// Wait for process to finish
141+
waitErr := cmd.Wait()
128142

129143
c.mu.Lock()
130144
c.proc = nil
131145
c.mu.Unlock()
132146

133-
if err != nil {
134-
// Get stderr from the ExitError
147+
if readErr != nil {
148+
slog.Error("claude CLI read failed", "session_id", sessionID, "error", readErr)
149+
return &PromptResult{StopReason: StopFailed, Summary: fmt.Sprintf("read: %v", readErr)}, nil
150+
}
151+
152+
if waitErr != nil {
135153
var stderr string
136-
if exitErr, ok := err.(*exec.ExitError); ok {
154+
if exitErr, ok := waitErr.(*exec.ExitError); ok {
137155
stderr = string(exitErr.Stderr)
138156
}
139157
slog.Error("claude CLI failed",
140158
"session_id", sessionID,
141-
"error", err,
159+
"error", waitErr,
142160
"stderr", stderr,
143161
)
144162
return &PromptResult{
145163
StopReason: StopFailed,
146-
Summary: fmt.Sprintf("claude CLI error: %v\n%s", err, stderr),
164+
Summary: fmt.Sprintf("claude CLI error: %v\n%s", waitErr, stderr),
147165
}, nil
148166
}
149167

internal/github/graphql.go

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ import (
66
"encoding/json"
77
"fmt"
88
"io"
9+
"log/slog"
910
"net/http"
1011
"strings"
1112
)
@@ -315,7 +316,9 @@ func (c *GraphQLClient) FetchIssueDetails(ctx context.Context, items []WorkItemR
315316

316317
data, err := c.doGraphQL(ctx, query, map[string]any{"id": item.IssueID})
317318
if err != nil {
318-
// Non-fatal: continue with partial data
319+
// Mark item as having incomplete data — eligibility checker will skip it
320+
items[i].Pass2Failed = true
321+
slog.Warn("pass 2 fetch failed for item", "issue_id", item.IssueID, "error", err)
319322
continue
320323
}
321324

internal/github/models.go

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@ type WorkItemRaw struct {
2323
URL string
2424
CreatedAt string
2525
UpdatedAt string
26+
Pass2Failed bool // true if Pass 2 enrichment failed — dependency data may be incomplete
2627
}
2728

2829
// BlockerRefRaw is a dependency/blocker reference from GitHub.

internal/github/source.go

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -95,6 +95,7 @@ func NormalizeWorkItem(raw WorkItemRaw, priorityMap map[string]int) NormalizedIt
9595
URL: raw.URL,
9696
CreatedAt: raw.CreatedAt,
9797
UpdatedAt: raw.UpdatedAt,
98+
Pass2Failed: raw.Pass2Failed,
9899
}
99100

100101
// Derive work_item_id
@@ -172,6 +173,7 @@ type NormalizedItem struct {
172173
URL string
173174
CreatedAt string
174175
UpdatedAt string
176+
Pass2Failed bool
175177
Repository *NormalizedRepo
176178
BlockedBy []NormalizedBlocker
177179
SubIssues []NormalizedChild

internal/github/writeback.go

Lines changed: 12 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -7,21 +7,25 @@ import (
77
"fmt"
88
"io"
99
"net/http"
10+
"strings"
1011
)
1112

1213
// WriteBack handles deterministic GitHub write-back operations.
1314
type WriteBack struct {
14-
baseURL string
15-
token string
16-
client *http.Client
15+
baseURL string // REST API base (e.g., https://api.github.com)
16+
graphqlEndpoint string // GraphQL endpoint (e.g., https://api.github.com/graphql)
17+
token string
18+
client *http.Client
1719
}
1820

1921
// NewWriteBack creates a new write-back client.
20-
func NewWriteBack(baseURL, token string) *WriteBack {
22+
// graphqlEndpoint should be the full GraphQL URL (not derived from baseURL).
23+
func NewWriteBack(baseURL, graphqlEndpoint, token string) *WriteBack {
2124
return &WriteBack{
22-
baseURL: baseURL,
23-
token: token,
24-
client: &http.Client{},
25+
baseURL: strings.TrimSuffix(baseURL, "/"),
26+
graphqlEndpoint: graphqlEndpoint,
27+
token: token,
28+
client: &http.Client{},
2529
}
2630
}
2731

@@ -252,8 +256,7 @@ func (wb *WriteBack) graphqlPost(ctx context.Context, query string, variables ma
252256
return nil, err
253257
}
254258

255-
// GraphQL endpoint is baseURL + /graphql (or baseURL itself if it's the graphql endpoint)
256-
endpoint := wb.baseURL + "/graphql"
259+
endpoint := wb.graphqlEndpoint
257260
req, err := http.NewRequestWithContext(ctx, "POST", endpoint, bytes.NewReader(jsonBody))
258261
if err != nil {
259262
return nil, err

internal/github/writeback_reuse_test.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -50,7 +50,7 @@ func TestWriteBack_ReusesExistingPR(t *testing.T) {
5050
}))
5151
defer server.Close()
5252

53-
wb := ghub.NewWriteBack(server.URL, "ghp_test")
53+
wb := ghub.NewWriteBack(server.URL, server.URL+"/graphql", "ghp_test")
5454
result, err := wb.UpsertPR(context.Background(), ghub.PRParams{
5555
Owner: "org",
5656
Repo: "repo",
@@ -107,7 +107,7 @@ func TestWriteBack_CreatesNewPRWhenNoneExists(t *testing.T) {
107107
}))
108108
defer server.Close()
109109

110-
wb := ghub.NewWriteBack(server.URL, "ghp_test")
110+
wb := ghub.NewWriteBack(server.URL, server.URL+"/graphql", "ghp_test")
111111
result, err := wb.UpsertPR(context.Background(), ghub.PRParams{
112112
Owner: "org", Repo: "repo", Title: "New PR",
113113
HeadBranch: "symphony/org_repo_1", BaseBranch: "main", Draft: true,

internal/github/writeback_test.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -35,7 +35,7 @@ func TestWriteBack_CreatePR(t *testing.T) {
3535
}))
3636
defer server.Close()
3737

38-
wb := ghub.NewWriteBack(server.URL, "ghp_test")
38+
wb := ghub.NewWriteBack(server.URL, server.URL+"/graphql", "ghp_test")
3939

4040
result, err := wb.UpsertPR(context.Background(), ghub.PRParams{
4141
Owner: "org",
@@ -75,7 +75,7 @@ func TestWriteBack_CommentOnIssue(t *testing.T) {
7575
}))
7676
defer server.Close()
7777

78-
wb := ghub.NewWriteBack(server.URL, "ghp_test")
78+
wb := ghub.NewWriteBack(server.URL, server.URL+"/graphql", "ghp_test")
7979

8080
url, err := wb.CommentOnIssue(context.Background(), "org", "repo", 42, "PR created: https://github.com/org/repo/pull/99")
8181
if err != nil {

internal/orchestrator/convert.go

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@ func ConvertNormalizedItem(n ghub.NormalizedItem) WorkItem {
2222
URL: n.URL,
2323
CreatedAt: n.CreatedAt,
2424
UpdatedAt: n.UpdatedAt,
25+
Pass2Failed: n.Pass2Failed,
2526
}
2627

2728
if n.Repository != nil {

internal/orchestrator/eligibility.go

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,11 @@ func IsEligible(item WorkItem, cfg EligibilityConfig, state *State, maxConcurren
3131
return false, "missing title"
3232
}
3333

34+
// Reject items with incomplete dependency data (Pass 2 failed)
35+
if item.Pass2Failed {
36+
return false, "incomplete data (dependency fetch failed) — will retry next poll"
37+
}
38+
3439
// Content type must be executable
3540
if !containsCI(cfg.ExecutableItemTypes, item.ContentType) {
3641
return false, "content_type not executable: " + item.ContentType

0 commit comments

Comments
 (0)