Repository navigation
feat(todo): add shared: team to scope a todo list to one team - #4782
steilerDev wants to merge 4 commits into
Conversation
aheritier
added
area/config
|
@steilerDev could you configure Claude code to not co-author your commits please? We don't care about the harness used (even if docker-agent is the best). Thanks |
aheritier
left a comment
aheritier
left a comment
There was a problem hiding this comment.
Reviewed config parsing/migration, native and browser todo creation, team-scope propagation, and session lifetimes against the PR description. See inline comments.
CI operational status: verified gate SUCCESS at 4ecf631. The PR is non-draft and conflict-free; GitHub reports merge state BLOCKED.
Issue context: needs-decision—no linked GitHub/Jira acceptance criteria were provided or discovered. The PR description supplied the review checklist; all PR comments, reviews, and threads were retrieved.
Validation: reviewed source at the verified head through GitHub. Local tests were not run because the checkout is at a different commit.
| "random": teamloader.Creator(random.CreateToolSet), | ||
| "todo": teamloader.CreatorFromToolset(func(ts latest.Toolset) (tools.ToolSet, error) { | ||
| if ts.Shared { | ||
| if ts.Shared != latest.SharedScopeNone { |
There was a problem hiding this comment.
[blocking] Keep team-scoped and legacy shared todos separate in the browser. Confidence: high.
Here is the failing case: within one browser registry, create a todo toolset with SharedScopeTeam and another with SharedScopeProcess. Both return the same sharedTodos() instance, so a todo created through either is visible and mutable through the other. Native todo.Creator keeps those stores separate, and TestCreator_SharedScopes explicitly asserts that a team instance differs from a process instance.
Handle SharedScopeTeam through the context-aware team scope, retaining the existing browser behavior for SharedScopeProcess. Add a browser regression test covering both values together; the existing browser test exercises only SharedScopeProcess.
There was a problem hiding this comment.
Fixed in 39631af: the browser todo creator now sends team through tools.TeamScoped and keeps true on the per-registry sharedTodos(). Added a cmd/wasm test that uses both values in one registry and checks they stay separate; I confirmed it fails against the previous creator.
| | Value | Todo list is shared by | | ||
| | ---------------- | ------------------------------------------------------------------------------------------------------------ | | ||
| | `false` | nothing: each agent has its own list | | ||
| | `team` | the agents of one team: root, its sub-agents and background agents. Each session or tab has its own list | |
There was a problem hiding this comment.
[should-fix] Describe isolation per team load, not unconditionally per session. Confidence: high.
The new scope is created only in LoadWithConfig, and its todo instance is retained by the loaded agents. In serve a2a, pkg/a2a/server.go:66 loads one team at startup and pkg/a2a/adapter.go:102–119 creates distinct sessions/runtimes using that same team. Likewise, pkg/mcp/server.go:278–280 creates a fresh session for each invocation but reuses the loaded team. Consequently, shared: team todos remain visible across those conversations.
Qualify this table row and the recommendation below it: separate team loads, such as TUI tabs, are isolated; sessions using the same loaded team are not. If per-conversation isolation is intended for these servers too, it needs session-scoped storage and regression coverage rather than only a load-scoped cache.
There was a problem hiding this comment.
Agreed, I verified serve a2a and serve mcp both reuse one loaded team across sessions. Fixed in 39631af: the table row and the recommendation now scope team to a team load, say TUI tabs are isolated, and state that sessions reusing one loaded team share its list. Same wording in the schema description and examples/README.md. I did not add session-scoped storage; that seems like a separate change if you want per-conversation isolation for those servers.
`shared: true` returns a process-wide singleton, so in the TUI every tab (a separate team load in the same process) reads and writes the same todo list. There was no way to share a list between the agents of one team without also sharing it across sessions. `shared` now also accepts `team`: all agents of one team load share a list, while each session or tab gets its own. `true` keeps its meaning and `false` stays per-agent. - latest config: `Toolset.Shared` becomes `SharedScope`, which unmarshals `true`, `false` and `"team"` and marshals back to the same spelling so JSON clones and older-version upgrades keep working. - tools: `WithTeamScope` / `TeamScoped` hold the per-load instances in ctx. - teamloader: the root load opens a team scope; external agents joining the team inherit it. - todo: `Creator` is the registry creator that resolves the scope. `CreateToolSet` is unchanged apart from the new type. - schema, docs and an example updated.
…docs to team loads - wasm: the browser todo creator mapped every non-empty `shared` value to one per-registry list, so `team` and `true` read and wrote the same todos. `team` now goes through the context-aware team scope; `true` keeps its per-registry behavior. Add a regression test using both values together. - docs: `team` is scoped to a team load, not a session. TUI tabs are isolated, but sessions that reuse one loaded team (`serve a2a`, `serve mcp`) share that team's list. Same wording in the schema description and the examples index.
39631af to
43e2baf
Compare
aheritier
added
the
status/needs-rebase
|
👋 This PR has merge conflicts with the base branch. Please rebase or merge the latest base branch and resolve them. I've moved it to draft and added status/needs-rebase; it'll be picked back up automatically once the conflicts are cleared. |
Keep both the team scope opened in LoadWithConfig and main's cleanup of loaded agents on a failed load.
aheritier
removed
the
status/needs-rebase
|
@aheritier thanks, and sorry for the noise.
Could you take another look when you have a moment? The |
aheritier
left a comment
aheritier
left a comment
There was a problem hiding this comment.
Reviewed all 18 changed paths against the PR description's scope, compatibility, browser, validation, documentation, and test claims. See inline comment.
CI operational status: verified gate SUCCESS at 044691d; lint and Linux/Windows tests passed. test-race was skipped. The PR is non-draft and conflict-free; GitHub reports merge state BLOCKED. Local tests were not run because the checkout is at a different commit.
Issue context: needs-decision—no explicit linked GitHub/Jira acceptance criteria were supplied or discovered, including closing-issue references. The PR description supplied the review checklist. PR comments, reviews, and thread state were retrieved completely.
Prior threads:
- 4431649598: addressed; the browser now separates team and legacy shared stores, with a regression test exercising both together.
- 4431649600: addressed; documentation now describes isolation per team load and explicitly covers sessions reusing a loaded team.
Open questions: Confirm that the PR description is the complete acceptance criteria, or provide the governing issue.
|
|
||
| // External agents join the importing team, so only the root load opens a scope. | ||
| if externalDepthFromContext(ctx) == 0 { | ||
| ctx = tools.WithTeamScope(ctx) |
There was a problem hiding this comment.
[blocking] Preserve the team scope when rebuilding background worktree agents. Confidence: high.
The scope installed here exists only on the load's local context. When a background agent uses worktree: true, pkg/runtime/background_worktree.go:84 calls r.team.ForWorkingDir(ctx, wt.Dir) with the runtime context. That rebuild reaches the working-directory factory in pkg/teamloader/teamloader.go:552–556, whose ctx parameter replaces the scoped load context when calling agentToolOptions. Nothing restores the team scope, so todo.Creator calls TeamScoped without a scope and gets a fresh private list for every rebuilt agent.
Consequently, a worktree background agent cannot see its parent's team todos, and even agents within the rebuilt team do not share a list. This contradicts the new mode's documented inclusion of background agents; the legacy process-scoped mode does not lose its shared store along this path.
Retain and reapply the originating team's scope during working-directory rebuilds without sharing it across unrelated loads. Add a regression test that creates a team todo, rebuilds through ForWorkingDir using an ordinary runtime context, and verifies visibility and updates across the parent and rebuilt agents.
There was a problem hiding this comment.
Fixed in 637a5bd: the working-dir factory now rejoins the scope of the load it was created in (tools.InheritTeamScope), so agents rebuilt through Team.ForWorkingDir on the runtime context share the parent's shared: team list, and other loads stay isolated.
Regression test: TestLoadWithConfig_SharedTodoTeamSurvivesWorkingDirRebuild rebuilds via ForWorkingDir on an ordinary context and checks the parent and rebuilt agents share one list while an unrelated load does not. It fails without the fix (rebuilt agents get private lists). I also added unit tests for InheritTeamScope.
Local run: all packages I could build pass except the SSRF tests (*RejectsLocalAddresses, TestFetch_DefaultRefusesNonPublicAddresses), pkg/skills TestProjectSearchDirs and reasoningblock, which fail identically on 044691d. The root package, cmd/root and e2e/* did not build in my environment (out of disk), so CI is the check for those.
Worktree background agents rebuild the team with Team.ForWorkingDir on the runtime's context, which carries no team scope, so todo.Creator gave every rebuilt agent a private list. The working-dir factory now rejoins the scope of the load it was created in (tools.InheritTeamScope), so rebuilt agents share the parent team's list and other loads stay isolated.
There was a problem hiding this comment.
🟢 Approval recommended
No active review issues are reported for the team-scoped todo sharing change.
No open issues 🎉
What changed in this PR
Adds shared: team to the todo toolset so agents within one loaded team can share a list without sharing it with other team loads, such as separate TUI tabs.
Changes:
- Extends the latest configuration and schema to accept
team, preserving existing boolean behavior. - Adds context-based team scoping, including external agents, worktree rebuilds, and browser support.
- Adds scope-isolation and serialization tests, documentation, and an example.
| File | Description |
|---|---|
pkg/tools/team_scope.go |
Adds context-based scope creation, inheritance, and toolset reuse. |
pkg/tools/team_scope_test.go |
Tests sharing, isolation, inheritance, and fallback behavior. |
pkg/tools/builtin/todo/todo.go |
Adds a context-aware creator for team-scoped todos. |
pkg/tools/builtin/todo/todo_test.go |
Tests each sharing scope. |
pkg/teamloader/toolsets/toolsets.go |
Registers the context-aware todo creator. |
pkg/teamloader/teamloader.go |
Establishes team scopes and preserves them during agent rebuilds. |
pkg/teamloader/teamloader_test.go |
Tests sharing across loads and worktree rebuilds. |
pkg/config/latest/validate.go |
Keeps shared scopes restricted to todo toolsets. |
pkg/config/latest/validate_test.go |
Updates validation tests for explicit scope values. |
pkg/config/latest/types.go |
Defines sharing scopes and JSON/YAML serialization. |
pkg/config/latest/shared_scope_test.go |
Tests parsing, rejection, round trips, and serialized values. |
pkg/config/config_test.go |
Updates migration assertions for the new scope type. |
examples/shared-todo-team.yaml |
Demonstrates two agents sharing a team-local list. |
examples/README.md |
Lists the new team-sharing example. |
docs/tools/todo/index.md |
Explains sharing options and team-load lifetime. |
cmd/wasm/toolsets.go |
Adds browser team scoping while preserving per-registry boolean sharing. |
cmd/wasm/toolsets_test.go |
Tests separation between team and boolean sharing. |
agent-schema.json |
Accepts team and corrects the shared-property description. |
Review effort: Balanced
aheritier
left a comment
aheritier
left a comment
There was a problem hiding this comment.
Reviewed current head 637a5bd, including all 18 changed paths and the changes since the preceding review. Checked boolean compatibility, serialization/migration, team-load isolation, imported-agent scope propagation, browser scope separation, worktree rebuilds, validation, and schema/docs/example updates. The PR description establishes delivery scope; there are no linked delivery issues or Jira references.
Code quality: see inline comments.
Prior threads:
- 4431649598: addressed — cmd/wasm/toolsets.go:69-75 now separates team-scoped todos from the per-registry true singleton; a regression test covers both values together.
- 4431649600: addressed — the description, todo docs, schema, and example listing now distinguish team-load isolation from sessions reusing one team.
- 4472966991: addressed — pkg/teamloader/teamloader.go:555-557 restores the originating scope in working-directory factories; the new regression test checks parent/rebuilt sharing and unrelated-load isolation.
CI / merge readiness: not merge-ready. Checks associated with this head show gate, typescript, test-linux, test-windows, and lint failing in run 41985837566; test-race was skipped. Cross-compilation, image, and CodeQL checks passed. The PR is not draft and GitHub reports it conflict-free, but the review decision remains CHANGES_REQUESTED. The lint failure is at pkg/tools/team_scope_test.go:57 (testifylint useless-assert), separate from the substantive findings.
| return s.marshalValue(), nil | ||
| } | ||
|
|
||
| func (s SharedScope) MarshalJSON() ([]byte, error) { |
There was a problem hiding this comment.
[blocking] High confidence: register the new JSON representation with the TypeScript SDK generator. With the base branch integrated, sdk/typescript/gen/schema.go:69-70 rejects custom JSON marshalers unless they have an override, and defaultConfig's overrides contain no SharedScope entry. Consequently task check-ts stops with latest.SharedScope implements json.Marshaler: add a schema override; the current CI logs demonstrate this failure, along with the schema-generation tests failing on Linux and Windows. The checked-in generated Toolset contract also still declares shared?: boolean, so SDK users cannot express the new value. Add a schema override for boolean | "team", regenerate the SDK/schema artifacts, and verify their encoded-value coverage. Marking this type as merely reflected would incorrectly describe its boolean-or-string wire representation.
| // SharedScope is how far a toolset's state is shared. YAML accepts a boolean | ||
| // (true shares across the whole process) or "team" (shared by the agents of | ||
| // one team load only). | ||
| type SharedScope string |
There was a problem hiding this comment.
[should-fix] High confidence: update the SDK restriction test's sample generation for this new string-backed type. sdk/typescript/gen/toolsets_test.go:122 obtains a sample through sampleValue; its string branch at lines 197-198 returns example.com. SharedScope rejects that value before Toolset.validate runs, so TestToolsetRestrictionsMatchValidate can no longer exercise the todo-only restriction and fails for the other toolset types. This is independently visible in both platform CI logs (shared on a shell toolset: invalid shared value "example.com"), and remains after adding the schema override. Supply a valid nonzero SharedScope sample, such as team or true, so the test reaches the restriction it is checking.
Problem
shared: trueon thetodotoolset returns a process-wide singleton. In the TUI every tab is a separate team load inside one process, so all tabs read and write the same todo list. There is no way to let the agents of one team share a list without also sharing it across sessions.Change
sharednow also acceptsteam:false(default)teamserve a2a,serve mcp) share ittruelatestonly):Toolset.Sharedis nowSharedScope. It unmarshalstrue,falseand"team"(YAML and JSON) and marshals back to the same spelling, soCloneThroughJSONand upgrades from older versions keep working. Older config types are untouched.tools.WithTeamScope/tools.TeamScopedkeep per-load instances on the context.LoadWithConfigopens a scope for the root load; external agents imported into the team inherit it instead of opening their own.teamgoes through the same context-aware team scope;truekeeps its per-registry list. The two are separate lists.todo.Creator(registered inteamloader/toolsets) resolves the scope.CreateToolSetkeeps its signature. Outside a team load,teambehaves like an unshared toolset.sharedis stilltodo-only; the schema description previously said "for think tool", which was wrong.docs/tools/todo, and a newexamples/shared-todo-team.yaml.Testing
pkg/tools: scope isolation and the no-scope fallback.pkg/config/latest: parse, invalid values, JSON/YAML round trip, marshalled spelling,teamon a non-todo toolset.pkg/tools/builtin/todo: every scope value.cmd/wasm:teamandtrueused together stay separate (fails on the previous creator).pkg/teamloader: a realLoadWithConfigwith two sub-agents, checkingfalse/team/trueacross two loads (two tabs).golangci-lint v2.14.0,go run ./lint .,go mod tidy --diffand theGOOS=jstests forcmd/wasmandtodopass locally.go test ./...passes except four SSRF tests (*RejectsLocalAddresses,TestFetch_DefaultRefusesNonPublicAddresses) that also fail on unmodifiedmainin my sandbox, where an outbound proxy intercepts the requests.Notes
truekeeps meaning "whole process". I did not change its behavior, to avoid breaking anyone relying on cross-session sharing.thinkis unaffected:sharedwas already rejected for non-todo toolsets.🤖 Generated with Claude Code