feat(agents): Add Codex support

[dpup]

• Add moat codex command to run OpenAI Codex CLI in isolated containers with automatic credential injection
• Support both OpenAI API keys and ChatGPT subscription tokens via moat grant openai
• Extract shared session management logic to internal/session package (used by both Claude and Codex)
• Add StartAttached runtime method to properly handle TUI applications that need TTY connected before process
  starts
• Rename examples from claude-code to agent-claude and add new agent-codex example

[claude]

📦 Previous Review (superseded by newer review)

Code Review: PR #51 - Add Codex Support

Overall assessment: 7.5/10 - Solid foundation with critical gaps that need addressing.

---

Summary

Strengths:

• Well-designed shared session management abstraction
• Thoughtful credential injection security using placeholder strategy
• StartAttached pattern correctly solves TUI initialization
• Code follows Go conventions and project patterns
• Comprehensive error messages guide users effectively

Critical Issues:

• Zero test coverage for new OpenAI credential and session code
• Apple container race condition in StartAttached implementation
• Token classification lacks validation (could misclassify tokens)
• Session persistence missing file locking (concurrent access issues)
• Detached mode UX gap (no run ID output for users)

---

Must-Fix Before Merge

1. Test Coverage Gaps

Files missing tests:

• internal/credential/openai.go - No openai_test.go
• internal/session/session.go - No session_test.go
• internal/codex/ package - No integration tests

Required tests: Token classification, credential creation/validation, staging directory generation, session persistence

Location: internal/credential/openai.go:280
The IsCodexToken() function accepts ANY non-"sk-" string without validation (empty strings, JWTs, malformed tokens all pass).

2. Apple Container Race Condition

Location: internal/container/apple.go:921-935

The Apple runtime StartAttached() starts the container then attaches (race condition). Docker's implementation correctly attaches first, then starts. Container may exit before attach completes.

3. Detached Mode UX Issue

Location: cmd/moat/cli/codex_run.go:196-199

When running with -d flag, users get NO output showing the run ID. They don't know how to attach later. Need to add else clause printing run ID and moat attach command.

4. Session Concurrency Safety

Location: internal/session/session.go (save method)

File writes lack locking - concurrent sessions can corrupt metadata. Need file locking or atomic write-rename pattern.

5. Network Host Validation

Location: cmd/moat/cli/codex_run.go:131-142

User-provided --allow-host values are appended without validation. Invalid hosts cause runtime failures.

---

Should-Fix

6. Token Classification Edge Cases (openai.go:280-283)
Add length/structure validation. Empty strings, JWTs from other services, malformed tokens all currently pass.

7. Credential Expiration Handling (codex_run.go:74)
hasCredential() doesn't check expiration - expired tokens still get added to grants, causing runtime failures.

8. Flag Naming Confusion (codex.go:99)
The --noyolo flag uses a double-negative. Rename to --auto or --full-auto with default true.

9. Codex Login Timeout (grant.go:574)
2-minute timeout may be too short if browser opens slowly. Consider 5 minutes or make configurable.

10. Documentation Gaps
Missing docs for: MCP server configuration, credential placeholder strategy, migration guide for renamed examples.

---

Positive Findings

Excellent Security:

• Placeholder strategy (sk-moat-proxy-injected-placeholder-0000...) prevents token leakage
• Real tokens never reach container filesystem
• Proxy properly injects Bearer tokens for api.openai.com and chatgpt.com

Clean Architecture:

• Session abstraction is reusable across agents
• Helper functions are well-designed (resolveWorkspacePath, parseEnvFlags, hasDependency)
• Error messages guide users with actionable steps

Code Quality:

• Follows Go conventions and project patterns
• Naming is consistent and clear
• Comprehensive error handling with context

---

Testing Checklist

Before merge:

• openai_test.go with token classification tests
• session_test.go with persistence and concurrency tests
• Integration test for moat codex execution
• Detached mode prints run ID
• Apple container StartAttached documented or fixed
• File locking for session metadata
• Network host validation
• Token expiration warnings

