From fc28b975cf0586e197959bb9dd6fd623cd1f44f1 Mon Sep 17 00:00:00 2001 From: Test Date: Tue, 28 Jul 2026 13:15:33 -0300 Subject: [PATCH] feat: strengthen local agent execution Local execution previously depended too much on model behavior. Research could invent causes without reading the repository, editors could stop before completing a plan, reviewers could loop after tests passed, and retry limits did not bound the full workflow. This made local models cheaper to run, but not trustworthy. Turn the workflow into an evidence-driven execution pipeline. Research is grounded in repository files, editors can complete planned multi-file changes, reviews end with an explicit verdict, deterministic checks remain authoritative, and caller attempt budgets control the internal conductor. Agent commands now propagate cancellation through their full process trees. Validate the result with a complete Ollama/Qwen run: diagnose an incorrect Go implementation, plan the repair, apply the patch, pass go test ./..., and finish model review without paid inference. Regression tests cover evidence boundaries, model fallback, ranged reads, planned files, process cancellation, telemetry, and Git isolation inside commit hooks. Make the maintained CLI tell the same story. Local-model telemetry no longer claims calls or token precision it cannot observe, stale monitor and imperative release commands are removed, and the public workflow remains protected by the repository quality contract. A future change should instrument every model stage through one provider wrapper and isolate the remaining experimental surfaces. --- _roadmap.md | 3 + cmd/gptcode/do.go | 22 +- cmd/gptcode/do_test.go | 13 ++ cmd/gptcode/main.go | 73 ------- cmd/gptcode/release.go | 183 ----------------- internal/agents/analyzer.go | 2 +- internal/agents/editor.go | 108 ++++++++-- internal/agents/editor_test.go | 194 +++++++++++++++++- internal/agents/query.go | 2 +- internal/agents/review.go | 2 +- internal/agents/reviewer.go | 54 ++++- internal/agents/reviewer_test.go | 35 ++++ internal/autonomous/analyzer.go | 59 +++++- internal/autonomous/analyzer_test.go | 34 +++ internal/config/model_selector.go | 10 +- internal/config/model_selector_test.go | 28 +++ internal/llm/loop_detector.go | 10 +- internal/llm/loop_detector_test.go | 11 + internal/maestro/conductor.go | 58 +++++- internal/maestro/conductor_plan_files_test.go | 32 +++ internal/maestro/requested_verification.go | 12 +- .../maestro/requested_verification_test.go | 44 ++++ internal/modes/autonomous.go | 4 + internal/observability/observer.go | 18 +- internal/observability/observer_test.go | 16 ++ internal/processutil/combined_output.go | 14 ++ internal/processutil/combined_output_unix.go | 36 ++++ .../processutil/combined_output_windows.go | 14 ++ internal/refactor/breaking.go | 35 +++- internal/tools/tools.go | 96 +++++++-- internal/tools/tools_test.go | 72 +++++++ test/breaking_changes_test.go | 19 +- 32 files changed, 983 insertions(+), 330 deletions(-) delete mode 100644 cmd/gptcode/release.go create mode 100644 internal/agents/reviewer_test.go create mode 100644 internal/maestro/conductor_plan_files_test.go create mode 100644 internal/observability/observer_test.go create mode 100644 internal/processutil/combined_output.go create mode 100644 internal/processutil/combined_output_unix.go create mode 100644 internal/processutil/combined_output_windows.go diff --git a/_roadmap.md b/_roadmap.md index 1c6e267..7252165 100644 --- a/_roadmap.md +++ b/_roadmap.md @@ -34,5 +34,8 @@ engineering portfolio. - [x] Align the Go module path with the public repository so `go install ...@latest` is supported. - [x] Report module versions correctly for both GoReleaser and `go install`, and keep Go/Actions dependencies monitored. - [x] Publish a reproducible engineering case study of the Go data-race workflow. +- [x] Validate the complete research, edit, deterministic verification, and review workflow with a local Ollama model. +- [x] Propagate cancellation through agent-run command process trees and report local-model telemetry without false precision. +- [x] Remove the disconnected legacy `monitor` and imperative `release` command implementations while preserving public-surface compatibility tests. - [ ] Retire or isolate the legacy Live, training, Supabase, and experimental command surfaces with compatibility tests. - [ ] Raise coverage in legacy workflow packages without presenting the public fixture as repository-wide coverage. diff --git a/cmd/gptcode/do.go b/cmd/gptcode/do.go index 504f710..15711a8 100644 --- a/cmd/gptcode/do.go +++ b/cmd/gptcode/do.go @@ -11,6 +11,7 @@ import ( "github.com/jadercorrea/gptcode/internal/config" "github.com/jadercorrea/gptcode/internal/intelligence" + "github.com/jadercorrea/gptcode/internal/langdetect" "github.com/jadercorrea/gptcode/internal/live" "github.com/jadercorrea/gptcode/internal/llm" "github.com/jadercorrea/gptcode/internal/modes" @@ -143,7 +144,7 @@ func runDoExecutionWithRetry(ctx context.Context, task string, verbose bool, max } startTime := time.Now() - err := runDoExecution(ctx, task, verbose, supervised, setup, currentBackend, currentEditorModel) + err := runDoExecution(ctx, task, verbose, supervised, setup, currentBackend, currentEditorModel, maxAttempts) elapsed := time.Since(startTime).Milliseconds() if err == nil { @@ -331,7 +332,7 @@ func shouldPromptForRetry(interactive bool) bool { return interactive && term.IsTerminal(int(os.Stdin.Fd())) } -func runDoExecution(ctx context.Context, task string, verbose bool, supervised bool, setup *config.Setup, backendName string, editorModel string) error { +func runDoExecution(ctx context.Context, task string, verbose bool, supervised bool, setup *config.Setup, backendName string, editorModel string, maxAttempts int) error { backendCfg := setup.Backend[backendName] cwd, _ := os.Getwd() @@ -372,10 +373,7 @@ func runDoExecution(ctx context.Context, task string, verbose bool, supervised b fmt.Fprintf(os.Stderr, "Analyzing task complexity...\n") } // Detect language - language := setup.Defaults.Lang - if language == "" { - language = "go" // default - } + language := executionLanguage(cwd, setup.Defaults.Lang) liveClient := live.GetClient() var reportConfig *live.ReportConfig @@ -389,6 +387,7 @@ func runDoExecution(ctx context.Context, task string, verbose bool, supervised b } executor := modes.NewAutonomousExecutorWithLive(queryProvider, cwd, queryModel, language, liveClient, reportConfig, backendName) + executor.SetMaxAttempts(maxAttempts) return executor.Execute(ctx, task) } @@ -428,3 +427,14 @@ func runDoExecution(ctx context.Context, task string, verbose bool, supervised b return nil } + +func executionLanguage(cwd, configured string) string { + detected := langdetect.DetectLanguage(cwd) + if detected != langdetect.Unknown { + return string(detected) + } + if configured != "" { + return configured + } + return string(langdetect.Go) +} diff --git a/cmd/gptcode/do_test.go b/cmd/gptcode/do_test.go index 48ad631..70a6ec6 100644 --- a/cmd/gptcode/do_test.go +++ b/cmd/gptcode/do_test.go @@ -1,6 +1,8 @@ package main import ( + "os" + "path/filepath" "testing" "github.com/jadercorrea/gptcode/internal/config" @@ -32,6 +34,17 @@ func TestSelectDoModelsPrefersConfiguredAgentModels(t *testing.T) { } } +func TestExecutionLanguagePrefersRepositoryOverGlobalDefault(t *testing.T) { + repository := t.TempDir() + if err := os.WriteFile(filepath.Join(repository, "mix.exs"), []byte("defmodule Demo.MixProject do\nend\n"), 0644); err != nil { + t.Fatal(err) + } + + if got := executionLanguage(repository, "go"); got != "elixir" { + t.Fatalf("execution language = %q, want repository language elixir", got) + } +} + func TestRetryPromptRequiresExplicitInteractiveMode(t *testing.T) { if shouldPromptForRetry(false) { t.Fatal("non-interactive execution must not prompt for retry input") diff --git a/cmd/gptcode/main.go b/cmd/gptcode/main.go index 183f332..55a7a60 100644 --- a/cmd/gptcode/main.go +++ b/cmd/gptcode/main.go @@ -20,7 +20,6 @@ import ( "github.com/jadercorrea/gptcode/internal/elixir" "github.com/jadercorrea/gptcode/internal/feedback" "github.com/jadercorrea/gptcode/internal/langdetect" - "github.com/jadercorrea/gptcode/internal/live" "github.com/jadercorrea/gptcode/internal/llm" "github.com/jadercorrea/gptcode/internal/memory" "github.com/jadercorrea/gptcode/internal/ml" @@ -203,78 +202,6 @@ func init() { rootCmd.AddCommand(reviewCmd) } -// monitorCmd scans local AI agent logs and reports real API usage to the Live Dashboard -var monitorCmd = &cobra.Command{ - Use: "monitor", - Short: "Scan local AI agents and report real API usage to Live Dashboard", - Long: `Discovers installed AI coding agents (Antigravity, Cursor, Windsurf, etc.), -reads their log files, counts today's API calls per model, and reports real -usage data to the gptcode Live Dashboard. - -This is the 'fuel gauge' โ€” shows how much of your daily model quota is used.`, - RunE: func(cmd *cobra.Command, args []string) error { - fmt.Println("๐Ÿ” Scanning local AI agents...") - fmt.Println() - - result, err := live.Monitor() - if err != nil { - return fmt.Errorf("scan failed: %w", err) - } - - if len(result.Agents) == 0 { - fmt.Println("No AI agent activity found today.") - return nil - } - - // Display results - fmt.Printf("๐Ÿ“Š Found %d API calls across %d agent(s) today:\n\n", result.TotalAPICalls, len(result.Agents)) - for _, u := range result.Agents { - pct := int(u.QuotaUsed * 100) - bar := renderBar(u.QuotaUsed) - exhaustedTag := "" - if u.Exhausted { - exhaustedTag = " โš ๏ธ HIT RATE LIMIT" - } - fmt.Printf(" %s (%s)\n", u.DisplayName(), u.Agent) - fmt.Printf(" %s %d%% ยท %d calls ยท %s โ†’ %s%s\n\n", bar, pct, u.APICalls, u.FirstCall, u.LastCall, exhaustedTag) - } - - // Report to Live Dashboard - liveURL := os.Getenv("GPTCODE_LIVE_URL") - if liveURL == "" { - liveURL = "https://gptcode.live" - } - - reportConfig := live.DefaultReportConfig() - reportConfig.SetBaseURL(liveURL) - - fmt.Println("๐Ÿ“ก Reporting to Live Dashboard...") - if err := result.ReportToLive(reportConfig); err != nil { - fmt.Printf("โš ๏ธ Report error: %v\n", err) - } else { - fmt.Printf("โœ… Reported to %s\n", liveURL) - } - - return nil - }, -} - -func renderBar(quota float64) string { - filled := int(quota * 10) - if filled > 10 { - filled = 10 - } - empty := 10 - filled - bar := "" - for i := 0; i < filled; i++ { - bar += "โ—" - } - for i := 0; i < empty; i++ { - bar += "โ—‹" - } - return bar -} - func newBuilderAndLLM(lang, mode, hint string) (*prompt.Builder, llm.Provider, string, error) { setup, err := config.LoadSetup() if err != nil { diff --git a/cmd/gptcode/release.go b/cmd/gptcode/release.go deleted file mode 100644 index 61ef164..0000000 --- a/cmd/gptcode/release.go +++ /dev/null @@ -1,183 +0,0 @@ -package main - -import ( - "fmt" - "os" - "os/exec" - "regexp" - "strings" - - "github.com/spf13/cobra" -) - -var releaseCmd = &cobra.Command{ - Use: "release [major|minor|patch]", - Short: "Create a new release (bumps version, creates tag, pushes)", - Args: cobra.ExactArgs(1), - RunE: func(cmd *cobra.Command, args []string) error { - bumpType := args[0] - if bumpType != "major" && bumpType != "minor" && bumpType != "patch" { - return fmt.Errorf("invalid bump type: %s (use major, minor, or patch)", bumpType) - } - - fmt.Printf("Creating %s release...\n", bumpType) - - // Get current version - currentVersion, err := getCurrentVersion() - if err != nil { - return fmt.Errorf("failed to get current version: %w", err) - } - fmt.Printf("Current version: %s\n", currentVersion) - - // Bump version - newVersion, err := bumpVersion(currentVersion, bumpType) - if err != nil { - return fmt.Errorf("failed to bump version: %w", err) - } - fmt.Printf("New version: %s\n", newVersion) - - // Update version in code - if err := updateVersion(newVersion); err != nil { - return fmt.Errorf("failed to update version: %w", err) - } - - // Generate changelog - changelog, err := generateChangelog(currentVersion) - if err != nil { - fmt.Printf("Warning: failed to generate changelog: %v\n", err) - changelog = "Release " + newVersion - } - - // Create git tag - if err := createTag(newVersion, changelog); err != nil { - return fmt.Errorf("failed to create tag: %w", err) - } - - // Push to trigger release - fmt.Println("\nPushing to trigger release...") - if err := push(); err != nil { - return fmt.Errorf("failed to push: %w", err) - } - - fmt.Printf(` -โœ… Release %s created and pushed! - -The release will be built and published automatically. -Check https://github.com/jadercorrea/gptcode/releases -`, newVersion) - - return nil - }, -} - -func getCurrentVersion() (string, error) { - // Try to get from git tags first - out, err := exec.Command("git", "describe", "--tags", "--abbrev=0").Output() - if err == nil { - version := strings.TrimSpace(string(out)) - if strings.HasPrefix(version, "v") { - return version, nil - } - return "v" + version, nil - } - - // Fallback to reading version file or using default - return "v0.0.0", nil -} - -func bumpVersion(current, bumpType string) (string, error) { - // Remove v prefix - version := strings.TrimPrefix(current, "v") - - // Parse version numbers - parts := strings.Split(version, ".") - if len(parts) != 3 { - return "", fmt.Errorf("invalid version format: %s", current) - } - - var major, minor, patch int - fmt.Sscanf(version, "%d.%d.%d", &major, &minor, &patch) - - switch bumpType { - case "major": - major++ - minor = 0 - patch = 0 - case "minor": - minor++ - patch = 0 - case "patch": - patch++ - } - - return fmt.Sprintf("v%d.%d.%d", major, minor, patch), nil -} - -func updateVersion(version string) error { - // Update version in cmd/gptcode/main.go - versionRE := regexp.MustCompile(`var version = "v[^"]+"`) - - data, err := os.ReadFile("cmd/gptcode/main.go") - if err != nil { - return err - } - - newData := versionRE.ReplaceAllString(string(data), fmt.Sprintf(`var version = "%s"`, version)) - - if err := os.WriteFile("cmd/gptcode/main.go", []byte(newData), 0644); err != nil { - return err - } - - // Also update goreleaser.yaml if exists - goreleaserFiles := []string{ - "goreleaser.yaml", - ".goreleaser.yaml", - "release.md", - } - - for _, f := range goreleaserFiles { - if data, err := os.ReadFile(f); err == nil { - newData := versionRE.ReplaceAllString(string(data), fmt.Sprintf(`var version = "%s"`, version)) - os.WriteFile(f, []byte(newData), 0644) - } - } - - return nil -} - -func generateChangelog(sinceVersion string) (string, error) { - out, err := exec.Command("git", "log", sinceVersion+"..HEAD", "--pretty=format:- %s (%h)", "--no-merges").Output() - if err != nil { - return "", err - } - - lines := strings.Split(strings.TrimSpace(string(out)), "\n") - var changes []string - for _, line := range lines { - if strings.TrimSpace(line) != "" { - changes = append(changes, line) - } - } - - if len(changes) == 0 { - return "No changes since " + sinceVersion, nil - } - - return strings.Join(changes, "\n"), nil -} - -func createTag(version, changelog string) error { - // Create annotated tag - cmd := exec.Command("git", "tag", "-a", version, "-m", changelog) - if err := cmd.Run(); err != nil { - return fmt.Errorf("failed to create tag: %w", err) - } - - fmt.Printf("Created tag: %s\n", version) - return nil -} - -func push() error { - cmd := exec.Command("git", "push", "origin", "main", "--tags") - return cmd.Run() -} diff --git a/internal/agents/analyzer.go b/internal/agents/analyzer.go index 217b011..6d881e2 100644 --- a/internal/agents/analyzer.go +++ b/internal/agents/analyzer.go @@ -167,7 +167,7 @@ Do NOT suggest changes. Just report what exists.`, task) } } } - result := tools.ExecuteToolFromLLM(llmCall, a.cwd) + result := tools.ExecuteToolFromLLMContext(ctx, llmCall, a.cwd) content := result.Result if result.Error != "" { diff --git a/internal/agents/editor.go b/internal/agents/editor.go index 232e503..6d9d82b 100644 --- a/internal/agents/editor.go +++ b/internal/agents/editor.go @@ -91,6 +91,12 @@ func NewEditorWithFileValidation(provider llm.Provider, cwd string, model string } } +// SetExpectedFiles constrains writes to planned files and lets the editor hand +// control back as soon as every planned file has been changed. +func (e *EditorAgent) SetExpectedFiles(files []string) { + e.allowedFiles = append([]string(nil), files...) +} + const editorPrompt = `You are a code editor and executor. Your job is to modify files AND execute shell commands. WORKFLOW: @@ -183,11 +189,40 @@ func (e *EditorAgent) Execute(ctx context.Context, history []llm.ChatMessage, st "type": "string", "description": "File path", }, + "start_line": map[string]interface{}{ + "type": "integer", + "description": "Optional 1-based first line to read", + }, + "end_line": map[string]interface{}{ + "type": "integer", + "description": "Optional 1-based last line to read (inclusive)", + }, }, "required": []string{"path"}, }, }, }, + map[string]interface{}{ + "type": "function", + "function": map[string]interface{}{ + "name": "search_code", + "description": "Find matching lines before reading or patching a large file", + "parameters": map[string]interface{}{ + "type": "object", + "properties": map[string]interface{}{ + "pattern": map[string]interface{}{ + "type": "string", + "description": "Search pattern (regular expression)", + }, + "file_pattern": map[string]interface{}{ + "type": "string", + "description": "Optional file pattern such as '*.ex'", + }, + }, + "required": []string{"pattern"}, + }, + }, + }, map[string]interface{}{ "type": "function", "function": map[string]interface{}{ @@ -307,15 +342,20 @@ func (e *EditorAgent) Execute(ctx context.Context, history []llm.ChatMessage, st return "", nil, err } - // Emit LLM request event to observer - if e.observer != nil && resp.TokenUsage != nil { - // Calculate cost (free models have 0 cost) - cost := calculateCost(e.model, resp.TokenUsage.PromptTokens, resp.TokenUsage.CompletionTokens) + // Record the call even when a local provider omits token accounting. + // Zero token counts are more accurate than claiming no LLM call occurred. + if e.observer != nil { + tokensIn, tokensOut := 0, 0 + if resp.TokenUsage != nil { + tokensIn = resp.TokenUsage.PromptTokens + tokensOut = resp.TokenUsage.CompletionTokens + } + cost := calculateCost(e.model, tokensIn, tokensOut) e.observer.Emit(&observability.LLMRequestEvent{ BaseEvent: observability.BaseEvent{Time: time.Now()}, Model: e.model, - TokensIn: resp.TokenUsage.PromptTokens, - TokensOut: resp.TokenUsage.CompletionTokens, + TokensIn: tokensIn, + TokensOut: tokensOut, Cost: cost, Duration: llmDuration, }) @@ -367,7 +407,7 @@ func (e *EditorAgent) Execute(ctx context.Context, history []llm.ChatMessage, st } } - result := tools.ExecuteToolWithObserver(llmCall, e.cwd, e.observer) + result := tools.ExecuteToolWithObserverContext(ctx, llmCall, e.cwd, e.observer) if len(result.ModifiedFiles) > 0 { modifiedFiles = append(modifiedFiles, result.ModifiedFiles...) } @@ -421,11 +461,22 @@ func (e *EditorAgent) Execute(ctx context.Context, history []llm.ChatMessage, st fmt.Fprintf(os.Stderr, "[EDITOR] Executed %s: %s\n", tc.Name, result.Result[:min(50, len(result.Result))]) } } - if len(modifiedFiles) > 0 { - return "Changes applied; awaiting deterministic validation", modifiedFiles, nil + if e.allExpectedFilesModified(modifiedFiles) { + return "All planned files changed; awaiting deterministic validation", modifiedFiles, nil } continue } + if editRequested(messages) && len(modifiedFiles) == 0 { + messages = append(messages, + llm.ChatMessage{Role: "assistant", Content: resp.Text}, + llm.ChatMessage{ + Role: "user", + Content: "No files were modified. This is an implementation task: use the available file tools " + + "to inspect and apply the requested change. Do not merely describe the solution.", + }, + ) + continue + } return resp.Text, modifiedFiles, nil } @@ -460,7 +511,7 @@ func (e *EditorAgent) Execute(ctx context.Context, history []llm.ChatMessage, st } } - result := tools.ExecuteToolWithObserver(llmCall, e.cwd, e.observer) + result := tools.ExecuteToolWithObserverContext(ctx, llmCall, e.cwd, e.observer) if len(result.ModifiedFiles) > 0 { modifiedFiles = append(modifiedFiles, result.ModifiedFiles...) } @@ -514,14 +565,37 @@ func (e *EditorAgent) Execute(ctx context.Context, history []llm.ChatMessage, st fmt.Fprintf(os.Stderr, "[EDITOR] Executed %s: %s\n", tc.Name, result.Result[:min(50, len(result.Result))]) } } - if len(modifiedFiles) > 0 { - return "Changes applied; awaiting deterministic validation", modifiedFiles, nil + if e.allExpectedFilesModified(modifiedFiles) { + return "All planned files changed; awaiting deterministic validation", modifiedFiles, nil } } + if editRequested(messages) && len(modifiedFiles) == 0 { + return "", nil, fmt.Errorf("editor stopped without modifying any files after %d attempts", maxToolChainDepth) + } + return "Editor reached max iterations", modifiedFiles, nil } +func (e *EditorAgent) allExpectedFilesModified(modifiedFiles []string) bool { + if len(e.allowedFiles) == 0 { + return false + } + for _, expected := range e.allowedFiles { + found := false + for _, modified := range modifiedFiles { + if modified == expected || strings.HasSuffix(modified, expected) || strings.HasSuffix(expected, modified) { + found = true + break + } + } + if !found { + return false + } + } + return true +} + func min(a, b int) int { if a < b { return a @@ -555,6 +629,7 @@ func containsEditKeywords(text string) bool { editKeywords := []string{ "write_file", "apply_patch", // Tool calls "modify file", "create file", "update file", "patch file", + "fix ", "implement ", "create ", "add test", "add regression", "add to", "append to", "insert into", "delete from", "remove from", "rename", "move file", "rewrite", @@ -568,6 +643,15 @@ func containsEditKeywords(text string) bool { return false } +func editRequested(messages []llm.ChatMessage) bool { + for _, message := range messages { + if message.Role == "user" && containsEditKeywords(message.Content) { + return true + } + } + return false +} + func (e *EditorAgent) validateFileWrite(args map[string]interface{}) error { if len(e.allowedFiles) == 0 { return nil diff --git a/internal/agents/editor_test.go b/internal/agents/editor_test.go index 724ff07..634cbca 100644 --- a/internal/agents/editor_test.go +++ b/internal/agents/editor_test.go @@ -8,6 +8,7 @@ import ( "testing" "github.com/jadercorrea/gptcode/internal/llm" + "github.com/jadercorrea/gptcode/internal/observability" ) // mockProvider simulates LLM responses for testing @@ -29,6 +30,21 @@ func (m *mockProvider) ChatStream(ctx context.Context, req llm.ChatRequest, call return nil } +func TestEditorRecordsLLMCallWhenProviderOmitsTokenUsage(t *testing.T) { + provider := &mockProvider{responses: []llm.ChatResponse{{Text: "No changes needed."}}} + observer := observability.NewObserver() + editor := NewEditorWithObserver(provider, t.TempDir(), "local-model", observer) + + if _, _, err := editor.Execute(context.Background(), []llm.ChatMessage{{ + Role: "user", Content: "Inspect the implementation", + }}, nil); err != nil { + t.Fatal(err) + } + if calls := observer.Summary().LLMCalls; calls != 1 { + t.Fatalf("expected one recorded LLM call, got %d", calls) + } +} + // Test: Query task (read file) should return file content immediately func TestEditor_QueryTask_ReturnsContent(t *testing.T) { tmpDir := t.TempDir() @@ -88,6 +104,7 @@ func TestEditor_CreateFileTask_ExecutesAndReturns(t *testing.T) { }, }, }, + {Text: "Implemented output.txt."}, }, } @@ -105,14 +122,14 @@ func TestEditor_CreateFileTask_ExecutesAndReturns(t *testing.T) { if err != nil { t.Fatalf("Expected no error, got: %v", err) } - if result != "Changes applied; awaiting deterministic validation" { + if result != "Implemented output.txt." { t.Errorf("Expected success message, got: %q", result) } if len(modifiedFiles) != 1 || modifiedFiles[0] != "output.txt" { t.Errorf("Expected modifiedFiles=['output.txt'], got: %v", modifiedFiles) } - if mock.callCount != 1 { - t.Errorf("Expected 1 LLM call, got: %d", mock.callCount) + if mock.callCount != 2 { + t.Errorf("Expected write and completion calls, got: %d", mock.callCount) } // Verify file was actually created @@ -279,6 +296,7 @@ func TestEditor_EditTask_ContinuesUntilDone(t *testing.T) { }, }, }, + {Text: "Renamed old to new."}, }, } @@ -308,11 +326,11 @@ func TestEditor_EditTask_ContinuesUntilDone(t *testing.T) { if result == "package main\n\nfunc old() {}\n" { t.Skip("KNOWN ISSUE: read_file returns early even for edit tasks. Need to fix: only return early for pure query tasks.") } - if result != "Changes applied; awaiting deterministic validation" { + if result != "Renamed old to new." { t.Errorf("Expected editor completion after patch, got: %q", result) } - if mock.callCount != 2 { - t.Errorf("Expected 2 LLM calls (read, patch), got: %d", mock.callCount) + if mock.callCount != 3 { + t.Errorf("Expected 3 LLM calls (read, patch, completion), got: %d", mock.callCount) } } @@ -431,6 +449,170 @@ func TestEditor_QueryThenEdit_ReturnsAfterEdit(t *testing.T) { } } +func TestEditor_EditTask_RetriesWhenModelReturnsWithoutToolCalls(t *testing.T) { + tmpDir := t.TempDir() + + mock := &mockProvider{ + responses: []llm.ChatResponse{ + {Text: "I will update the file."}, + { + ToolCalls: []llm.ChatToolCall{{ + ID: "1", + Name: "write_file", + Arguments: `{"path":"result.txt","content":"implemented"}`, + }}, + }, + {Text: "Implemented result.txt."}, + }, + } + + editor := NewEditor(mock, tmpDir, "test-model") + result, modifiedFiles, err := editor.Execute(context.Background(), []llm.ChatMessage{{ + Role: "user", + Content: "Create result.txt with the implementation.", + }}, nil) + + if err != nil { + t.Fatalf("expected retry to recover, got error: %v", err) + } + if result != "Implemented result.txt." { + t.Fatalf("expected deterministic validation handoff, got %q", result) + } + if len(modifiedFiles) != 1 || modifiedFiles[0] != "result.txt" { + t.Fatalf("expected result.txt to be modified, got %v", modifiedFiles) + } + if mock.callCount != 3 { + t.Fatalf("expected retry, write, and completion calls, got %d", mock.callCount) + } +} + +func TestEditor_EditTask_ModifiesEveryPlannedFileBeforeCompleting(t *testing.T) { + tmpDir := t.TempDir() + mock := &mockProvider{ + responses: []llm.ChatResponse{ + {ToolCalls: []llm.ChatToolCall{{ + ID: "1", + Name: "write_file", + Arguments: `{"path":"implementation.go","content":"package example"}`, + }}}, + {ToolCalls: []llm.ChatToolCall{{ + ID: "2", + Name: "write_file", + Arguments: `{"path":"implementation_test.go","content":"package example"}`, + }}}, + {Text: "Implementation and regression tests completed."}, + }, + } + + editor := NewEditor(mock, tmpDir, "test-model") + result, modifiedFiles, err := editor.Execute(context.Background(), []llm.ChatMessage{{ + Role: "user", + Content: "Files to create:\n- implementation.go\n- implementation_test.go\n" + + "Implement the change and its regression tests.", + }}, nil) + + if err != nil { + t.Fatalf("expected multi-file edit to complete: %v", err) + } + if result != "Implementation and regression tests completed." { + t.Fatalf("unexpected completion: %q", result) + } + if len(modifiedFiles) != 2 { + t.Fatalf("expected both planned files to be modified, got %v", modifiedFiles) + } + if mock.callCount != 3 { + t.Fatalf("expected two writes and completion, got %d calls", mock.callCount) + } +} + +func TestEditor_EditTask_StopsWhenEveryExpectedFileWasModified(t *testing.T) { + tmpDir := t.TempDir() + mock := &mockProvider{ + responses: []llm.ChatResponse{{ + ToolCalls: []llm.ChatToolCall{{ + ID: "1", + Name: "write_file", + Arguments: `{"path":"counter.go","content":"package counter"}`, + }}, + }}, + } + + editor := NewEditor(mock, tmpDir, "test-model") + editor.SetExpectedFiles([]string{"counter.go"}) + result, modifiedFiles, err := editor.Execute(context.Background(), []llm.ChatMessage{{ + Role: "user", + Content: "Modify counter.go to fix the bug.", + }}, nil) + + if err != nil { + t.Fatalf("expected edit to complete: %v", err) + } + if result != "All planned files changed; awaiting deterministic validation" { + t.Fatalf("unexpected completion: %q", result) + } + if len(modifiedFiles) != 1 || modifiedFiles[0] != "counter.go" { + t.Fatalf("unexpected modified files: %v", modifiedFiles) + } + if mock.callCount != 1 { + t.Fatalf("expected no extra model completion call, got %d", mock.callCount) + } +} + +func TestEditor_ExpectedFilesRejectsSuffixCollision(t *testing.T) { + tmpDir := t.TempDir() + mock := &mockProvider{ + responses: []llm.ChatResponse{{ + ToolCalls: []llm.ChatToolCall{{ + ID: "1", + Name: "write_file", + Arguments: `{"path":"evilcounter.go","content":"package counter"}`, + }}, + }}, + } + + editor := NewEditor(mock, tmpDir, "test-model") + editor.SetExpectedFiles([]string{"counter.go"}) + _, modifiedFiles, _ := editor.Execute(context.Background(), []llm.ChatMessage{{ + Role: "user", + Content: "Modify counter.go.", + }}, nil) + + if len(modifiedFiles) != 0 { + t.Fatalf("suffix collision escaped the planned-file boundary: %v", modifiedFiles) + } + if _, err := os.Stat(filepath.Join(tmpDir, "evilcounter.go")); !os.IsNotExist(err) { + t.Fatalf("unexpected file write, stat error: %v", err) + } +} + +func TestEditor_EditTask_FailsWhenModelNeverUsesTools(t *testing.T) { + tmpDir := t.TempDir() + responses := make([]llm.ChatResponse, 10) + for i := range responses { + responses[i] = llm.ChatResponse{Text: "The change should be applied."} + } + mock := &mockProvider{responses: responses} + + editor := NewEditor(mock, tmpDir, "test-model") + result, modifiedFiles, err := editor.Execute(context.Background(), []llm.ChatMessage{{ + Role: "user", + Content: "Fix the bug by modifying code.go.", + }}, nil) + + if err == nil { + t.Fatal("expected an error when an edit task completes without modifying files") + } + if !strings.Contains(err.Error(), "without modifying any files") { + t.Fatalf("expected actionable error, got %v", err) + } + if result != "" { + t.Fatalf("expected no success result, got %q", result) + } + if len(modifiedFiles) != 0 { + t.Fatalf("expected no modified files, got %v", modifiedFiles) + } +} + // Test: Show file content vs grep - both should return immediately but with different content func TestEditor_ShowVsGrep_BothReturnImmediately(t *testing.T) { tmpDir := t.TempDir() diff --git a/internal/agents/query.go b/internal/agents/query.go index 79c31f8..5c1e2ca 100644 --- a/internal/agents/query.go +++ b/internal/agents/query.go @@ -153,7 +153,7 @@ func (q *QueryAgent) Execute(ctx context.Context, history []llm.ChatMessage, sta if statusCallback != nil { statusCallback(fmt.Sprintf("Query: Executing %s...", tc.Name)) } - result := tools.ExecuteToolFromLLM(llmCall, q.cwd) + result := tools.ExecuteToolFromLLMContext(ctx, llmCall, q.cwd) content := result.Result if result.Error != "" { diff --git a/internal/agents/review.go b/internal/agents/review.go index 5846ed3..324084c 100644 --- a/internal/agents/review.go +++ b/internal/agents/review.go @@ -189,7 +189,7 @@ func (r *ReviewAgent) Execute(ctx context.Context, history []llm.ChatMessage, st if statusCallback != nil { statusCallback(fmt.Sprintf("Review: Executing %s...", tc.Name)) } - result := tools.ExecuteToolFromLLM(llmCall, r.cwd) + result := tools.ExecuteToolFromLLMContext(ctx, llmCall, r.cwd) content := result.Result if result.Error != "" { diff --git a/internal/agents/reviewer.go b/internal/agents/reviewer.go index 260e603..8726143 100644 --- a/internal/agents/reviewer.go +++ b/internal/agents/reviewer.go @@ -249,7 +249,7 @@ Be precise and specific.`, plan, filesStr) // Tool call processing loop for validation - Maestro controls outer retry logic. // Lower internal limit (5) since most validations complete in 2-3 iterations. - maxIterations := 5 + maxIterations := 3 for i := 0; i < maxIterations; i++ { if os.Getenv("GPTCODE_DEBUG") == "1" { fmt.Fprintf(os.Stderr, "[VALIDATOR] Iteration %d/%d\n", i+1, maxIterations) @@ -297,7 +297,7 @@ Be precise and specific.`, plan, filesStr) Name: tc.Name, Arguments: tc.Arguments, } - result := tools.ExecuteToolFromLLM(llmCall, v.cwd) + result := tools.ExecuteToolFromLLMContext(ctx, llmCall, v.cwd) content := result.Result if result.Error != "" { @@ -333,11 +333,51 @@ Be precise and specific.`, plan, filesStr) } } - return &ReviewResult{ - Success: false, - Issues: []string{"Validator reached max iterations"}, - Suggestions: "Unable to complete review", - }, nil + history = append(history, llm.ChatMessage{ + Role: "user", + Content: `Tool use is now complete. Based only on the evidence already collected, return a final verdict. +Start with exactly SUCCESS or FAIL. If FAIL, list concrete unmet requirements.`, + }) + resp, err := v.provider.Chat(ctx, llm.ChatRequest{ + SystemPrompt: reviewerPrompt, + Messages: history, + Model: v.model, + }) + if err != nil { + return nil, err + } + result := &ReviewResult{Suggestions: resp.Text} + verdict := explicitReviewVerdict(resp.Text) + result.Success = verdict == "success" + if verdict == "" { + return nil, fmt.Errorf("reviewer returned no explicit SUCCESS or FAIL verdict") + } + if !result.Success { + result.Issues = extractIssues(resp.Text) + if len(result.Issues) == 0 { + result.Issues = []string{"Reviewer did not provide a conclusive evidence-based verdict"} + } + } + return result, nil +} + +func explicitReviewVerdict(text string) string { + for _, line := range strings.Split(text, "\n") { + line = strings.TrimSpace(line) + if line == "" { + continue + } + first := strings.ToUpper(strings.Trim(strings.Fields(line)[0], ":.-")) + switch first { + case "SUCCESS": + return "success" + case "FAIL": + return "fail" + default: + return "" + } + } + return "" } func containsSuccess(text string) bool { diff --git a/internal/agents/reviewer_test.go b/internal/agents/reviewer_test.go new file mode 100644 index 0000000..41d18b2 --- /dev/null +++ b/internal/agents/reviewer_test.go @@ -0,0 +1,35 @@ +package agents + +import ( + "context" + "testing" + + "github.com/jadercorrea/gptcode/internal/llm" +) + +func TestReviewerForcesFinalVerdictAfterToolRounds(t *testing.T) { + toolResponse := llm.ChatResponse{ToolCalls: []llm.ChatToolCall{{ + ID: "read", Name: "read_file", Arguments: `{"path":"counter.go"}`, + }}} + provider := &mockProvider{responses: []llm.ChatResponse{ + toolResponse, toolResponse, toolResponse, {Text: "SUCCESS\nAll requirements are met."}, + }} + result, err := NewReviewer(provider, t.TempDir(), "local").Review( + context.Background(), "Tests pass", []string{"counter.go"}, nil, + ) + if err != nil { + t.Fatal(err) + } + if !result.Success { + t.Fatalf("expected final synthesis to succeed, got %#v", result) + } + if provider.callCount != 4 { + t.Fatalf("expected three tool rounds and one synthesis, got %d calls", provider.callCount) + } +} + +func TestExplicitReviewVerdictDoesNotMistakeCriterionForFailure(t *testing.T) { + if verdict := explicitReviewVerdict("2. go test ./... runs without errors (tests pass)"); verdict != "" { + t.Fatalf("expected ambiguous text to have no verdict, got %q", verdict) + } +} diff --git a/internal/autonomous/analyzer.go b/internal/autonomous/analyzer.go index 60c21b0..ec825e6 100644 --- a/internal/autonomous/analyzer.go +++ b/internal/autonomous/analyzer.go @@ -4,6 +4,8 @@ import ( "context" "encoding/json" "fmt" + "os" + "path/filepath" "regexp" "strings" @@ -50,16 +52,21 @@ func (a *TaskAnalyzer) DeepAnalyze(ctx context.Context, task string) (string, er provider = llm.NewChatCompletion(backendCfg.BaseURL, backendName) } + evidence := repositoryEvidence(a.cwd, extractFileMentions(task), 24*1024) deepAnalysisPrompt := fmt.Sprintf(`You are a senior software engineer performing DEEP ANALYSIS of a bug fix task. TASK: %s +REPOSITORY EVIDENCE: +%s + INSTRUCTIONS: -1. Identify the ROOT CAUSE of the bug (not just symptoms) -2. Determine what files likely need to be modified -3. Identify any edge cases or potential regressions -4. Outline the APPROACH for fixing this bug +1. Base every claim on the repository evidence above. Never invent behavior, files, or root causes. +2. Identify the ROOT CAUSE. If evidence is insufficient, say "UNKNOWN โ€” more repository evidence is required". +3. Determine what files likely need to be modified +4. Identify any edge cases or potential regressions +5. Outline the APPROACH for fixing this bug Be specific and technical. Focus on the actual code changes needed. @@ -67,7 +74,7 @@ Return your analysis in this format: ROOT CAUSE: [2-3 sentence explanation of the root cause] FILES TO MODIFY: [list of likely files] EDGE CASES: [potential edge cases to consider] -APPROACH: [high-level approach to fix]`, task) +APPROACH: [high-level approach to fix]`, task, evidence) response, err := provider.Chat(ctx, llm.ChatRequest{ SystemPrompt: "You are a senior software engineer performing deep analysis.", @@ -82,6 +89,48 @@ APPROACH: [high-level approach to fix]`, task) return response.Text, nil } +func repositoryEvidence(cwd string, paths []string, limit int) string { + if len(paths) == 0 { + return "No explicit repository files were identified in the task." + } + root, err := filepath.EvalSymlinks(cwd) + if err != nil { + return "The repository root could not be resolved." + } + var evidence strings.Builder + for _, path := range paths { + clean := filepath.Clean(path) + if filepath.IsAbs(clean) || clean == ".." || strings.HasPrefix(clean, ".."+string(filepath.Separator)) { + continue + } + resolved, err := filepath.EvalSymlinks(filepath.Join(root, clean)) + if err != nil { + continue + } + relative, err := filepath.Rel(root, resolved) + if err != nil || relative == ".." || strings.HasPrefix(relative, ".."+string(filepath.Separator)) { + continue + } + content, err := os.ReadFile(resolved) + if err != nil { + continue + } + remaining := limit - evidence.Len() + if remaining <= 0 { + break + } + entry := fmt.Sprintf("\n--- %s ---\n%s\n", clean, content) + if len(entry) > remaining { + entry = entry[:remaining] + } + evidence.WriteString(entry) + } + if evidence.Len() == 0 { + return "The files named in the task could not be read." + } + return evidence.String() +} + // TaskAnalysis represents the result of analyzing a task type TaskAnalysis struct { Intent string `json:"intent"` diff --git a/internal/autonomous/analyzer_test.go b/internal/autonomous/analyzer_test.go index 76f3933..d2edde4 100644 --- a/internal/autonomous/analyzer_test.go +++ b/internal/autonomous/analyzer_test.go @@ -2,12 +2,46 @@ package autonomous import ( "context" + "os" + "path/filepath" + "strings" "testing" "github.com/jadercorrea/gptcode/internal/agents" "github.com/jadercorrea/gptcode/internal/llm" ) +func TestRepositoryEvidenceReadsOnlyFilesNamedByTask(t *testing.T) { + root := t.TempDir() + if err := os.WriteFile(filepath.Join(root, "counter.go"), []byte("func Add(a, b int) int { return a - b }\n"), 0600); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(root, "secret.txt"), []byte("do not include"), 0600); err != nil { + t.Fatal(err) + } + evidence := repositoryEvidence(root, []string{"counter.go"}, 4096) + if !strings.Contains(evidence, "return a - b") { + t.Fatalf("expected source evidence, got %q", evidence) + } + if strings.Contains(evidence, "do not include") { + t.Fatal("included a file that was not named by the task") + } +} + +func TestRepositoryEvidenceRejectsSymlinkOutsideRepository(t *testing.T) { + root := t.TempDir() + outside := filepath.Join(t.TempDir(), "secret.go") + if err := os.WriteFile(outside, []byte("outside secret"), 0600); err != nil { + t.Fatal(err) + } + if err := os.Symlink(outside, filepath.Join(root, "linked.go")); err != nil { + t.Fatal(err) + } + if evidence := repositoryEvidence(root, []string{"linked.go"}, 4096); strings.Contains(evidence, "outside secret") { + t.Fatal("repository evidence followed a symlink outside the repository") + } +} + func TestExtractVerb(t *testing.T) { tests := []struct { task string diff --git a/internal/config/model_selector.go b/internal/config/model_selector.go index f5c1c51..f065996 100644 --- a/internal/config/model_selector.go +++ b/internal/config/model_selector.go @@ -8,6 +8,8 @@ import ( "path/filepath" "strings" "time" + + "github.com/jadercorrea/gptcode/internal/catalog" ) type ActionType string @@ -100,7 +102,13 @@ func (ms *ModelSelector) loadCatalog() error { catalogPath := filepath.Join(configDir(), "models_catalog.json") data, err := os.ReadFile(catalogPath) if err != nil { - return err + if !os.IsNotExist(err) { + return err + } + data = catalog.GetDefaultModels() + if len(data) == 0 { + return fmt.Errorf("user catalog not found and embedded catalog is empty") + } } var rawCatalog map[string]interface{} diff --git a/internal/config/model_selector_test.go b/internal/config/model_selector_test.go index c17552e..6cb9a80 100644 --- a/internal/config/model_selector_test.go +++ b/internal/config/model_selector_test.go @@ -1,9 +1,37 @@ package config import ( + "os" + "path/filepath" "testing" ) +func TestNewModelSelectorUsesEmbeddedCatalogWhenUserCatalogIsMissing(t *testing.T) { + home := t.TempDir() + t.Setenv("HOME", home) + + setup := &Setup{Backend: map[string]BackendConfig{ + "ollama": { + Type: "ollama", + DefaultModel: "qwen3-coder:latest", + AgentModels: AgentModels{Editor: "qwen3-coder:latest"}, + }, + }} + setup.Defaults.Backend = "ollama" + setup.Defaults.Mode = "local" + + selector, err := NewModelSelector(setup) + if err != nil { + t.Fatalf("expected embedded catalog fallback, got %v", err) + } + if selector == nil { + t.Fatal("expected a usable selector") + } + if _, err := os.Stat(filepath.Join(home, ".gptcode", "models_catalog.json")); !os.IsNotExist(err) { + t.Fatalf("fallback must not require creating a user catalog, stat error: %v", err) + } +} + func TestSelectModelPrefersConfiguredEditor(t *testing.T) { setup := &Setup{ Backend: map[string]BackendConfig{ diff --git a/internal/llm/loop_detector.go b/internal/llm/loop_detector.go index fb41335..c6a5297 100644 --- a/internal/llm/loop_detector.go +++ b/internal/llm/loop_detector.go @@ -28,6 +28,7 @@ type LoopDetector struct { FileModifications int ReadOperations int Intent string // "query", "edit", "plan", "research" + MaxIterations int // Optional caller-provided limit } // NewLoopDetector creates a new loop detector with Claude Code-style thresholds @@ -95,7 +96,7 @@ func (ld *LoopDetector) ShouldContinue() (shouldContinue bool, reason string) { // Intent-aware safety limits (inspired by Claude Code --max-turns) maxIterations := ld.getMaxIterationsForIntent() - if ld.Iteration >= maxIterations { + if ld.Iteration > maxIterations { return false, fmt.Sprintf("Safety limit reached: %d iterations for %s intent", maxIterations, ld.Intent) } @@ -120,6 +121,9 @@ func (ld *LoopDetector) ShouldContinue() (shouldContinue bool, reason string) { // getMaxIterationsForIntent returns the maximum iterations based on task intent func (ld *LoopDetector) getMaxIterationsForIntent() int { + if ld.MaxIterations > 0 { + return ld.MaxIterations + } limits := map[string]int{ "query": 10, // Query tasks are typically shorter "edit": 15, // Reduced from 25 to prevent runaway edits @@ -134,6 +138,10 @@ func (ld *LoopDetector) getMaxIterationsForIntent() int { return limits[""] } +func (ld *LoopDetector) SetMaxIterations(max int) { + ld.MaxIterations = max +} + // RecordFileModification increments the file modification counter func (ld *LoopDetector) RecordFileModification() { ld.FileModifications++ diff --git a/internal/llm/loop_detector_test.go b/internal/llm/loop_detector_test.go index 972aeb0..ec45589 100644 --- a/internal/llm/loop_detector_test.go +++ b/internal/llm/loop_detector_test.go @@ -85,3 +85,14 @@ func TestLoopDetector_DifferentToolCallsNoLoop(t *testing.T) { } } } + +func TestLoopDetectorHonorsConfiguredMaxIterations(t *testing.T) { + detector := NewLoopDetector("edit") + detector.SetMaxIterations(1) + if ok, reason := detector.ShouldContinue(); !ok { + t.Fatalf("first attempt should run: %s", reason) + } + if ok, _ := detector.ShouldContinue(); ok { + t.Fatal("second attempt should be rejected when max iterations is one") + } +} diff --git a/internal/maestro/conductor.go b/internal/maestro/conductor.go index 2561057..7ac660c 100644 --- a/internal/maestro/conductor.go +++ b/internal/maestro/conductor.go @@ -33,6 +33,7 @@ type Conductor struct { liveReportConfig *live.ReportConfig // For Live Dashboard HTTP reporting liveClient *live.Client // For Live Dashboard WebSocket reporting progressCallback ProgressCallback // For real-time progress updates + maxAttempts int // Telemetry mu sync.Mutex @@ -41,6 +42,10 @@ type Conductor struct { currentModel string } +func (c *Conductor) SetMaxAttempts(max int) { + c.maxAttempts = max +} + // NewConductor creates a new Maestro conductor func NewConductor( selector *config.ModelSelector, @@ -327,6 +332,7 @@ Do not add, remove, or rename exported symbols when API stability is required.`, intent = "query" } c.loopDetector = llm.NewLoopDetector(intent) + c.loopDetector.SetMaxIterations(c.maxAttempts) if os.Getenv("GPTCODE_DEBUG") == "1" { fmt.Fprintf(os.Stderr, "[MAESTRO] LoopDetector initialized with intent=%s\n", intent) @@ -371,6 +377,7 @@ Do not add, remove, or rename exported symbols when API stability is required.`, // Create editor with selected model and observer editProvider := c.createProvider(editBackend) editor := agents.NewEditorWithObserver(editProvider, c.cwd, editModel, c.Observer) + editor.SetExpectedFiles(plannedFiles(plan)) // Execute with editor fmt.Println("Executing changes...") @@ -529,7 +536,14 @@ Do not add, remove, or rename exported symbols when API stability is required.`, fmt.Println("Validating...") c.ReportProgress("validation", "Running tests and checks") start = time.Now() - review, err := reviewer.Review(ctx, plan, modifiedFiles, nil) + reviewPlan := fmt.Sprintf(`%s + +DETERMINISTIC VERIFICATION EVIDENCE (authoritative): +Command: %s +Exit status: 0 +Output: +%s`, plan, strings.Join(verificationCommand, " "), verificationOutput) + review, err := reviewer.Review(ctx, reviewPlan, modifiedFiles, nil) elapsed = time.Since(start) c.ReportProgress("validation", "Validation complete") c.selector.RecordUsage(reviewBackend, reviewModel, err == nil, errorMsg(err)) @@ -878,3 +892,45 @@ func (c *Conductor) isQueryTask(task, plan string, modifiedFiles []string) bool return false } + +func plannedFiles(plan string) []string { + var files []string + inFilesSection := false + + for _, rawLine := range strings.Split(plan, "\n") { + line := strings.TrimSpace(rawLine) + lower := strings.ToLower(line) + + if strings.HasPrefix(lower, "## files to modify") || + strings.HasPrefix(lower, "## files to create") { + inFilesSection = true + continue + } + if strings.HasPrefix(line, "## ") { + inFilesSection = false + continue + } + if !inFilesSection || !strings.HasPrefix(line, "- ") { + continue + } + + item := strings.TrimSpace(strings.TrimPrefix(line, "- ")) + if strings.EqualFold(item, "none") { + continue + } + if start := strings.Index(item, "`"); start >= 0 { + if end := strings.Index(item[start+1:], "`"); end >= 0 { + item = item[start+1 : start+1+end] + } + } else if description := strings.Index(item, " ("); description >= 0 { + item = item[:description] + } + + item = strings.TrimSpace(item) + if item != "" { + files = append(files, item) + } + } + + return files +} diff --git a/internal/maestro/conductor_plan_files_test.go b/internal/maestro/conductor_plan_files_test.go new file mode 100644 index 0000000..06a32b9 --- /dev/null +++ b/internal/maestro/conductor_plan_files_test.go @@ -0,0 +1,32 @@ +package maestro + +import ( + "reflect" + "testing" +) + +func TestPlannedFilesExtractsModifyAndCreateSections(t *testing.T) { + plan := `# Plan + +## Files to modify +- ` + "`counter.go`" + ` (fix implementation) + +## Files to create +- ` + "`counter_regression_test.go`" + ` + +## Success Criteria +- tests pass +` + + want := []string{"counter.go", "counter_regression_test.go"} + if got := plannedFiles(plan); !reflect.DeepEqual(got, want) { + t.Fatalf("plannedFiles() = %v, want %v", got, want) + } +} + +func TestPlannedFilesIgnoresNone(t *testing.T) { + plan := "## Files to modify\n- None\n\n## Changes\n- Run tests" + if got := plannedFiles(plan); len(got) != 0 { + t.Fatalf("plannedFiles() = %v, want no files", got) + } +} diff --git a/internal/maestro/requested_verification.go b/internal/maestro/requested_verification.go index 10dec2d..66a2136 100644 --- a/internal/maestro/requested_verification.go +++ b/internal/maestro/requested_verification.go @@ -4,10 +4,11 @@ import ( "context" "fmt" "os" - "os/exec" "path/filepath" "regexp" "strings" + + "github.com/jadercorrea/gptcode/internal/processutil" ) var safeVerificationToken = regexp.MustCompile(`^(?:-[A-Za-z0-9=._/-]+|[A-Za-z0-9.][A-Za-z0-9=._/-]*)$`) @@ -15,11 +16,12 @@ var safeVerificationToken = regexp.MustCompile(`^(?:-[A-Za-z0-9=._/-]+|[A-Za-z0- func requestedVerificationCommand(task string) []string { fields := strings.Fields(task) for index := 0; index+1 < len(fields); index++ { - if !strings.EqualFold(fields[index], "go") || !strings.EqualFold(fields[index+1], "test") { + executable := strings.ToLower(fields[index]) + if (executable != "go" && executable != "mix") || !strings.EqualFold(fields[index+1], "test") { continue } - command := []string{"go", "test"} + command := []string{executable, "test"} for _, field := range fields[index+2:] { token := strings.TrimRight(field, ",:") if token != "./..." { @@ -79,9 +81,7 @@ func runRequestedVerification(ctx context.Context, cwd string, command []string) if len(command) == 0 { return "", nil } - process := exec.CommandContext(ctx, command[0], command[1:]...) - process.Dir = cwd - output, err := process.CombinedOutput() + output, err := processutil.CombinedOutput(ctx, cwd, command[0], command[1:]...) if err != nil { return string(output), fmt.Errorf("%s failed: %w", strings.Join(command, " "), err) } diff --git a/internal/maestro/requested_verification_test.go b/internal/maestro/requested_verification_test.go index 8451c2e..74b7c64 100644 --- a/internal/maestro/requested_verification_test.go +++ b/internal/maestro/requested_verification_test.go @@ -2,12 +2,46 @@ package maestro import ( "context" + "errors" "os" + "os/exec" "path/filepath" "reflect" + "runtime" + "strings" "testing" + "time" ) +func TestRunRequestedVerificationCancelsProcessTree(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("Unix process-group behavior") + } + root := t.TempDir() + pidFile := filepath.Join(root, "child.pid") + ctx, cancel := context.WithTimeout(context.Background(), 300*time.Millisecond) + defer cancel() + + _, err := runRequestedVerification(ctx, root, []string{ + "sh", "-c", "sleep 30 & echo $! > child.pid; wait", + }) + if !errors.Is(err, context.DeadlineExceeded) { + t.Fatalf("expected deadline error, got %v", err) + } + data, readErr := os.ReadFile(pidFile) + if readErr != nil { + t.Fatal(readErr) + } + pid := strings.TrimSpace(string(data)) + if processExists(pid) { + t.Fatalf("child process %s survived cancellation", pid) + } +} + +func processExists(pid string) bool { + return exec.Command("sh", "-c", "kill -0 "+pid).Run() == nil +} + func TestRequestedVerificationCommandUsesExplicitRaceCheck(t *testing.T) { got := requestedVerificationCommand("Fix it. Verify with go test -race ./...") want := []string{"go", "test", "-race", "./..."} @@ -24,6 +58,16 @@ func TestRequestedVerificationCommandUsesScopedGoCheck(t *testing.T) { } } +func TestRequestedVerificationCommandUsesScopedMixCheck(t *testing.T) { + got := requestedVerificationCommand( + "Fix it. Run mix test test/teiserver_web/components/core_components_test.exs and report exact evidence.", + ) + want := []string{"mix", "test", "test/teiserver_web/components/core_components_test.exs"} + if !reflect.DeepEqual(got, want) { + t.Fatalf("command = %v, want %v", got, want) + } +} + func TestRequestedVerificationCommandRejectsShellSyntax(t *testing.T) { got := requestedVerificationCommand("Run go test ./...; rm -rf /") if got != nil { diff --git a/internal/modes/autonomous.go b/internal/modes/autonomous.go index e1e063d..14dadde 100644 --- a/internal/modes/autonomous.go +++ b/internal/modes/autonomous.go @@ -84,6 +84,10 @@ func (a *AutonomousExecutor) Execute(ctx context.Context, task string) error { return a.executor.Execute(ctx, task) } +func (a *AutonomousExecutor) SetMaxAttempts(max int) { + a.conductor.SetMaxAttempts(max) +} + // ShouldUseAutonomous determines if a task should use autonomous mode // This is a lightweight heuristic check before full analysis. // The real complexity scoring happens in TaskAnalyzer.estimateComplexity() diff --git a/internal/observability/observer.go b/internal/observability/observer.go index 29703e5..619bdb1 100644 --- a/internal/observability/observer.go +++ b/internal/observability/observer.go @@ -102,6 +102,7 @@ type ExecutionSummary struct { FilesModified []string `json:"files_modified"` FilesDeleted []string `json:"files_deleted"` ToolCalls map[string]int `json:"tool_calls"` + ToolDuration time.Duration `json:"tool_duration_ms"` LLMCalls int `json:"llm_calls"` TokensIn int `json:"tokens_in"` TokensOut int `json:"tokens_out"` @@ -143,6 +144,7 @@ type AgentObserver struct { filesModified map[string]int64 filesDeleted []string toolCalls map[string]int + toolDuration time.Duration llmCalls int tokensIn int tokensOut int @@ -181,6 +183,7 @@ func (o *AgentObserver) Emit(event Event) { switch e := event.(type) { case *ToolCallEvent: o.toolCalls[e.Name]++ + o.toolDuration += e.Duration // If the tool event didn't calculate cost saved, let's do it based on active model if e.TokensSaved > 0 && e.CostSaved == 0 { @@ -316,6 +319,7 @@ func (o *AgentObserver) Summary() *ExecutionSummary { FilesModified: modified, FilesDeleted: o.filesDeleted, ToolCalls: o.toolCalls, + ToolDuration: o.toolDuration, LLMCalls: o.llmCalls, TokensIn: o.tokensIn, TokensOut: o.tokensOut, @@ -421,7 +425,7 @@ func (o *AgentObserver) PrintSummary() { totalToolCalls += count } if totalToolCalls > 0 { - avgPerCall := summary.Duration.Seconds() / float64(totalToolCalls) + avgPerCall := summary.ToolDuration.Seconds() / float64(totalToolCalls) fmt.Printf(" Avg per Tool Call: %.2fs\n", avgPerCall) } @@ -493,10 +497,14 @@ func (o *AgentObserver) PrintSummary() { fmt.Println(strings.Repeat("-", 40)) if summary.LLMCalls > 0 || summary.TokensIn > 0 || summary.TokensOut > 0 { - fmt.Printf(" API Calls: %d\n", summary.LLMCalls) - fmt.Printf(" Tokens In: %s\n", formatNumber(summary.TokensIn)) - fmt.Printf(" Tokens Out: %s\n", formatNumber(summary.TokensOut)) - fmt.Printf(" Total Tokens: %s\n", formatNumber(summary.TokensIn+summary.TokensOut)) + fmt.Printf(" Observed Model Calls: %d\n", summary.LLMCalls) + if summary.TokensIn+summary.TokensOut > 0 { + fmt.Printf(" Tokens In: %s\n", formatNumber(summary.TokensIn)) + fmt.Printf(" Tokens Out: %s\n", formatNumber(summary.TokensOut)) + fmt.Printf(" Total Tokens: %s\n", formatNumber(summary.TokensIn+summary.TokensOut)) + } else { + fmt.Println(" Token Usage: not reported by provider") + } fmt.Printf(" Total Cost: $%.4f\n", summary.TotalCost) if summary.TokensSaved > 0 { fmt.Printf(" Tokens Saved (RTK): %s\n", formatNumber(summary.TokensSaved)) diff --git a/internal/observability/observer_test.go b/internal/observability/observer_test.go new file mode 100644 index 0000000..a58eac7 --- /dev/null +++ b/internal/observability/observer_test.go @@ -0,0 +1,16 @@ +package observability + +import ( + "testing" + "time" +) + +func TestSummaryTracksToolDurationSeparatelyFromTaskDuration(t *testing.T) { + observer := NewObserver() + observer.Emit(&ToolCallEvent{Name: "apply_patch", Duration: 25 * time.Millisecond}) + + summary := observer.Summary() + if summary.ToolDuration != 25*time.Millisecond { + t.Fatalf("expected 25ms of tool time, got %s", summary.ToolDuration) + } +} diff --git a/internal/processutil/combined_output.go b/internal/processutil/combined_output.go new file mode 100644 index 0000000..c60cc11 --- /dev/null +++ b/internal/processutil/combined_output.go @@ -0,0 +1,14 @@ +package processutil + +import ( + "context" + "os/exec" +) + +// CombinedOutput runs a command and ensures cancellation is delegated to the +// platform implementation, which may need to terminate an entire process tree. +func CombinedOutput(ctx context.Context, cwd, name string, args ...string) ([]byte, error) { + command := exec.Command(name, args...) + command.Dir = cwd + return combinedOutput(ctx, command) +} diff --git a/internal/processutil/combined_output_unix.go b/internal/processutil/combined_output_unix.go new file mode 100644 index 0000000..e5eabc6 --- /dev/null +++ b/internal/processutil/combined_output_unix.go @@ -0,0 +1,36 @@ +//go:build !windows + +package processutil + +import ( + "bytes" + "context" + "os/exec" + "syscall" +) + +func combinedOutput(ctx context.Context, command *exec.Cmd) ([]byte, error) { + command.SysProcAttr = &syscall.SysProcAttr{Setpgid: true} + var output bytes.Buffer + command.Stdout = &output + command.Stderr = &output + if err := command.Start(); err != nil { + return output.Bytes(), err + } + + var waitErr error + done := make(chan struct{}) + go func() { + waitErr = command.Wait() + close(done) + }() + + select { + case <-done: + return output.Bytes(), waitErr + case <-ctx.Done(): + _ = syscall.Kill(-command.Process.Pid, syscall.SIGKILL) + <-done + return output.Bytes(), ctx.Err() + } +} diff --git a/internal/processutil/combined_output_windows.go b/internal/processutil/combined_output_windows.go new file mode 100644 index 0000000..dbbd1cd --- /dev/null +++ b/internal/processutil/combined_output_windows.go @@ -0,0 +1,14 @@ +//go:build windows + +package processutil + +import ( + "context" + "os/exec" +) + +func combinedOutput(ctx context.Context, command *exec.Cmd) ([]byte, error) { + contextCommand := exec.CommandContext(ctx, command.Path, command.Args[1:]...) + contextCommand.Dir = command.Dir + return contextCommand.CombinedOutput() +} diff --git a/internal/refactor/breaking.go b/internal/refactor/breaking.go index cb1f52c..fb798fb 100644 --- a/internal/refactor/breaking.go +++ b/internal/refactor/breaking.go @@ -107,8 +107,7 @@ func (c *BreakingCoordinator) DetectAndCoordinate(ctx context.Context) (*Breakin } func (c *BreakingCoordinator) detectBreakingChanges() ([]BreakingChange, error) { - cmd := exec.Command("git", "diff", "--name-only", "HEAD") - cmd.Dir = c.workDir + cmd := repositoryGitCommand(c.workDir, "diff", "--name-only", "HEAD") output, err := cmd.Output() if err != nil { return nil, err @@ -140,8 +139,7 @@ func (c *BreakingCoordinator) analyzeFileChanges(file string) ([]BreakingChange, } relPath, _ := filepath.Rel(c.workDir, file) - cmd := exec.Command("git", "show", fmt.Sprintf("HEAD:%s", relPath)) - cmd.Dir = c.workDir + cmd := repositoryGitCommand(c.workDir, "show", fmt.Sprintf("HEAD:%s", relPath)) oldContent, err := cmd.Output() if err != nil { return nil, nil @@ -160,6 +158,35 @@ func (c *BreakingCoordinator) analyzeFileChanges(file string) ([]BreakingChange, return c.compareASTs(file, oldAST, currentAST), nil } +var localGitEnvironment = map[string]struct{}{ + "GIT_ALTERNATE_OBJECT_DIRECTORIES": {}, + "GIT_OBJECT_DIRECTORY": {}, + "GIT_DIR": {}, + "GIT_WORK_TREE": {}, + "GIT_IMPLICIT_WORK_TREE": {}, + "GIT_GRAFT_FILE": {}, + "GIT_INDEX_FILE": {}, + "GIT_NO_REPLACE_OBJECTS": {}, + "GIT_REPLACE_REF_BASE": {}, + "GIT_PREFIX": {}, + "GIT_SHALLOW_FILE": {}, + "GIT_COMMON_DIR": {}, +} + +func repositoryGitCommand(dir string, args ...string) *exec.Cmd { + cmd := exec.Command("git", args...) + cmd.Dir = dir + cmd.Env = make([]string, 0, len(os.Environ())) + for _, variable := range os.Environ() { + name, _, _ := strings.Cut(variable, "=") + if _, local := localGitEnvironment[name]; local { + continue + } + cmd.Env = append(cmd.Env, variable) + } + return cmd +} + func (c *BreakingCoordinator) parseFile(content []byte) (*ast.File, error) { fset := token.NewFileSet() return parser.ParseFile(fset, "", content, parser.ParseComments) diff --git a/internal/tools/tools.go b/internal/tools/tools.go index adb69b0..bc0e5da 100644 --- a/internal/tools/tools.go +++ b/internal/tools/tools.go @@ -1,6 +1,7 @@ package tools import ( + "context" "encoding/json" "fmt" "io" @@ -12,6 +13,7 @@ import ( "time" "github.com/jadercorrea/gptcode/internal/observability" + "github.com/jadercorrea/gptcode/internal/processutil" "gopkg.in/yaml.v3" ) @@ -30,6 +32,7 @@ type ToolResult struct { Tool string `json:"tool"` Result string `json:"result"` Error string `json:"error,omitempty"` + Err error `json:"-"` ModifiedFiles []string `json:"modified_files,omitempty"` TokensSaved int `json:"tokens_saved,omitempty"` } @@ -48,6 +51,14 @@ func GetAvailableTools() []map[string]interface{} { "type": "string", "description": "Relative path to the file from repository root", }, + "start_line": map[string]interface{}{ + "type": "integer", + "description": "Optional 1-based first line to read", + }, + "end_line": map[string]interface{}{ + "type": "integer", + "description": "Optional 1-based last line to read (inclusive)", + }, }, "required": []string{"path"}, }, @@ -241,13 +252,17 @@ func GetAvailableTools() []map[string]interface{} { } func ExecuteTool(call ToolCall, workdir string) ToolResult { + return ExecuteToolContext(context.Background(), call, workdir) +} + +func ExecuteToolContext(ctx context.Context, call ToolCall, workdir string) ToolResult { switch call.Name { case "read_file": return readFile(call, workdir) case "list_files": return listFiles(call, workdir) case "run_command": - return runCommand(call, workdir) + return runCommandContext(ctx, call, workdir) case "search_code": return searchCode(call, workdir) case "read_guideline": @@ -277,6 +292,10 @@ type LLMToolCall struct { } func ExecuteToolFromLLM(call LLMToolCall, workdir string) ToolResult { + return ExecuteToolFromLLMContext(context.Background(), call, workdir) +} + +func ExecuteToolFromLLMContext(ctx context.Context, call LLMToolCall, workdir string) ToolResult { var argsMap map[string]interface{} if err := json.Unmarshal([]byte(call.Arguments), &argsMap); err != nil { return ToolResult{ @@ -290,7 +309,7 @@ func ExecuteToolFromLLM(call LLMToolCall, workdir string) ToolResult { Arguments: argsMap, } - return ExecuteTool(toolCall, workdir) + return ExecuteToolContext(ctx, toolCall, workdir) } func readFile(call ToolCall, workdir string) ToolResult { @@ -310,9 +329,35 @@ func readFile(call ToolCall, workdir string) ToolResult { result := string(content) lines := strings.Split(result, "\n") + startLine, hasStart := numericArgument(call.Arguments, "start_line") + endLine, hasEnd := numericArgument(call.Arguments, "end_line") + if hasStart || hasEnd { + if !hasStart { + startLine = 1 + } + if !hasEnd { + endLine = startLine + 199 + } + if startLine < 1 || endLine < startLine || startLine > len(lines) { + return ToolResult{Tool: "read_file", Error: fmt.Sprintf( + "invalid line range %d-%d for file with %d lines", startLine, endLine, len(lines))} + } + if endLine > len(lines) { + endLine = len(lines) + } + + numbered := make([]string, 0, endLine-startLine+1) + for lineNumber := startLine; lineNumber <= endLine; lineNumber++ { + numbered = append(numbered, fmt.Sprintf("%d: %s", lineNumber, lines[lineNumber-1])) + } + return ToolResult{Tool: "read_file", Result: strings.Join(numbered, "\n")} + } + if len(lines) > 200 { truncated := strings.Join(lines[:200], "\n") - result = truncated + fmt.Sprintf("\n... (truncated, %d total lines)", len(lines)) + result = truncated + fmt.Sprintf( + "\n... (truncated, %d total lines; use start_line and end_line to read a specific range)", + len(lines)) } return ToolResult{ @@ -321,6 +366,21 @@ func readFile(call ToolCall, workdir string) ToolResult { } } +func numericArgument(arguments map[string]interface{}, name string) (int, bool) { + value, ok := arguments[name] + if !ok { + return 0, false + } + switch number := value.(type) { + case float64: + return int(number), true + case int: + return number, true + default: + return 0, false + } +} + func listFiles(call ToolCall, workdir string) ToolResult { pathArg, _ := call.Arguments["path"].(string) pattern, _ := call.Arguments["pattern"].(string) @@ -368,7 +428,7 @@ func listFiles(call ToolCall, workdir string) ToolResult { } } -func runCommand(call ToolCall, workdir string) ToolResult { +func runCommandContext(ctx context.Context, call ToolCall, workdir string) ToolResult { command, ok := call.Arguments["command"].(string) if !ok { return ToolResult{Tool: "run_command", Error: "command parameter required"} @@ -382,9 +442,7 @@ func runCommand(call ToolCall, workdir string) ToolResult { } } - cmd := exec.Command("sh", "-c", command) - cmd.Dir = workdir - output, err := cmd.CombinedOutput() + output, err := processutil.CombinedOutput(ctx, workdir, "sh", "-c", command) exitCode := 0 if err != nil { @@ -405,6 +463,7 @@ func runCommand(call ToolCall, workdir string) ToolResult { if err != nil { result.Error = err.Error() + result.Err = err } return result @@ -509,10 +568,22 @@ func writeFile(call ToolCall, workdir string) ToolResult { // ExecuteToolWithObserver wraps ExecuteToolFromLLM and emits events to the observer func ExecuteToolWithObserver(call LLMToolCall, workdir string, observer observability.Observer) ToolResult { + return ExecuteToolWithObserverContext(context.Background(), call, workdir, observer) +} + +func ExecuteToolWithObserverContext(ctx context.Context, call LLMToolCall, workdir string, observer observability.Observer) ToolResult { start := time.Now() + existedBefore := make(map[string]bool) + var arguments map[string]interface{} + if err := json.Unmarshal([]byte(call.Arguments), &arguments); err == nil { + if path, ok := arguments["path"].(string); ok { + _, err := os.Stat(filepath.Join(workdir, path)) + existedBefore[path] = err == nil + } + } // Execute the tool - result := ExecuteToolFromLLM(call, workdir) + result := ExecuteToolFromLLMContext(ctx, call, workdir) // Emit events if observer is provided if observer != nil { @@ -535,13 +606,10 @@ func ExecuteToolWithObserver(call LLMToolCall, workdir string, observer observab // Emit file modification events for _, file := range result.ModifiedFiles { // Determine operation type (create vs modify) - operation := "modify" + operation := "create" fullPath := filepath.Join(workdir, file) - if info, err := os.Stat(fullPath); err == nil { - // Check if file was just created (very recent) - if time.Since(info.ModTime()) < time.Second*2 { - operation = "create" - } + if existedBefore[file] { + operation = "modify" } var bytes int64 diff --git a/internal/tools/tools_test.go b/internal/tools/tools_test.go index d0a9967..0006cfa 100644 --- a/internal/tools/tools_test.go +++ b/internal/tools/tools_test.go @@ -1,12 +1,56 @@ package tools import ( + "context" + "errors" + "fmt" "os" "path/filepath" "strings" "testing" + "time" + + "github.com/jadercorrea/gptcode/internal/observability" ) +func TestExecuteToolFromLLMContextCancelsRunCommand(t *testing.T) { + ctx, cancel := context.WithTimeout(context.Background(), 100*time.Millisecond) + defer cancel() + start := time.Now() + result := ExecuteToolFromLLMContext(ctx, LLMToolCall{ + Name: "run_command", + Arguments: `{"command":"sleep 30"}`, + }, t.TempDir()) + if !errors.Is(result.Err, context.DeadlineExceeded) { + t.Fatalf("expected deadline error, got %#v", result) + } + if elapsed := time.Since(start); elapsed > 2*time.Second { + t.Fatalf("cancellation took too long: %s", elapsed) + } +} + +func TestExecuteToolWithObserverReportsExistingFileAsModified(t *testing.T) { + root := t.TempDir() + if err := os.WriteFile(filepath.Join(root, "counter.go"), []byte("return a - b"), 0600); err != nil { + t.Fatal(err) + } + observer := observability.NewObserver() + result := ExecuteToolWithObserver(LLMToolCall{ + Name: "apply_patch", + Arguments: `{"path":"counter.go","search":"return a - b","replace":"return a + b"}`, + }, root, observer) + if result.Error != "" { + t.Fatal(result.Error) + } + summary := observer.Summary() + if len(summary.FilesModified) != 1 || summary.FilesModified[0] != "counter.go" { + t.Fatalf("expected counter.go to be modified, got %#v", summary) + } + if len(summary.FilesCreated) != 0 { + t.Fatalf("did not expect a created file, got %#v", summary.FilesCreated) + } +} + func TestProjectMap(t *testing.T) { t.Run("basic structure", func(t *testing.T) { tmpDir, err := os.MkdirTemp("", "gptcode_test") @@ -207,6 +251,34 @@ func TestApplyPatch(t *testing.T) { }) } +func TestReadFile_ReturnsRequestedLineRange(t *testing.T) { + tmpDir := t.TempDir() + filePath := filepath.Join(tmpDir, "large.txt") + lines := make([]string, 300) + for i := range lines { + lines[i] = fmt.Sprintf("line %d", i+1) + } + if err := os.WriteFile(filePath, []byte(strings.Join(lines, "\n")), 0644); err != nil { + t.Fatal(err) + } + + result := readFile(ToolCall{ + Name: "read_file", + Arguments: map[string]interface{}{ + "path": "large.txt", + "start_line": float64(150), + "end_line": float64(152), + }, + }, tmpDir) + + if result.Error != "" { + t.Fatalf("read_file failed: %s", result.Error) + } + if result.Result != "150: line 150\n151: line 151\n152: line 152" { + t.Fatalf("unexpected range:\n%s", result.Result) + } +} + func TestPathSecurity(t *testing.T) { parent := t.TempDir() workDir := filepath.Join(parent, "repository") diff --git a/test/breaking_changes_test.go b/test/breaking_changes_test.go index 86be82b..d207355 100644 --- a/test/breaking_changes_test.go +++ b/test/breaking_changes_test.go @@ -95,6 +95,7 @@ if err != nil { } coordinator := refactor.NewBreakingCoordinator(provider, "test-model", tmpDir) + t.Setenv("GIT_INDEX_FILE", filepath.Join(t.TempDir(), "foreign-index")) result, err := coordinator.DetectAndCoordinate(context.Background()) if err != nil { @@ -136,12 +137,24 @@ func isolatedGitCommand(dir string, args ...string) *exec.Cmd { cmd.Dir = dir cmd.Env = make([]string, 0, len(os.Environ())) for _, variable := range os.Environ() { - if strings.HasPrefix(variable, "GIT_INDEX_FILE=") || - strings.HasPrefix(variable, "GIT_DIR=") || - strings.HasPrefix(variable, "GIT_WORK_TREE=") { + if strings.HasPrefix(variable, "GIT_") { continue } cmd.Env = append(cmd.Env, variable) } + cmd.Env = append(cmd.Env, "GIT_CONFIG_GLOBAL="+os.DevNull, "GIT_CONFIG_NOSYSTEM=1") return cmd } + +func TestIsolatedGitCommandClearsInheritedGitEnvironment(t *testing.T) { + t.Setenv("GIT_EXTERNAL_DIFF", "must-not-leak") + t.Setenv("GIT_OBJECT_DIRECTORY", "/tmp/must-not-leak") + + cmd := isolatedGitCommand(t.TempDir(), "status") + for _, variable := range cmd.Env { + if strings.HasPrefix(variable, "GIT_EXTERNAL_DIFF=") || + strings.HasPrefix(variable, "GIT_OBJECT_DIRECTORY=") { + t.Fatalf("inherited Git environment leaked into isolated command: %s", variable) + } + } +}