Skip to content

[dotnet-port-api] Add TodoProvider session helpers - #1003

Merged
Quim Muntal (qmuntal) merged 3 commits into
mainfrom
dotnet-port-api-todo-session-helpers-c7ebc42e6a54636e
Sep 9, 2026
Merged

[dotnet-port-api] Add TodoProvider session helpers#1003
Quim Muntal (qmuntal) merged 3 commits into
mainfrom
dotnet-port-api-todo-session-helpers-c7ebc42e6a54636e

Conversation

@michelle-clayton-work

Copy link
Copy Markdown
Contributor

Summary

Add session-first TodoProvider read helpers in Go with GetAllTodos(*agent.Session) and GetRemainingTodos(*agent.Session), while keeping the existing option-based helpers as compatibility wrappers. This ports the narrow public API portion of the upstream .NET todo provider graduation work so the Go harness exposes direct session-based todo reads, and it updates focused tests plus the parity comparison doc.

Ported .NET PRs

Breaking Changes

No. Existing GetAllItems and GetRemainingItems helpers remain available; this change only adds the session-first exported helpers.

Tests and Examples

  • go test ./agent/harness/todo
  • go test -race ./agent/harness/todo
  • Added focused tests for session-based todo reads and nil-session behavior.
  • No examples updated; this upstream change was a narrow helper API addition.

Notes

  • Skipped the broader AgentMode portion of upstream PR #7052 because open tracking issue [dotnet-port-api] Align agentmode session helpers #568 already covers that area.
  • Left the remaining graduation work from #7052 and larger harness changes for separate PRs to keep this nightly port small and reviewable.

Generated by .NET to Go API Porting Agent · 599.5 AIC · ⌖ 58 AIC · ⊞ 21.7K ·

Closes #575

Port the session-first TodoProvider read helpers from upstream .NET so Go exposes direct session-based access alongside the existing option-based helpers.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot AI lite review requested due to automatic review settings September 4, 2026 22:29
@github-actions github-actions Bot added area:agent Changes files in the agent area size:large At most 300 changed lines across at most 10 files labels Sep 4, 2026

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.

🔵 Needs a closer look

loadState dereferences a nil session and can panic (breaking the new nil-session test and any invocation without WithSession).

Pull request overview

This PR ports the session-first TodoProvider read-helper API from the upstream .NET implementation by adding exported GetAllTodos(*agent.Session) / GetRemainingTodos(*agent.Session) methods, while keeping the existing option-based helpers as compatibility wrappers.

Changes:

  • Add session-first todo read helpers (GetAllTodos, GetRemainingTodos) and refactor existing option-based helpers to delegate.
  • Refactor internal state access/locking to take *agent.Session directly and reuse in provider/tool code paths.
  • Add focused tests for session-based reads (including nil-session behavior) and update the parity comparison doc.
File summaries
File Description
agent/harness/todo/todo.go Adds session-first public read helpers and refactors state access to use *agent.Session.
agent/harness/todo/todo_test.go Adds coverage for new session-first helpers and nil-session behavior; extends race test to call new helpers.
docs/dotnet-go-sdk-feature-comparison.md Notes parity improvement: todo provider now exposes session-first read helpers.
Review details

Suppressed comments (1)

agent/harness/todo/todo.go:155

  • loadState now dereferences session without checking for nil (session.Get), which will panic when callers pass nil or omit WithSession. This breaks the newly added nil-session test and can crash provide()/tool closures when invoked without a session option; return an empty state when session is nil to preserve the previous behavior.
func (p *Provider) loadState(session *agent.Session) *state {
	var s state
	if found, _ := session.Get(stateKey, &s); found {
		return &s
	}
  • Files reviewed: 3/3 changed files
  • Comments generated: 0
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

@github-actions github-actions Bot added kind:code Changes production behavior or code kind:docs Changes documentation or comments kind:tests Changes tests, fixtures, or test infrastructure pending-auto-risk Automatic risk classification is in progress risk:medium Contained production impact requiring normal review depth and removed pending-auto-risk Automatic risk classification is in progress labels Sep 4, 2026
…session-helpers-c7ebc42e6a54636e

# Conflicts:
#	agent/harness/todo/todo.go
#	docs/dotnet-go-sdk-feature-comparison.md
@github-actions

This comment has been minimized.

@github-actions github-actions Bot added public-api-change Pull Request changes public APIs pending-auto-risk Automatic risk classification is in progress risk:medium Contained production impact requiring normal review depth and removed risk:medium Contained production impact requiring normal review depth pending-auto-risk Automatic risk classification is in progress labels Sep 5, 2026

@qmuntal Quim Muntal (qmuntal) left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Don't care about back-compat for now. Remove GetAllItems and GetRemainingItems. Also, don't use the Get prefix, it is not idiomatic.

Comment thread agent/harness/todo/todo.go Outdated
return remaining
}