---

Per CLAUDE.md Guidelines

• Follows Conventional Commits format (feat(agents): Add Codex support)
• Good error messages with actionable steps
• No Claude Co-Authored-By lines in commits
• Documentation accuracy: MCP behavior undocumented
• Test coverage: Critical gap for new code

---

Verdict: Strong implementation with thoughtful design, but test coverage is mandatory before production merge. The credential and session code must be tested to ensure security and reliability.

[claude]

📦 Previous Review (superseded by newer review)

Pull Request Review: feat(agents): Add Codex support

This PR adds comprehensive support for OpenAI's Codex CLI with credential management, session handling, and MCP server integration.

Overall Assessment

Strengths:

• Well-structured implementation following existing patterns
• Comprehensive test coverage for new functionality
• Good separation of concerns with the new internal/session package
• Security-conscious credential handling via proxy injection

Areas for Improvement:

• Some potential race conditions and error handling issues
• Missing validation in a few critical paths
• Missing tests for some code paths

---

Code Quality & Best Practices

✅ Excellent:

1. Shared Session Management (internal/session/session.go)

   • Clean abstraction, atomic file operations, thread-safe, good tests

2. OpenAI Credential Handling (internal/credential/openai.go)

   • Comprehensive token detection, multiple sources, well-tested (394 lines)

3. Helper Extraction (cmd/moat/cli/helpers.go)

   • DRY principle applied well, focused and reusable functions

⚠️ Issues to Address:

1. StartAttached Error Handling (internal/container/apple.go:956)

   • Silent error when killing orphaned attach process
   • Recommendation: Log the error

2. Session ID Validation (internal/session/session.go:37)

   • Allows problematic IDs like "a-" or "---b---"
   • Recommendation: Stricter validation

3. Missing Grant Validation (cmd/moat/cli/codex_run.go:62-86)

   • Grants added without validation
   • Recommendation: Validate against known providers early

---

Security Concerns

✅ Good Practices:

• Credential placeholders (never write real tokens to filesystem)
• Proxy-based injection at network layer
• Proper file permissions (0600)
• Token validation before storage

⚠️ Considerations:

1. Keychain Access (internal/credential/openai.go:196-216)

   • No validation before parsing JSON
   • Recommendation: Add size limits

2. Host Validation (cmd/moat/cli/helpers.go:129-164)

   • Multiple wildcards pass; no private IP checks
   • Recommendation: Restrict wildcards to single-level

---

Test Coverage

✅ Well Tested:

• internal/credential/openai_test.go: 394 lines
• internal/session/session_test.go: 476 lines

❌ Missing Tests:

• No tests for internal/codex/generate.go
• No tests for cmd/moat/cli/codex_run.go
• Helper functions lack tests

---

Recommendations Summary

Must Fix (High Priority):

1. Add tests for internal/codex/generate.go
2. Add validation for grant names
3. Log errors when skipping corrupted sessions

Should Fix (Medium Priority):
4. Stricter session ID validation
5. Document initialization order for store reference
6. Add helper function tests
7. Consider extracting shared agent runner abstraction

Nice to Have (Low Priority):
8. Add Codex MCP configuration examples
9. Consider caching credential checks
10. More restrictive host validation

---

Final Verdict

Approve with Recommendations ✅

Solid implementation with good test coverage for core functionality. Code quality is high, security model is sound. Main improvements:

1. Add tests for untested code paths
2. Reduce duplication between Claude and Codex runners
3. Minor validation and error handling improvements

PR is production-ready as-is. Addressing recommendations would improve maintainability.

Estimated Effort: Must Fix: 2-3h | Should Fix: 4-6h | Nice to Have: 2-4h

Great work! Session management extraction and OpenAI credential handling are particularly well done.

[claude]

📦 Previous Review (superseded by newer review)

