Skip to content

feat(controller): add team-scoped worker tool approval endpoints for L2 humans - #1216

Open
LUOSENGWA wants to merge 1 commit into
agentscope-ai:mainfrom
LUOSENGWA:feat/l2-worker-approval
Open

feat(controller): add team-scoped worker tool approval endpoints for L2 humans#1216
LUOSENGWA wants to merge 1 commit into
agentscope-ai:mainfrom
LUOSENGWA:feat/l2-worker-approval

Conversation

@LUOSENGWA

@LUOSENGWA LUOSENGWA commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

What

Team-scoped read/write proxy for each QwenPaw worker's tool-execution security level:

GET  /api/v1/workers/{name}/approval  -> {"approval_level": "AUTO"}
PUT  /api/v1/workers/{name}/approval  <- {"approval_level": "STRICT"}

approval_level lives in the worker's agent profile (agent.json) and decides which tool calls run automatically and which pause for a human approval. The worker's qwenpaw app already exposes it on GET/PUT /workspace/running-config (the field round-trips through the running-config object and is written back into the agent profile by the app itself, under its per-file path lock). This PR adds the Controller proxy so L2 humans can read and set the level of workers in their own teams — no docker exec, no SSH, no dashboard access to the worker app required.

Design

Decision Rationale
Four fixed levels: STRICT (every tool needs approval) / SMART (low-risk auto-allowed) / AUTO (only guarded tools — upstream default) / OFF (guard disabled) Matches the QwenPaw agent-profile semantics exactly. Any other value is rejected 400 before the worker is touched — the upstream running-config model accepts any string, so an unvalidated proxy would write garbage into agent.json. The proxy is the validation boundary.
Write scope: admin/manager any team; L2 human own teams only; team leaders read-only (403 on PUT) Same layered boundary as the knowledge base write API (#1208): scope (own team) → role (leader denied) → cross-team hidden as 404 (W8 anti-probing: the authorizer deliberately allows the action for humans cross-team, like ActionGet, so the middleware's 403 cannot be used to probe which workers exist in other teams — the handler is the real boundary).
Safe write: GET current config → change only approval_levelPUT the full object back The upstream PUT persists whatever full object it is sent; a partial-body proxy would wipe unrelated running-config fields. Every other field round trips verbatim (asserted in tests). Upstream 409 (concurrent config change under the path lock) passes through status+body so clients retry with a fresh GET.
Minimal response shape ({"approval_level": ...}) The full running-config object is an internal surface; clients (frontends, plugins) only need the level.
Embedded mode only Same worker addressing as the checkpoint proxy (effective container prefix + system-wins console port). Kube mode has no stable worker pod DNS → uniform 503 before any worker lookup.
Version gate A worker on a QwenPaw version without the running-config router surfaces the upstream 404 verbatim — clients can show "worker upgrade required" instead of "worker missing". (Production 2.0.1 workers verified to serve this API: 24/24 read + write chain tested 8/29.)
Audit log Every successful change logs worker, new level, caller, role.

Tests

  • 18 handler tests: in-scope L2 read/write, cross-team 404 (W8), in-scope leader read 200 / write 403, standalone-worker hidden from scoped callers (admin still 200), default level when the profile has none, pre-2.x version-gate 404 passthrough, unreachable worker 502, kube mode 503, invalid level values (strict/YOLO/``/Auto → `400`, zero upstream calls), invalid bodies, full-object round trip asserted (the PUT carries all original fields, only the level changed), `409` passthrough.
  • 3 authorizer cases: pin the W8 decision (human approval write allowed cross-team at the authorizer, hidden by the handler) and the leader/worker denials.
  • 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 & runtime

Related


摘要

为每个 QwenPaw worker 的工具执行安全级别(agent.jsonapproval_level——决定哪些工具调用自动执行、哪些暂停等人工审批)加团队范围的 Controller 代理,L2 人类可管理自己团队内 worker 的该级别:

GET  /api/v1/workers/{name}/approval  -> {"approval_level": "AUTO"}
PUT  /api/v1/workers/{name}/approval  <- {"approval_level": "STRICT"}

worker 的 qwenpaw app 已在 GET/PUT /workspace/running-config 暴露该字段(经运行配置对象往返、由 app 自身写回 agent profile,带 per-file path lock)。本 PR 加 Controller 代理,L2 不再需要 docker exec/SSH 即可读改审批级别。

设计定案:四档固定值(STRICT/SMART/AUTO/OFF,上游默认 AUTO)——其他值在触碰 worker 前 400(上游模型不校验取值,代理是校验边界);写范围与知识库写 API(#1208)同界:scope(自己团队)→ 角色(leader PUT 403 只读)→ 跨团队 404 隐藏(W8:authorizer 对 human 跨团队放行如 ActionGet,由 handler 隐藏为 404,防存在性探测);安全写=GET 全量→仅改 approval_level→PUT 回全量(上游 PUT 持久化收到的完整对象,部分体会抹掉无关字段;其余字段原样往返,测试断言),上游 409 状态+body 透传;响应面最小化(只回 approval_level);仅 embedded 模式(worker 寻址与 checkpoint 代理相同,kube 503);旧版 worker 无 running-config 路由→原样透传上游 404(版本门;生产 2.0.1 已实测 24/24 可读可写);每次成功变更记审计日志。

测试:18 个 handler 用例(scope/W8/leader 只读/版本门/非法值/全量往返断言/409 透传/kube 503/unreachable 502)+ 3 个 authorizer 用例钉死 W8 边界。纯 Controller 侧代理+授权,无 worker app 改动、无 CRD 改动。Runtime scope:仅 QwenPaw worker(代理的是 qwenpaw app 自己的路由,copaw 无对应物,legacy worker 走版本门 404)。

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

The ability for L2 users to manage tool-approval settings for Workers in their own teams is reasonable, but the sensitive boundary should be separated from normal L2 configuration.

A default L2 user may be allowed to read the current level and switch among the normal guarded modes, subject to the agreed product policy. However, setting approval_level=OFF disables Tool Guard and allows all tool calls to execute directly, so it should require an explicit FullAccess-like capability granted by an admin. It should not be available automatically to every L2 account.

Please define a dedicated permission for this security-policy operation instead of treating it as ordinary Worker configuration. The API should check both team scope and the caller’s assigned capability.

There is already a public design Issue, #1217. Please formally link this PR to that Issue and extend it to cover:

  1. Which approval levels default L2 users may select.
  2. Whether disabling Tool Guard requires FullAccess.
  3. How an admin grants and revokes that capability.
  4. Audit requirements for security-level changes.
  5. The actual concurrency contract of the upstream running-config API.

Please let the community discuss and confirm this permission model before the PR proceeds.


允许 L2 用户管理本团队 Worker 的工具审批设置是合理的,但敏感操作需要与普通 L2 配置能力分开。

默认 L2 可以读取当前等级,并根据最终确认的产品策略在常规受保护等级之间切换。但是,approval_level=OFF 会关闭 Tool Guard,使所有工具调用直接执行,因此应要求由 Admin 显式授予类似 FullAccess 的能力,不能默认开放给所有 L2 账号。

建议为安全策略修改定义独立权限,而不是将其视为普通 Worker 配置。接口需要同时检查团队范围和账号被授予的 capability。

目前已经有公开设计 Issue #1217。请正式将本 PR 与该 Issue 关联,并补充讨论:

  1. 默认 L2 可以选择哪些审批等级。
  2. 关闭 Tool Guard 是否必须具有 FullAccess。
  3. Admin 如何授予和收回该能力。
  4. 安全等级变更需要记录哪些审计信息。
  5. 上游 running-config API 实际提供的并发保证。

请先让社区参与并确认这套权限模型,再继续推进本 PR。

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

Summary

Adds team-scoped GET/PUT /api/v1/workers/{name}/approval endpoints proxying the QwenPaw worker running-config so L2 humans can manage worker tool-approval levels. The structure follows the existing checkpoint-proxy pattern and applies the W8 anti-probing design consistently, but the code-level review found two serious correctness issues in the GET→modify→PUT safe-write path, plus several smaller concerns.

Note: this review intentionally does not repeat the permission-model feedback already raised in the existing CHANGES_REQUESTED review (approval_level=OFF capability gating, linking design issue #1217) — the findings below are code-level only.

Findings

  • [Critical] worker_approval.go:254,326io.LimitReader(resp.Body, 4096) on the upstream running-config GET: any config larger than 4 KiB is truncated, and since the safe-write PUT sends the whole (now truncated) object back, fields beyond the cutoff are silently dropped from agent.json. This defeats the stated "every other field round-trips verbatim" guarantee.
  • [Critical] worker_approval.go:376 — in the 409 passthrough branch w.WriteHeader(...) is called before w.Header().Set("Content-Type", ...), so the header is ignored; clients get the conflict body without the JSON content type.
  • [Warning] worker_approval.go:248,321 — upstream calls use h.http.Get(...) without NewRequestWithContext, so request context/cancellation is not propagated (inconsistent with the checkpoint proxy).
  • [Warning] worker_approval.go:343 — if upstream returns a JSON null 200 body, cfg stays nil and cfg["approval_level"] = ... panics.
  • [Warning] worker_approval.go:265 — a non-string upstream approval_level silently falls back to AUTO instead of surfacing the shape mismatch.
  • [Warning] worker_approval.go:273,330 — 404 passthrough replaces the upstream body with a generic message, contradicting the documented "verbatim passthrough" behavior.
  • [Warning] worker_approval.go:344json.Marshal error discarded with _.
  • [Info] worker_approval_test.go — no coverage for upstream non-OK/non-404 statuses (the default 502 path) or for running-config bodies larger than 4096 bytes (the truncation risk above).

Suggestions

  1. Remove the 4 KiB cap on the upstream GET round-trip path (or raise it well above the maximum realistic running-config size and fail loudly — e.g., 502 with a clear message — if the limit is hit instead of silently truncating). Keep a request-body limit on the client-facing PUT, but the safe-write must round-trip the full upstream object.
  2. Set Content-Type before WriteHeader in the 409 branch.
  3. Switch upstream calls to http.NewRequestWithContext(r.Context(), ...) to match the checkpoint proxy.
  4. Guard the nil-map case (if cfg == nil { cfg = map[string]any{} } or explicit error) and treat a non-string approval_level as a 502 data-shape error.
  5. Add tests for upstream 500 and for a >4 KiB running-config round trip.

Automated review by github-manager-bot

Findings

  • [WARNING] agentteams-controller/internal/server/worker_approval.go:248 — getWorkerApproval dials the upstream worker with h.http.Get(...), which does not propagate the request context. Inconsistent with the checkpoint proxy (NewRequestWithContext) and means cancellation/deadlines from the caller are ignored.
  • [CRITICAL] agentteams-controller/internal/server/worker_approval.go:254 — Upstream GET response for /workspace/running-config is read with io.LimitReader(resp.Body, 4096). Because the PUT safe-write round-trips the entire object, a running-config larger than 4096 bytes is truncated and fields below the cutoff are silently dropped on the subsequent PUT.
  • [WARNING] agentteams-controller/internal/server/worker_approval.go:265 — If the upstream returns approval_level as a non-string type (e.g., number), the type assertion fails and the handler silently falls back to AUTO, masking a data-shape mismatch instead of returning an error.
  • [WARNING] agentteams-controller/internal/server/worker_approval.go:273 — The GET 404 passthrough claims in the comment to surface the upstream 404 verbatim, but the code replaces the body with a generic {"message":"Not Found"} via httputil.WriteError.
  • [WARNING] agentteams-controller/internal/server/worker_approval.go:321 — The safe-write preflight GET also uses h.http.Get(...), missing request-context propagation and cancellation handling.
  • [CRITICAL] agentteams-controller/internal/server/worker_approval.go:326 — Same 4096-byte LimitReader as line 254 on the preflight GET. Combined with the safe-write PUT, this is the place that can corrupt the worker's running-config by truncating it before modification.
  • [WARNING] agentteams-controller/internal/server/worker_approval.go:330 — The PUT preflight 404 path also replaces the upstream body with a generic {"message":"Not Found"}, contradicting the documented verbatim passthrough behavior.
  • [WARNING] agentteams-controller/internal/server/worker_approval.go:343 — If the upstream GET returns a JSON null body, cfg is nil and cfg["approval_level"] panics. The code assumes the 200 body is always an object.
  • [WARNING] agentteams-controller/internal/server/worker_approval.go:344 — json.Marshal(cfg) error is discarded with _; although unlikely for a map, a failed marshal would send a zero-length body upstream.
  • [WARNING] agentteams-controller/internal/server/worker_approval.go:372 — In the 409 passthrough branch, w.WriteHeader(http.StatusConflict) is called before w.Header().Set("Content-Type", "application/json"). In net/http, headers set after WriteHeader are ignored, so the JSON conflict body is returned without the intended Content-Type.
  • [INFO] agentteams-controller/internal/server/worker_approval_test.go:1 — No test covers an upstream GET returning a non-OK/non-404 status (e.g., 500) for either the read handler or the PUT preflight, so the default 502 path is unexercised.
  • [INFO] agentteams-controller/internal/server/worker_approval_test.go:1 — No test covers a running-config body larger than 4096 bytes or an oversized client request body, so the LimitReader truncation risk is unverified.

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

Summary

Re-review of the amended head (0e033e6): both Critical findings from the previous review are fixed, and the new OFF restriction addresses the core security concern from the maintainer's CHANGES_REQUESTED review. The maintainer's design/process items (formal design-issue linkage and community confirmation of the permission model) remain open, so the approval decision is left to the maintainer.

Verified fixes:

  • [Critical → fixed] The upstream running-config read is now capped at 1 MiB with LimitReader(..., cap+1) overflow detection and an explicit 502 on overflow (worker_approval.go:83-91) — no more silent truncation, so the GET→modify→PUT safe-write can no longer drop fields from agent.json.
  • [Critical → fixed] The 409 passthrough branch now sets Content-Type before WriteHeader (worker_approval.go:358-361) and passes the upstream body through verbatim.
  • [New in this revision] approval_level=OFF is now rejected with 403 for human callers (only STRICT/SMART/AUTO), while admin/manager keep the full range — documented as an interim hardcoded boundary until the elevated-capability design (#1220) lands. This addresses the Tool-Guard-disable concern raised in the maintainer's review.

CI note: integration-tests (controller-cr, SHARD_C_TESTS, qwenpaw, qwenpaw) is failing on this PR's latest run while the same shard passes on #1210 — please investigate whether the failure is PR-specific or flaky.

Local verification: go test ./internal/server/ ./internal/auth/ green on this head.


Automated review by github-manager-bot

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

Summary

This PR introduces GET/PUT /api/v1/workers/{name}/approval endpoints that let L2 humans manage tool-approval levels for workers in their own teams while keeping team leaders read-only and reserving the OFF level for admin/manager roles. The implementation is security-minded: cross-team access is hidden as 404, levels are validated before touching the worker, and the safe-write pattern round-trips the full running-config. A few issues remain, including an inconsistent response-size cap on the PUT path that can silently truncate large upstream responses, and a non-atomic read-modify-write sequence that is racy for concurrent L2 humans; none of these are blocking security bugs.


Automated review by github-manager-bot

return
}
defer up.Body.Close()
upBody, err := io.ReadAll(io.LimitReader(up.Body, upstreamConfigMax))

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The PUT response is read with io.LimitReader(up.Body, upstreamConfigMax) instead of upstreamConfigMax+1 and lacks the explicit size-exceeded check used by fetchUpstreamConfig. An oversized upstream response is silently truncated rather than surfaced as a 502, which can mask upstream anomalies and makes the read path inconsistent.

"setting approval_level=OFF requires the elevated tool-approval capability (L2 permission design, #1220); use STRICT, SMART, or AUTO")
return
}
// Safe write: the upstream PUT persists the *full* running-config

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The safe-write pattern (GET current config, modify approval_level, PUT full config back) is not atomic. Two concurrent L2 humans can fetch the same running config, set different levels, and have the second PUT overwrite the first; the upstream 409 only covers path-lock/reindex contention, not general concurrent modification. Consider documenting this limitation or using an optimistic concurrency token if the upstream API supports one.

// explicitly. Admin/manager keep the full range (any-worker scope).
// This hardcoded boundary becomes the capability lookup once #1220
// lands.
if payload.ApprovalLevel == "OFF" && caller.Role == authpkg.RoleHuman {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The OFF elevation gate is intentionally role-based until #1220 lands, but the caller.Role == RoleHuman check allows any non-human principal that passes the authorizer to disable Tool Guard. Ensure the #1220 capability-based replacement explicitly enumerates which roles/capabilities may set OFF rather than relying on a non-human default.

// L2 humans may change the tool-approval level of workers in
// their own teams. Like ActionGet this action is NOT rejected
// cross-team at the authorizer level: the middleware resolves
// the worker's team, and a 403 here would let a scoped caller

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The comment says "the middleware resolves the worker's team" and "a 403 from the middleware would let a scoped caller probe", but the actual team resolution and 404 hiding happen in the ApprovalHandler.approvalScope handler, not in the middleware. Update the comment to avoid misleading future maintainers about where the W8 boundary is enforced.

up := approvalUpstream(t, "AUTO", &putBody)
defer up.Close()
h := newTestApprovalHandler(t, "embedded", up,
approvalTeamWithWorkers("market-team", "market-analyst")...)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The handler tests cover admin and L2 human roles, but the docs state that managers also have full write range. Add a manager-role test case for PUT /approval to guard against regressions in the role boundary.

@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. The current head fixes the running-config truncation and 409 response issues, enforces team scope, and keeps restricted to admin/manager until the #1220 full-access capability and audit path land. The remaining read-modify-write concurrency limitation is documented and non-blocking, and the full CI rerun is green.\n\n---\n\nLGTM。当前版本已修复 running-config 截断和 409 响应问题,落实团队范围校验,并在 #1220 的 full-access capability 与审计链路落地前,将 保持为仅 Admin/Manager 可用。剩余的读改写并发限制已有说明且不阻塞,完整 CI 重跑已通过。

@shiyiyue1102

Copy link
Copy Markdown
Collaborator

Thanks for the update. PR #1212 has now been merged into main and overlaps this PR in the authorizer paths, so GitHub currently reports this branch as conflicting. Please merge or rebase the latest main and resolve the conflicts while preserving the reviewed behavior: team-scoped L2 users may select guarded modes only, and approval_level=OFF remains restricted until the #1220 full_access capability lands. Once the branch is updated and CI reruns, we will re-check the new head for merge.\n\n---\n\n感谢更新。PR #1212 已合入 main,并与本 PR 的 authorizer 路径存在重叠,因此 GitHub 当前报告该分支存在冲突。请合并或 rebase 最新 main 并解决冲突,同时保留已评审行为:本团队 L2 只能选择受保护档位,approval_level=OFF 在 #1220 的 full_access capability 落地前仍保持受限。分支更新并重新跑完 CI 后,我们会基于新 head 再次确认合并。

…L2 humans

Proxies a minimal read/write surface of each QwenPaw worker's
/workspace/running-config API so L2 humans can manage the tool-execution
security level (approval_level) of workers in their own teams:

  GET  /api/v1/workers/{name}/approval -> {"approval_level": "AUTO"}
  PUT  /api/v1/workers/{name}/approval <- {"approval_level": "STRICT"}

- Levels: STRICT / SMART / AUTO (upstream default) / OFF — validated
  against the fixed four-token set before the worker is touched (the
  upstream model accepts any string; the proxy is the validation boundary).
- Write scope: admin/manager any team; L2 human own teams only (W8:
  cross-team workers hide as 404); team leaders read-only (403 on PUT,
  same boundary as the knowledge base write API).
- Safe write: the upstream PUT persists the full running-config object, so
  the proxy does GET -> change only approval_level -> PUT the whole object
  back; all other fields round trip verbatim. Upstream 409 passes through.
- Embedded mode only (same addressing as the checkpoint proxy); kube mode
  503; pre-2.x workers surface the upstream 404 (version gate).
- Every successful change is audit-logged.

Tests: 23 handler tests (scope/W8/leader-read-only/version-gate/
invalid-values/full-object round trip/409 passthrough/kube 502-503) +
3 authorizer cases pinning the W8 boundary.
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