func copyItems(items []Item) []Item {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

use slices.Clone and remove copyItems

Comment thread agent/harness/todo/todo.go Outdated
return result
}

func sessionFromOptions(opts []agent.Option) *agent.Session {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Inline this function, that add value by iself.

Signed-off-by: michelle-clayton-work <mclayton+github@microsoft.com>
@github-actions github-actions Bot added pending-auto-risk Automatic risk classification is in progress and removed risk:medium Contained production impact requiring normal review depth labels Sep 9, 2026
@github-actions

github-actions Bot commented Sep 9, 2026

Copy link
Copy Markdown
Contributor

Scope: public API, user-visible behavior

Changed Go contract: agent/harness/todo/todo.go — the final revision (c708a5f) exposes (*todo.Provider).AllTodos(session *agent.Session) []Item and (*todo.Provider).RemainingTodos(session *agent.Session) []Item, and removes the previously public GetAllItems(opts ...agent.Option) / GetRemainingItems(opts ...agent.Option) helpers entirely (no compatibility wrappers remain, despite the PR description saying they would be kept).

Upstream evidence reviewed: dotnet/src/Microsoft.Agents.AI/Harness/Todo/TodoProvider.cs at upstream commit e57f046d8a248869fad1a2c0ea6761345efce852 (microsoft/agent-framework#7052) — GetAllTodosAsync(AgentSession session, CancellationToken) and GetRemainingTodosAsync(AgentSession session, CancellationToken). Also compared against the existing Go sibling harness provider agent/harness/agentmode/agentmode.go (GetMode(opts ...agent.Option) string) for in-repo naming precedent.

Result: findings reported.

  1. Breaking change contradicts PR description: The PR body states "Existing GetAllItems and GetRemainingItems helpers remain available; this change only adds the session-first exported helpers" and "Breaking Changes: No." The actual diff removes both methods and changes the signature/semantics of item access, which is a breaking public API removal, not an addition-only change.
  2. Naming diverges from upstream and in-repo precedent: Upstream's session-first methods are GetAllTodosAsync/GetRemainingTodosAsync (the Get prefix is retained, only the Async suffix is Go-idiomatically dropped). The Go PR instead drops the Get prefix (AllTodos/RemainingTodos), which breaks the naming parallel with upstream and with the existing agentmode.GetMode helper in the same harness package family.

Suggested resolution: keep GetAllItems/GetRemainingItems as thin compatibility wrappers as originally described, or rename the new methods to GetAllTodos/GetRemainingTodos to match both upstream and the agentmode.GetMode convention, and correct the PR description if a breaking change is intended.

Doc update in docs/dotnet-go-sdk-feature-comparison.md and the nil-session handling are otherwise consistent with upstream intent and were not re-flagged (nil-tolerant reads pre-date this PR and are a reasonable Go-idiom divergence from .NET's ArgumentNullException).

Generated by Go API Consistency Review Agent for #1003 · copilot · auto · 76 AIC · ⌖ 5.84 AIC · ⊞ 9.6K ·

@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 #1003 · copilot · auto · 76 AIC · ⌖ 5.84 AIC · ⊞ 9.6K

Comment thread agent/harness/todo/todo.go
@github-actions github-actions Bot added the risk:high Large blast radius, difficult rollback, or sensitive behavior label Sep 9, 2026
@github-actions github-actions Bot removed the pending-auto-risk Automatic risk classification is in progress label Sep 9, 2026
@qmuntal
Quim Muntal (qmuntal) added this pull request to the merge queue Sep 9, 2026
Merged via the queue into main with commit c08e9eb Sep 9, 2026
36 checks passed
@qmuntal
Quim Muntal (qmuntal) deleted the dotnet-port-api-todo-session-helpers-c7ebc42e6a54636e branch September 9, 2026 18:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:agent Changes files in the agent area kind:code Changes production behavior or code kind:docs Changes documentation or comments kind:tests Changes tests, fixtures, or test infrastructure public-api-change Pull Request changes public APIs risk:high Large blast radius, difficult rollback, or sensitive behavior size:large At most 300 changed lines across at most 10 files

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[dotnet-port-api] Add TodoProvider session helpers

3 participants