Pull Request Review: feat(agents): Add Codex support

This is a solid PR that adds OpenAI Codex CLI support to moat with comprehensive credential management, session tracking, and test coverage. The implementation follows existing patterns from the Claude integration and includes several quality improvements. Here are my findings:

✅ Strengths

1. Excellent Code Reuse and Refactoring

• The extraction of session management into internal/session eliminates ~400 lines of duplication
• Shared helper functions in cmd/moat/cli/helpers.go promote DRY principles
• Consistent patterns between Claude and Codex implementations

2. Comprehensive Test Coverage

• internal/credential/openai_test.go: Token classification, validation, expiration handling
• internal/session/session_test.go: Persistence, concurrency, edge cases, security (path traversal)
• internal/codex/generate_test.go: MCP config generation with grants
• cmd/moat/cli/helpers_test.go: Path resolution, env parsing, host validation
• Tests cover edge cases, error paths, and concurrent access

3. Security Considerations

• Session ID validation prevents path traversal attacks (validSessionID regex)
• Atomic write-rename pattern prevents session metadata corruption
• Credential expiration checking in hasCredential()
• Host validation for --allow-host flag
• Proper mutex locking for concurrent session access

4. Good Error Messages

• Clear, actionable error messages (e.g., telling users to run codex login)
• Generic error messages in credential validation to avoid information disclosure
• Validation errors explain exactly what's wrong

🐛 Potential Issues

1. Race Condition in Apple Container StartAttached (Line 919-1000 in apple.go)
The comment mentions "attach before start to avoid race condition" but the implementation has potential timing issues:

// Start attach process
attachCmd := exec.CommandContext(ctx, r.containerBin, "attach", containerID)
if err := attachCmd.Start(); err != nil { ... }

// Brief delay to let attach establish
time.Sleep(100 * time.Millisecond)

// Start the container
cmd := exec.CommandContext(ctx, r.containerBin, "start", containerID)

The 100ms sleep is brittle. If the attach process takes longer to initialize (slow system, high load), the race condition still exists. Consider:

• Using a more robust synchronization mechanism
• Detecting when attach is ready (e.g., checking for specific output/signal)
• Adding retries or exponential backoff

2. Orphaned Process Cleanup (Line 990-995 in apple.go)

if attachProcess != nil && attachProcess.Process != nil {
    if err := attachProcess.Process.Kill(); err != nil {
        log.Debug("failed to kill orphaned attach process", "error", err)
    }
}

The error is only logged. Consider:

• Checking if the process is still running before attempting to kill
• Using a more graceful shutdown (SIGTERM before SIGKILL)
• Tracking orphaned processes for later cleanup

3. Token Classification Logic (IsCodexToken in openai.go:276)
The function uses length (10 chars) as a threshold, but this is fragile:

if len(token) < 10 {
    return false // Too short to be any valid OpenAI token
}

This could incorrectly classify truncated or malformed tokens. Consider:

• Using more robust token format validation
• Checking for JWT structure for subscription tokens (eyJ... prefix)
• Adding explicit token type detection rather than elimination

4. Missing Context Cancellation (getFromKeychain in openai.go:194)

cmd := exec.Command("security", "find-generic-password", ...)
output, err := cmd.Output()

The command doesn't use context, so it can't be canceled. Should use exec.CommandContext.

5. Silent Error Handling (listLocked in session.go:152)

if err != nil {
    slog.Debug("skipping corrupted session", "id", entry.Name(), "error", err)
    continue
}

While logging is good, corrupted sessions indicate a problem. Consider:

• Logging at WARN level instead of DEBUG
• Adding metrics/alerting for corrupted sessions
• Offering a repair/cleanup command

⚠️ Code Quality Concerns

1. Magic Numbers
Several magic numbers lack explanation:

• ttyStartupDelay = 200 * time.Millisecond (attach.go:24)
• time.Sleep(100 * time.Millisecond) in StartAttached
• Timeout of 5*time.Minute for Codex login

