Skip to content

Commit d6b0bf0

Browse files
CopilotCopilot
authored
refactor: semantic function clustering improvements
- Move sysListServersHandler from session.go to system_tools.go - Merge strutil/session_suffix.go + json_clone.go into strutil.go - Export config.ValidateAndNormalizeIntegrityField, remove duplicate guard validation - Add doc.go note about intentional per-logger factory pattern
1 parent c798f73 commit d6b0bf0

12 files changed

Lines changed: 88 additions & 85 deletions

‎internal/config/guard_policy_test.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1421,7 +1421,7 @@ func TestValidateAndNormalizeIntegrityField(t *testing.T) {
14211421

14221422
for _, tt := range tests {
14231423
t.Run(tt.name, func(t *testing.T) {
1424-
got, err := validateAndNormalizeIntegrityField(tt.fieldPath, tt.raw, tt.optional)
1424+
got, err := ValidateAndNormalizeIntegrityField(tt.fieldPath, tt.raw, tt.optional)
14251425
if tt.wantErrContains != "" {
14261426
require.Error(t, err)
14271427
assert.ErrorContains(t, err, tt.wantErrContains)

‎internal/config/guard_policy_validation.go‎

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -98,7 +98,7 @@ func NormalizeGuardPolicy(policy *GuardPolicy) (*NormalizedGuardPolicy, error) {
9898
return nil, fmt.Errorf("policy must include allow-only")
9999
}
100100

101-
integrity, err := validateAndNormalizeIntegrityField("allow-only.min-integrity", policy.AllowOnly.MinIntegrity, false)
101+
integrity, err := ValidateAndNormalizeIntegrityField("allow-only.min-integrity", policy.AllowOnly.MinIntegrity, false)
102102
if err != nil {
103103
return nil, err
104104
}
@@ -144,14 +144,14 @@ func NormalizeGuardPolicy(policy *GuardPolicy) (*NormalizedGuardPolicy, error) {
144144

145145
// Validate and normalize disapproval-integrity (optional; empty means feature
146146
// uses Rust-side default of "none" when endorsement/disapproval is evaluated).
147-
normalized.DisapprovalIntegrity, err = validateAndNormalizeIntegrityField("allow-only.disapproval-integrity", policy.AllowOnly.DisapprovalIntegrity, true)
147+
normalized.DisapprovalIntegrity, err = ValidateAndNormalizeIntegrityField("allow-only.disapproval-integrity", policy.AllowOnly.DisapprovalIntegrity, true)
148148
if err != nil {
149149
return nil, err
150150
}
151151

152152
// Validate and normalize endorser-min-integrity (optional; empty means feature
153153
// uses Rust-side default of "approved" when evaluating reactor eligibility).
154-
normalized.EndorserMinIntegrity, err = validateAndNormalizeIntegrityField("allow-only.endorser-min-integrity", policy.AllowOnly.EndorserMinIntegrity, true)
154+
normalized.EndorserMinIntegrity, err = ValidateAndNormalizeIntegrityField("allow-only.endorser-min-integrity", policy.AllowOnly.EndorserMinIntegrity, true)
155155
if err != nil {
156156
return nil, err
157157
}
@@ -204,7 +204,9 @@ func NormalizeGuardPolicy(policy *GuardPolicy) (*NormalizedGuardPolicy, error) {
204204
}
205205
}
206206

207-
func validateAndNormalizeIntegrityField(fieldPath, raw string, optional bool) (string, error) {
207+
// ValidateAndNormalizeIntegrityField validates and normalizes a named integrity-level field.
208+
// It wraps NormalizeIntegrityLevel and prefixes the field path in any error message.
209+
func ValidateAndNormalizeIntegrityField(fieldPath, raw string, optional bool) (string, error) {
208210
v, err := NormalizeIntegrityLevel(raw, optional)
209211
if err != nil {
210212
return "", fmt.Errorf("%s %w", fieldPath, err)

‎internal/guard/guard_test.go‎

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -858,7 +858,7 @@ func TestBuildStrictLabelAgentPayload(t *testing.T) {
858858

859859
_, err := buildStrictLabelAgentPayload(input)
860860
require.Error(t, err)
861-
assert.Equal(t, "invalid integrity value: expected one of none|unapproved|approved|merged", err.Error())
861+
assert.Equal(t, "integrity must be one of: none, unapproved, approved, merged", err.Error())
862862
})
863863
}
864864

@@ -976,7 +976,7 @@ func TestBuildStrictLabelAgentPayloadExtendedGuard(t *testing.T) {
976976
}
977977
_, err := buildStrictLabelAgentPayload(input)
978978
require.Error(t, err)
979-
assert.ErrorContains(t, err, "invalid integrity value")
979+
assert.ErrorContains(t, err, "integrity must be one of")
980980
})
981981

982982
t.Run("blocked-users validation - not an array", func(t *testing.T) {
@@ -1144,15 +1144,15 @@ func TestBuildStrictLabelAgentPayloadExtendedGuard(t *testing.T) {
11441144
input["allow-only"].(map[string]interface{})["disapproval-integrity"] = 99
11451145
_, err := buildStrictLabelAgentPayload(input)
11461146
require.Error(t, err)
1147-
assert.ErrorContains(t, err, "invalid disapproval-integrity value: expected one of none|unapproved|approved|merged")
1147+
assert.ErrorContains(t, err, "disapproval-integrity must be one of")
11481148
})
11491149

11501150
t.Run("disapproval-integrity validation - invalid string value", func(t *testing.T) {
11511151
input := validBase()
11521152
input["allow-only"].(map[string]interface{})["disapproval-integrity"] = "high"
11531153
_, err := buildStrictLabelAgentPayload(input)
11541154
require.Error(t, err)
1155-
assert.ErrorContains(t, err, "invalid disapproval-integrity value: expected one of none|unapproved|approved|merged")
1155+
assert.ErrorContains(t, err, "disapproval-integrity must be one of")
11561156
})
11571157

11581158
t.Run("disapproval-integrity validation - valid", func(t *testing.T) {
@@ -1168,15 +1168,15 @@ func TestBuildStrictLabelAgentPayloadExtendedGuard(t *testing.T) {
11681168
input["allow-only"].(map[string]interface{})["endorser-min-integrity"] = true
11691169
_, err := buildStrictLabelAgentPayload(input)
11701170
require.Error(t, err)
1171-
assert.ErrorContains(t, err, "invalid endorser-min-integrity value: expected one of none|unapproved|approved|merged")
1171+
assert.ErrorContains(t, err, "endorser-min-integrity must be one of")
11721172
})
11731173

11741174
t.Run("endorser-min-integrity validation - invalid string value", func(t *testing.T) {
11751175
input := validBase()
11761176
input["allow-only"].(map[string]interface{})["endorser-min-integrity"] = "critical"
11771177
_, err := buildStrictLabelAgentPayload(input)
11781178
require.Error(t, err)
1179-
assert.ErrorContains(t, err, "invalid endorser-min-integrity value: expected one of none|unapproved|approved|merged")
1179+
assert.ErrorContains(t, err, "endorser-min-integrity must be one of")
11801180
})
11811181

11821182
t.Run("endorser-min-integrity validation - valid", func(t *testing.T) {

‎internal/guard/wasm_test.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -513,7 +513,7 @@ func TestBuildStrictLabelAgentPayloadExtended(t *testing.T) {
513513

514514
_, err := buildStrictLabelAgentPayload(policy)
515515
require.Error(t, err)
516-
assert.ErrorContains(t, err, "invalid min-integrity value")
516+
assert.ErrorContains(t, err, "min-integrity must be one of")
517517
})
518518

519519
t.Run("valid allow-only policy succeeds", func(t *testing.T) {

‎internal/guard/wasm_validate.go‎

Lines changed: 4 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -10,25 +10,16 @@ import (
1010
// AllowedIntegrityLevels is derived from the canonical integrity levels in config.
1111
var AllowedIntegrityLevels = config.AllIntegrityLevels()
1212

13-
func invalidIntegrityFieldError(fieldName string) error {
14-
return fmt.Errorf(
15-
"invalid %s value: expected one of %s",
16-
fieldName,
17-
strings.Join(AllowedIntegrityLevels, "|"),
18-
)
19-
}
20-
2113
// validateIntegrityField returns an error if raw is not a valid integrity-level
2214
// string. fieldName is used in the error message (e.g. "disapproval-integrity").
15+
// It delegates to config.ValidateAndNormalizeIntegrityField for validation.
2316
func validateIntegrityField(fieldName string, raw interface{}) error {
2417
s, ok := raw.(string)
2518
if !ok {
26-
return invalidIntegrityFieldError(fieldName)
27-
}
28-
if _, err := config.NormalizeIntegrityLevel(s, false); err == nil {
29-
return nil
19+
s = ""
3020
}
31-
return invalidIntegrityFieldError(fieldName)
21+
_, err := config.ValidateAndNormalizeIntegrityField(fieldName, s, false)
22+
return err
3223
}
3324

3425
// checkBoolFailure returns a non-nil error if the given raw response map

‎internal/guard/wasm_validate_test.go‎

Lines changed: 8 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -211,50 +211,50 @@ func TestValidateIntegrityField(t *testing.T) {
211211
fieldName: "disapproval-integrity",
212212
raw: 42,
213213
wantErr: true,
214-
wantErrContains: "invalid disapproval-integrity value",
214+
wantErrContains: "disapproval-integrity must be one of",
215215
},
216216
{
217217
name: "bool returns error",
218218
fieldName: "endorser-min-integrity",
219219
raw: true,
220220
wantErr: true,
221-
wantErrContains: "invalid endorser-min-integrity value",
221+
wantErrContains: "endorser-min-integrity must be one of",
222222
},
223223
{
224224
name: "nil returns error",
225225
fieldName: "min-integrity",
226226
raw: nil,
227227
wantErr: true,
228-
wantErrContains: "invalid min-integrity value",
228+
wantErrContains: "min-integrity must be one of",
229229
},
230230
{
231231
name: "slice returns error",
232232
fieldName: "disapproval-integrity",
233233
raw: []string{"none"},
234234
wantErr: true,
235-
wantErrContains: "invalid disapproval-integrity value",
235+
wantErrContains: "disapproval-integrity must be one of",
236236
},
237237
// Invalid string value
238238
{
239239
name: "unknown integrity level returns error",
240240
fieldName: "disapproval-integrity",
241241
raw: "invalid",
242242
wantErr: true,
243-
wantErrContains: "invalid disapproval-integrity value",
243+
wantErrContains: "disapproval-integrity must be one of",
244244
},
245245
{
246246
name: "empty string returns error",
247247
fieldName: "endorser-min-integrity",
248248
raw: "",
249249
wantErr: true,
250-
wantErrContains: "invalid endorser-min-integrity value",
250+
wantErrContains: "endorser-min-integrity must be one of",
251251
},
252252
{
253253
name: "whitespace-only string returns error",
254254
fieldName: "min-integrity",
255255
raw: " ",
256256
wantErr: true,
257-
wantErrContains: "invalid min-integrity value",
257+
wantErrContains: "min-integrity must be one of",
258258
},
259259
// Valid integrity levels
260260
{
@@ -306,7 +306,7 @@ func TestValidateIntegrityField(t *testing.T) {
306306
fieldName: "disapproval-integrity",
307307
raw: "bad",
308308
wantErr: true,
309-
wantErrContains: "none|unapproved|approved|merged",
309+
wantErrContains: "must be one of",
310310
},
311311
}
312312

@@ -324,10 +324,3 @@ func TestValidateIntegrityField(t *testing.T) {
324324
})
325325
}
326326
}
327-
328-
func TestInvalidIntegrityFieldError(t *testing.T) {
329-
err := invalidIntegrityFieldError("test-field")
330-
require.Error(t, err)
331-
assert.ErrorContains(t, err, "test-field")
332-
assert.ErrorContains(t, err, "none|unapproved|approved|merged")
333-
}

‎internal/logger/doc.go‎

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,4 +8,20 @@
88
//
99
// These APIs target different sinks and can be used together when a message should
1010
// appear in multiple outputs.
11+
//
12+
// # Per-type setup and error-handler functions
13+
//
14+
// Each logger type has its own setup* and handleError* functions (e.g.
15+
// setupFileLogger / handleFileLoggerError, setupMarkdownLogger /
16+
// handleMarkdownLoggerError). These are intentionally not collapsed into a
17+
// single generic helper because each type has unique initialization behaviour:
18+
//
19+
// - JSONLLogger has no fallback path (a failure returns an error directly).
20+
// - ToolsLogger writes a one-time header and closes the file immediately after
21+
// opening, so its setup function owns that lifecycle step.
22+
// - FileLogger, MarkdownLogger, and RPCLogger each open a persistent file and
23+
// wire up different formatters or rotate strategies.
24+
//
25+
// The per-type functions are bundled via the loggerFactory[T] generic defined in
26+
// common.go, which handles the shared open-file / call-setup / call-onError flow.
1127
package logger

‎internal/server/session.go‎

Lines changed: 0 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -165,18 +165,6 @@ func (us *UnifiedServer) sysInitHandler(ctx context.Context, req *sdk.CallToolRe
165165
return us.callAndLogSysTool(sessionID, "session initialization", "sys_init")
166166
}
167167

168-
func (us *UnifiedServer) sysListServersHandler(ctx context.Context, _ *sdk.CallToolRequest, _ interface{}) (*sdk.CallToolResult, interface{}, error) {
169-
sessionID := us.getSessionID(ctx)
170-
logger.LogInfo("client", "MCP sys_list_servers request, session=%s", truncateSessionID(sessionID))
171-
172-
if err := us.requireSession(ctx); err != nil {
173-
logger.LogError("client", "MCP sys_list_servers failed: session not initialized, session=%s", sessionID)
174-
return mcp.NewErrorCallToolResult(err)
175-
}
176-
177-
return us.callAndLogSysTool(truncateSessionID(sessionID), "sys_list_servers", "sys_list_servers")
178-
}
179-
180168
// getSessionKeys returns a list of active session IDs for debugging
181169
func (us *UnifiedServer) getSessionKeys() []string {
182170
us.sessionMu.RLock()

‎internal/server/system_tools.go‎

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,12 @@
11
package server
22

33
import (
4+
"context"
45
"fmt"
56

67
"github.com/github/gh-aw-mcpg/internal/logger"
78
"github.com/github/gh-aw-mcpg/internal/mcp"
9+
sdk "github.com/modelcontextprotocol/go-sdk/mcp"
810
)
911

1012
var logSys = logger.New("server:system_tools")
@@ -40,3 +42,17 @@ func (s *SysServer) ListServers() (interface{}, error) {
4042

4143
return mcp.BuildMCPTextResponse(fmt.Sprintf("Configured MCP Servers:\n%s", serverList)), nil
4244
}
45+
46+
// sysListServersHandler handles sys___list_servers tool calls.
47+
// It validates that a session exists and delegates to callAndLogSysTool.
48+
func (us *UnifiedServer) sysListServersHandler(ctx context.Context, _ *sdk.CallToolRequest, _ interface{}) (*sdk.CallToolResult, interface{}, error) {
49+
sessionID := us.getSessionID(ctx)
50+
logger.LogInfo("client", "MCP sys_list_servers request, session=%s", truncateSessionID(sessionID))
51+
52+
if err := us.requireSession(ctx); err != nil {
53+
logger.LogError("client", "MCP sys_list_servers failed: session not initialized, session=%s", sessionID)
54+
return mcp.NewErrorCallToolResult(err)
55+
}
56+
57+
return us.callAndLogSysTool(truncateSessionID(sessionID), "sys_list_servers", "sys_list_servers")
58+
}

‎internal/strutil/json_clone.go‎

Lines changed: 0 additions & 21 deletions
This file was deleted.

0 commit comments

Comments
 (0)