Skip to content

Commit 87b13c9

Browse files
committed
add review-notifications enable and poll-interval flags
Review notifications are gated by a dedicated flag family, deliberately not a capability group: the feature is client-side mechanics, not a Gerrit operation class. --review-notifications (default off) and --review-notifications-poll-interval (default 60s) follow the established flag-over-mirror-over-default precedence with GERRIT_MCP_REVIEW_NOTIFICATIONS* mirrors. The interval is resolved and validated even when the feature is off, so a typo fails loudly at startup instead of surfacing on a later enable. Unparsable and non-positive intervals join the aggregated startup error naming the flag and value. Refs: #50
1 parent f9e5521 commit 87b13c9

5 files changed

Lines changed: 237 additions & 38 deletions

File tree

internal/config/config.go

Lines changed: 79 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@ import (
1212
"io"
1313
"strconv"
1414
"strings"
15+
"time"
1516

1617
"dev.gaijin.team/go/golib/e"
1718
"dev.gaijin.team/go/golib/fields"
@@ -31,6 +32,8 @@ const (
3132

3233
const defaultGroups = "read"
3334

35+
const defaultPollInterval = "60s"
36+
3437
// Env variable names for connection identity and credentials.
3538
const (
3639
EnvURL = "GERRIT_URL"
@@ -39,11 +42,13 @@ const (
3942
)
4043

4144
var (
42-
errEnvMissing = e.New("required environment variable is not set")
43-
errUnknownGroups = e.New("unknown capability groups")
44-
errNoGroups = e.New("no capability groups enabled")
45-
errParseFlags = e.New("parse flags")
46-
errInvalidBool = e.New("invalid boolean flag value")
45+
errEnvMissing = e.New("required environment variable is not set")
46+
errUnknownGroups = e.New("unknown capability groups")
47+
errNoGroups = e.New("no capability groups enabled")
48+
errParseFlags = e.New("parse flags")
49+
errInvalidBool = e.New("invalid boolean flag value")
50+
errInvalidDuration = e.New("invalid duration flag value")
51+
errIntervalNotPositive = e.New("poll interval must be positive")
4752
)
4853

4954
// Config is the resolved server configuration.
@@ -68,6 +73,14 @@ type Config struct {
6873
// (the default), trail-leaving operations are refused on changes not
6974
// owned by the authenticated account.
7075
AllowForeignChanges bool
76+
// ReviewNotifications enables the review-notifications feature: polling
77+
// Gerrit for activity on subscribed changes and pushing it into the
78+
// agent's session. Off by default.
79+
ReviewNotifications bool
80+
// ReviewNotificationsPollInterval is the cadence of the review
81+
// notifications poller. Always resolved and validated, even when the
82+
// feature is disabled.
83+
ReviewNotificationsPollInterval time.Duration
7184
}
7285

7386
// behaviorFlag is one CLI flag with its GERRIT_MCP_* env mirror. The flag
@@ -115,21 +128,37 @@ func Load(args []string, getenv func(string) string) (*Config, error) {
115128
usage: "refuse trail-leaving operations on changes not owned by the authenticated account",
116129
fallback: "true",
117130
}
131+
reviewNotifications := behaviorFlag{
132+
name: "review-notifications",
133+
mirror: "GERRIT_MCP_REVIEW_NOTIFICATIONS",
134+
usage: "poll Gerrit for activity on subscribed changes and push it into the agent's session",
135+
fallback: "false",
136+
}
137+
pollInterval := behaviorFlag{
138+
name: "review-notifications-poll-interval",
139+
mirror: "GERRIT_MCP_REVIEW_NOTIFICATIONS_POLL_INTERVAL",
140+
usage: "cadence of the review notifications poller, as a Go duration (e.g. 60s)",
141+
fallback: defaultPollInterval,
142+
}
118143

119-
err := resolveFlags(args, getenv, []*behaviorFlag{&groups, &includeTools, &excludeTools, &projects, &ownChanges})
144+
err := resolveFlags(args, getenv, []*behaviorFlag{
145+
&groups, &includeTools, &excludeTools, &projects, &ownChanges, &reviewNotifications, &pollInterval,
146+
})
120147
if err != nil {
121148
return nil, err
122149
}
123150

124151
cfg := &Config{
125-
GerritURL: getenv(EnvURL),
126-
Username: getenv(EnvUsername),
127-
Token: getenv(EnvToken),
128-
Groups: nil,
129-
IncludeTools: splitList(includeTools.value),
130-
ExcludeTools: splitList(excludeTools.value),
131-
Projects: splitList(projects.value),
132-
AllowForeignChanges: false,
152+
GerritURL: getenv(EnvURL),
153+
Username: getenv(EnvUsername),
154+
Token: getenv(EnvToken),
155+
Groups: nil,
156+
IncludeTools: splitList(includeTools.value),
157+
ExcludeTools: splitList(excludeTools.value),
158+
Projects: splitList(projects.value),
159+
AllowForeignChanges: false,
160+
ReviewNotifications: false,
161+
ReviewNotificationsPollInterval: 0,
133162
}
134163

135164
errs := missingEnv(cfg)
@@ -141,6 +170,20 @@ func Load(args []string, getenv func(string) string) (*Config, error) {
141170
cfg.AllowForeignChanges = !ownOnly
142171
}
143172

173+
notifications, err := parseBool(reviewNotifications)
174+
if err != nil {
175+
errs = append(errs, err)
176+
} else {
177+
cfg.ReviewNotifications = notifications
178+
}
179+
180+
interval, err := parsePollInterval(pollInterval)
181+
if err != nil {
182+
errs = append(errs, err)
183+
} else {
184+
cfg.ReviewNotificationsPollInterval = interval
185+
}
186+
144187
parsed, err := parseGroups(groups.value)
145188
if err != nil {
146189
errs = append(errs, err)
@@ -217,6 +260,28 @@ func parseBool(bf behaviorFlag) (bool, error) {
217260
return v, nil
218261
}
219262

263+
// parsePollInterval parses a resolved duration flag value and rejects
264+
// non-positive intervals, naming the flag and the offending value in the
265+
// error.
266+
func parsePollInterval(bf behaviorFlag) (time.Duration, error) {
267+
d, err := time.ParseDuration(strings.TrimSpace(bf.value))
268+
if err != nil {
269+
return 0, errInvalidDuration.WithFields(
270+
fields.F("flag", bf.name),
271+
fields.F("value", bf.value),
272+
)
273+
}
274+
275+
if d <= 0 {
276+
return 0, errIntervalNotPositive.WithFields(
277+
fields.F("flag", bf.name),
278+
fields.F("value", bf.value),
279+
)
280+
}
281+
282+
return d, nil
283+
}
284+
220285
// splitList splits a comma-separated list, trimming whitespace and dropping
221286
// empty entries. An empty input yields nil.
222287
func splitList(s string) []string {

internal/config/config_test.go

Lines changed: 128 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@ package config_test
22

33
import (
44
"testing"
5+
"time"
56

67
"github.com/stretchr/testify/assert"
78
"github.com/stretchr/testify/require"
@@ -247,6 +248,133 @@ func Test_Load_OwnChanges(t *testing.T) {
247248
})
248249
}
249250

251+
func Test_Load_ReviewNotifications(t *testing.T) {
252+
t.Parallel()
253+
254+
t.Run("zero config leaves the feature disabled with the default interval", func(t *testing.T) {
255+
t.Parallel()
256+
257+
cfg, err := config.Load(nil, env(secrets()))
258+
require.NoError(t, err)
259+
assert.False(t, cfg.ReviewNotifications)
260+
assert.Equal(t, 60*time.Second, cfg.ReviewNotificationsPollInterval)
261+
})
262+
263+
t.Run("flag enables", func(t *testing.T) {
264+
t.Parallel()
265+
266+
cfg, err := config.Load([]string{"--review-notifications", "true"}, env(secrets()))
267+
require.NoError(t, err)
268+
assert.True(t, cfg.ReviewNotifications)
269+
})
270+
271+
t.Run("env mirror enables", func(t *testing.T) {
272+
t.Parallel()
273+
274+
m := secrets()
275+
276+
m["GERRIT_MCP_REVIEW_NOTIFICATIONS"] = "true"
277+
278+
cfg, err := config.Load(nil, env(m))
279+
require.NoError(t, err)
280+
assert.True(t, cfg.ReviewNotifications)
281+
})
282+
283+
t.Run("enable flag wins over env mirror", func(t *testing.T) {
284+
t.Parallel()
285+
286+
m := secrets()
287+
288+
m["GERRIT_MCP_REVIEW_NOTIFICATIONS"] = "true"
289+
290+
cfg, err := config.Load([]string{"--review-notifications=false"}, env(m))
291+
require.NoError(t, err)
292+
assert.False(t, cfg.ReviewNotifications)
293+
})
294+
295+
t.Run("interval resolves from flag", func(t *testing.T) {
296+
t.Parallel()
297+
298+
cfg, err := config.Load([]string{"--review-notifications-poll-interval", "2m"}, env(secrets()))
299+
require.NoError(t, err)
300+
assert.Equal(t, 2*time.Minute, cfg.ReviewNotificationsPollInterval)
301+
})
302+
303+
t.Run("interval resolves from env mirror", func(t *testing.T) {
304+
t.Parallel()
305+
306+
m := secrets()
307+
308+
m["GERRIT_MCP_REVIEW_NOTIFICATIONS_POLL_INTERVAL"] = "30s"
309+
310+
cfg, err := config.Load(nil, env(m))
311+
require.NoError(t, err)
312+
assert.Equal(t, 30*time.Second, cfg.ReviewNotificationsPollInterval)
313+
})
314+
315+
t.Run("interval flag wins over env mirror", func(t *testing.T) {
316+
t.Parallel()
317+
318+
m := secrets()
319+
320+
m["GERRIT_MCP_REVIEW_NOTIFICATIONS_POLL_INTERVAL"] = "30s"
321+
322+
cfg, err := config.Load([]string{"--review-notifications-poll-interval", "90s"}, env(m))
323+
require.NoError(t, err)
324+
assert.Equal(t, 90*time.Second, cfg.ReviewNotificationsPollInterval)
325+
})
326+
327+
t.Run("invalid enable value names the flag", func(t *testing.T) {
328+
t.Parallel()
329+
330+
_, err := config.Load([]string{"--review-notifications", "nope"}, env(secrets()))
331+
require.Error(t, err)
332+
assert.ErrorContains(t, err, "invalid boolean flag value")
333+
assert.ErrorContains(t, err, "review-notifications")
334+
assert.ErrorContains(t, err, "nope")
335+
})
336+
337+
t.Run("unparsable interval names the flag and value", func(t *testing.T) {
338+
t.Parallel()
339+
340+
_, err := config.Load([]string{"--review-notifications-poll-interval", "soon"}, env(secrets()))
341+
require.Error(t, err)
342+
assert.ErrorContains(t, err, "invalid duration flag value")
343+
assert.ErrorContains(t, err, "review-notifications-poll-interval")
344+
assert.ErrorContains(t, err, "soon")
345+
})
346+
347+
t.Run("zero interval rejected", func(t *testing.T) {
348+
t.Parallel()
349+
350+
_, err := config.Load([]string{"--review-notifications-poll-interval", "0s"}, env(secrets()))
351+
require.Error(t, err)
352+
assert.ErrorContains(t, err, "poll interval must be positive")
353+
assert.ErrorContains(t, err, "0s")
354+
})
355+
356+
t.Run("negative interval rejected", func(t *testing.T) {
357+
t.Parallel()
358+
359+
_, err := config.Load([]string{"--review-notifications-poll-interval", "-15s"}, env(secrets()))
360+
require.Error(t, err)
361+
assert.ErrorContains(t, err, "poll interval must be positive")
362+
assert.ErrorContains(t, err, "-15s")
363+
})
364+
365+
t.Run("interval error aggregates with other errors", func(t *testing.T) {
366+
t.Parallel()
367+
368+
m := secrets()
369+
delete(m, "GERRIT_TOKEN")
370+
371+
_, err := config.Load([]string{"--review-notifications-poll-interval", "never"}, env(m))
372+
require.Error(t, err)
373+
assert.ErrorContains(t, err, "invalid duration flag value")
374+
assert.ErrorContains(t, err, "GERRIT_TOKEN")
375+
})
376+
}
377+
250378
func Test_Load_Connection(t *testing.T) {
251379
t.Parallel()
252380

internal/gerritclient/gerritclient_test.go

Lines changed: 10 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -15,14 +15,16 @@ import (
1515

1616
func testConfig(url string) *config.Config {
1717
return &config.Config{
18-
GerritURL: url,
19-
Username: "bot",
20-
Token: "s3cret",
21-
Groups: []config.Group{config.GroupRead},
22-
IncludeTools: nil,
23-
ExcludeTools: nil,
24-
Projects: nil,
25-
AllowForeignChanges: false,
18+
GerritURL: url,
19+
Username: "bot",
20+
Token: "s3cret",
21+
Groups: []config.Group{config.GroupRead},
22+
IncludeTools: nil,
23+
ExcludeTools: nil,
24+
Projects: nil,
25+
AllowForeignChanges: false,
26+
ReviewNotifications: false,
27+
ReviewNotificationsPollInterval: 0,
2628
}
2729
}
2830

internal/registry/registry_test.go

Lines changed: 10 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -13,14 +13,16 @@ import (
1313

1414
func cfg(groups []config.Group, include, exclude []string) *config.Config {
1515
return &config.Config{
16-
GerritURL: "https://gerrit.example.com",
17-
Username: "bot",
18-
Token: "s3cret",
19-
Groups: groups,
20-
IncludeTools: include,
21-
ExcludeTools: exclude,
22-
Projects: nil,
23-
AllowForeignChanges: false,
16+
GerritURL: "https://gerrit.example.com",
17+
Username: "bot",
18+
Token: "s3cret",
19+
Groups: groups,
20+
IncludeTools: include,
21+
ExcludeTools: exclude,
22+
Projects: nil,
23+
AllowForeignChanges: false,
24+
ReviewNotifications: false,
25+
ReviewNotificationsPollInterval: 0,
2426
}
2527
}
2628

internal/tools/get-change_test.go

Lines changed: 10 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -53,14 +53,16 @@ func session(t *testing.T, gerritHandler http.HandlerFunc, mutate ...func(*confi
5353
t.Cleanup(srv.Close)
5454

5555
cfg := &config.Config{
56-
GerritURL: srv.URL,
57-
Username: "bot",
58-
Token: "s3cret",
59-
Groups: []config.Group{config.GroupRead},
60-
IncludeTools: nil,
61-
ExcludeTools: nil,
62-
Projects: nil,
63-
AllowForeignChanges: false,
56+
GerritURL: srv.URL,
57+
Username: "bot",
58+
Token: "s3cret",
59+
Groups: []config.Group{config.GroupRead},
60+
IncludeTools: nil,
61+
ExcludeTools: nil,
62+
Projects: nil,
63+
AllowForeignChanges: false,
64+
ReviewNotifications: false,
65+
ReviewNotificationsPollInterval: 0,
6466
}
6567
for _, m := range mutate {
6668
m(cfg)

0 commit comments

Comments
 (0)