Skip to content

Conversation

@yimsk
Copy link
Contributor

@yimsk yimsk commented Jan 7, 2026

Summary

Closes #123

Adds AI chat feature using Amazon Bedrock Converse API - a "self-hosted Amazon Q Developer" experience.

Changes

  • internal/ai/bedrock.go: Bedrock Converse API client with streaming
  • internal/ai/tools.go: 6 tools (list_services, list_resources, query_resources, get_resource_detail, tail_logs, search_aws_docs)
  • internal/ai/session.go: Chat session persistence (~/.config/claws/chat/sessions/)
  • internal/view/chat_overlay.go: Modal UI (viewport + textinput)
  • internal/config/file.go: AIConfig (model, max_sessions)
  • internal/app/app.go: A keybind opens chat overlay
  • View getters for context awareness

Usage

  • Press A to open chat overlay
  • AI knows current service/resource/log group context
  • Configure model in ~/.config/claws/config.yaml:
    ai:
      model: "global.anthropic.claude-haiku-4-5-20251001-v1:0"
      max_sessions: 10

Testing

  • Build passes
  • Lint passes
  • Tests pass
  • Manual testing needed

- internal/ai/: bedrock client, tools (6), session persistence
- chat_overlay.go: modal UI with viewport + textinput
- Context-aware: knows current service/resource/log group
- Config: ai.model, ai.max_sessions in config.yaml
- Keybind: A opens chat overlay
@claude
Copy link

claude bot commented Jan 7, 2026

Pull Request Review: Add AI Chat Overlay with Bedrock Converse API

Summary

This PR adds a comprehensive AI chat feature using Amazon Bedrock's Converse API. Overall, this is a well-structured addition with good separation of concerns.


Code Quality & Best Practices

Strengths

  1. Good separation of concerns - well-organized into logical packages
  2. Proper use of Go patterns (functional options, context propagation, error wrapping)
  3. Consistent with project architecture
  4. Clean UI integration with existing Bubbletea patterns

Areas for Improvement

1. Error Handling - Silent Failures (bedrock.go:218-220, 266, 289-291)

JSON marshal/unmarshal errors are silently ignored which could corrupt tool input data.
Recommendation: Log or handle errors explicitly.

2. Session ID Collision Risk (session.go:248-251)

Using UnixNano()%1000000 (only 6 digits) creates high collision risk.
Recommendation: Use full timestamp or crypto/rand for uniqueness.

3. Unbounded Tool Execution Loop (chat_overlay.go:220-233)

No max iteration limit - misbehaving model could create infinite loop.
Recommendation: Add maxToolIterations constant (e.g., 10).

4. Resource Leaks - Goroutine Management (bedrock.go:104-115)

processStream goroutine has no context cancellation after ConverseStream returns.
Recommendation: Pass context to processStream and monitor ctx.Done().

5. Type Assertion Without Check (tools.go:164)

Silent fallback to 0 on invalid input types.
Recommendation: Validate type assertion results.


Security Concerns

1. Sensitive Data in Session Files (session.go:194-202, 222)

Session files use 0644 permissions - world-readable.
Recommendation: Change to 0600 for both session files and current.json.

2. Input Validation (tools.go:252-275)

Log group names and filters passed to AWS API without validation.
Recommendation: Add basic validation (empty check, length limits).

3. No Rate Limiting

Tools make AWS API calls without rate limiting - could exhaust quotas.
Recommendation: Consider rate limiting for tail_logs and resource queries.


Performance Considerations

1. Non-Streaming UI (chat_overlay.go:203-236)

Uses Converse() instead of ConverseStream() - poor UX for long responses.
Recommendation: Use ConverseStream() for incremental updates.

2. Session Pruning on Every Save (session.go:75-85)

Full directory scan on every new session - O(n) file I/O.
Recommendation: Prune periodically or optimize to check file count first.

3. Sequential Tool Execution (chat_overlay.go:221-225)

Multiple tools execute sequentially instead of parallel.
Recommendation: Execute independent tools concurrently.

4. Resource Truncation (tools.go:227-231)

Hard 50 resource limit severely restricts AI capability.
Recommendation: Smarter pagination or truncation strategy.


Test Coverage

CRITICAL: No Tests

PR adds 1,386 lines with zero test coverage.

Missing tests for:

  • Session ID uniqueness
  • Tool execution edge cases
  • JSON marshaling error paths
  • Tool iteration limits
  • Streaming event processing
  • Session persistence

Recommendation: Add unit tests for at least:

  • generateSessionID() uniqueness
  • Execute() with invalid inputs
  • Session save/load round-trips

Summary & Verdict

Must Fix (Blocking):

  1. Add test coverage - at least basic unit tests
  2. Fix session ID collision risk
  3. Add tool iteration limit
  4. Fix file permissions (0600)

Should Fix (Recommended):

  1. Handle JSON marshaling errors
  2. Add context cancellation
  3. Use streaming UI
  4. Add input validation

Nice to Have:

  1. Parallel tool execution
  2. Rate limiting / cost controls
  3. Better resource truncation
  4. Documentation (godoc, IAM permissions)

Great work on this feature! Once the Must Fix items are addressed, this will be an excellent addition.

@yimsk yimsk closed this Jan 7, 2026
@yimsk yimsk deleted the feature/123-ai-chat branch January 7, 2026 23:19
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.

2 participants