Skip to content

refactor: extract floopDirExists and logDecision helpers#263

Merged
nvandessel merged 5 commits intomainfrom
refactor/hook-and-subagent-helpers
Mar 27, 2026
Merged

refactor: extract floopDirExists and logDecision helpers#263
nvandessel merged 5 commits intomainfrom
refactor/hook-and-subagent-helpers

Conversation

@nvandessel
Copy link
Copy Markdown
Owner

Summary

  • Extract floopDirExists(root string) bool helper in cmd/floop/cmd_hook.go to replace 4 repeated filepath.Join + os.Stat + os.IsNotExist checks
  • Add logDecision(fields map[string]any) method on SubagentClient in internal/llm/subagent.go to encapsulate the nil-check on the decisions logger, replacing 7 occurrences

Test plan

  • go vet ./cmd/floop/... ./internal/llm/... — clean
  • go test ./internal/llm/... — all pass
  • go test ./... — only pre-existing build failures (CGO lancedb linker, consolidation promote.go)
  • go fmt ./... — clean

🤖 Generated with Claude Code

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 <noreply@anthropic.com>
@greptile-apps
Copy link
Copy Markdown

greptile-apps bot commented Mar 27, 2026

Greptile Summary

This PR is a clean refactoring that eliminates two categories of repeated boilerplate: a floopDirExists(root string) bool helper in cmd/floop/cmd_hook.go that consolidates 4 identical filepath.Join + os.Stat + os.IsNotExist checks, and a logDecision(fields map[string]any) method on SubagentClient in internal/llm/subagent.go that wraps the 7 repeated if c.decisions != nil { c.decisions.Log(...) } nil-guards. A new test covers the previously-untested "CLI not found" decision-logging path.

  • Semantics fully preserved: floopDirExists returns true for both a successful stat (err == nil) and any non-IsNotExist error (e.g. permission denied), exactly mirroring the original if os.IsNotExist(err) { return } guards.
  • logDecision is nil-safe: the nil-check on c.decisions is retained inside the helper, so all callers are unaffected whether or not a DecisionLogger is configured.
  • No behavioural change: all call sites in runHookPrompt, runHookActivate, hookLog, and runDetectCorrection behave identically before and after.
  • New test TestSubagentClient_DetectAvailability_CLINotFound_LogsDecision follows the established pattern and correctly validates the \"CLI executable not found\" decision entry.

Confidence Score: 5/5

Safe to merge — pure refactoring with no behavioural changes and full test coverage of the new helper paths.

All changes are mechanical de-duplication with semantics verified to be identical to the original code. No new logic, no changed error handling, no new dependencies.

No files require special attention.

Important Files Changed

Filename Overview
cmd/floop/cmd_hook.go Extracts floopDirExists helper replacing 4 identical os.Stat/os.IsNotExist patterns; semantics fully preserved
internal/llm/subagent.go Adds logDecision method encapsulating the nil-guard on c.decisions, replacing 7 identical blocks cleanly
internal/llm/subagent_test.go Adds test covering the CLI not found decision-log path; follows the established test pattern in the file

Reviews (4): Last reviewed commit: "Merge branch 'main' into refactor/hook-a..." | Re-trigger Greptile

@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Mar 27, 2026

Codecov Report

❌ Patch coverage is 89.47368% with 6 lines in your changes missing coverage. Please review.
✅ Project coverage is 78.68%. Comparing base (33dd3e3) to head (0ec941a).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
internal/llm/subagent.go 87.50% 6 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #263      +/-   ##
==========================================
+ Coverage   78.58%   78.68%   +0.09%     
==========================================
  Files         185      185              
  Lines       18706    18702       -4     
==========================================
+ Hits        14700    14715      +15     
+ Misses       2773     2755      -18     
+ Partials     1233     1232       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

nvandessel and others added 3 commits March 27, 2026 22:45
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 <noreply@anthropic.com>
- 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 <noreply@anthropic.com>
DuplicateContentError is defined in PR #264, not this branch.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@nvandessel nvandessel enabled auto-merge (squash) March 27, 2026 23:38
@nvandessel nvandessel merged commit 686b17f into main Mar 27, 2026
18 checks passed
@nvandessel nvandessel deleted the refactor/hook-and-subagent-helpers branch March 27, 2026 23:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants