feat(agents): Conveniences for running Claude and support for OAuth based authentication

[dpup]

• Add moat claude command for easy Claude Code execution in isolated containers
• Support Claude Pro/Max subscriptions (OAuth) in addition to API keys
• Add session management for Claude Code runs

Details

New moat claude command:

  # Start Claude Code interactively
  moat claude

  # Run with a prompt
  moat claude -p "fix the failing tests"

  # Add GitHub access
  moat claude --grant github

Credential support:

• moat grant anthropic now imports OAuth credentials from Claude Code's macOS keychain
• Falls back to API key if no OAuth credentials found
• Credentials injected via proxy (never exposed to container)

Session tracking:

• moat claude sessions lists Claude Code sessions
• Sessions track workspace, state, and last accessed time
• Syncs state with actual container status

Code improvements:

• Extract shared execution logic into cmd/moat/cli/exec.go
• Add provider setup abstraction for credential-specific container configuration
• Add claude-code as installable dependency
• Fix various lint issues (unused code, shadowed variables, preallocation)

[claude]

PR Review: Claude Code Integration

This PR adds a comprehensive moat claude command with OAuth support and session management. Overall, this is a well-structured addition with good separation of concerns and excellent test coverage. Here's my detailed review:

✅ Strengths

Architecture & Design

• Excellent abstraction: The ProviderSetup interface in internal/credential/provider.go is a clean, extensible design for handling provider-specific configuration
• Good separation of concerns: exec.go properly extracts shared execution logic from the original run.go, reducing duplication
• Smart credential detection: Auto-detecting Claude Code OAuth credentials from macOS keychain or filesystem is user-friendly
• Proper cleanup handling: The cleanup path mechanism in ProviderSetup ensures temp directories are properly removed

Code Quality

• Strong test coverage: All new packages have comprehensive unit tests with good edge case coverage
• Security-conscious: OAuth tokens and API keys are handled separately with appropriate security measures
• Error messages: Good, actionable error messages (e.g., in claude.go:106 suggesting to run claude first)
• Documentation: Inline comments explain complex logic well (e.g., the proxy security model differences)

🔍 Issues Found

1. Race Condition in Session State Updates (Medium Priority)

Location: cmd/moat/cli/claude_sessions.go:63-89

The session state synchronization has a potential race condition:

for _, s := range sessions {
    r, err := runMgr.Get(s.RunID)
    // ... determine newState ...
    if s.State != newState {
        s.State = newState
        _ = sessionMgr.UpdateState(s.ID, newState)  // Error ignored
    }
}

Issues:

• Errors from UpdateState are silently ignored (line 87)
• Modifying s.State locally doesn't affect the session object returned to the user
• No locking between reading and writing session state

Recommendation: At minimum, log the error. Consider whether state updates should be atomic or if eventual consistency is acceptable here.

2. Unused Variable Assignment (Low Priority)

Location: cmd/moat/cli/exec.go:169

lastSigTime = now
_ = lastSigTime // Mark as used for linter (value read in next iteration)

This is a workaround for a linter warning, but it's not ideal. The variable IS used in the next iteration. Recommendation: Remove the _ = lastSigTime line - the linter should recognize the loop usage.

3. Path Validation in Session Manager (Low Priority)

Location: internal/claude/session.go:184-186

The validSessionID regex validates session IDs but doesn't prevent directory traversal if someone crafts a malicious ID like ../../../etc. While filepath.Join helps, consider:

• Using filepath.Clean on the result
• Explicitly checking for path separators in the ID
• Or using a whitelist-only approach (alphanumeric + hyphen only, which the regex does, but be defensive)

Current code:

if !validSessionID.MatchString(session.ID) {
    return fmt.Errorf("invalid session ID: %s", session.ID)
}
sessionDir := filepath.Join(m.dir, session.ID)

Recommendation: Add an extra safety check:

sessionDir := filepath.Join(m.dir, session.ID)
if !strings.HasPrefix(sessionDir, m.dir) {
    return fmt.Errorf("invalid session ID: directory traversal detected")
}

4. Potential OAuth Token Expiry Handling

Location: internal/credential/claude.go:44

The IsExpired() check exists but I don't see it being used anywhere in the flow. When credentials are loaded in claude_run.go:118, there's no check for expiration.

Recommendation: Consider adding expiry checking when loading credentials and warning users if their OAuth token is expired.

5. File Permissions on Credentials Directory (Security)