Consider adding comments explaining why these specific values were chosen.

2. Incomplete Error Context (helpers.go:69-91)

func hasCredential(provider credential.Provider) bool {
    // Multiple error cases return false
    // Hard to distinguish between "no credential" vs "system error"
}

Callers can't distinguish between "credential doesn't exist" and "error accessing credential store". Consider returning an error or using a result struct.

3. Potential Information Leakage (ValidateKey in openai.go:105)

body, _ := io.ReadAll(resp.Body)
var errResp struct { ... }
_ = json.Unmarshal(body, &errResp)

The error response is read but not used. While generic messages are returned (good!), the code could be clearer about intentionally discarding the detailed error.

🎯 Performance Considerations

1. Sequential Grant Validation (codex_run.go:70-75)

for _, g := range cfg.Grants {
    if grantErr := addGrant(g); grantErr != nil {
        return grantErr
    }
}

Grant validation calls credential.ValidateGrant() sequentially. If this involves I/O, consider validating in parallel.

2. Multiple File Reads in Session List (session.go:150)

for _, entry := range entries {
    session, err := m.loadLocked(entry.Name())
    // Each call reads metadata.json
}

For large session directories, this could be slow. Consider:

• Caching session metadata in memory
• Using a database for session storage
• Adding pagination for session listing

📝 Documentation & Style

1. Missing Package Documentation

• internal/codex/provider.go has no package doc comment
• cmd/moat/cli/helpers.go lacks package-level documentation

2. Inconsistent Naming

• codexKeychainService and codexCredentialsFile are private (good!)
• But naming convention differs from other constants (consider codexKey chainServiceName)

3. Test Helper Function (openai_test.go:382-394)
The contains and containsHelper functions reinvent strings.Contains. Just use the standard library function.

4. Good Conventional Commits Usage ✅
All commits follow the conventional commits format properly.

🔒 Security Review

1. Credential Injection ✅

• Proper use of placeholder tokens
• Network whitelist includes necessary domains
• Proxy-based credential injection prevents leakage

2. Path Traversal Protection ✅

• Session ID validation prevents ../ attacks
• Workspace path resolution validates directories

3. Race Condition Protection ⚠️

• Mutex locking in session manager (good)
• Atomic write-rename for session files (good)
• Apple container attach race (needs improvement)

📊 Test Coverage Assessment

Coverage is Strong:

• Unit tests for all new packages
• Edge cases well covered (empty strings, invalid inputs)
• Concurrent access tested
• Error paths validated

Missing Coverage:

• No E2E tests for Codex integration
• No tests for cmd/moat/cli/codex.go (the cobra command setup)
• Limited integration tests for proxy credential injection with Codex

🚀 Recommendations

High Priority:

1. Fix Apple Container Race Condition: Replace sleep-based synchronization with a more robust mechanism
2. Add Context to Keychain Call: Use exec.CommandContext in getFromKeychain
3. Improve Token Classification: Use JWT structure detection for subscription tokens

Medium Priority:
4. Document magic numbers and timing values
5. Add E2E tests for Codex integration
6. Consider using WARN level for corrupted sessions

Low Priority:
7. Remove custom contains helper in tests, use strings.Contains
8. Add package documentation to new packages
9. Consider parallel grant validation if I/O bound

✨ Overall Assessment

This is a well-executed PR that:

• Follows established patterns
• Includes comprehensive tests
• Has good security practices
• Provides clear error messages
• Properly refactors shared code

The main concerns are around timing/race conditions in the Apple container attach logic and some minor code quality improvements. The core functionality is solid and the test coverage gives confidence in the implementation.

Recommendation: Approve with minor changes requested (fix the race condition and context cancellation issues).

Great work on the refactoring and test coverage! 🎉

[claude]

Pull Request Review: feat(agents): Add Codex support

