Extract shared globmatch package from toolpolicy and runtime#79
Conversation
Create pkg/shared/globmatch with Pattern, Compile, CompileAll, MatchesAny, and MakePredicate. Both pkg/agents/toolpolicy and pkg/runtime now use the shared package instead of their own compiledPattern implementations. ~121 lines removed. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe changes consolidate duplicated glob pattern-matching logic across files into a new shared Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pkg/shared/globmatch/globmatch.go (1)
57-68: Consider documenting thatMatchesexpects pre-normalized input.The
Matchesmethod compares againstp.valuewhich is already lowercased duringCompile. Callers usingMatchesorMatchesAnydirectly (not viaMakePredicate) must pass lowercased, trimmed names for correct matching. This is implicit fromMakePredicate's behavior but could be clarified in the docstring.📝 Suggested docstring clarification
-// Matches reports whether the pattern matches the given name. +// Matches reports whether the pattern matches the given name. +// The name should be pre-normalized (lowercased and trimmed) for correct matching, +// as patterns are normalized during Compile. func (p Pattern) Matches(name string) bool {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/shared/globmatch/globmatch.go` around lines 57 - 68, Update the docstring for Pattern.Matches to explicitly state that the method expects the input name to be pre-normalized (lowercased and trimmed) because p.value is lowercased during Compile; also add the same clarification to MatchesAny and any public helpers like MakePredicate so callers know MakePredicate performs normalization but calling Matches/MatchesAny directly requires normalized input.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/runtime/pruning.go`:
- Around line 3-11: Reorder the imports in pkg/runtime/pruning.go so they follow
standard Go grouping: standard library packages first (fmt, slices, strings,
time), then external modules (github.com/openai/openai-go/v3), and finally
internal project packages (github.com/beeper/agentremote/pkg/shared/globmatch);
update the import block accordingly and run goimports/go fmt to ensure the hook
no longer modifies the file.
---
Nitpick comments:
In `@pkg/shared/globmatch/globmatch.go`:
- Around line 57-68: Update the docstring for Pattern.Matches to explicitly
state that the method expects the input name to be pre-normalized (lowercased
and trimmed) because p.value is lowercased during Compile; also add the same
clarification to MatchesAny and any public helpers like MakePredicate so callers
know MakePredicate performs normalization but calling Matches/MatchesAny
directly requires normalized input.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f0ba41a6-a7a6-4341-95c9-4b3303e7cb3b
📒 Files selected for processing (3)
pkg/agents/toolpolicy/policy.gopkg/runtime/pruning.gopkg/shared/globmatch/globmatch.go
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: build-agentremote-docker (amd64)
- GitHub Check: build-agentremote-docker (arm64)
- GitHub Check: Lint
- GitHub Check: build-docker
🧰 Additional context used
🪛 GitHub Actions: Go
pkg/runtime/pruning.go
[error] 1-1: pre-commit hook 'go-imports-repo' failed (exit code 1). Hook modified files to fix Go import ordering/formatting (moved 'github.com/beeper/agentremote/pkg/shared/globmatch' import).
🔇 Additional comments (8)
pkg/shared/globmatch/globmatch.go (4)
1-15: LGTM!Clean package structure with a well-documented
Patterntype. The three-kind approach (all, exact, regex) is a reasonable design for glob matching.
21-39: LGTM!The
Compilefunction correctly handles all pattern types. The use ofregexp.QuoteMetafollowed by replacing\*with.*is the right approach for converting glob wildcards to regex while escaping other special characters.
41-55: LGTM!The
CompileAllfunction correctly filters out empty/whitespace patterns, preventing them from affecting match results.
80-91: LGTM!The
MakePredicatefunction correctly implements deny-wins semantics with proper normalization. The "empty allow = allow all" behavior is well-documented and matches the expected semantics.pkg/runtime/pruning.go (1)
126-134: LGTM!Clean refactor that delegates to the shared
globmatchpackage while preserving the existing function signature and semantics. The nil config check is retained for safety.pkg/agents/toolpolicy/policy.go (3)
3-9: LGTM!Import ordering looks correct with internal packages grouped together after standard library imports.
386-389: LGTM!Clean composition:
ExpandToolGroupshandles group expansion and normalization, thenglobmatch.CompileAllcompiles the resulting patterns.
391-393: LGTM!The function correctly delegates to
globmatch.MakePredicate. The nil policy case is handled by the caller (FilterToolsByPolicyreturns early on line 397-398), so no defensive check is needed here.
| import ( | ||
| "fmt" | ||
| "regexp" | ||
| "slices" | ||
| "strings" | ||
| "time" | ||
|
|
||
| "github.com/beeper/agentremote/pkg/shared/globmatch" | ||
| "github.com/openai/openai-go/v3" | ||
| ) |
There was a problem hiding this comment.
Fix import ordering to pass pre-commit hook.
The pipeline failure indicates the go-imports-repo hook modified files due to incorrect import ordering. The globmatch import (internal package) should be grouped separately from or ordered correctly relative to external packages.
🔧 Suggested fix for import ordering
import (
"fmt"
"slices"
"strings"
"time"
- "github.com/beeper/agentremote/pkg/shared/globmatch"
"github.com/openai/openai-go/v3"
+
+ "github.com/beeper/agentremote/pkg/shared/globmatch"
)Note: The exact ordering depends on your project's import grouping conventions. Typically Go imports are grouped as: standard library, then external packages, then internal packages.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| import ( | |
| "fmt" | |
| "regexp" | |
| "slices" | |
| "strings" | |
| "time" | |
| "github.com/beeper/agentremote/pkg/shared/globmatch" | |
| "github.com/openai/openai-go/v3" | |
| ) | |
| import ( | |
| "fmt" | |
| "slices" | |
| "strings" | |
| "time" | |
| "github.com/openai/openai-go/v3" | |
| "github.com/beeper/agentremote/pkg/shared/globmatch" | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/runtime/pruning.go` around lines 3 - 11, Reorder the imports in
pkg/runtime/pruning.go so they follow standard Go grouping: standard library
packages first (fmt, slices, strings, time), then external modules
(github.com/openai/openai-go/v3), and finally internal project packages
(github.com/beeper/agentremote/pkg/shared/globmatch); update the import block
accordingly and run goimports/go fmt to ensure the hook no longer modifies the
file.
1 participant
Summary
Test plan
go build ./...passesgo test ./...passesgo vet ./...passes🤖 Generated with Claude Code