Skip to content

fix(controller): checkpoint proxy scoped check compared member name as team name - #1214

Open
LUOSENGWA wants to merge 1 commit into
agentscope-ai:mainfrom
LUOSENGWA:fix/checkpoint-scoped-team
Open

fix(controller): checkpoint proxy scoped check compared member name as team name#1214
LUOSENGWA wants to merge 1 commit into
agentscope-ai:mainfrom
LUOSENGWA:fix/checkpoint-scoped-team

Conversation

@LUOSENGWA

Copy link
Copy Markdown
Contributor

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

findTeamMember returns (team *Team, memberName string, isLeader bool, err error). The checkpoint proxy used memberName in caller.TeamMatches(...) — but TeamMatches expects 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 received 404 when reading checkpoints of their own team's workers, even though #1186 documented that exact access path as supported. Cross-team callers also got 404, so the suite (which only asserted cross-team 404) 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

  • Compare against teamObj.Name (first return value) — the same resolution chain as ResourceHandler.GetWorker and the new workspace-files proxy.
  • All six findTeamMember call sites were audited: this was the only one misusing the member-name return value.
  • Behavior change is strictly additive for in-scope callers (they now get the documented 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 returned 404.
  • Existing cross-team 404 and leader cases still pass.
  • Full go test ./... green on the main baseline (the only failure is a pre-existing environment issue in internal/executor: the sandbox lacks the unzip binary, which CI has).

Scope

  • Pure controller-side authorization logic; no worker-app API is involved, so there is no runtime version gate (applies to workers on any runtime — the checkpoint proxy is runtime-agnostic).
  • Split out of the workspace-files PR (#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 聚焦。

…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.

@shiyiyue1102 shiyiyue1102 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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 oss-maintainer left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

LGTM. Verified the fix against the current codebase:

  • findTeamMember returns (*v1beta1.Team, memberName string, isLeader bool, err) — the old code destructured the second return value (member/worker name) and passed it to caller.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 to teamName == "", and TeamMatches("") returns false, so the documented "standalone workers hide as 404" behavior is preserved.
  • The new TestCheckpoint_L2HumanInScopeAllowed regression 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

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.

3 participants