Llm update

[allisoneer]

Important

This PR enhances namespace management and LLM configuration in the server, adding RBAC roles and improving error handling and testing.

• RBAC Configuration:
  • Added namespace_permissions_patch.yaml and namespace_permissions_binding.yaml to kustomization.yaml.
  • New ClusterRole and ClusterRoleBinding for namespace management.
• Server Logic:
  • Introduced LLMDefinition struct in server.go for detailed LLM configuration.
  • Updated createAgent and createTask to ensure namespaces exist before proceeding.
  • Added ensureNamespaceExists() to handle namespace creation.
  • Enhanced error handling and resource cleanup in createAgent.
• Testing:
  • Added tests for namespace auto-creation in server_test.go.
  • Validated LLM fields and provider in createAgent.
  • Tested error handling for namespace creation failures.

^{This description was created by for 722ff02. You can customize this summary. It will automatically update as commits are pushed.}

[ellipsis-dev]

Important

Looks good to me! 👍

Reviewed everything up to 722ff02 in 2 minutes and 20 seconds. Click for details.

• Reviewed 762 lines of code in 5 files
• Skipped 0 files when reviewing.
• Skipped posting 5 draft comments. View those below.
• Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.

1. acp/internal/server/server.go:264

• Draft comment:
  Check pre‐existing LLM conflict: When an LLM with the given name already exists, a 409 is returned. In the auto‐namespace test an LLM is pre‐created so the endpoint may fail unexpectedly. Consider reusing the existing LLM instead of erroring.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50% This is a design suggestion about error handling behavior. While the comment identifies a potential issue with test environments, changing error handling to silently reuse existing resources instead of erroring could mask real conflicts in production. The current behavior of returning 409 on conflicts is a standard REST API pattern. Without more context about the testing requirements and production use cases, this change could introduce subtle bugs. The comment raises a valid point about test environment compatibility. However, modifying core API behavior to accommodate tests may not be the right solution. The proper solution may be to modify the tests to handle 409 responses appropriately or clean up pre-existing resources, rather than changing the API's conflict handling behavior. The comment should be deleted as it suggests a potentially problematic change to core API behavior without strong justification. Test issues should be handled in the test code.

2. acp/internal/server/server.go:830

• Draft comment:
  LLM type inconsistency in updateAgent: UpdateAgentRequest uses a string for LLM while CreateAgentRequest uses a structured LLMDefinition. Align these types to avoid confusion.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

3. acp/internal/server/server.go:691

• Draft comment:
  MCP server key extraction is brittle: Splitting the server name by '-' may break if agent names include dashes. Consider a more robust naming scheme or delimiter.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

4. acp/internal/server/server.go:775

• Draft comment:
  Optimize provider lookup: In validateLLMProvider, the valid providers are checked by a loop. Using a map or a set would be more efficient as the list grows.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50% While technically correct that a map lookup would be O(1) vs O(n) for the loop, this is a very small list of just 5 items. The performance difference would be negligible. The current implementation is also more readable and maintainable. The optimization would add complexity for virtually no benefit. The comment is technically accurate - a map would be more efficient. And if the list of providers grew very large in the future, this could become more important. However, with only 5 items, the performance difference is insignificant. Premature optimization would make the code more complex for no practical benefit. This comment suggests an optimization that would add complexity while providing no meaningful performance benefit given the small fixed size of the list.

5. acp/internal/server/server_test.go:673

• Draft comment:
  Typo: There's an extra trailing whitespace after the string "openai". Consider removing the trailing space.
• Reason this comment was not posted:
  Comment looked like it was already resolved.

Workflow ID: wflow_V2Y9YiiJ9KB22fJY

^{You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.}

[dexhorthy]

needs to just proceed quietly