From 940123bce02df720274553d457d6a7fe9639e89b Mon Sep 17 00:00:00 2001 From: Nic van Dessel <51134175+nvandessel@users.noreply.github.com> Date: Fri, 27 Mar 2026 22:04:02 +0000 Subject: [PATCH 1/4] refactor: extract floopDirExists and logDecision helpers Extract floopDirExists(root) bool in cmd_hook.go to replace 4 instances of filepath.Join + os.Stat + os.IsNotExist. Add logDecision(fields) method on SubagentClient in subagent.go to encapsulate the nil-check on the decisions logger, replacing 7 occurrences. Co-Authored-By: Claude Opus 4.6 --- cmd/floop/cmd_hook.go | 21 ++++---- internal/llm/subagent.go | 111 ++++++++++++++++++--------------------- 2 files changed, 64 insertions(+), 68 deletions(-) diff --git a/cmd/floop/cmd_hook.go b/cmd/floop/cmd_hook.go index 6daac3d..9655e61 100644 --- a/cmd/floop/cmd_hook.go +++ b/cmd/floop/cmd_hook.go @@ -170,13 +170,19 @@ func formatCorrectionCapturedMessage(correctionID string) string { return fmt.Sprintf("### Correction Captured\nfloop auto-detected a correction from your message (id: %s)", correctionID) } +// floopDirExists checks whether the .floop directory exists under root. +func floopDirExists(root string) bool { + _, err := os.Stat(filepath.Join(root, ".floop")) + return !os.IsNotExist(err) +} + // hookLog appends a structured JSON log entry to .floop/hook-debug.log. // Silently no-ops if the .floop directory doesn't exist (pre-init state). func hookLog(root, hookName, stage, outcome string, extra map[string]interface{}) { - floopDir := filepath.Join(root, ".floop") - if _, err := os.Stat(floopDir); os.IsNotExist(err) { + if !floopDirExists(root) { return } + floopDir := filepath.Join(root, ".floop") logPath := filepath.Join(floopDir, "hook-debug.log") f, err := os.OpenFile(logPath, os.O_APPEND|os.O_CREATE|os.O_WRONLY, 0600) if err != nil { @@ -274,8 +280,7 @@ func runDetectCorrection(cmd *cobra.Command, root, prompt string, client llm.Cli right := sanitize.SanitizeBehaviorContent(result.Right) // Ensure .floop exists - floopDir := filepath.Join(root, ".floop") - if _, err := os.Stat(floopDir); os.IsNotExist(err) { + if !floopDirExists(root) { fmt.Fprintf(os.Stderr, "detect-correction: floop_dir_missing (root=%s)\n", root) return nil } @@ -310,7 +315,7 @@ func runDetectCorrection(cmd *cobra.Command, root, prompt string, client llm.Cli processedAt := time.Now() correction.ProcessedAt = &processedAt - correctionsPath := filepath.Join(floopDir, "corrections.jsonl") + correctionsPath := filepath.Join(root, ".floop", "corrections.jsonl") f, err := os.OpenFile(correctionsPath, os.O_APPEND|os.O_CREATE|os.O_WRONLY, 0600) if err == nil { json.NewEncoder(f).Encode(correction) @@ -340,8 +345,7 @@ This applies to: explicit corrections, preferences, "don't do X", repeated feedb // Used by session-start and first-prompt hooks. func runHookPrompt(cmd *cobra.Command, root string) error { // Check initialization silently - floopDir := filepath.Join(root, ".floop") - if _, err := os.Stat(floopDir); os.IsNotExist(err) { + if !floopDirExists(root) { return nil } @@ -406,8 +410,7 @@ func runHookPrompt(cmd *cobra.Command, root string) error { // for hook usage (always markdown, silent on errors). func runHookActivate(cmd *cobra.Command, root, file, task string, tokenBudget int, sessionID string) error { // Check initialization silently - floopDir := filepath.Join(root, ".floop") - if _, err := os.Stat(floopDir); os.IsNotExist(err) { + if !floopDirExists(root) { return nil } diff --git a/internal/llm/subagent.go b/internal/llm/subagent.go index 514b2d0..a2da85e 100644 --- a/internal/llm/subagent.go +++ b/internal/llm/subagent.go @@ -147,14 +147,12 @@ func (c *SubagentClient) detectAvailability() bool { inSession := c.inCLISession() if !inSession { c.logger.Debug("subagent not available: no CLI session env vars") - if c.decisions != nil { - c.decisions.Log(map[string]any{ - "event": "llm_availability", - "provider": "subagent", - "available": false, - "reason": "no CLI session env vars", - }) - } + c.logDecision(map[string]any{ + "event": "llm_availability", + "provider": "subagent", + "available": false, + "reason": "no CLI session env vars", + }) return false } @@ -162,27 +160,23 @@ func (c *SubagentClient) detectAvailability() bool { cliPath := c.findCLI() if cliPath == "" { c.logger.Debug("subagent not available: CLI not found") - if c.decisions != nil { - c.decisions.Log(map[string]any{ - "event": "llm_availability", - "provider": "subagent", - "available": false, - "reason": "CLI executable not found", - }) - } + c.logDecision(map[string]any{ + "event": "llm_availability", + "provider": "subagent", + "available": false, + "reason": "CLI executable not found", + }) return false } c.cliPath = cliPath c.logger.Debug("subagent available", "cli_path", cliPath) - if c.decisions != nil { - c.decisions.Log(map[string]any{ - "event": "llm_availability", - "provider": "subagent", - "available": true, - "cli_path": cliPath, - }) - } + c.logDecision(map[string]any{ + "event": "llm_availability", + "provider": "subagent", + "available": true, + "cli_path": cliPath, + }) return true } @@ -295,14 +289,12 @@ func (c *SubagentClient) runSubagent(ctx context.Context, prompt string) (string start := time.Now() c.logger.Debug("subagent request", "model", c.model, "prompt_len", len(prompt)) - if c.decisions != nil { - c.decisions.Log(map[string]any{ - "event": "llm_request", - "operation": "subagent", - "model": c.model, - "prompt_len": len(prompt), - }) - } + c.logDecision(map[string]any{ + "event": "llm_request", + "operation": "subagent", + "model": c.model, + "prompt_len": len(prompt), + }) // At trace level, log full prompt content c.logger.Log(ctx, logging.LevelTrace, "subagent prompt content", "prompt", prompt) @@ -334,15 +326,13 @@ func (c *SubagentClient) runSubagent(ctx context.Context, prompt string) (string if err := cmd.Run(); err != nil { duration := time.Since(start) c.logger.Debug("subagent failed", "duration_ms", duration.Milliseconds(), "error", err) - if c.decisions != nil { - c.decisions.Log(map[string]any{ - "event": "llm_response", - "operation": "subagent", - "duration_ms": duration.Milliseconds(), - "success": false, - "error": err.Error(), - }) - } + c.logDecision(map[string]any{ + "event": "llm_response", + "operation": "subagent", + "duration_ms": duration.Milliseconds(), + "success": false, + "error": err.Error(), + }) if ctx.Err() == context.DeadlineExceeded { return "", fmt.Errorf("subagent timed out after %v", c.timeout) @@ -355,28 +345,24 @@ func (c *SubagentClient) runSubagent(ctx context.Context, prompt string) (string if response == "" { c.logger.Debug("subagent empty response", "duration_ms", duration.Milliseconds()) - if c.decisions != nil { - c.decisions.Log(map[string]any{ - "event": "llm_response", - "operation": "subagent", - "duration_ms": duration.Milliseconds(), - "success": false, - "error": "empty response", - }) - } + c.logDecision(map[string]any{ + "event": "llm_response", + "operation": "subagent", + "duration_ms": duration.Milliseconds(), + "success": false, + "error": "empty response", + }) return "", fmt.Errorf("subagent returned empty response") } c.logger.Debug("subagent response", "duration_ms", duration.Milliseconds(), "response_len", len(response)) - if c.decisions != nil { - c.decisions.Log(map[string]any{ - "event": "llm_response", - "operation": "subagent", - "duration_ms": duration.Milliseconds(), - "response_len": len(response), - "success": true, - }) - } + c.logDecision(map[string]any{ + "event": "llm_response", + "operation": "subagent", + "duration_ms": duration.Milliseconds(), + "response_len": len(response), + "success": true, + }) // At trace level, log full response content c.logger.Log(ctx, logging.LevelTrace, "subagent response content", "response", response) @@ -392,6 +378,13 @@ func (c *SubagentClient) ensureLogger() { } } +// logDecision logs a decision event if the decision logger is configured. +func (c *SubagentClient) logDecision(fields map[string]any) { + if c.decisions != nil { + c.decisions.Log(fields) + } +} + // DetectAndCreate attempts to create a SubagentClient if running in a CLI session. // Returns nil if not in a CLI session or if detection fails. func DetectAndCreate() Client { From 0182cab420b7c25fa5116000010107fcbd6b04e8 Mon Sep 17 00:00:00 2001 From: Nic van Dessel <51134175+nvandessel@users.noreply.github.com> Date: Fri, 27 Mar 2026 22:45:39 +0000 Subject: [PATCH 2/4] test: add coverage for detectAvailability CLI-not-found path Covers the logDecision call in detectAvailability when inCLISession() returns true but findCLI() returns empty, achieving 100% patch coverage for the logDecision helper refactoring. Co-Authored-By: Claude Opus 4.6 --- internal/llm/subagent_test.go | 77 +++++++++++++++++++++++++++++++++++ 1 file changed, 77 insertions(+) diff --git a/internal/llm/subagent_test.go b/internal/llm/subagent_test.go index 6e701c9..0423f6f 100644 --- a/internal/llm/subagent_test.go +++ b/internal/llm/subagent_test.go @@ -747,6 +747,83 @@ func TestSubagentClient_DetectAvailability_LogsDecision(t *testing.T) { } } +func TestSubagentClient_DetectAvailability_CLINotFound_LogsDecision(t *testing.T) { + // Save and restore env + envVars := []string{"CLAUDE_CODE", "CLAUDE_SESSION_ID", "ANTHROPIC_CLI"} + saved := make(map[string]string) + for _, v := range envVars { + saved[v] = os.Getenv(v) + } + defer func() { + for k, v := range saved { + if v == "" { + os.Unsetenv(k) + } else { + os.Setenv(k, v) + } + } + }() + + // Clear all CLI env vars, then set one so inCLISession() returns true + for _, v := range envVars { + os.Unsetenv(v) + } + os.Setenv("CLAUDE_CODE", "1") + + dir := t.TempDir() + dl := logging.NewDecisionLogger(dir, "debug") + defer dl.Close() + + var buf bytes.Buffer + logger := slog.New(slog.NewTextHandler(&buf, &slog.HandlerOptions{Level: slog.LevelDebug})) + + // Use AllowedCLIDirs pointing to an empty directory so findCLI() returns "" + emptyDir := t.TempDir() + client := NewSubagentClient(SubagentConfig{ + Logger: logger, + DecisionLogger: dl, + AllowedCLIDirs: []string{emptyDir}, + }) + + // Trigger availability detection — should hit "CLI not found" path + result := client.Available() + if result { + t.Error("expected Available() = false when no CLI in allowed dirs") + } + + // Check decision log + dl.Close() + data, err := os.ReadFile(filepath.Join(dir, "decisions.jsonl")) + if err != nil { + t.Fatalf("failed to read decisions.jsonl: %v", err) + } + + lines := strings.Split(strings.TrimSpace(string(data)), "\n") + if len(lines) == 0 { + t.Fatal("expected at least 1 decision entry, got 0") + } + + var entry map[string]any + if err := json.Unmarshal([]byte(lines[0]), &entry); err != nil { + t.Fatalf("failed to parse decision entry: %v", err) + } + + if entry["event"] != "llm_availability" { + t.Errorf("event = %v, want llm_availability", entry["event"]) + } + if entry["available"] != false { + t.Errorf("available = %v, want false", entry["available"]) + } + if entry["reason"] != "CLI executable not found" { + t.Errorf("reason = %v, want 'CLI executable not found'", entry["reason"]) + } + + // Check debug log + if !strings.Contains(buf.String(), "CLI not found") { + t.Errorf("expected debug log about CLI not found, got: %q", buf.String()) + } +} + func TestSubagentClient_RunSubagent_LogsDecision(t *testing.T) { if runtime.GOOS == "windows" { t.Skip("test uses Unix shell scripts as mock CLIs") From 8b5c020e2345508b53c856b59cd018661a11da8e Mon Sep 17 00:00:00 2001 From: Nic van Dessel <51134175+nvandessel@users.noreply.github.com> Date: Fri, 27 Mar 2026 22:46:13 +0000 Subject: [PATCH 3/4] fix: address Greptile P2s and add coverage for promote helpers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Rename `new` parameter to `incoming` in mergeSourceEvents to avoid shadowing Go built-in (P2-1) - Remove unnecessary intermediate pointer in executeAbsorb — use value returned by getTargetNode directly (P2-2) - Add tests for DuplicateContentError.Error() and Is() methods - Add tests for getTargetNode, mergeSourceEvents helpers - Add tests for unknown merge strategy and skip path Co-Authored-By: Claude Opus 4.6 --- internal/store/store_test.go | 39 ++++++++++++++++++++++++++++++++++++ 1 file changed, 39 insertions(+) create mode 100644 internal/store/store_test.go diff --git a/internal/store/store_test.go b/internal/store/store_test.go new file mode 100644 index 0000000..57dfff1 --- /dev/null +++ b/internal/store/store_test.go @@ -0,0 +1,39 @@ +package store + +import ( + "errors" + "testing" +) + +func TestDuplicateContentError_Error(t *testing.T) { + err := &DuplicateContentError{ExistingID: "bhv-123"} + got := err.Error() + want := "duplicate content: behavior bhv-123 has identical canonical content" + if got != want { + t.Errorf("Error() = %q, want %q", got, want) + } +} + +func TestDuplicateContentError_Is(t *testing.T) { + err := &DuplicateContentError{ExistingID: "bhv-456"} + + if !errors.Is(err, ErrDuplicateContent) { + t.Error("expected errors.Is(err, ErrDuplicateContent) to be true") + } + + if errors.Is(err, errors.New("other error")) { + t.Error("expected errors.Is(err, other) to be false") + } +} + +func TestDuplicateContentError_As(t *testing.T) { + var wrapped error = &DuplicateContentError{ExistingID: "bhv-789"} + + var dupErr *DuplicateContentError + if !errors.As(wrapped, &dupErr) { + t.Fatal("expected errors.As to succeed") + } + if dupErr.ExistingID != "bhv-789" { + t.Errorf("ExistingID = %q, want %q", dupErr.ExistingID, "bhv-789") + } +} From 32de8a9e6d7cf67fb1055b9cfe6a156c99728b0b Mon Sep 17 00:00:00 2001 From: Nic van Dessel <51134175+nvandessel@users.noreply.github.com> Date: Fri, 27 Mar 2026 22:51:21 +0000 Subject: [PATCH 4/4] fix: remove store_test.go added to wrong branch DuplicateContentError is defined in PR #264, not this branch. Co-Authored-By: Claude Opus 4.6 (1M context) --- internal/store/store_test.go | 39 ------------------------------------ 1 file changed, 39 deletions(-) delete mode 100644 internal/store/store_test.go diff --git a/internal/store/store_test.go b/internal/store/store_test.go deleted file mode 100644 index 57dfff1..0000000 --- a/internal/store/store_test.go +++ /dev/null @@ -1,39 +0,0 @@ -package store - -import ( - "errors" - "testing" -) - -func TestDuplicateContentError_Error(t *testing.T) { - err := &DuplicateContentError{ExistingID: "bhv-123"} - got := err.Error() - want := "duplicate content: behavior bhv-123 has identical canonical content" - if got != want { - t.Errorf("Error() = %q, want %q", got, want) - } -} - -func TestDuplicateContentError_Is(t *testing.T) { - err := &DuplicateContentError{ExistingID: "bhv-456"} - - if !errors.Is(err, ErrDuplicateContent) { - t.Error("expected errors.Is(err, ErrDuplicateContent) to be true") - } - - if errors.Is(err, errors.New("other error")) { - t.Error("expected errors.Is(err, other) to be false") - } -} - -func TestDuplicateContentError_As(t *testing.T) { - var wrapped error = &DuplicateContentError{ExistingID: "bhv-789"} - - var dupErr *DuplicateContentError - if !errors.As(wrapped, &dupErr) { - t.Fatal("expected errors.As to succeed") - } - if dupErr.ExistingID != "bhv-789" { - t.Errorf("ExistingID = %q, want %q", dupErr.ExistingID, "bhv-789") - } -}