Task Creation Enhancement: Support for ContextWindow (or userMessage)

[allisoneer]

Summary

Implements the ability to create tasks with either userMessage OR contextWindow, but not both.
This enhancement allows for more flexible task creation, supporting both simple queries and
multi-turn conversations.

Changes

• Modified TaskSpec to make UserMessage optional and added optional ContextWindow field
• Created a validation package with shared validation functions for consistent checks across API and controller
• Updated API handler to validate and support the new structure
• Enhanced controller to properly handle both input methods
• Added test coverage for all new functionality
• Created sample YAML example for contextWindow usage

Validation Rules

1. Exactly one of userMessage or contextWindow must be provided
2. If using contextWindow:
   • All messages must have valid roles (system, user, assistant, tool)
   • At least one user message must be present
   • If no system message exists, the agent's system prompt will be prepended

Behavior

When contextWindow is used, the controller:

• Preserves all provided messages
• Prepends the agent's system message if none exists in the context
• Uses the last user message for the UserMsgPreview field
• Copies the entire context to status.ContextWindow

Testing

• Unit tests for all validation scenarios
• End-to-end validation with controller tests
• API validation tests for input combinations

---

Important

Enhance task creation to support either userMessage or contextWindow, with validation and comprehensive test coverage.

• Behavior:
  • TaskSpec now supports userMessage or contextWindow, but not both.
  • contextWindow must have valid roles and at least one user message.
  • If no system message in contextWindow, agent's system prompt is prepended.
• Validation:
  • New validation package with ValidateTaskMessageInput() and GetUserMessagePreview().
  • Validation ensures only one of userMessage or contextWindow is provided.
• Controller:
  • task_controller.go updated to handle contextWindow logic.
  • prepareForLLM() sets up initial context and phase.
• API:
  • server.go updated to validate task creation requests.
  • createTask() checks for valid userMessage or contextWindow.
• Testing:
  • New tests in task_controller_test.go for various scenarios.
  • API tests in server_test.go for task creation validation.
• Documentation:
  • Updated README.md and crd-reference.md to reflect changes.
  • Sample YAML for contextWindow added in acp_v1alpha1_context_window_task.yaml.

^{This description was created by for 7e63816. It will automatically update as commits are pushed.}

[allisoneer]

I can hear dex providing feedback to me already, so before this is out of draft state, I will:

• move validation stuff to the utils folder. That's already there, is probably the right place for this stuff.
• update documentation (readme and anywhere else)
• test locally/manually with both options, and test the API as well.

[ellipsis-dev]

❌ Changes requested. Reviewed everything up to 7e63816 in 2 minutes and 37 seconds

More details

• Looked at 724 lines of code in 14 files
• Skipped 0 files when reviewing.
• Skipped posting 9 drafted comments based on config settings.

1. acp/test/utils/task.go:15

• Draft comment:
  TestTask now supports both userMessage and contextWindow. Make sure tests cover the case when only one is set.
• Reason this comment was not posted:
  Confidence changes required: 50% <= threshold 50%
  None

2. acp/internal/controller/task/task_controller.go:192

• Draft comment:
  When prepending the system message in prepareForLLM, consider verifying that agent.Spec.System is non-empty. An empty system message might lead to an uninformative context window.
• 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 the suggestion seems reasonable at first glance, there are several factors to consider: 1) The agent was already validated earlier in validateTaskAndAgent 2) If agent validation is required, it should be done at the agent level, not here 3) An empty system message, while not ideal, won't break anything 4) The comment is speculative ("might lead to") rather than pointing out a definite issue.
  I might be wrong about agent validation - I don't actually see where agent.Spec.System is validated. And an empty system message could potentially cause issues with some LLM providers.
  However, this is still a speculative comment about a hypothetical issue. If system message validation is needed, it should be handled at the agent level or in the validation package, not here. This function's responsibility is just to prepare the context window.
  The comment should be deleted because it's speculative and suggests validation that belongs elsewhere in the codebase if needed.

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

• Draft comment:
  Good job validating input using ValidateTaskMessageInput. Consider logging the error details before returning so that troubleshooting API input issues is easier.
• Reason this comment was not posted:
  Confidence changes required: 30% <= threshold 50%
  None

4. acp/test/utils/task.go:23

• Draft comment:
  The helper functions for setting up and tearing down test tasks are clear. Ensure that any future updates to task spec fields are mirrored here to keep test utilities in sync.
• Reason this comment was not posted:
  Confidence changes required: 10% <= threshold 50%
  None

5. acp/internal/controller/task/task_controller.go:174

• Draft comment:
  The revised prepareForLLM function cleanly handles 'userMessage' vs 'contextWindow'. The use of GetUserMessagePreview to set a preview is good; ensure that any edge cases (e.g., very long messages) are covered in tests.
• Reason this comment was not posted:
  Confidence changes required: 20% <= threshold 50%
  None

6. acp/README.md:26

• Draft comment:
  Consider changing 'permission' to 'permissions' in the sentence "Make sure you have the proper permission to the registry if the above commands don’t work." to ensure correct pluralization.
• 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.

7. acp/README.md:50

• Draft comment:
  Consider revising 'Ensure that the samples has default values to test it out.' to 'Ensure that the samples have default values to test them out.' for proper subject-verb agreement.
• 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.

8. acp/README.md:65

• Draft comment:
  Consider changing 'UnDeploy' to 'Undeploy' in the header 'UnDeploy the controller from the cluster:' for consistency with the make target 'undeploy' and standard capitalization.
• 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.

9. acp/config/example-resources.md:198

• Draft comment:
  There is an unnecessary backslash before the exclamation mark in "Happy deploying!" on line 198. Consider removing it to correct this typographical error.
• 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.

Workflow ID: wflow_hY5fRIrtxH6EBsmD

---

Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

[allisoneer]

Last update:

• realized utils folder is in the test directory so did not move it
• updated documentation
• tested locally and works great!

[dexhorthy]

does it have to end with a user message?

if i POST a contextWindow with tool calls, do the tool calls get created or are they assumed to exist?

not suggesting any new implementation, just clarity in the docs what should be expected. It can have a "for now" on it for now

[allisoneer]

Updated docs to reflect current status:

1. Tool calls/messages in the contextWindow are NOT executed - they're treated purely as conversation history
2. The last message can be any valid role (user, assistant, tool) - the controller will send the entire context to the
   LLM
3. The UserMsgPreview comes from the last user message, not necessarily the last message in the context

A potential improvement might be to require it to end with a user message. I think openai might still respond properly if most recent is a presumed-done "tool" but the api might get upset if it's another assistant.