fix(agentgate): collapse repeated approval prompts into one card

internal/agentgate/approval.go

@@ -112,13 +112,22 @@ type Manager struct {
 
 	mu           sync.Mutex
 	cache        map[string]types.Decision
+	burst        map[string]burstEntry
+	burstTTL     time.Duration // burst-memo lifetime; see burstCacheTTL
 	pending      map[string]*pendingEntry
 	totalPrompts int
 	timeout      time.Duration // prompt deadline; see approvalTimeout
 	recent       []time.Time
 	totalTripped bool
 }
 
+// burstEntry is a short-lived memo of a once-scoped decision, used to collapse
+// a burst of identical traps into a single prompt. See burstCacheTTL.
+type burstEntry struct {
+	decision types.Decision
+	expires  time.Time
+}
+
 // pendingEntry is the internal record of an in-flight approval — the
 // resolution channel plus the metadata needed to surface the request to a
 // reconnecting FE via ListPending.
@@ -140,28 +149,47 @@ type resolution struct {
 // request.
 const approvalTimeout = 10 * time.Minute
 
+// burstCacheTTL is how long a once-scoped decision ("Allow once" / "Deny") is
+// remembered so an immediately-repeated identical request reuses it instead of
+// prompting again.
+//
+// Why this exists: a single logical command can trap the gate many times. The
+// common case is a PATH search — anything launched through a wrapper such as
+// timeout(1), env(1) or nohup(1) is resolved with execvp(3), which issues one
+// execve(2) per PATH entry until one succeeds. Each of those probes carries
+// byte-identical argv, so each produced its own approval card and one
+// `timeout 110 git push …` could stack ten of them. Bare `git push` traps once
+// because the shell stats its way to the absolute path first.
+//
+// The window only has to span the burst itself (probes arrive ~1ms apart), so
+// it is deliberately short: a genuinely new invocation a few seconds later
+// still prompts. The trade-off is that a script re-running the same command
+// inside the window rides the first decision — which is why this is measured
+// in seconds, not minutes, and why "Allow for session" remains the only way to
+// stop being asked at all.
+const burstCacheTTL = 5 * time.Second
+
 // NewManager constructs a Manager. Zero-valued RateLimits fields disable that cap.
 func NewManager(bots BotRouter, limits types.RateLimits) *Manager {
 	return &Manager{
-		bots:    bots,
-		limits:  limits,
-		timeout: approvalTimeout,
-		now:     time.Now,
-		idGen:   randomID,
-		cache:   map[string]types.Decision{},
-		pending: map[string]*pendingEntry{},
+		bots:     bots,
+		limits:   limits,
+		timeout:  approvalTimeout,
+		burstTTL: burstCacheTTL,
+		now:      time.Now,
+		idGen:    randomID,
+		cache:    map[string]types.Decision{},
+		burst:    map[string]burstEntry{},
+		pending:  map[string]*pendingEntry{},
 	}
 }
 
 // Request prompts the user (or returns a cached / rate-limited result).
 // Outcome.Decision is always Allow or Deny — never Approve.
 func (m *Manager) Request(ctx context.Context, channelID string, req ApprovalRequest) Outcome {
 	if req.CacheKey != "" {
-		m.mu.Lock()
-		d, ok := m.cache[req.CacheKey]
-		m.mu.Unlock()
-		if ok {
-			return Outcome{Decision: d, FromCache: true, Reason: "cache-hit"}
+		if out, ok := m.lookupCached(req.CacheKey); ok {
+			return out
 		}
 	}
 
@@ -326,6 +354,38 @@ func (m *Manager) checkLimits() (Outcome, bool) {
 	return Outcome{}, true
 }
 
+// lookupCached resolves a CacheKey against the session cache first, then the
+// short-lived burst memo. An expired burst entry is dropped on read so the
+// caller prompts again.
+func (m *Manager) lookupCached(key string) (Outcome, bool) {
+	m.mu.Lock()
+	defer m.mu.Unlock()
+	if d, ok := m.cache[key]; ok {
+		return Outcome{Decision: d, FromCache: true, Reason: "cache-hit"}, true
+	}
+	e, ok := m.burst[key]
+	if !ok {
+		return Outcome{}, false
+	}
+	if !m.now().Before(e.expires) {
+		delete(m.burst, key)
+		return Outcome{}, false
+	}
+	return Outcome{Decision: e.decision, FromCache: true, Reason: "burst-hit"}, true
+}
+
+// rememberBurstLocked memoises a once-scoped decision for burstTTL, sweeping
+// entries that have already expired. Caller holds m.mu.
+func (m *Manager) rememberBurstLocked(key string, d types.Decision) {
+	now := m.now()
+	for k, e := range m.burst {
+		if !now.Before(e.expires) {
+			delete(m.burst, k)
+		}
+	}
+	m.burst[key] = burstEntry{decision: d, expires: now.Add(m.burstTTL)}
+}
+
 func (m *Manager) applyResolution(cacheKey string, r resolution) Outcome {
 	var d types.Decision
 	persist := false
@@ -341,9 +401,16 @@ func (m *Manager) applyResolution(cacheKey string, r resolution) Outcome {
 		d = types.DecisionDeny
 		persist = true
 	}
-	if persist && cacheKey != "" {
+	if cacheKey != "" {
 		m.mu.Lock()
-		m.cache[cacheKey] = d
+		switch {
+		case persist:
+			// Session scope supersedes any burst memo for the same key.
+			m.cache[cacheKey] = d
+			delete(m.burst, cacheKey)
+		case m.burstTTL > 0:
+			m.rememberBurstLocked(cacheKey, d)
+		}
 		m.mu.Unlock()
 	}
 	return Outcome{Decision: d, Actor: r.actor}

internal/agentgate/approval_test.go

@@ -259,6 +259,171 @@ func (s *ApprovalSuite) TestResolveEmptyCacheKeySessionSkipsCache() {
 	require.Equal(s.T(), 0, size)
 }
 
+// --- Burst memo ---
+//
+// A single logical command can trap the gate many times with byte-identical
+// argv — most often a PATH search, where execvp(3) issues one execve(2) per
+// PATH entry. These cover the short-lived memo that collapses such a burst
+// into one prompt without granting session scope.
+
+// movableClock returns a clock function plus a knob to advance it, so the
+// burst TTL can be crossed without sleeping.
+func movableClock() (now func() time.Time, advance func(time.Duration)) {
+	var mu sync.Mutex
+	t := fixedNow
+	return func() time.Time {
+			mu.Lock()
+			defer mu.Unlock()
+			return t
+		}, func(d time.Duration) {
+			mu.Lock()
+			defer mu.Unlock()
+			t = t.Add(d)
+		}
+}
+
+func (s *ApprovalSuite) TestOnceDecisionCollapsesRepeatBurst() {
+	bot := &fakeBot{}
+	m := s.newManager(bot, types.RateLimits{})
+
+	outCh := s.request(m, ApprovalRequest{CacheKey: "execve:git:push origin"})
+	reqID := s.waitForPending(m, 1)
+	require.NoError(s.T(), m.Resolve(reqID, DecisionOnce, "u"))
+	require.Equal(s.T(), types.DecisionAllow, (<-outCh).Decision)
+
+	// The next PATH probe arrives microseconds later with the same key.
+	var promptFired bool
+	out := m.Request(context.Background(), "chan1", ApprovalRequest{
+		CacheKey: "execve:git:push origin",
+		OnPrompt: func() { promptFired = true },
+	})
+
+	require.Equal(s.T(), types.DecisionAllow, out.Decision)
+	require.True(s.T(), out.FromCache)
+	require.Equal(s.T(), "burst-hit", out.Reason)
+	require.False(s.T(), promptFired)
+	require.Equal(s.T(), 1, bot.sendCount, "the burst must cost exactly one card")
+
+	// Once-scope must still stay out of the session cache.
+	m.mu.Lock()
+	_, cached := m.cache["execve:git:push origin"]
+	m.mu.Unlock()
+	require.False(s.T(), cached)
+}
+
+func (s *ApprovalSuite) TestDenyOnceCollapsesRepeatBurst() {
+	bot := &fakeBot{}
+	m := s.newManager(bot, types.RateLimits{})
+
+	outCh := s.request(m, ApprovalRequest{CacheKey: "k"})
+	reqID := s.waitForPending(m, 1)
+	require.NoError(s.T(), m.Resolve(reqID, DecisionDeny, "u"))
+	<-outCh
+
+	out := m.Request(context.Background(), "chan1", ApprovalRequest{CacheKey: "k"})
+	require.Equal(s.T(), types.DecisionDeny, out.Decision)
+	require.Equal(s.T(), "burst-hit", out.Reason)
+	require.Equal(s.T(), 1, bot.sendCount)
+}
+
+func (s *ApprovalSuite) TestBurstMemoExpiresAndPromptsAgain() {
+	bot := &fakeBot{}
+	m := s.newManager(bot, types.RateLimits{})
+	now, advance := movableClock()
+	m.now = now
+
+	outCh := s.request(m, ApprovalRequest{CacheKey: "k"})
+	reqID := s.waitForPending(m, 1)
+	require.NoError(s.T(), m.Resolve(reqID, DecisionOnce, "u"))
+	<-outCh
+
+	advance(m.burstTTL) // exactly at the deadline — expired, not "still valid"
+
+	outCh = s.request(m, ApprovalRequest{CacheKey: "k"})
+	reqID = s.waitForPending(m, 1)
+	require.NoError(s.T(), m.Resolve(reqID, DecisionOnce, "u"))
+	require.Equal(s.T(), types.DecisionAllow, (<-outCh).Decision)
+	require.Equal(s.T(), 2, bot.sendCount, "an expired memo must prompt again")
+}
+
+func (s *ApprovalSuite) TestSessionDecisionSupersedesBurstMemo() {
+	bot := &fakeBot{}
+	m := s.newManager(bot, types.RateLimits{})
+
+	// Two identical traps land before either is answered, so both prompt.
+	// The first click memoises once-scope; the second must supersede it.
+	firstCh := s.request(m, ApprovalRequest{ID: "a", CacheKey: "k"})
+	secondCh := s.request(m, ApprovalRequest{ID: "b", CacheKey: "k"})
+	s.waitForPending(m, 2)
+	require.NoError(s.T(), m.Resolve("a", DecisionOnce, "u"))
+	<-firstCh
+	require.NoError(s.T(), m.Resolve("b", DecisionSession, "u"))
+	<-secondCh
+
+	m.mu.Lock()
+	_, stillMemoed := m.burst["k"]
+	cached := m.cache["k"]
+	m.mu.Unlock()
+	require.False(s.T(), stillMemoed, "session scope must drop the stale once-memo")
+	require.Equal(s.T(), types.DecisionAllow, cached)
+
+	out := m.Request(context.Background(), "chan1", ApprovalRequest{CacheKey: "k"})
+	require.Equal(s.T(), types.DecisionAllow, out.Decision)
+	require.Equal(s.T(), "cache-hit", out.Reason)
+}
+
+func (s *ApprovalSuite) TestZeroBurstTTLDisablesMemo() {
+	bot := &fakeBot{}
+	m := s.newManager(bot, types.RateLimits{})
+	m.burstTTL = 0
+
+	outCh := s.request(m, ApprovalRequest{CacheKey: "k"})
+	reqID := s.waitForPending(m, 1)
+	require.NoError(s.T(), m.Resolve(reqID, DecisionOnce, "u"))
+	<-outCh
+
+	m.mu.Lock()
+	size := len(m.burst)
+	m.mu.Unlock()
+	require.Equal(s.T(), 0, size)
+}
+
+func (s *ApprovalSuite) TestBurstMemoSweepsExpiredKeys() {
+	bot := &fakeBot{}
+	m := s.newManager(bot, types.RateLimits{})
+	m.burst["stale"] = burstEntry{decision: types.DecisionAllow, expires: fixedNow.Add(-time.Second)}
+	m.burst["fresh"] = burstEntry{decision: types.DecisionAllow, expires: fixedNow.Add(time.Minute)}
+
+	outCh := s.request(m, ApprovalRequest{CacheKey: "k"})
+	reqID := s.waitForPending(m, 1)
+	require.NoError(s.T(), m.Resolve(reqID, DecisionOnce, "u"))
+	<-outCh
+
+	m.mu.Lock()
+	_, stale := m.burst["stale"]
+	_, fresh := m.burst["fresh"]
+	_, added := m.burst["k"]
+	m.mu.Unlock()
+	require.False(s.T(), stale, "writing a memo sweeps expired ones")
+	require.True(s.T(), fresh)
+	require.True(s.T(), added)
+}
+
+func (s *ApprovalSuite) TestEmptyCacheKeyLeavesNoBurstMemo() {
+	bot := &fakeBot{}
+	m := s.newManager(bot, types.RateLimits{})
+
+	outCh := s.request(m, ApprovalRequest{CacheKey: ""})
+	reqID := s.waitForPending(m, 1)
+	require.NoError(s.T(), m.Resolve(reqID, DecisionOnce, "u"))
+	<-outCh
+
+	m.mu.Lock()
+	size := len(m.burst)
+	m.mu.Unlock()
+	require.Equal(s.T(), 0, size)
+}
+
 // --- Resolve errors ---
 
 func (s *ApprovalSuite) TestResolveUnknownReqIDError() {