fix(controller): checkpoint proxy scoped check compared member name as team name - #1214
Open
LUOSENGWA wants to merge 1 commit into
Open
fix(controller): checkpoint proxy scoped check compared member name as team name#1214LUOSENGWA wants to merge 1 commit into
LUOSENGWA wants to merge 1 commit into
Conversation
…s team name findTeamMember's second return value is the member (worker) name, not the team name, so the scoped-caller check fed the worker name to TeamMatches which always failed: every team leader and L2 human received 404 on their own team's workers' checkpoints. Resolve the Team CR name instead (same chain as ResourceHandler.GetWorker) and add the missing in-scope regression test — the previous suite only covered the cross-team 404, which passed for the wrong reason. All six findTeamMember call sites audited; this was the only one misusing the member-name return value.
LUOSENGWA
force-pushed
the
fix/checkpoint-scoped-team
branch
from
September 2, 2026 06:08
f50cf48 to
0d3752d
Compare
shiyiyue1102
approved these changes
Sep 3, 2026
shiyiyue1102
left a comment
Collaborator
There was a problem hiding this comment.
LGTM. This focused fix now compares scoped callers against the owning Team CR name instead of the Worker member name, and the new in-scope L2 regression test covers the previously broken path. CI is green.
LGTM。这个聚焦修复改为使用所属 Team CR 名称进行 scoped caller 判断,不再误用 Worker 成员名;新增的本团队 L2 回归测试也覆盖了此前失效的访问路径。CI 已通过。
oss-maintainer
approved these changes
Sep 3, 2026
oss-maintainer
left a comment
There was a problem hiding this comment.
LGTM. Verified the fix against the current codebase:
findTeamMemberreturns(*v1beta1.Team, memberName string, isLeader bool, err)— the old code destructured the second return value (member/worker name) and passed it tocaller.TeamMatches(...), which expects a team name. Since a worker's member name practically never equals its Team CR name, the scoped check was effectively always false for in-scope leaders/L2 humans.- The new code uses
teamObj.Name(first return value) with a proper nil guard: standalone workers (teamObj == nil) resolve toteamName == "", andTeamMatches("")returns false, so the documented "standalone workers hide as 404" behavior is preserved. - The new
TestCheckpoint_L2HumanInScopeAllowedregression test covers exactly the previously-broken path (in-scope L2 human → 200), complementing the existing cross-team 404 cases. - Consistent with the resolution chain used by
ResourceHandler.GetWorker. Trivial, well-scoped change.
Automated review by github-manager-bot
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
4-line fix in the worker checkpoint proxy (
agentteams-controller/internal/server/worker_checkpoints.go): the scoped-caller check now compares the caller's team against the Team CR name (findTeamMember's first return value) instead of the member (worker) name (its second return value). Plus one regression test.Why
findTeamMemberreturns(team *Team, memberName string, isLeader bool, err error). The checkpoint proxy usedmemberNameincaller.TeamMatches(...)— butTeamMatchesexpects the team name. For any real team the worker's member name differs from the team name, so the check was always false: every team leader and every L2 human received404when reading checkpoints of their own team's workers, even though#1186documented that exact access path as supported. Cross-team callers also got404, so the suite (which only asserted cross-team404) passed for the wrong reason and never noticed.Effect: the documented L2/leader checkpoint read path silently did not work for the roles it was built for.
Fix
teamObj.Name(first return value) — the same resolution chain asResourceHandler.GetWorkerand the new workspace-files proxy.findTeamMembercall sites were audited: this was the only one misusing the member-name return value.200); cross-team/unknown callers are unchanged (404, W8 anti-probing preserved).Test
TestCheckpoint_L2HumanInScopeAllowed(new): an L2 human in the worker's own team reads its checkpoints →200. Before the fix this returned404.404and leader cases still pass.go test ./...green on themainbaseline (the only failure is a pre-existing environment issue ininternal/executor: the sandbox lacks theunzipbinary, which CI has).Scope
#1208) per review, to keep that PR focused on the storage/authorization feature.摘要
worker checkpoint 代理(#1186)的 scoped 调用方检查把调用方团队与
findTeamMember的第二个返回值(成员/worker 名)而非团队名做比较,导致TeamMatches恒为 false:所有团队 leader / L2 人类读自己团队 worker 的 checkpoints 一律404——#1186 文档承诺的访问路径对目标角色实际不工作。跨团队调用方同样拿404,而原测试套件只断言跨团队404(在 bug 存在时也"恰好"通过),因此从未暴露。修复:改与第一个返回值(Team CR 名)比较——与
ResourceHandler.GetWorker同款链路。全量审计 6 个findTeamMember调用点,仅此处误用成员名返回值。行为变化对 in-scope 调用方纯增益(从错误的404变为文档承诺的200),跨团队/未知调用方不变(404,W8 防探测保持)。新增回归测试
TestCheckpoint_L2HumanInScopeAllowed。纯 controller 侧授权逻辑,不涉及 worker app API,无 runtime 版本门。按 review 要求从 #1208 拆出,保持该 PR 聚焦。