Location: internal/claude/provider.go:127

baseDir := filepath.Join(home, ".moat", "claude-creds")
if mkdirErr := os.MkdirAll(baseDir, 0700); mkdirErr != nil {

Good use of 0700 permissions! However, the temp subdirectory created by os.MkdirTemp may have different permissions depending on the system. Recommendation: Explicitly set permissions on the temp directory after creation to ensure 0700.

6. Missing Error Context

Location: cmd/moat/cli/claude_run.go:223-226

if sessionErr = sessionMgr.Create(absPath, r.ID, r.Name, grants); sessionErr != nil {
    log.Debug("failed to create session", "error", sessionErr)
}

Session creation failures are only logged at debug level. If sessions are an important feature, this should at least be a warning. Users might wonder why their sessions aren't showing up.

💡 Suggestions

1. Code Duplication in Tests

The mockProxyConfigurer is duplicated between internal/credential/provider_test.go and internal/claude/provider_test.go. Consider moving it to a shared test helper package or file.

2. Constants for Magic Strings

Location: internal/credential/provider.go:109

return len(token) > 10 && token[:10] == "sk-ant-oat"

Consider defining the OAuth token prefix as a constant for better maintainability:

const OAuthTokenPrefix = "sk-ant-oat"

func IsOAuthToken(token string) bool {
    return len(token) > len(OAuthTokenPrefix) && 
           strings.HasPrefix(token, OAuthTokenPrefix)
}

3. Time Format Consistency

Location: cmd/moat/cli/claude_sessions.go:148-175

The formatTimeAgo function is useful and could be shared across the codebase. Consider moving it to a internal/format or internal/util package if other commands might benefit from human-readable time formatting.

4. Documentation Enhancement

The README examples are excellent, but consider adding:

• What happens when OAuth tokens expire (does moat handle refresh?)
• Limitations of the OAuth credential import (e.g., tokens are session-bound)
• Security implications of credential injection

5. Dependency Declaration

Location: internal/deps/registry.yaml

The claude-code dependency is added to the registry. Good! But consider documenting the version strategy - is this always latest, or should it be pinnable?

🎯 Performance Considerations

1. Session Listing Performance: The List() function in session.go:118-146 reads all session files every time. This could be slow with many sessions. Consider:

   • Adding an LRU cache for session metadata
   • Lazy loading only when needed
   • Implementing pagination for moat claude sessions

2. Concurrent Session State Updates: In claude_sessions.go, state updates happen sequentially in a loop. If there are many sessions, this could be slow. Consider using goroutines with a wait group for concurrent updates.

🔒 Security Review

Positive

• Proper separation of OAuth vs API key handling ✅
• Credentials never exposed to container (proxy injection) ✅
• Read-only mounts where appropriate ✅
• Secure file permissions (0600 for credentials, 0700 for directories) ✅
• No credential leakage in logs ✅

Concerns

• Credential file cleanup: While cleanup is implemented, if the process crashes, temp credential files in ~/.moat/claude-creds/run-* may remain. Consider adding a cleanup routine on startup to remove stale directories.
• Mount validation: The mounts in provider.go are read-only where possible, which is good. However, ~/.claude.json is mounted read-write (line 81). Document why this is necessary.

📊 Test Coverage Assessment

Excellent test coverage overall:

• ✅ Unit tests for all new credential logic
• ✅ OAuth token parsing and validation
• ✅ Provider setup configuration
• ✅ Session management
• ❓ Missing: Integration tests for the full moat claude flow (though this may be in e2e tests)

🎨 Code Style & Conventions

The code follows the project's conventions well:

• ✅ Uses conventional commits format (based on PR title)
• ✅ Good error messages with context
• ✅ Follows Go idioms and formatting
• ✅ Proper use of interfaces for abstraction

Minor: The comment style is mostly consistent, but some places use // comments where doc comments (// FunctionName ...) would be more appropriate for exported functions.

📝 Documentation

The documentation updates in README.md are clear and helpful. The examples tell a good story. One suggestion: add a troubleshooting section for common issues like:

• OAuth credentials not found
• Token expiration
• Permission errors

Final Verdict

This is a high-quality PR with good architecture, solid test coverage, and thoughtful implementation. The issues found are mostly minor with a few medium-priority items around error handling and edge cases.

Recommended Actions Before Merge:

1. High Priority: Fix or document the session state update error handling (issue feat(routing): add hostname-based service routing for agents #1)
2. Medium Priority: Add OAuth token expiry checking (issue feat(proxy): Add HTTPS support to routing proxy #4)
3. Medium Priority: Add stale credential directory cleanup
4. Low Priority: Address the other minor issues as time permits

The PR is ready for merge after addressing the high-priority items. Great work! 🚀

[claude]

Code Review: OAuth and Claude Code Support

This is high-quality work with excellent abstractions. Here are my findings:

Strengths:

• Clean ProviderSetup interface with good extensibility
• Comprehensive test coverage for new functionality
• Good refactoring with exec.go reducing code duplication
• Strong security practices (credential injection, file permissions)
• Excellent user experience with session management

Critical Issues:

1. Resource Cleanup (internal/claude/provider.go:108-116): Temporary credential directories may accumulate if cleanup fails. Need defer pattern to ensure cleanup.

2. Shadowed Variable Bug (cmd/moat/cli/exec.go:169): Line 169 has suspicious linter workaround that suggests lastSigTime shadowing issue. This could break double-press detection.

3. Silent Error Handling (cmd/moat/cli/claude_sessions.go:64-88): UpdateState errors are ignored, leading to stale UI state.

Medium Priority:

4. OAuth Token Refresh: Code extracts RefreshToken but never uses it. Need to implement refresh or document limitation.

5. Keychain Error Messages (internal/credential/claude.go:72-80): Should capture stderr for better error diagnostics.

6. Dependency Checking (cmd/moat/cli/claude_run.go:258-263): hasDependency could incorrectly match node-sass@4 as node. Need more precise matching.

Minor:

• Session states should use typed enum instead of string constants
• CleanupOldSessions exists but is never called
• Missing test coverage for CLI execution paths

Security: Good overall. Credentials properly injected, secure file permissions, directory traversal prevention.

Performance: Session listing scales linearly - fine for typical usage.

Overall: Approve with minor fixes recommended. Core functionality is solid. Main action items: fix shadowed variable bug and ensure cleanup reliability.

[claude]

Code Review - PR #38: Claude Code Integration

This is an excellent addition to Moat! The implementation is well-structured, thoroughly tested, and follows the project's conventions. Here's my detailed review:

🎯 Overall Assessment

Strong Points:

• ✅ Excellent code organization with clear separation of concerns
• ✅ Good test coverage (unit tests for credential handling, provider setup, sessions)
• ✅ Proper error handling with actionable error messages
• ✅ Security-conscious design (credentials never exposed to containers)
• ✅ Well-documented with helpful examples in README
• ✅ Follows conventional commits format

📝 Code Quality

Architecture (Excellent)

The refactoring to extract shared execution logic into exec.go is well-done:

• ExecFlags and ExecOptions provide clean abstractions
• ExecuteRun() centralizes run lifecycle management
• Provider setup pattern (ProviderSetup interface) is extensible and clean
• Session management is properly isolated

Security (Good with minor note)

Credential Handling:

• ✅ OAuth tokens properly extracted from macOS Keychain
• ✅ Tokens injected via proxy (never in environment/filesystem visible to agent)
• ✅ Different auth headers for OAuth vs API keys (Bearer vs x-api-key)
• ✅ Proper use of anthropic-beta header for OAuth

Note: The OAuth token detection in internal/credential/provider.go:108-110:

func IsOAuthToken(token string) bool {
    return len(token) > 10 && token[:10] == "sk-ant-oat"
}

This is a reasonable heuristic but might be fragile if Anthropic changes token prefixes. Consider adding a comment about this being based on observed token format.

File Mounts (Potential Issue)

In internal/claude/provider.go:74-82, the code mounts ~/.claude.json with ReadOnly: false:

mounts = append(mounts, container.MountConfig{
    Source:   hostClaudeJSON,
    Target:   filepath.Join(containerHome, ".claude.json"),
    ReadOnly: false,  // ⚠️
})

Question: Is this intentional? Allowing the container to modify the host's ~/.claude.json file could be a security concern. Consider:

1. If it's only for reading onboarding state, make it read-only
2. If Claude Code must write to it, document why in a comment
3. Consider copying it to a temp location and mounting that instead

Session Management (Good)

Strengths:

• Proper state synchronization between sessions and runs (claude_sessions.go:63-89)
• Clean JSON persistence
• Input validation with regex for session IDs
• Cleanup functionality for old sessions

Minor improvement: The session state sync in runSessionsList silently ignores errors (_ = sessionMgr.UpdateState(...)). Consider logging these failures at debug level.

Error Messages (Excellent)

Following the CLAUDE.md guidance perfectly. Examples:

"Claude Code credentials not found at %s\n" +
"  Have you logged into Claude Code? Run 'claude' to authenticate first"

Clear, actionable, tells users exactly what to do.

🐛 Potential Issues

1. Path Injection in Session ID (Low Risk)

internal/claude/session.go:175-178:

if !validSessionID.MatchString(id) {
    return fmt.Errorf("invalid session ID: %s", id)
}
sessionDir := filepath.Join(m.dir, id)

Good validation present! The regex prevents path traversal. ✅

2. Race Condition in Cleanup (Low Risk)

internal/claude/provider.go:112-115:

func (a *AnthropicSetup) Cleanup(cleanupPath string) {
    if cleanupPath != "" {
        _ = os.RemoveAll(cleanupPath)
    }
}

If cleanup fails, it's silently ignored. This is probably fine for temp directories, but consider logging at debug level.

3. Missing Validation (Low Risk)

cmd/moat/cli/claude_run.go:118-120:

if hasAnthropicCredential() {
    addGrant("anthropic")
}

hasAnthropicCredential() returns false on any error (key loading, store access). This is safe but might hide issues. Consider debug logging when it returns false due to errors vs simply no credential.

🚀 Performance Considerations

Mount Optimization

The code conditionally mounts host files only if they exist (good!):

if _, err := os.Stat(hostClaudeJSON); err == nil {
    mounts = append(mounts, ...)
}

This avoids unnecessary mounts. ✅

Session Listing

List() loads all session files and sorts them. For large numbers of sessions, this could be slow. Consider:

• Caching or indexing if session count grows
• Pagination for the CLI output
• Background cleanup of old sessions

Not urgent - unlikely to have enough sessions for this to matter in practice.

✅ Test Coverage (Excellent)

Well-structured tests covering:

• OAuth token expiration logic (claude_test.go)
• Provider proxy configuration for both OAuth and API keys (provider_test.go)
• Credential extraction from keychain and files
• Mock implementations for testing

📚 Documentation

README Updates (Good)

• Clear examples of the new moat claude command
• Explains credential flow well
• Shows expected output

Minor suggestion:

The README shows:

Found Claude Code credentials.
  Subscription: claude_pro
  Expires: 2025-02-15T10:30:00Z

Consider using a relative time format ("expires in 25 days") for better UX, matching the formatTimeAgo pattern used in sessions.

🔧 Recommendations

High Priority

1. Review ~/.claude.json mount permissions (provider.go:82)

• Document why it needs write access, or make it read-only

Medium Priority

2. Add debug logging for credential detection

func hasAnthropicCredential() bool {
    key, err := credential.DefaultEncryptionKey()
    if err != nil {
        log.Debug("failed to get encryption key", "error", err)  // Add this
        return false
    }
    // ... rest of function
}

3. Add cleanup error logging

func (a *AnthropicSetup) Cleanup(cleanupPath string) {
    if cleanupPath != "" {
        if err := os.RemoveAll(cleanupPath); err != nil {
            log.Debug("failed to cleanup credentials dir", "path", cleanupPath, "error", err)
        }
    }
}

Low Priority (Nice to Have)

4. Add integration test for the full moat claude flow (if not already in e2e tests)

5. Consider adding session cleanup command: moat claude sessions clean --older-than 30d

6. Token refresh: The OAuth token handling doesn't include refresh logic. Document whether refresh is handled by Claude Code itself or if tokens are long-lived.

✨ Excellent Patterns to Highlight

1. Provider abstraction - The ProviderSetup interface is well-designed and makes it easy to add new credential providers

2. Callback pattern in ExecOptions.OnRunCreated - Clean way to inject session creation without coupling

3. Dependency injection - The code ensures claude-code is installed via the dependency system rather than assuming it exists

4. Separation of concerns - Credential extraction, proxy configuration, and session management are cleanly separated

🎬 Conclusion

This is production-ready code with just a few minor improvements to consider. The architecture is solid, tests are comprehensive, and it follows all the project conventions. Great work!

Approve with minor suggestions - the high-priority item about ~/.claude.json permissions should be reviewed, but it doesn't block merging if it's intentional.

---

Review generated with analysis of 22 files, 2655 additions, and focused testing of credential/session management