Skip to content

feat(agents): First class support for gemini#118

Merged
dpup merged 9 commits into
mainfrom
feat/gemini-cli
Feb 7, 2026
Merged

feat(agents): First class support for gemini#118
dpup merged 9 commits into
mainfrom
feat/gemini-cli

Conversation

@dpup
Copy link
Copy Markdown
Collaborator

@dpup dpup commented Feb 2, 2026

Summary

  • Add first-class moat gemini command for running Google Gemini CLI in isolated containers
  • Add moat grant gemini with two auth methods: import existing Gemini CLI OAuth credentials, or API key
  • Add proxy token substitution for transparent OAuth credential injection without exposing tokens to containers
  • Add background OAuth token refresh via RefreshableProvider interface (generic, not Gemini-specific)

Design decisions

OAuth uses a different API backend than API key mode. Gemini CLI routes to cloudcode-pa.googleapis.com (Cloud Code Private
API) for OAuth and generativelanguage.googleapis.com (Google AI SDK) for API keys. These are different APIs with different
contracts. The proxy injects credentials for the correct host based on auth type.

Token substitution for startup validation. Gemini CLI validates OAuth tokens at startup by POSTing to
oauth2.googleapis.com/tokeninfo. The container has placeholder tokens in ~/.gemini/oauth_creds.json — the proxy replaces
placeholders with real tokens in the Authorization header and request body before forwarding. Google validates the real token and
returns a real response. This avoids both faking responses and putting real tokens in the container.

RefreshableProvider interface. Background token refresh is opt-in via an interface on ProviderSetup. The core run lifecycle
(manager.go, run.go) has zero provider-specific code — it checks for the interface and manages io.Closer instances
generically. Any future provider with expiring tokens can implement this.

oauth_creds.json with far-future expiry. The placeholder credentials file has expiry_date set to 1 year in the future so
Gemini CLI doesn't attempt its own token refresh (which would fail on placeholder refresh tokens). The proxy handles real expiry
and refresh on the host side.

New proxy capabilities

  • RemoveRequestHeader(host, header) — strips client-sent headers that conflict with injected credentials
  • SetTokenSubstitution(host, placeholder, realToken) — replaces placeholder strings in request headers and bodies before
    forwarding

Files

New packages:

  • internal/gemini/ — Provider setup, credential refresh, session management, MCP config generation, settings types

New CLI:

  • cmd/moat/cli/gemini.gomoat gemini command and moat gemini sessions subcommand
  • cmd/moat/cli/gemini_run.go — Run logic, dependency injection, network allow list

Modified:

  • internal/credential/provider.goProxyConfigurer interface gains RemoveRequestHeader and SetTokenSubstitution; new
    RefreshableProvider interface
  • internal/proxy/proxy.go — Token substitution, header removal, both HTTP and CONNECT paths
  • internal/run/manager.go — Generic credential refresh lifecycle via RefreshableProvider
  • internal/run/run.gocredentialClosers []io.Closer replaces provider-specific fields

Docs:

  • docs/content/guides/08-running-gemini.md — Full user guide
  • docs/content/concepts/02-credentials.md — Gemini credential section
  • docs/content/reference/01-cli.mdmoat gemini and moat grant gemini reference
  • docs/content/reference/02-agent-yaml.mdgemini.sync_logs and gemini.mcp fields

@claude
Copy link
Copy Markdown

claude Bot commented Feb 2, 2026

📦 Previous Review (superseded by newer review)

Pull Request Review: First class support for Gemini

This is a well-architected PR that adds comprehensive Gemini CLI support to Moat. The implementation follows the existing patterns established for Claude Code and demonstrates strong attention to security, credential management, and code quality.

Strengths

Architecture & Design

  • Clean abstraction via interfaces: The RefreshableProvider interface is well-designed and generic. The core run lifecycle has zero provider-specific code, making future OAuth-based providers easy to add.
  • Security-first credential handling: Token substitution pattern is excellent - placeholder tokens in containers, real tokens never exposed, proxy handles all substitution at the network layer.
  • Proper separation of concerns: The split between internal/gemini/ (provider setup) and internal/credential/ (auth mechanics) is clean and logical.

Implementation Quality

  • Robust OAuth token refresh: Background refresh with exponential backoff (30s to 5min), proper invalid_grant detection for revoked tokens, graceful shutdown via io.Closer.
  • Comprehensive test coverage: 9 test files with unit tests for all critical paths (OAuth import, token refresh, proxy configuration, credential validation).
  • Thread-safe proxy operations: All proxy credential map updates use proper mutex locking (SetCredential, SetTokenSubstitution).
  • Good error messages: Clear, actionable errors (e.g., gemini_oauth.go:54-56 tells users exactly what to do when credentials are missing).

