-
Notifications
You must be signed in to change notification settings - Fork 51
fix(tasks): omit params from the list-tasks response
#377
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -74,6 +74,49 @@ class TaskResponse(Task): | |
| ) | ||
|
|
||
|
|
||
| class TaskSummary(BaseModel): | ||
| """Lean list-response shape. Omits `params` (the arbitrary create-time | ||
| payload, which can carry per-caller secrets and PII); fetch GET /tasks/{id} | ||
| for the full record.""" | ||
|
|
||
| id: str = Field( | ||
| ..., | ||
| title="Unique Task ID", | ||
| ) | ||
| name: str | None = Field( | ||
| None, | ||
| title="Unique name of the task", | ||
| ) | ||
| status: TaskStatus | None = Field( | ||
| None, | ||
| title="The current status of the task", | ||
| ) | ||
| status_reason: str | None = Field( | ||
| None, | ||
| title="The reason for the current task status", | ||
| ) | ||
| created_at: datetime | None = Field( | ||
| None, | ||
| title="The timestamp when the task was created", | ||
| ) | ||
| updated_at: datetime | None = Field( | ||
| None, | ||
| title="The timestamp when the task was last updated", | ||
| ) | ||
| cleaned_at: datetime | None = Field( | ||
| None, | ||
| title="The timestamp when the task's content was cleaned for retention compliance; null when active", | ||
| ) | ||
| task_metadata: dict[str, Any] | None = Field( | ||
| None, | ||
| title="Task metadata", | ||
| ) | ||
| agents: list["Agent"] | None = Field( | ||
| default=None, | ||
| title="Agents associated with this task (only populated when 'agents' view is requested)", | ||
| ) | ||
|
Comment on lines
+114
to
+117
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Shouldn't this be maintained on the agent side instead? The agent should know which tasks its writing to or able to write to instead of the task maintaining which agents are associated to it.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This field isn't introduced here. |
||
|
|
||
|
|
||
| class UpdateTaskRequest(BaseModel): | ||
| task_metadata: dict[str, Any] | None = Field( | ||
| None, | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe an integration test would be nice to make sure the fields we deem "sensitive" are omitted (in the case that someone adds them back later). Can be followup
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is already covered in the PR:
test_list_tasks_omits_params_in_responseandtest_list_tasks_omits_params_for_null_params_taskassertparamsis absent from list items (populated and null cases), andtest_get_task_by_id_includes_params_in_responseverifies the single-task fetch still returns it. If you want the regression caught one layer earlier, I can also add a model-level guard asserting"params" not in TaskSummary.model_fields.