This is a well-structured PR that adds OpenAI Codex CLI support to moat, following established patterns from the Claude integration. The code quality is generally excellent with good test coverage and thoughtful architecture. Below is my detailed feedback.

---

✅ Strengths

1. Excellent Code Reuse & Refactoring

• The extraction of shared session management to internal/session eliminates ~400 lines of duplication
• Helper functions consolidated in cmd/moat/cli/helpers.go promote DRY principles
• Consistent patterns between Claude and Codex implementations make the codebase more maintainable

2. Strong Test Coverage

• Comprehensive tests for openai.go covering token classification, validation, and edge cases
• Session manager tests include concurrency scenarios and corruption handling
• Helper function tests verify path resolution, env parsing, and host validation
• MCP config generation tests cover various scenarios

3. Security Best Practices

• Credentials never written to container filesystem (proxy injection only)
• Placeholder tokens used to bypass local validation while keeping real tokens secure
• Proper token expiration handling with fallback for ChatGPT subscription tokens
• Input validation for grants, hosts, and environment variables

4. StartAttached Innovation

• Adding StartAttached() to the runtime interface solves a real problem for TUI applications
• Well-documented rationale for why TTY must be attached before process starts
• Properly implemented for both Docker and Apple runtimes with appropriate timing delays

5. Comprehensive Documentation

• Clear docstrings explaining security model and credential flow
• Example directory with working agent.yaml and README
• Good inline comments explaining non-obvious logic (e.g., JWT structure detection)

[claude]

🔍 Issues & Suggestions

Critical Issues

1. Race Condition in Apple Container StartAttached (apple.go:926-1013)

The current implementation has a race condition with a 500ms sleep that's a fragile timing assumption. If the attach command takes longer to start (e.g., under load), the container may start before attach is ready, defeating the purpose of StartAttached.

Recommendation: Use synchronization primitives instead of sleep:

• Have the attach goroutine signal when it's ready (using a channel or sync.WaitGroup)
• Only start the container after receiving the ready signal
• Add a timeout to prevent indefinite blocking

2. Missing Credential Expiration Refresh Logic

The code checks for expired credentials (hasCredential() in helpers.go:86-90) but doesn't handle refresh. For ChatGPT subscription tokens with refresh tokens, there's no automatic refresh mechanism. Users will get opaque errors when tokens expire.

Recommendation:

• Add a credential refresh flow for OAuth tokens with refresh_token
• Or provide clear user-facing error messages explaining how to re-authenticate

High Priority Issues

3. Inconsistent Error Handling in Session Manager (session.go:150-157)

Corrupted sessions are silently skipped with only a warning log. While logged, this could hide serious problems (disk corruption, concurrent access issues).

Recommendation:

• Consider a recovery mechanism or flagging corrupted sessions for user attention
• Add metrics/counters for corrupted sessions to detect patterns
• Provide a moat sessions repair command to help users clean up

4. Token Classification Edge Cases (openai.go:295-329)