Documentation

  • Complete user documentation: Well-structured guide at docs/content/guides/08-running-gemini.md with clear examples for both OAuth and API key modes.
  • Updated reference docs: CLI commands and agent.yaml fields properly documented.
  • Inline code documentation: Good package-level docs (e.g., internal/gemini/doc.go) and detailed comments explaining non-obvious decisions.

Issues & Recommendations

Security Concerns

1. Token substitution body reading (internal/proxy/proxy.go:358-367)

The current implementation reads the entire request body into memory for token substitution:

bodyBytes, err := io.ReadAll(req.Body)

Issues:

  • No size limit - a malicious or buggy client could send GB-sized bodies, causing OOM
  • Inefficient for large legitimate requests (e.g., file uploads with auth in body)

Recommendation:

// Limit body size for substitution (e.g., 1MB - auth requests are small)
const maxSubstitutionBodySize = 1 * 1024 * 1024

if req.Body != nil && req.ContentLength > 0 && req.ContentLength < maxSubstitutionBodySize {
    bodyBytes, err := io.ReadAll(io.LimitReader(req.Body, maxSubstitutionBodySize))
    // ... substitution logic
} else {
    // Skip substitution for large/streaming bodies
}

This is particularly important because token substitution is only needed for Google's /tokeninfo endpoint, which has small request bodies.

2. Client credentials in npm package comments (internal/credential/gemini.go:17-37)

The OAuth client ID and secret are correctly sourced from the public npm package, but the comments should explicitly note the security implications:

// GeminiOAuthClientID is the public OAuth client ID used by Gemini CLI.
//
// Security note: This is an "installed application" OAuth client per Google's model.
// The client secret is not actually secret (it's in the public npm package).
// Real security comes from Google's OAuth flow (user must authenticate via browser)
// and the fact that refresh tokens are user-specific and never shared.

This helps future maintainers understand why hardcoding these values is acceptable.

Code Quality Issues

3. Duplicate logic in GeminiSetup.populateStagingDir (internal/gemini/provider.go:104-139)

The OAuth placeholder token logic duplicates the OAuthCreds struct defined elsewhere. Consider extracting into a testable helper function.

4. Magic number in refresh timing (internal/gemini/credential_refresh.go:14)

refreshBuffer = 5 * time.Minute

The choice should be documented: Google OAuth access tokens typically have 1 hour lifetime. We refresh 5 minutes early to ensure tokens don't expire during active requests.

5. Missing context cancellation in refreshLoop (internal/gemini/credential_refresh.go:64-109)

The refreshLoop can't be canceled during a long sleep. If a container stops while waiting for the next refresh, shutdown is delayed. Use a timer with select on both the timer channel and the stop channel for prompt shutdown.

Documentation Gaps

6. Missing network access documentation

The PR adds --allow-host flag (cmd/moat/cli/gemini_run.go:34) but the user guide doesn't mention it. Add to docs/content/guides/08-running-gemini.md.

7. gemini.sync_logs field undocumented behavior

The agent.yaml reference mentions gemini.sync_logs (docs/content/reference/02-agent-yaml.md) but doesn't explain what it does or when to use it. Either document it properly or remove it if it's not implemented.

Minor Issues

8. Inconsistent error formatting

Some errors use sentence case with periods, others don't. Pick one style and apply consistently (Go stdlib uses sentence case, no period).

9. Test coverage gap for error paths

internal/gemini/credential_refresh_test.go tests the happy path but doesn't test network failures, malformed JSON responses, or rate limiting.

10. Proxy credential map growth

The proxy stores credentials in unbounded maps. For long-running processes with many runs, this could grow. From my reading, proxy instances are per-run and short-lived, so this is likely fine. A comment clarifying the lifecycle would help.

Testing Recommendations

11. E2E test for Gemini flow

Consider adding an E2E test similar to the existing ones in internal/e2e/ that creates a run with Gemini grant, verifies proxy credential injection, confirms token substitution works for tokeninfo calls, and tests both OAuth and API key modes.

Summary

This is high-quality work. The OAuth token refresh architecture is particularly well-done and sets a good pattern for future providers. The security model is sound (token substitution is clever), and the code is well-tested.

