refactor: hard cut Team and Worker CR contracts#1072
Conversation
Change-Id: I656dbec78c738857734391ee43f28e65f617b16e
Change-Id: I236bc6d34682c5e45697e02f1339ed72dcb7e957
📊 CI Metrics ReportSummary
By Role
Per-Test Breakdown
Trends✅ 3 test(s) improved (fewer LLM calls) Generated by AgentTeams CI on 2026-07-25 05:10:37 UTC |
Change-Id: I3f41015551b028286370a3148895afd6267d2e28
Change-Id: I6823e52a911568ffceb0cf45c37603df28160494
Change-Id: Idd80e2138a060c348fd298363a65604c24547d1f
Change-Id: I1fe49a7dc84bc0ecf849edd7bba5f26db18cb7e1
Change-Id: I82743e6b5b832cf6adc842ea38516509b55b25ca
Change-Id: Ie587682bbdce738655b815bec2984535960071f6
Change-Id: Ie303a07c558340473d7149638d768cba759d100c
Change-Id: I63e16bd32160d80ff12dc81bb6efe287820437e4
|
@johnlanni @Jing-ze @maplefeng-a The latest checks are all passing and this PR is ready for review. Could you take a look when convenient? |
maplefeng-a
left a comment
There was a problem hiding this comment.
Thanks for the hard-cut cleanup. The direction looks aligned with moving Team/Worker to explicit CR ownership, but I think two contract edges should be fixed before this lands:
-
The "block Worker deletion while referenced by a Team" protection currently only covers the REST API path.
DeleteWorkerrejects a referenced worker throughfindTeamForMember, but direct Worker CR deletion still goes throughWorkerReconciler'sDeletionTimestamp -> reconcileDeletepath and removes the finalizer after cleanup. I also do not see a validating webhook/admission path in this PR. That meanskubectl delete worker <team-member>can still delete a Worker that is referenced by a Team, which violates the new declarative CR contract. Could we add the same protection on the CR path, preferably via validation/admission, or otherwise clearly narrow the documented guarantee to the REST/CLI path? -
validateTeamWorkerMembersstill accepts an empty member role (case "", "worker"), but the generated Team CRD now requiresworkerMembers[].roleand restricts it toteam_leader|worker. A REST caller that omits the non-leader role can pass server-side validation and then fail at the Kubernetes write layer, likely as a generic 500 becausewriteK8sErrordoes not mapInvalid. Could we either reject empty roles in the REST validator or normalize them to"worker"before writing the CR?
CI is green on the current head; these look like contract/API consistency issues rather than test failures.
Change-Id: I2b2c448d8ea3f8267a428ef925398dd8781940d7
Change-Id: I0b6a028fdb54e22db8e96f4e2676e6474a1fe18a
|
Thanks for catching these contract gaps. Fixed in d96f1ed:
Validated with the full agentteams-controller test suite (go test ./...). |
Change-Id: Ie32c2f73083f19ea0f913edd9e075fcba91616c2
Change-Id: I8a3d34053424a0fe21c98f710f8dba9904f1a0c4
maplefeng-a
left a comment
There was a problem hiding this comment.
The contract gaps I raised have been addressed, and the current head is green. Approving.
What changed
WorkerCRs the sole owners of runtime configuration, identity, resources, skills, MCP/package settings, channel policy, and lifecycle.TeamCRs reference existing Workers throughspec.workerMembers, with exactly oneteam_leader.agtCLI, authorization, Manager skills/scripts, installers, documentation, Helm CRDs, and integration tests to use the terminal CR model.Why
The Team and Worker CRDs are new contracts and do not need migration compatibility. Keeping inline members, registry files, annotations, and CR-backed state in parallel created multiple sources of truth and made ownership ambiguous.
This change hard-cuts the repository to one end-to-end model: Worker owns runtime state; Team owns membership and coordination.
Impact
spec.leader,spec.workers, registry files, Team/role annotations, and CLI compatibility flags are no longer accepted.Validation
GOCACHE=/tmp/agentteams-hard-cut-go-cache go test ./...go build ./...TestTeamintegration tests.shfilesgit diff --check