Skip to content

feat(controller): add team-scoped worker knowledge base file endpoints (+ checkpoint scoped-access fix) - #1208

Merged
shiyiyue1102 merged 4 commits into
agentscope-ai:mainfrom
LUOSENGWA:feat/worker-workspace-files
Sep 11, 2026
Merged

feat(controller): add team-scoped worker knowledge base file endpoints (+ checkpoint scoped-access fix)#1208
shiyiyue1102 merged 4 commits into
agentscope-ai:mainfrom
LUOSENGWA:feat/worker-workspace-files

Conversation

@LUOSENGWA

@LUOSENGWA LUOSENGWA commented Aug 31, 2026

Copy link
Copy Markdown
Contributor

Team-scoped worker knowledge base file endpoints: read, write, download

Summary

L2 humans (and team leaders) currently have no path to their team's knowledge base — not read, not write. The only way to reach a worker's workspace files is the Controller's /docker/ proxy, which is admin/Manager-only: an L2 human's Matrix token gets 403 on every attempt, and the checkpoint endpoints (#1186) expose file indexes and change summaries, not the knowledge text itself.

This PR proxies four QwenPaw workspace-file endpoints through the Controller, following the exact fixed-subpath proxy pattern established by the worker checkpoint endpoints (#1186):

  • GET /api/v1/workers/{name}/workspace-files/tree?path=memory|digest[/...] — paginated directory listing of the knowledge trees
  • GET /api/v1/workers/{name}/workspace-files/file-metadata?path=... — size/etag/modified of one knowledge file
  • GET /api/v1/workers/{name}/workspace-files/file-content?path=...[&offset=&limit=] — bounded UTF-8 chunk read with next_offset continuation
  • PUT /api/v1/workers/{name}/workspace-files/file-content?path=... — ETag-guarded save of one knowledge file (create or update)
  • GET /api/v1/workers/{name}/workspace-files/file-download?path=... — bounded stream of one knowledge file with attachment headers

The addressable surface is a path allowlist, identical for reads and writes: MEMORY.md (long-term memory), memory/** (daily notes) and digest/** (distilled knowledge). Everything else in the workspace — SOUL.md, PROFILE.md, TODO.md, checkpoints/, skills/, and all dot directories (.copaw/agent.json carries the worker's Matrix token and storage credentials) — is rejected with 400 before the request reaches the worker, even though the upstream path resolver would serve a directly requested dot path.

Write access is per-user, L1-configurable, and an explicit opt-in. The Human CRD gains spec.workspaceFileAccess: read | readwrite (default is read when empty). Admin/manager (L1) may write any team's knowledge base; an L2 human may write only workers in their own teams and only while their Human CR is explicitly set to readwrite — an empty or missing value means read-only, so a controller upgrade cannot silently grant pre-existing L2 humans the new ability to modify worker knowledge files. L1 grants a user write access by setting the field to readwrite (and revokes it by clearing the field or setting read). Team leaders stay read-only on this API. The concurrent-write hazard this design exists to close: workers auto-append to their memory files, so the proxy requires If-Match on existing files (a skipped ETag check is a lost update) and forbids it on new files; upstream ETag mismatches pass through as 409.

Split out per review: the checkpoint proxy scoped-caller fix (member name vs team name) and the copaw-worker mcp>=1, <2 dependency pin are now separate, focused PRs — neither is included in this PR. This PR is the workspace-files feature only.

What's included

  1. agentteams-controller/api/v1beta1/types.go + CRD manifests:
    • HumanSpec gains workspaceFileAccess (read | readwrite, optional, default read when empty — write is an explicit opt-in) — the per-user knowledge base write permission L1 sets per user.
    • config/crd/humans.agentteams.io.yaml + helm/agentteams/crds/humans.agentteams.io.yaml updated in sync (enum-validated; make check-crd-sync green). No deepcopy regeneration needed (plain string, covered by the struct copy).
  2. agentteams-controller/internal/auth/authorizer.go:
    • New action workspace-files-write. For RoleHuman on resource worker: allowed at the authorizer level for the same W8 reason as reads — the middleware resolves the worker's team, and a cross-team denial there would leak worker existence via 403 (the read path hides it as 404). The handler is the real boundary (404 hiding + per-user flag + allowlist). Deliberately a separate action from update, so this PR does not entangle with the L2 worker-update authorization work.
    • Admin/manager already have full access; no other role changes.
  3. agentteams-controller/internal/server/worker_workspace_files.go (new):
    • WorkspaceFilesHandler — fixed-subpath proxy (tree / file-metadata / file-content GET+PUT / file-download) to the worker's qwenpaw app on the shared docker network. Embedded mode only; kube mode returns a uniform 503 before any worker lookup (no 404/503 existence split).
    • validateKbPath — the knowledge allowlist: exact root matching (memory, digest, MEMORY.mdmemories/ and memoryX/ are not prefixes of memory/), file roots are single top-level files (MEMORY.md/foo is rejected — MEMORY.md is one file, not a directory; nested files must live under memory/ or digest/), no dot segments (kills .copaw/, .git/, .qwenpaw/, .reme_store_v1/), no .././absolute/backslash/NUL, ≤4 segments, ≤255 bytes per segment (mirroring the upstream segment limit).
    • Strict per-subpath query whitelist (unknown or duplicate parameters → 400, same semantics as the checkpoint proxy); limit bounds mirror the upstream caps (tree 1..500 = MAX_PAGE_SIZE; file-content 1..1048576 = MAX_CHUNK_SIZE); cursor is an opaque passthrough; offset ≥ 0.
    • root=workspace is pinned server-side and is not part of the client query surface — the QwenPaw default root=project is the primary bound project directory, not the knowledge base. The upstream query is rebuilt from the validated values only.
    • proxyWorkspaceFileWrite (PUT file-content): write-role boundary (admin/manager any team; L2 human own teams + explicit workspaceFileAccess="readwrite" — empty/missing = read-only; team leader 403; cross-team hidden as 404), then a file-metadata existence probe deciding the If-Match policy (existing → header mandatory; new → header forbidden), 1 MiB body cap, non-empty content (an empty write would truncate a worker's memory file), verbatim passthrough of 400/404/409/422, and an audit log line per successful write (worker, path, caller, role, bytes, create).
    • file-download: the upstream StreamingResponse (256 KiB chunks, Content-Disposition: attachment, Content-Length, ETag) is streamed through with those headers forwarded — clients save the file under the worker's file name.
    • Upstream address resolution identical to the checkpoint proxy: effective container prefix + service.EffectiveWorkerConsolePort (system-wins env chain — a conflicting spec.env port is discarded, so the proxy always dials the port the container listens on).
  4. agentteams-controller/internal/server/http.go: +5 lines across the PR — handler construction, the GET /api/v1/workers/{name}/workspace-files/{sub} route, and the PUT .../workspace-files/file-content route registered with RequireAuthz(workspace-files-write, "worker").
  5. Tests — 41 in total: 24 read-path (forwarding with root-pinning assertion / offset+limit forwarding / metadata forwarding / cursor passthrough / allowlist positive table / sensitive-path rejection table — .copaw/agent.json and friends / traversal rejection / non-KB rejection incl. prefix-confusion names / unknown+dup query rejection / bound violations / missing-path rejection / write subpaths (file-upload, restore) rejected via GET / invalid worker name / unknown worker 404 / L2 in-scope 200 (the purpose case) / L2 cross-team 404 / team-leader cross-team 404 / standalone worker hidden from scoped callers but visible to admin / kube 503 before lookup / unreachable 502 / upstream-404 passthrough / bounded upstream-500 / prefix+port resolution matrix (8 cases) / end-to-end effective prefix+port dialing) + 17 write/download (create without If-Match / update with matching If-Match / existing-file-without-If-Match rejected before the worker is touched / new-file-with-If-Match rejected / 409 conflict passthrough / cross-team 404 / read-only human 403 / empty workspaceFileAccess (upgrade default) 403 / in-scope leader 403 / admin 200 / 1 MiB+1 rejected before the worker / write allowlist table incl. SOUL.md and MEMORY.md/foo / kube 503 / non-write subpaths + invalid bodies 400 / download 200 with attachment headers forwarded / download cross-team 404 / download allowlist even for admin incl. MEMORY.md/foo).
  6. docs/usage/project-workflow-api.md (+ docs/zh-cn/usage/project-workflow-api.md): "Worker knowledge base (workspace files) endpoints" sections — endpoint table (4 endpoints), read scope + write scope, concurrency (ETag) contract, write limits, the workspaceFileAccess field, allowlist, root-pinning, version-gate contract (the file-metadata?path=MEMORY.md probe that distinguishes "worker on QwenPaw < 2.1" from "file missing"), error-code table.

Data boundary & security

  • One boundary for reads and writes: the allowlist (MEMORY.md / memory/** / digest/**) applies to both. SOUL.md is the team owner's domain and is never writable through this API — not by L1, not by anyone (the identity-and-soul boundary is enforced by the path allowlist, not by the role matrix).
  • Write scope is layered: handler team-scope (cross-team hidden as 404 — the same W8 rule as reads; the authorizer deliberately does not reject cross-team for this action, because the middleware resolves the worker's team and a 403 there would leak existence) → per-user workspaceFileAccess (403 unless explicitly readwrite; also 403 when the Human CR cannot be read) → leader 403 → If-Match policy (400) → 1 MiB / non-empty body (400) → allowlist (400). Any single layer failing closed still protects the fleet.
  • Optimistic concurrency is not skippable: the worker app permits a bare overwrite (no If-Match) of an existing file, which would silently lose the worker's own appends between the L2 client's read and write. The proxy probes file-metadata first and makes If-Match mandatory in that case — the client cannot downgrade the guarantee.
  • Allowlist, not denylist: every other workspace location fails closed to 400. The upstream app's own path hardening (no .., no absolute, symlink-confined, dot entries hidden in listings) is not relied upon for dot directories: a directly requested .copaw/agent.json resolves fine upstream, which is exactly why the Controller-side allowlist exists. Covered by explicit tests for both read and download.
  • No new credentials: L2 humans authenticate with their existing Matrix access tokens via the existing CompositeAuthenticator (feat(controller): add project/workflow query API for human-visible workflows #1169); scope is their accessibleTeams. No admin token, no new roles.
  • Auditability: every successful knowledge base write is logged by the controller (worker, path, caller, role, bytes, create-or-update).
  • Version gate: the workspace file router exists since QwenPaw 2.1.0 (a439042c, #6504). A worker on QwenPaw < 2.1 returns upstream 404 for every subpath; it is passed through verbatim, and the documented file-metadata?path=MEMORY.md probe (present in every initialized workspace) lets clients show "worker upgrade required" instead of "file missing". Current fleet workers (2.0.1) light up automatically after the 2.1+ migration (feat(qwenpaw): upgrade QwenPaw Worker runtime to 2.2 #1174 / feat(manager): upgrade Manager runtime to QwenPaw 2.2 #1175) — no Controller redeploy.
  • Existence probing: unknown worker, out-of-scope worker, and standalone worker (for scoped callers) all return the identical 404; kube mode returns 503 before any lookup.
  • Known, accepted: the 502 fallback echoes a 4 KiB-bounded upstream body (same pattern as the checkpoint proxy) — upstream app bodies are not user-controlled, so this is information-disclosure-only, not injection.

Tests

  • internal/server: +41 workspace-files tests, all green (full package green) — incl. the upgrade regression (empty workspaceFileAccess403, no upstream write) and the file-root contract cases (MEMORY.md/foo rejected on read/write/download). internal/auth: +2 authorizer cases pinning the W8 decision (the write action is deliberately authorizer-allowed cross-team so the handler can hide it as 404).
  • Full go test ./... against the latest main baseline — zero regressions (the only failure is a pre-existing environment issue in internal/executor: the sandbox image lacks the unzip binary, which CI has).
  • gofmt / go vet clean; tests/check-agentteams-rename-defaults.sh passes (no legacy brand strings).

Related


摘要

L2 人类(与团队 leader)目前没有触达本团队知识库的任何路径——读没有,写也没有。能到达 worker 工作区文件的唯一通道是 Controller 的 /docker/ 代理,而它仅放行 admin/Manager:L2 人类用自己的 Matrix 令牌请求一律 403;checkpoint 端点(#1186)暴露的是文件索引与变更摘要,不是知识原文。

本 PR 通过 Controller 代理 QwenPaw 工作区文件 API 的四个端点(读 3 + 写 1 + 下载 1,共 5 条路由中的 4 个子路径),沿用 #1186 worker checkpoint 端点已确立的固定子路径代理模式:

  • GET /api/v1/workers/{name}/workspace-files/tree?path=memory|digest[/...] — 知识目录树分页列表
  • GET /api/v1/workers/{name}/workspace-files/file-metadata?path=... — 单个知识文件的 size/etag/修改时间
  • GET /api/v1/workers/{name}/workspace-files/file-content?path=...[&offset=&limit=] — 有界 UTF-8 分块读取,next_offset 续读
  • PUT /api/v1/workers/{name}/workspace-files/file-content?path=... — ETag 保护的保存(新建或更新一个知识文件)
  • GET /api/v1/workers/{name}/workspace-files/file-download?path=... — 有界流式下载(附件头透传)

可寻址面是读写同界的路径白名单MEMORY.md(长期记忆)、memory/**(日记)、digest/**(沉淀知识)。工作区内其他一切位置——SOUL.mdPROFILE.mdTODO.mdcheckpoints/skills/ 以及所有 dot 目录(.copaw/agent.json 承载 worker 的 Matrix 令牌与存储凭据)——在请求到达 worker 之前即被 400 拒绝,尽管上游路径解析器对直接请求的 dot 路径是会正常解析的。

写权限逐用户、L1 可配、显式 opt-in。 Human CRD 新增 spec.workspaceFileAccess: read | readwrite缺省为 read)。admin/manager(L1)可写任意团队的知识库;L2 人类仅可写自己团队内的 worker,且其 Human CR 显式设为 readwrite 时——空值/未设置即只读,因此 Controller 升级不会静默授予既有 L2 人类新的写权限;L1 把字段设为 readwrite 即授予某用户写权限(清空字段或设 read 即收回)。团队 leader 在本 API 上保持只读。本设计要关掉的并发风险:worker 会自动向自己的记忆文件追加,因此代理对已存在文件强制要求 If-Match(跳过 ETag 检查即丢更新),对新建文件禁止携带;上游 ETag 不匹配原样透传 409

按 review 要求拆分(独立 PR):checkpoint 代理的 scoped 调用方修复(成员名 vs 团队名比较)与 copaw-worker mcp>=1, <2 依赖 pin 各自独立成 focused PR——本 PR 不再包含,只含 workspace-files 功能。

包含内容

  1. agentteams-controller/api/v1beta1/types.go + CRD manifest:
    • HumanSpec 新增 workspaceFileAccessread | readwrite,可选,缺省为 read、写权限显式 opt-in)——L1 可逐用户授予/收回的知识库写权限。
    • config/crd/humans.agentteams.io.yaml + helm/agentteams/crds/humans.agentteams.io.yaml 同步更新(enum 校验;make check-crd-sync 绿)。纯 string 字段由结构体拷贝覆盖,无需重新生成 deepcopy。
  2. agentteams-controller/internal/auth/authorizer.go
    • 新动作 workspace-files-writeRoleHumanworker 资源:authorizer 层放行——与读路径同款的 W8 理由:中间件会解析 worker 的团队,若在此处拒绝跨团队,403 会泄露 worker 存在性(读路径隐藏为 404)。handler 才是真边界(404 隐藏 + 逐用户标志 + 白名单)。刻意独立于 update 动作,与 L2 worker 更新授权的工作互不纠缠。
    • admin/manager 本就有全权;其他角色无变化。
  3. agentteams-controller/internal/server/worker_workspace_files.go(新建):
    • WorkspaceFilesHandler — 固定子路径代理(tree / file-metadata / file-content GET+PUT / file-download)转发至共享 docker 网络内 worker 的 qwenpaw app。仅 embedded 模式;kube 模式在任何 worker 查找之前统一返回 503(不产生 404/503 存在性分裂)。
    • validateKbPath — 知识白名单:根名精确匹配(memorydigestMEMORY.md——memories/memoryX/ 不构成 memory/ 前缀)、文件根只能是单个顶层文件(MEMORY.md/foo 被拒——MEMORY.md 是一个文件不是目录,嵌套文件必须在 memory/digest/ 下)、禁 dot 段(.copaw/.git/.qwenpaw/.reme_store_v1/ 全灭)、禁 .././绝对路径/反斜杠/NUL、≤4 段、每段 ≤255 字节(对齐上游段长上限)。
    • 严格的逐子路径查询白名单(未知或重复参数 → 400,与 checkpoint 代理同款语义);limit 边界镜像上游上限(tree 1..500 = MAX_PAGE_SIZE;file-content 1..1048576 = MAX_CHUNK_SIZE);cursor 不透明透传;offset ≥ 0。
    • root=workspace 由服务端固定,不属于客户端查询面——QwenPaw 默认 root=project 是主绑定项目目录,不是知识库。上游查询仅由校验后的值重建。
    • proxyWorkspaceFileWrite(PUT file-content):写角色边界(admin/manager 任意团队;L2 人类自己团队 + 显式 workspaceFileAccess="readwrite"——空值/未设置即只读;团队 leader 403;跨团队隐藏为 404)→ file-metadata 存在性探测决定 If-Match 策略(已存在→必填;新建→禁带)→ 1 MiB body 上限 → 非空 content(空写会截断 worker 记忆文件)→ 400/404/409/422 原样透传 → 每次成功写一条审计日志(worker、路径、调用者、角色、字节数、新建/更新)。
    • file-download:上游 StreamingResponse(256 KiB 分块、Content-Disposition: attachmentContent-LengthETag)流式透传并转发这些头——客户端按 worker 的文件名保存。
    • 上游地址解析与 checkpoint 代理一致:生效容器前缀 + service.EffectiveWorkerConsolePort(system-wins env 链——冲突的 spec.env 端口被丢弃,代理恒指向容器实际监听端口)。
  4. agentteams-controller/internal/server/http.go:全 PR +5 行——handler 构造、GET .../workspace-files/{sub} 路由、PUT .../workspace-files/file-content 路由(RequireAuthz(workspace-files-write, "worker"))。
  5. 测试——共 41 个:24 个读路径(转发含 root 固定断言 / offset+limit 转发 / metadata 转发 / cursor 透传 / 白名单正向表 / 敏感路径拒绝表——.copaw/agent.json 一族 / 穿越拒绝 / 非 KB 拒绝(含前缀混淆名)/ 未知+重复查询拒绝 / 越界值 / 缺 path 拒绝 / 写子路径(file-upload、restore)经 GET 拒绝 / 非法 worker 名 / 未知 worker 404 / L2 in-scope 200(本 PR 目的用例) / L2 跨团队 404 / 团队 leader 跨团队 404 / standalone worker 对 scoped 调用方隐藏、对 admin 可见 / kube 503 先于查找 / 不可达 502 / 上游 404 透传 / 上游 500 有界 / 前缀+端口解析矩阵(8 例)/ 端到端生效前缀+端口拨号)+ 17 个写/下载(无 If-Match 新建 / 带匹配 If-Match 更新 / 已存在文件无 If-Match 在触碰 worker 前被拒 / 新建文件带 If-Match 被拒 / 409 冲突透传 / 跨团队 404 / 只读人类 403 / workspaceFileAccess(升级默认)403 / 范围内 leader 403 / admin 200 / 超 1 MiB 在触碰 worker 前被拒 / 写白名单表(含 SOUL.md 与 MEMORY.md/foo / kube 503 / 非写子路径+非法 body 400 / 下载 200 且附件头透传 / 下载跨团队 404 / 下载白名单(admin 同样适用,含 MEMORY.md/foo)。
  6. docs/usage/project-workflow-api.md(+ docs/zh-cn/usage/project-workflow-api.md):"Worker 知识库(工作区文件)端点" 节——端点表(4 端点)、读范围 + 写范围、并发(ETag)契约、写限制、workspaceFileAccess 字段、白名单、root 固定、版本门契约(用 file-metadata?path=MEMORY.md 探测区分"worker 为 QwenPaw < 2.1"与"文件不存在")、错误码表。

数据边界与安全

  • 读写同界:白名单(MEMORY.md / memory/** / digest/**)对读写同样适用。SOUL.md 属于团队 owner 域,任何人(含 L1)都不能经本 API 写——身份与灵魂边界由路径白名单强制,而不是靠角色矩阵。
  • 写范围分层:handler 团队范围(跨团队隐藏为 404——与读同款 W8 规则;authorizer 对此动作刻意不拒跨团队,因为中间件解析了 worker 的团队,此处 403 会泄露存在性)→ 逐用户 workspaceFileAccess(非显式 readwrite403;Human CR 读不到同样 403)→ leader 403 → If-Match 策略(400)→ 1 MiB / 非空 body(400)→ 白名单(400)。任何单层 fail-closed 都保护着全集群。
  • 乐观并发不可绕过:worker app 允许对已存在文件"裸覆盖"(不带 If-Match),那会静默丢掉 worker 自己在读-写间隙里的追加。代理先探测 file-metadata,该情形下强制 If-Match——客户端无法降级这个保证。
  • 白名单而非黑名单:其余一切工作区位置 fail-closed 为 400。不依赖上游自身的路径加固(禁 ..、禁绝对路径、符号链接受限于工作区、目录列表隐藏 dot 项)——直接请求的 dot 目录在上游是能解析的,这正是 Controller 侧白名单存在的原因,读与下载均有显式测试覆盖。
  • 零新凭据:L2 人类沿用现有 Matrix 访问令牌经 CompositeAuthenticatorfeat(controller): add project/workflow query API for human-visible workflows #1169)认证,范围 = accessibleTeams。无 admin 令牌、无新角色。
  • 可审计:每次成功写知识库文件均由 controller 记录日志(worker、路径、调用者、角色、字节数、新建/更新)。
  • 版本门:工作区文件路由自 QwenPaw 2.1.0 起存在(a439042c,#6504)。QwenPaw < 2.1 的 worker 对所有子路径返回上游 404,原样透传;文档化的 file-metadata?path=MEMORY.md 探测(每个已初始化工作区必有该文件)让客户端能显示"需升级 worker"而非"文件不存在"。现网 2.0.1 worker 在 2.1+ 迁移(feat(qwenpaw): upgrade QwenPaw Worker runtime to 2.2 #1174 / feat(manager): upgrade Manager runtime to QwenPaw 2.2 #1175)后自动点亮,无需重部署 Controller。
  • 存在性探测:未知 worker、越权 worker、standalone worker(对 scoped 调用方)返回完全相同的 404;kube 模式在任何查找前返回 503
  • 已知并接受502 回显 4 KiB 有界上游 body(与 checkpoint 代理同模式)——上游 app 的 body 不受用户控制,属信息披露而非注入。

测试

  • internal/server:+41 个 workspace-files 测试,全绿(整包绿)——含升级回归(空 workspaceFileAccess403,不触碰上游写)与文件根契约用例(MEMORY.md/foo 在读/写/下载三条路径均被拒)。internal/auth:+2 个 authorizer 用例钉死 W8 定案(写动作在 authorizer 层刻意放行跨团队,由 handler 隐藏为 404)。
  • 基于最新 main 基线全量 go test ./... — 零回归(唯一失败是 internal/executor 的既有环境问题:沙箱镜像缺 unzip 二进制,CI 有)。
  • gofmt / go vet 干净;tests/check-agentteams-rename-defaults.sh 通过(无遗留品牌字符串)。

相关

@LUOSENGWA LUOSENGWA changed the title feat(controller):Read-only worker knowledge base file endpoints (+ checkpoint scoped-access fix) feat(controller): add read-only worker knowledge base file endpoints (+ checkpoint scoped-access fix) Aug 31, 2026
@LUOSENGWA LUOSENGWA changed the title feat(controller): add read-only worker knowledge base file endpoints (+ checkpoint scoped-access fix) feat(controller): add team-scoped worker knowledge base file endpoints (+ checkpoint scoped-access fix) Sep 1, 2026

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

Thanks for the update. The team-scoped knowledge-base direction is reasonable, and the current CI plus the focused controller tests are green, but I think three issues should be addressed before merging:

  1. An empty workspaceFileAccess is treated as readwrite. This means upgrading the Controller silently grants every existing L2 Human a new ability to modify Worker knowledge files. Please make write access an explicit opt-in (for example, empty/default → read) and add upgrade/default regression coverage, or document and obtain agreement on the intended permission migration.

  2. validateKbPath accepts MEMORY.md/foo because it checks only the first segment. This contradicts the documented contract that MEMORY.md is one top-level file. Please require exactly one segment for file roots and add read/write/download regression cases.

  3. Please split the checkpoint scope fix and the mcp<2 dependency pin into focused PRs. The same dependency patch is currently duplicated in #1209#1212, while this PR is also becoming the base of the Human-update change; keeping these unrelated commits bundled makes the dependency and rebase order unnecessarily fragile.

Because this changes public storage and authorization behavior, please also link an issue or design discussion before merge.


感谢更新。按团队范围访问知识库的方向是合理的,当前 CI 和 Controller 定向测试也已通过,但合并前仍建议处理三个问题:

  1. workspaceFileAccess 为空时会按 readwrite 处理。这意味着 Controller 升级后,所有既有 L2 Human 会被静默授予修改 Worker 知识文件的新权限。建议将写权限改成显式开启,例如空值/默认值按 read 处理,并补充升级及默认权限回归测试;如果确实需要默认开放写权限,则应明确记录并确认该权限迁移决策。

  2. validateKbPath 目前只检查首个路径段,因此会放行 MEMORY.md/foo,与文档中“MEMORY.md 是单个顶层文件”的契约不一致。请对文件根路径要求恰好一个 segment,并补充读、写、下载三条路径的回归测试。

  3. 请将 checkpoint scope 修复和 mcp<2 依赖修复拆成独立 PR。相同的依赖 patch 目前还重复存在于 #1209#1212,同时本 PR 又开始成为 Human 更新功能的基础依赖;继续混合会让依赖关系和 rebase 顺序变得不必要地脆弱。

另外,本 PR 修改了公开的存储及授权行为,合并前请关联对应 Issue 或设计讨论。

@LUOSENGWA

LUOSENGWA commented Sep 1, 2026

Copy link
Copy Markdown
Contributor Author

@shiyiyue1102 One more note on the red CI on this PR (separate from the review points): all four controller-cr SHARD_C shards fail at copaw-runtime startup with ImportError: cannot import name 'streamablehttp_client' from 'mcp.client.streamable_http' — the known upstream mcp 2.1.1 breaking rename that currently makes main's own CI red (agentscope 1.0.18 still imports the old name; the copaw chain has no upper cap). Per the requested split, the mcp>=1, <2 pin is no longer part of this PR — it is the focused PR #1215, which is currently 17/17 green.

Suggested order: land #1215 first; I'll then rebase this PR (and #1214 / #1216, same situation — none of them touch copaw/, so the rebase is a no-op) onto the new main, and CI should go green with no further code changes on this branch.

@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 read/write proxy endpoints for worker knowledge base files (MEMORY.md, memory/**, digest/**) with a Human CRD workspaceFileAccess opt-in write gate — a well-designed, security-conscious change. Verified against main@ac22c88: allowlist validation is fail-closed (dot segments, traversal, backslash/NUL, depth and segment-length caps all rejected before reaching the worker); the scoped-caller chain uses findTeamMember(...).Name correctly (the #1214 fix pattern), standalone workers hide as 404 via TeamMatches("")==false; worker SAs cannot read other workers' files (authorizeWorkerSelfAction requireSelf); the Human CR lookup key matches the Matrix authenticator's Username: h.Name assignment; the write path defaults to read-only on empty workspaceFileAccess (no silent grant on upgrade), denies leaders, enforces the If-Match policy via existence probe, caps bodies at 1 MiB, and audit-logs every write. CRD manifests (config + helm) are enum-validated and in sync. 41 tests cover the allowlist, traversal, cross-team hiding, write-role matrix, ETag policy, and passthrough semantics. Only two informational notes (inline).

Findings

  • [Info] worker_workspace_files.go:567 — negligible create-path probe/write race (documented as accepted)
  • [Info] worker_workspace_files.go:510 — EqualFold vs case-sensitive CRD enum (harmless today)

Automated review by github-manager-bot

httputil.WriteError(w, http.StatusBadRequest, "If-Match header is required to update an existing file")
return
}
if !exists && ifMatch != "" {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[Info] Tiny TOCTOU window on the create path: if the worker app itself creates this exact path between the file-metadata probe (404 → create branch) and the bare PUT, the upstream write goes through without an ETag guard and would overwrite the just-created file (lost update). The window is milliseconds and MEMORY.md/memory//digest/ are created at workspace init, so this is negligible in practice — noting it only so the race is documented as accepted. The mandatory If-Match on the probe-existing path is the right call.

// workspaceFileAccess means "read". Defaulting to readwrite would
// silently grant every pre-existing L2 human a new ability to
// modify worker knowledge files on controller upgrade.
if !strings.EqualFold(human.Spec.WorkspaceFileAccess, "readwrite") {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[Info] strings.EqualFold compares case-insensitively while the CRD enum is case-sensitive lowercase (read/readwrite), so this is equivalent to an exact match today. Mentioning only because the lenient comparison would silently accept e.g. ReadWrite if the enum validation is ever relaxed — an exact == "readwrite" would keep the handler strictly bounded by the CRD contract.

@shiyiyue1102

Copy link
Copy Markdown
Collaborator

Re-reviewed head 090c09a4. The previous default-permission and MEMORY.md/foo findings are fixed, the unrelated changes have been split out, and #1217 now records the design discussion. CI and the local internal/server / internal/auth tests pass. One blocking issue remains:

[P1] The registered PUT route rejects every authorized write with HTTP 400. internal/server/http.go:132 registers the literal /api/v1/workers/{name}/workspace-files/file-content, but proxyWorkspaceFileWrite reads r.PathValue("sub") and rejects it unless it equals file-content. This route has no {sub} capture, so the value is always empty. Admins and humans with workspaceFileAccess=readwrite cannot save any file.

I reproduced this through the actual NewHTTPServer(...).Mux with an admin caller: PUT /api/v1/workers/market-writer/workspace-files/file-content?path=MEMORY.md returns 400 {"message":"unsupported workspace file write subpath"} before any worker lookup. The current write-test helper manually calls SetPathValue("sub", sub) and invokes the handler directly, masking the routing bug.

Please keep the fixed PUT route and remove the handler's dependency on a nonexistent sub path parameter, then add a regression test through the registered HTTP route. This needs fixing before merge.


已复查最新提交 090c09a4:上轮的默认权限和 MEMORY.md/foo 问题已修复,无关改动已拆出,也已通过 #1217 记录设计讨论。CI 及本地 internal/server / internal/auth 测试通过,但仍有一个阻塞问题:

[P1] 实际注册的 PUT 路由会让所有通过授权的写请求返回 HTTP 400。 internal/server/http.go:132 注册的是固定路径 /api/v1/workers/{name}/workspace-files/file-content,而 proxyWorkspaceFileWrite 读取 r.PathValue("sub") 并要求其为 file-content。该路由没有 {sub} 占位符,因此取值始终为空,Admin 和拥有 workspaceFileAccess=readwrite 的 Human 都无法保存文件。

已使用 admin 身份通过实际 NewHTTPServer(...).Mux 复现:PUT /api/v1/workers/market-writer/workspace-files/file-content?path=MEMORY.md 在查询 Worker 前即返回 400 {"message":"unsupported workspace file write subpath"}。现有写测试手动调用 SetPathValue("sub", sub) 后直接执行 handler,掩盖了路由问题。

建议保留固定 PUT 路由,移除 handler 对不存在的 sub 路径参数的依赖,并补充经过实际注册路由的回归测试。修复前不建议合并。

…L2 humans

L2 humans have no read path to their team's knowledge base: the /docker/
proxy is admin/Manager-only, and the checkpoint endpoints expose file
indexes, not the knowledge text. Proxy three read-only QwenPaw workspace
file endpoints (QwenPaw >= 2.1) through the Controller using the fixed-
subpath pattern from the worker checkpoint proxy:

  GET /api/v1/workers/{name}/workspace-files/tree
  GET /api/v1/workers/{name}/workspace-files/file-metadata
  GET /api/v1/workers/{name}/workspace-files/file-content

- Path allowlist (not a denylist): only MEMORY.md, memory/** and
  digest/** are addressable. Every other workspace location - SOUL.md,
  PROFILE.md, TODO.md, checkpoints/, skills/ and all dot directories
  (.copaw/agent.json carries the worker's credentials) - is rejected
  with 400 before the request reaches the worker. The upstream path
  resolver serves directly requested dot paths, which is why the
  allowlist lives in the Controller; covered by an explicit test.
- root=workspace pinned server-side (the QwenPaw default root=project is
  the primary bound project directory, not the knowledge base); the
  client query surface has no root parameter and the upstream query is
  rebuilt from validated values only.
- Strict per-subpath query whitelist (unknown/duplicate parameters are
  400); limit bounds mirror the upstream caps (tree 1..500, file-content
  1..1048576); cursor is an opaque passthrough; offset >= 0.
- Same address resolution as the checkpoint proxy: effective container
  prefix + system-wins console port (a conflicting spec.env port is
  discarded, so the proxy always dials the port the container listens
  on). Embedded mode only; kube mode returns a uniform 503 before any
  worker lookup.
- Authorization reuses the existing chain: RequireAuthz(ActionGet,
  "worker") (RoleHuman already granted by the L2 authentication work)
  plus the handler-level team-scope check, so unknown, out-of-scope and
  standalone workers are all hidden as 404. No authorizer changes.
- Version gate: a worker on QwenPaw < 2.1 has no workspace file router;
  the upstream 404 is passed through verbatim and the documented
  file-metadata?path=MEMORY.md probe distinguishes "worker upgrade
  required" from "file missing".
- Write endpoints (file-content PUT, file-upload) and binary streaming
  (file-download) are not in the subpath whitelist.

Tests: 24 new (forwarding with root-pinning assertion, allowlist
positive/negative tables, sensitive-path and traversal rejection,
write-subpath rejection, scope 200/404 matrix, kube 503 before lookup,
unreachable 502, 404 passthrough, bounded 500, prefix+port matrix).
go test ./... green against the main baseline (only pre-existing failure
is the sandbox-missing unzip binary in internal/executor); gofmt/vet
clean; rename gate passes.
…e download

- PUT /api/v1/workers/{name}/workspace-files/file-content: L1 writes any
  team; L2 humans write only own teams while Human CR
  workspaceFileAccess != "read" (default readwrite, L1-locked to read);
  team leaders stay read-only; cross-team hides as 404 (W8)
- authorizer: new workspace-files-write action, deliberately
  authorizer-allowed cross-team for RoleHuman — the middleware resolves
  the worker's team, and a 403 there would leak worker existence (the
  read path hides cross-team as 404; the handler is the real boundary,
  pinned by authorizer tests); separate action so it does not collide
  with the L2 worker-update PR
- If-Match mandatory for existing files (lost-update guard vs worker
  auto-append), forbidden for new files; 1 MiB write cap; empty content
  rejected; every write audit-logged
- GET /api/v1/workers/{name}/workspace-files/file-download: bounded stream
  with attachment headers forwarded, same KB allowlist and scope
- Human CRD gains workspaceFileAccess (read|readwrite); CRD synced to Helm
- 41 tests green (24 read + 16 write/download + 1 checkpoint regression +
  2 authorizer W8 cases); full controller suite green (1 pre-existing
  env-only failure: executor ZIP test needs unzip, present in CI)
… route

The registered write route is the fixed literal
/api/v1/workers/{name}/workspace-files/file-content (http.go) with no {sub}
capture, so r.PathValue("sub") in proxyWorkspaceFileWrite was always ""
and every authorized write (admin / human with
workspaceFileAccess=readwrite) was rejected with 400
"unsupported workspace file write subpath" before any worker lookup.

- keep the fixed PUT route as the single source of truth for the write
  subpath and drop the handler's dependency on the nonexistent sub path
  value (the now-unreferenced workspaceFileWriteSubpaths map is removed)
- add TestWorkspaceFilesWrite_RouteAcceptsAuthorizedWrite, which drives the
  request through the actual NewHTTPServer(...).Mux registration (no
  SetPathValue shims): an admin PUT reaches the upstream probe stage
  (502 against the dead pod URL in tests) instead of the phantom 400, and
  an unknown worker yields the 404 from the worker lookup
@LUOSENGWA
LUOSENGWA force-pushed the feat/worker-workspace-files branch from 090c09a to 53e2601 Compare September 11, 2026 06:44
@shiyiyue1102

Copy link
Copy Markdown
Collaborator

Re-reviewed 53e26014: the fixed PUT route no longer depends on a nonexistent sub capture, and the new regression test exercises the actual NewHTTPServer mux. Local internal/server and internal/auth tests pass, so the P1 routing finding is resolved.

Merge is still waiting on CI: the QwenPaw/QwenPaw SHARD_C job failed in test-18-team-config-verify, with Leader runtime context has team_leader role (expected to contain: 'member.role: team_leader').

Job: https://github.com/agentscope-ai/AgentTeams/actions/runs/34571257155/job/103176490682

Could you investigate this assertion and fix it, or establish that it is transient and rerun the failed job? The failure alone does not establish a regression from the workspace-files changes; we still need a passing current-head CI run before merging.

@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. Re-reviewed head 53e2601: the fixed PUT route no longer reads a nonexistent sub capture, with regression coverage through the actual NewHTTPServer mux. Default write access remains opt-in, MEMORY.md is restricted to a single top-level file, unrelated changes were split out, and #1217 documents the design. Local server/auth tests pass. Both previously failing integration jobs passed on the same-head rerun; current checks are successful with the expected conditional skips. The blocking findings from my previous review are addressed.

@shiyiyue1102
shiyiyue1102 merged commit fad187b into agentscope-ai:main Sep 11, 2026
37 of 39 checks passed
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