The IsCodexToken() function accepts eyJ.a.b as a valid JWT (parts have length > 0 but aren't valid base64).

Recommendation:

• Validate that parts are valid base64
• Consider using a JWT library for proper validation
• Add test cases for malformed JWTs (e.g., eyJ.invalid.., eyJ.a.)

5. Network Validation Doesn't Catch All Invalid Hosts (helpers.go:129-203)

The validateHost() function validates hostname structure but doesn't catch localhost/loopback addresses, private IP ranges, or link-local addresses.

Recommendation:

• If --allow-host is meant to only allow external hosts, add validation to reject private/local IPs
• If local IPs are intentionally allowed, document this in the flag description
• Consider separating --allow-host for external and --allow-local for internal

[claude]

Medium Priority Issues

6. Grant Validation Timing (codex_run.go:71-93)

Invalid grants from config file are only caught at runtime, not during config parsing.

Recommendation:

• Add grant validation during config.Load() to fail fast
• Provide better error context (which config file, which line if possible)

7. Potential Keychain Permission Issues (openai.go:199-222)

Permission denied errors or other keychain issues are silently ignored with fallback to file-based lookup.

Recommendation:

• Log keychain access failures at Debug level
• Distinguish between "not found" (expected) vs "permission denied" (unexpected)

8. Hardcoded Timeout Values

Several timing constants lack configurability:

• codex login timeout: 5 minutes
• Container start delay: 100ms
• TTY startup delay: 200ms
• Attach command delay: 500ms

Recommendation:

• Make these configurable via environment variables or flags for testing/debugging
• Document the rationale for each timeout value

9. Missing Context Propagation (openai.go:195-196)

getFromKeychain() creates a new background context instead of accepting one from caller.

Recommendation:

• Add context parameter to getFromKeychain() or remove the wrapper
• Ensure all I/O operations can be canceled via context

Low Priority / Polish

10. Test Coverage Gaps

Missing test coverage for:

• StartAttached() in both runtimes (race conditions, cancellation)
• Grant command menu loops (non-recursive fix should be tested)
• Codex MCP config validation errors
• Network host validation with IP addresses

Recommendation: Add integration/E2E tests for these scenarios.

[claude]

🎯 Performance & Security

Performance Considerations:

1. Repeated Credential Checks - hasCredential() reads from disk on every call. For commands that check multiple grants, this could be optimized with caching.

2. Session List Sorting - List() loads all sessions into memory. For large numbers of sessions, consider pagination, filesystem-level sorting, or lazy loading.

---

🔐 Security Assessment

Overall: Strong security model

✅ Good:

• Credentials never written to container filesystem
• Proxy-based injection prevents agent from seeing raw tokens
• Token validation before storage
• Atomic file writes prevent corruption
• Input validation on all user-provided data

⚠️ Monitor:

• Expired token handling (mentioned above)
• Keychain permission errors (silent fallback)
• Private IP ranges in --allow-host (if unintended)

---

📝 Documentation & Style

Adherence to CLAUDE.md:

• ✅ Uses Conventional Commits format
• ✅ Good error messages with actionable guidance
• ✅ Generic placeholder names in examples
• ✅ No Co-Authored-By lines
• ✅ Documentation matches actual behavior

Code style:

• Consistent Go formatting throughout
• Good use of constants for magic values
• Clear separation of concerns across packages

---

🧪 Test Quality

Strong test suite overall:

• Good coverage of happy paths and error cases
• Edge case handling (empty strings, whitespace, invalid tokens)
• Concurrent access testing in session manager
• Proper use of t.TempDir() for isolation

Suggestions:

• Add table-driven tests for grant validation
• Test the full end-to-end flow with a mock Codex CLI
• Add benchmarks for session operations with large datasets

[claude]

📊 Summary

Category	Rating	Notes
Code Quality	⭐⭐⭐⭐⭐	Excellent structure, DRY, well-organized
Test Coverage	⭐⭐⭐⭐	Comprehensive unit tests, needs more integration tests
Security	⭐⭐⭐⭐	Strong model, minor improvements possible
Documentation	⭐⭐⭐⭐⭐	Clear, thorough, matches implementation
Performance	⭐⭐⭐⭐	Good for typical use, optimization opportunities exist

Recommendation: Approve with requested changes for critical issues (#1, #2).

---

🔧 Action Items

Before Merge (Critical):

1. Fix Apple container StartAttached race condition
2. Add credential expiration/refresh handling or clear error messages

Follow-up PRs (Recommended):
3. Improve session corruption handling
4. Enhance token classification validation
5. Add network validation for private IPs
6. Add integration tests for StartAttached
7. Make timeout values configurable

---

Great work on this PR! The architecture is solid, the implementation is clean, and the test coverage is strong. The refactoring to extract shared session management is particularly well done. Once the critical race condition is addressed, this will be an excellent addition to the codebase.

[dpup]

Remaining flagged issues are acceptable for now.