Skip to content

[dotnet-port] Port TodoList_Complete reason requirement from .NET - #199

Merged
qmuntal merged 6 commits into
mainfrom
dotnet-port/todo-complete-with-reason-8f4efb574b9292e1
May 20, 2026
Merged

[dotnet-port] Port TodoList_Complete reason requirement from .NET#199
qmuntal merged 6 commits into
mainfrom
dotnet-port/todo-complete-with-reason-8f4efb574b9292e1

Conversation

@qmuntal

@qmuntal qmuntal commented May 19, 2026

Copy link
Copy Markdown
Member

…#5902

Port the upstream .NET change that replaces the plain-ID slice in TodoList_Complete with a structured CompleteInput{ID, Reason} slice. Agents are now prompted to include a completion reason, which improves auditability of todo lifecycle events.

Changes:

  • Add CompleteInput struct (id + reason fields)
  • Update completeTool to accept []CompleteInput instead of []int
  • Update tool description to mention the reason field
  • Update default instructions to ask agents to include a reason
  • Update all existing tests to use the new input format
  • Add TestCompleteTodos_WithReason and TestCompleteToolDescription_MentionsReason

…#5902

Port the upstream .NET change that replaces the plain-ID slice in
TodoList_Complete with a structured CompleteInput{ID, Reason} slice.
Agents are now prompted to include a completion reason, which improves
auditability of todo lifecycle events.

Changes:
- Add CompleteInput struct (id + reason fields)
- Update completeTool to accept []CompleteInput instead of []int
- Update tool description to mention the reason field
- Update default instructions to ask agents to include a reason
- Update all existing tests to use the new input format
- Add TestCompleteTodos_WithReason and TestCompleteToolDescription_MentionsReason

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@qmuntal
qmuntal marked this pull request as ready for review May 19, 2026 13:00
@qmuntal
qmuntal requested a review from a team as a code owner May 19, 2026 13:00
Copilot AI review requested due to automatic review settings May 19, 2026 13:00
@github-actions

This comment has been minimized.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Ports the upstream .NET change to the todo harness so TodoList_Complete takes structured completion entries (ID + reason), prompting agents to include a completion reason for better auditability.

Changes:

  • Introduces CompleteInput { id, reason } and updates TodoList_Complete to accept []CompleteInput.
  • Updates default instructions and tool description to explicitly request a completion reason.
  • Updates existing tests to use the new input shape and adds tests for the updated description / acceptance.
Show a summary per file
File Description
agent/harness/todo/todo.go Adds CompleteInput and updates TodoList_Complete schema/description/instructions to include a completion reason.
agent/harness/todo/todo_test.go Migrates tool-call JSON payloads to the new {id, reason} format and adds reason/description-focused tests.

Copilot's findings

Tip

Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

  • Files reviewed: 2/2 changed files
  • Comments generated: 2

Comment thread agent/harness/todo/todo.go
Comment thread agent/harness/todo/todo_test.go
@qmuntal

qmuntal commented May 19, 2026

Copy link
Copy Markdown
Member Author

@copilot fix codereview feedback,

@qmuntal
qmuntal enabled auto-merge (squash) May 19, 2026 13:42

Copilot AI commented May 19, 2026

Copy link
Copy Markdown
Contributor

@copilot fix codereview feedback,

Done in the latest commit. Both pieces of feedback are addressed:

  1. The TodoList_Complete handler now validates each entry upfront — strings.TrimSpace(item.Reason) == "" returns an error before any state mutation, so callers cannot slip past an empty or whitespace-only reason.
  2. TestCompleteTodos_EmptyReasonIsRejected covers the empty-string and whitespace-only cases and asserts both that an error is returned and that the item is not marked complete.

@github-actions

This comment has been minimized.

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generated by Go API Consistency Review Agent for issue #199 · ● 3.2M

Comment thread agent/harness/todo/todo.go Outdated
@qmuntal

qmuntal commented May 19, 2026

Copy link
Copy Markdown
Member Author

@copilot fix #199 (comment)

Copilot AI commented May 19, 2026

Copy link
Copy Markdown
Contributor

@copilot fix #199 (comment)

Removed the strict TrimSpace validation and the corresponding negative test to align with the upstream .NET TodoProvider, which accepts an empty/omitted reason without error. Added TestCompleteTodos_EmptyReasonIsAccepted to explicitly document this intentional parity with .NET.

@github-actions

This comment has been minimized.

@qmuntal

qmuntal commented May 20, 2026

Copy link
Copy Markdown
Member Author

@copilot fix golangci-lint

Copilot AI commented May 20, 2026

Copy link
Copy Markdown
Contributor

@copilot fix golangci-lint

Fixed — the gofumpt violation was extra alignment spaces in the struct literal in TestCompleteTodos_EmptyReasonIsAccepted. Reformatted with gofumpt.

@github-actions

Copy link
Copy Markdown
Contributor

Cross-repo Parity Review ✅

This PR faithfully ports the upstream .NET change (#5902) and stays aligned with the upstream implementation.

Checked against microsoft/agent-framework:

Surface .NET (dotnet/src/Microsoft.Agents.AI/Harness/Todo/) Go (agent/harness/todo/todo.go)
Input type name TodoCompleteInput (internal) CompleteInput (exported, todo.CompleteInput)
JSON field names "id", "reason" "id", "reason"
Tool description "Each entry has an ID and a reason describing how/why the item was completed." Identical wording ✅
Default instructions "Include a reason describing how the items were completed." Identical wording ✅

Python: No TodoProvider equivalent exists in the upstream Python packages — no Python parity gap.

Note on visibility: The .NET type is internal, while the Go type is exported. This is intentional — Go's reflection-based tool argument binding requires exported types for correct JSON deserialization. No parity concern here.

No cross-repo consistency issues found.

Generated by Go API Consistency Review Agent for issue #199 · ● 9.2M ·

@qmuntal
qmuntal merged commit cb4d935 into main May 20, 2026
23 of 25 checks passed
@qmuntal
qmuntal deleted the dotnet-port/todo-complete-with-reason-8f4efb574b9292e1 branch May 20, 2026 13:26
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.

[dotnet-port] Port TodoList_Complete reason requirement from .NET

4 participants