Recommended actions before merge:

  1. Fix the unbounded body read in token substitution (security issue feat(routing): add hostname-based service routing for agents #1)
  2. Improve shutdown responsiveness in refreshLoop (issue perf(audit): optimize Merkle tree rebuilding to O(log n) per append #5)
  3. Document the --allow-host flag (issue refactor(audit): extract magic numbers to named constants #6)
  4. Add error path test coverage (issue fix(audit): add minimum token length validation for TCP collector #9)

Nice-to-haves:

The other issues are minor polish items that can be addressed in follow-up PRs.

Recommendation: Approve with minor changes requested (issues #1 and #5 should be fixed before merge).


Great work on this feature! The architecture is solid and extensible.

@claude
Copy link
Copy Markdown

claude Bot commented Feb 3, 2026

Pull Request Review: First-class Gemini CLI Support

I've completed a thorough code review of this PR. Overall, this is a well-architected implementation with excellent security practices. The token substitution mechanism is particularly clever and effectively prevents credential exposure. Below are my findings organized by category.


✅ Strengths

1. Excellent Security Model

The credential handling is exemplary:

  • No token exposure: Real tokens never reach the container via placeholder substitution pattern
  • Token substitution: Proxy replaces placeholders at the network layer (internal/proxy/proxy.go:348-378)
  • Proper isolation: Placeholder in oauth_creds.json with far-future expiry prevents container-side refresh attempts

2. Clean Architecture - RefreshableProvider Interface

// internal/credential/provider.go:161-173
type RefreshableProvider interface {
    StartRefresh(p ProxyConfigurer, cred *Credential) io.Closer
}
  • Optional interface pattern: Providers only implement if needed
  • Generic lifecycle: Run manager handles all refreshable providers uniformly (internal/run/manager.go)
  • Clean shutdown: Returns io.Closer for proper goroutine cleanup
  • Zero provider-specific code in core run lifecycle

3. Robust Token Refresh Loop

internal/gemini/credential_refresh.go:66-153

  • Proactive refresh 5 minutes before expiry
  • Exponential backoff on failures (30s → 5m max)
  • Proper timer drain pattern (lines 84-90, 102-108)
  • Graceful shutdown via channel signaling
  • Thread-safe token updates with RWMutex

4. Proper Resource Cleanup

internal/run/manager.go:2483-2505

  • Correct cleanup order: credentials → proxy → SSH → temp dirs
  • No resource leaks (goroutines, file handles, temp directories)
  • Errors logged but don't block shutdown

5. Good Test Coverage

  • OAuth vs API key modes (internal/gemini/provider_test.go)
  • Token refresh success/failure paths (internal/credential/gemini_refresh_test.go)
  • Goroutine lifecycle (internal/gemini/credential_refresh_test.go:36-60)
  • Concurrent access patterns (lines 62-84)

⚠️ Issues Found

1. Error Handling Fragility (MEDIUM PRIORITY)

Location: internal/gemini/credential_refresh.go:94-97

if err := g.refreshNow(); err != nil {
    if strings.Contains(err.Error(), "invalid_grant") {
        log.Error("gemini refresh token revoked, stopping refresh", "error", err)
        return
    }
}

Problem: Relies on string matching to detect token revocation. Google's error message format is not guaranteed to be stable.

Impact: If Google changes error message format, revoked tokens won't be detected properly and refresh will retry indefinitely.

Location: internal/credential/gemini_refresh.go:66-72

if resp.StatusCode != http.StatusOK {
    var errResp struct {
        Error       string `json:"error"`
        Description string `json:"error_description"`
    }
    _ = json.Unmarshal(body, &errResp)  // Silently ignores unmarshal errors
    return nil, fmt.Errorf("token refresh failed: %s (%s)", errResp.Error, errResp.Description)
}

Recommendation: Create a typed error for OAuth errors:

// internal/credential/gemini_refresh.go
type GeminiOAuthError struct {
    Code        string // "invalid_grant", "invalid_request", etc.
    Description string
}

func (e *GeminiOAuthError) Error() string {
    return fmt.Sprintf("%s: %s", e.Code, e.Description)
}

func (e *GeminiOAuthError) IsRevoked() bool {
    return e.Code == "invalid_grant"
}

// In Refresh():
if resp.StatusCode != http.StatusOK {
    var errResp struct {
        Error       string `json:"error"`
        Description string `json:"error_description"`
    }
    if err := json.Unmarshal(body, &errResp); err != nil {
        return nil, fmt.Errorf("token refresh failed with status %d", resp.StatusCode)
    }
    return nil, &GeminiOAuthError{
        Code:        errResp.Error,
        Description: errResp.Description,
    }
}

// In credential_refresh.go:
if err := g.refreshNow(); err != nil {
    var oauthErr *GeminiOAuthError
    if errors.As(err, &oauthErr) && oauthErr.IsRevoked() {
        log.Error("gemini refresh token revoked, stopping refresh", "error", err)
        return
    }
    // ... existing retry logic
}

2. Test Coverage Gap (LOW PRIORITY)

Missing test: Token revocation path in the refresh loop

The test TestCredentialProvider_RefreshUpdatesProxy doesn't actually exercise the refresh logic because:

  • It sets TokenURL: "http://localhost:0/token" (line 23)
  • No mock HTTP server is created
  • The test only verifies initial state, not refresh behavior

Recommendation: Add a test that:

  1. Mocks GeminiTokenRefresher to return invalid_grant error
  2. Starts the refresh loop
  3. Verifies the loop terminates cleanly without retrying
func TestCredentialProvider_StopsOnRevokedToken(t *testing.T) {
    mock := newMockProxy()
    provider := NewCredentialProvider(
        "token",
        "revoked-refresh-token",
        time.Now().Add(1*time.Second),
        mock,
    )
    
    // Mock refresher that always returns invalid_grant
    provider.refresher = &mockTokenRefresher{
        err: &GeminiOAuthError{Code: "invalid_grant"},
    }
    
    provider.Start()
    
    // Wait for loop to terminate due to revoked token
    select {
    case <-provider.stopped:
        // Expected - refresh loop should exit
    case <-time.After(5 * time.Second):
        t.Fatal("refresh loop did not terminate after revoked token")
    }
}

3. Documentation Gaps (LOW PRIORITY)

Missing docstring: internal/gemini/provider.go:91-93

// Cleanup is a no-op for Gemini; staging directory is managed by the caller.
func (p *ProviderSetup) Cleanup() error {
    return nil
}

Undocumented pattern: Timer drain in internal/gemini/credential_refresh.go:84-90

// Drain the timer channel to prevent stale fires after Stop().
// See https://pkg.go.dev/time#Timer.Stop for the canonical pattern.
if !timer.Stop() {
    select {
    case <-timer.C:
    default:
    }
}

🔍 Other Observations

Token Substitution Body Size Limit

internal/proxy/proxy.go:346

const maxSubstitutionBodySize = 64 * 1024 // 64KB

Good design: Prevents memory issues with large uploads while still handling token validation requests.

ReplaceAll Behavior

internal/proxy/proxy.go:359

if replaced := strings.ReplaceAll(auth, sub.Placeholder, sub.RealToken); replaced != auth {

Uses ReplaceAll which could replace multiple occurrences. This is acceptable because:

  • Placeholder is "moat-proxy-injected" which is unlikely to appear multiple times in tokens
  • If it does appear, consistent substitution is correct behavior
  • No security risk

Context Handling in refreshNow()

internal/gemini/credential_refresh.go:133

ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second)

Correct: Uses Background() rather than accepting context parameter. This ensures refresh completes within timeout even during shutdown, preventing proxy from stalling.


📊 Summary

Category Rating Notes
Security ✅ Excellent No credential exposure, proper token isolation
Architecture ✅ Excellent Clean interfaces, zero provider-specific coupling
Error Handling ⚠️ Good Minor fragility with string matching
Resource Management ✅ Excellent Proper cleanup, no leaks
Test Coverage ⚠️ Good Most paths covered, some edge cases missing
Documentation ⚠️ Adequate Core logic documented, minor gaps

🎯 Recommendations

Must Fix (before merge)

None - no blocking issues found

Should Fix (high value, low effort)

  1. Create typed error for OAuth failures (described in Issue feat(routing): add hostname-based service routing for agents #1 above)
    • Improves reliability of revocation detection
    • Makes error handling more maintainable
    • ~30 lines of code

Nice to Have (future improvements)

  1. Add test for token revocation path (described in Issue Add Claude Code GitHub Workflow #2)
  2. Add documentation comments for empty Cleanup() method and timer drain pattern

✅ Approval

This is a high-quality implementation that follows Go best practices and maintains excellent security hygiene. The architecture is extensible (any future provider can implement RefreshableProvider), the code is clean and well-tested, and the security model is sound.

The identified issues are minor and can be addressed in follow-up PRs without blocking this merge.

Recommendation: APPROVE


Great work on this feature! The token substitution mechanism is particularly elegant and the RefreshableProvider interface is a clean abstraction that will serve future providers well.

Add a complete Gemini CLI provider following the established provider
framework pattern (Claude, Codex). This enables running Google's Gemini
CLI in isolated containers with transparent credential injection.

Key additions:
- Gemini provider package (internal/providers/gemini/) implementing
  CredentialProvider and AgentProvider interfaces
- Dual auth support: OAuth (imported from Gemini CLI) and API key
- Proxy token substitution for OAuth (cloudcode-pa.googleapis.com)
  and API key injection (generativelanguage.googleapis.com)
- Background OAuth token refresh with exponential backoff
- Container staging directory for settings.json and oauth_creds.json
- MCP server configuration generation for Gemini
- Session management for tracking Gemini sessions
- CLI commands: `moat gemini` and `moat gemini sessions`
- `moat grant gemini` with OAuth import and API key flows
- Container init script support (MOAT_GEMINI_INIT)
- gemini-cli added to dependency registry as npm package
- Documentation: guide, CLI reference, agent.yaml reference
- Example agent.yaml configuration
@dpup dpup merged commit b58b47a into main Feb 7, 2026
4 checks passed
@dpup dpup deleted the feat/gemini-cli branch February 7, 2026 19:38
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.

1 participant