fix(controller): grant Matrix power levels to human members on room join - #1210
fix(controller): grant Matrix power levels to human members on room join#1210LUOSENGWA wants to merge 3 commits into
Conversation
128e0d8 to
c4191f1
Compare
shiyiyue1102
left a comment
There was a problem hiding this comment.
Thanks for the update. The goal is valid, but there are several blocking authorization/state issues:
-
GET /rooms/{roomId}/state/{eventType}returns the state content object directly by default.GetRoomStateinstead unwraps acontentfield, and its test mocks a full event envelope. A compliant homeserver response is therefore read as nil. Please decode the actual wire response directly and update the test. -
EnsureRoomPowerLevelimplements “at least” semantics. If a Human changes from permissionLevel 1 (100) to 2 (50), the existing level 100 is retained. Please reconcile the exact desired level and add a 100→50 revocation test. -
The write reconstructs the power-level content and drops fields including
events,invite,notifications, and possible extension fields. Please preserve the complete existing content and mutate only the targetusersentry. -
Level 50 does not match the documented authority. Matrix defaults
kick,ban, andredactto 50, so an L2/L3 Human can act against lower-level workers. Please either explicitly accept/document this authority or configure thresholds that enforce the intended permission model.
The project-room path should also be wired into the runtime-facing project-management reference; currently the Manager is never instructed to pass --grant-admin. Please add coverage and resolve or rerun the failing qwenpaw/qwenpaw integration shard before re-review.
感谢更新。目标合理,但当前有几个阻塞性的授权与状态问题:
-
GET /rooms/{roomId}/state/{eventType}默认直接返回 state content。当前GetRoomState又读取了一层content,测试也错误模拟成完整 event,导致兼容规范的 homeserver 响应会被解析为 nil。请按真实响应格式解析并更新测试。 -
EnsureRoomPowerLevel使用“至少达到目标等级”的语义。Human 从 permissionLevel 1(100)降到 2(50)时仍会保留 100。请同步精确的目标等级,并增加 100→50 的权限撤销测试。 -
写回时重新构造了 power-level content,会丢失
events、invite、notifications和扩展字段。请保留完整的原始 content,只修改目标users条目。 -
Level 50 的实际权限与文档描述不符。Matrix 的
kick、ban、redact默认门槛都是 50,因此 L2/L3 Human 可以操作低权限 Worker。请明确接受并记录这项权限,或者配置阈值以实现预期权限模型。
项目房间路径也需要接入 Agent 实际读取的 project-management reference;目前 Manager 不会被指示传递 --grant-admin。请补充覆盖,并在复审前处理或重跑失败的 qwenpaw/qwenpaw 集成测试。
c4191f1 to
146414d
Compare
|
@shiyiyue1102 All five points addressed — the force-push carries the fixes (new head 1. 2. "At least" semantics. Correct. The 3. Write drops fields. Correct. The old write rebuilt the power-level content from a hardcoded whitelist, silently dropping 4. Level-50 authority. Accepted and documented, as one of the two options you raised. Matrix defaults gate 5. Project-room path not wired into the runtime reference. Correct — the flag was dead code for the Manager. The Manager reads The force-push regenerates the full CI, including the qwenpaw/qwenpaw integration shard you asked to be resolved or re-run. 感谢反馈,五点全部修复(force-push 携带修复,新 head 1. 2. "至少"语义——属实。 3. 写回丢字段——属实。旧写回按硬编码白名单重建 power-level content,静默丢掉 4. Level-50 权限——按你给的两个选项之一处理:明确接受并文档化。Matrix 默认把 5. 项目房间路径未接 runtime 参考——属实,flag 对 Manager 是死代码。Manager 运行时读的是 force-push 会重新生成全部 CI,包括你要求处理或重跑的 qwenpaw/qwenpaw 集成 shard。 |
oss-maintainer
left a comment
There was a problem hiding this comment.
Summary
Grants Matrix power levels to human members on room join via a declarative, self-healing reconcile: EnsureRoomPowerLevel does a read-modify-write with full m.room.power_levels content preservation, mapping spec.permissionLevel 1 -> 100 and 2/3 -> 50, and the observed-room pass backfills legacy rooms without manual migration. The controller logic is correct and idempotent (exact-match short-circuit avoids writes in steady state; per-room errors follow the existing non-fatal reconcile policy), and test coverage is thorough. One actionable gap in create-project.sh (see inline): --grant-admin users get a power-level override but are never invited to the room, so they cannot join to exercise it.
Findings
- [Warning] manager/agent/skills/project-management/scripts/create-project.sh:148 —
--grant-adminusers are added topower_level_content_override.usersbut never added toINVITE_LISTnor invited/auto-joined after creation, so the grant has no effect until they join via another path. - [Info] agentteams-controller/internal/matrix/client.go:760 —
GetRoomStateomits the trailing state key in the URL when empty, whileSetRoomStatealways includes it; consider aligning for strict homeservers. - [Info] agentteams-controller/internal/service/provisioner.go:1074 — shallow copy shares nested maps between
contentand the fetched state; safe today, but a defensive copy would guard against future caching. - [Info] agentteams-controller/internal/controller/human_reconcile_rooms.go:44 — healing pass issues one GET per desired room every reconcile cycle; documented trade-off, could be optimized later with a status/generation check.
With the create-project.sh invite gap addressed, this PR looks ready to merge.
Automated review by github-manager-bot
| @@ -129,7 +148,7 @@ ROOM_RESP=$(curl -sf -X POST ${AGENTTEAMS_MATRIX_URL}/_matrix/client/v3/createRo | |||
| "power_level_content_override": { | |||
There was a problem hiding this comment.
Users passed via --grant-admin are added to power_level_content_override.users at level 100, but they are never appended to INVITE_LIST and no post-creation /invite or auto-join step is added for them (unlike the admin user which gets both an invite entry and an explicit /invite + auto-join block). The grant-admin users will therefore not be members of the project room after creation — the power level entry only takes effect when they eventually join through some other path. Fix: inside the --grant-admin loop, also append each ${grant_id} to INVITE_LIST (before the closing ] is added on the existing line 117 equivalent), and add a post-creation invite/auto-join step analogous to the existing admin auto-join block.
| @@ -753,6 +760,38 @@ func (c *TuwunelClient) SetRoomState(ctx context.Context, roomID, eventType, sta | |||
| return nil | |||
There was a problem hiding this comment.
GetRoomState builds the URL as state/{eventType} when stateKey is empty, while the existing SetRoomState always uses state/{eventType}/{stateKey} (producing a trailing slash for empty stateKey, e.g. state/m.room.power_levels/). Both forms are valid per the Matrix spec and most homeservers accept either, but the inconsistency could matter against strict implementations. Consider aligning GetRoomState to the same state/%s/%s pattern used by SetRoomState for symmetry, or document the deliberate difference.
| @@ -1074,6 +1074,45 @@ func (p *Provisioner) EnsureRoomNonMember(ctx context.Context, roomID, userID, r | |||
| return p.matrix.KickFromRoom(ctx, roomID, userID, reason) | |||
There was a problem hiding this comment.
The shallow copy content[k] = v means nested objects (e.g. events, notifications maps) are shared between content and the cur map returned by GetRoomState. This is safe today because cur is freshly unmarshalled per call and not cached, and only the top-level users key is mutated. However, if a future caller or middleware caches GetRoomState results, the shared nested maps could be silently corrupted. A defensive deep-copy of cur (or at minimum of the users sub-map before mutation) would be more robust against future refactoring.
| @@ -44,29 +44,43 @@ func (r *HumanReconciler) reconcileHumanRooms(ctx context.Context, s *humanScope | |||
| next := make([]string, 0, len(h.Status.Rooms)+len(desired)) | |||
There was a problem hiding this comment.
The healing pass calls EnsureRoomPowerLevel (one GET, conditional PUT) for every desired room on every reconcile cycle, even when nothing has changed. For a human in N rooms this adds N Matrix API GETs per 5-minute requeue. The PR documents this trade-off and the exact-match short-circuit avoids writes, but consider adding a status annotation (e.g. a lastPowerLevelGranted map in status) or a generation check to skip the GET when spec.permissionLevel hasn't changed since the last successful grant. This is a future optimization, not a blocker.
Humans are invited into worker/team rooms by the human reconciler and into project rooms by the Manager, but nothing ever grants them a Matrix power level: they sit at the implicit 0 and 403 on every room operation. Make the grant declarative and self-healing. - Human room reconcile ensures the mapped level in every desired room (new + already-observed; the observed pass heals legacy rooms on the first cycle after deployment) - Level mapping from spec.permissionLevel: 1 -> 100 (co-owner), 2/3 -> 50 (Matrix default member authority; the level-50 kick/ban/ redact scope over sub-50 members is explicitly accepted and documented in docs/design/room-power-levels.md) - EnsureRoomPowerLevel reconciles only the target users entry to exactly the mapped level (a demotion 100 -> 50 actually lowers it) and writes back the complete existing content with only that entry mutated -- events/invite/notifications/extension fields survive; no write when already at exactly that level - matrix.Client.GetRoomState reads a state event's content with the admin identity, decoding the wire response directly (the state endpoint returns the content object, not an event envelope); 404 -> (nil, nil) - create-project.sh: optional --grant-admin <u1,u2,...> lifts the given humans to level 100 in the project room's creation-time override, and the runtime-facing project-management reference now instructs the Manager to pass it; tests/check-create-project-grant-admin.sh (wired into helm-lint) keeps the flag implemented and documented Tests: wire-format GetRoomState mock, exact-match no-write, 100 -> 50 demotion/revocation, extension-field preservation, merge preserving other users, legacy room, read error, state without a users map, second grant preserving the first; controller-level mapping/healing/non-fatal cases.
146414d to
4b474a9
Compare
oss-maintainer
left a comment
There was a problem hiding this comment.
Summary
Re-review of the amended head (4b474a9): all four blocking items from the maintainer's CHANGES_REQUESTED review are addressed, and the previous review's --grant-admin invite gap is fixed. Verified against the code, and go test ./internal/matrix/ ./internal/service/ ./internal/controller/ is green locally; the previously failing integration-tests (controller-cr, SHARD_C_TESTS, qwenpaw, qwenpaw) CI shard now passes on this PR.
Verification of the maintainer's blocking items:
- Wire decoding —
GetRoomState(internal/matrix/client.go) decodes the state-endpoint response as-is as the content object (no envelope unwrapping), 404 →(nil, nil), and the state-key path segment is now always included (matchingSetRoomState). - Exact-level semantics —
EnsureRoomPowerLevelwrites exactly the mapped level and performs no write when the user is already at exactly that level; demotion is covered byTestEnsureRoomPowerLevel_DemotionRevokesLevel(100 → 50 actually lowers the user). - Full-content preservation — the reconcile copies every existing content key and mutates only the target
usersentry, soevents/invite/notifications/extension fields survive. - Level-50 authority — explicitly documented and accepted with rationale in
docs/design/room-power-levels.md.
Additional verification:
create-project.shnow adds--grant-adminusers to the room invite list (previous review's Warning resolved) and gives them the level-100 override at creation.- The runtime-facing project-management reference now instructs
--grant-admin, and a contract check is registered in the helm-lint workflow. - CI: all checks pass, including the previously failing qwenpaw/qwenpaw integration shard.
Automated review by github-manager-bot
Grant Matrix power levels to human members on room join
Summary
Humans are invited into worker and team rooms by the human reconciler, and into project rooms by the Manager — but nothing ever grants them a Matrix power level. They sit at the implicit level 0 in every room they belong to, and 403 on every room operation: renaming the room, inviting a colleague, basic housekeeping. Rooms created before power levels existed at all fall back to the homeserver's strict defaults; the room works for the bots that own it and is unusable for the person it is meant for.
This PR makes the grant declarative and self-healing:
spec.permissionLevel: level 1 (admin equivalent) → 100, co-owning the rooms; levels 2/3 (team/worker scoped) → 50, Matrix's default member authority. That includes rename, invite, and kick/ban/redact of members strictly below 50 (the workers — the manager/leader at 100 can never be acted against); the design doc documents and accepts this authority explicitly rather than raising the thresholds.Provisioner.EnsureRoomPowerLevelreads the currentm.room.power_levels(newmatrix.Client.GetRoomState, admin identity; the state endpoint returns the content object directly and is decoded as such; 404 = state never set → empty), reconciles only the target user's entry to exactly the mapped level (a demotion 100 → 50 actually lowers it), and writes back the complete existing content with only that entry mutated —events,invite,notifications,users_default,state_default,ban/kick/redactand any extension field survive untouched. No write when the user already has exactly that level. Steady state is one GET per room per cycle and zero writes.Project rooms are created by the Manager, not the Controller.
create-project.shalready writes apower_level_content_override(manager + admin at 100, workers at 0) but had no way to lift a human operator; it now accepts an optional--grant-admin <u1,u2,...>that adds the given users (local parts or full Matrix IDs) at level 100 to the creation-time override.What's included
internal/matrix/client.go(+ test) —Client.GetRoomState: reads a state event's content with the admin identity, decoding the wire response directly (the state endpoint returns the content object, not an event envelope); missing state yields(nil, nil), not an error.internal/service/provisioner.go—EnsureRoomPowerLevel(exact-level reconciliation, full-content-preserving write, idempotent).internal/service/interfaces.go— method on the human provisioner interface.internal/controller/human_reconcile_rooms.go— the desired-room loop grants the level in every room, new and observed.internal/controller/human_controller_test.go— level mapping (1→100, 2→50), observed-room healing, non-fatal failure.internal/service/provisioner_power_test.go(new) — legacy room, merge preserving other users, extension fields (events/invite/notifications) surviving the write, exact-match no-write, 100 → 50 demotion actually lowering the user, read error, state without ausersmap, second grant preserving the first.test/testutil/mocks/— interface method + call recording on both provisioner mocks; the team-room test fake gains a stateful, JSON-round-tripped power-level store.manager/agent/skills/project-management/scripts/create-project.sh— optional--grant-admin.manager/agent/skills/project-management/references/create-project.md— the runtime-facing reference now instructs the Manager to pass--grant-adminfor human operators who need room administration.tests/check-create-project-grant-admin.sh(+ registration in.github/workflows/helm-lint.yml) — contract check that the flag stays implemented in the script and documented in the runtime reference.docs/design/room-power-levels.md— design contract, including the explicit acceptance of the level-50 authority.Data boundary
spec.permissionLevel.--grant-adminexisted) are healed by a one-time Manager-side operation, documented in the design doc — not automated here, since the Controller does not create or own project rooms.Tests
go test ./internal/matrix/ ./internal/service/ ./internal/controller/green.go test ./internal/... ./cmd/...: all packages ok except a pre-existing environment issue ininternal/executor(unzip binary absent in the test container) untouched by this diff.gofmt/go vetclean on the diff;bash -nand the new contract check pass.Related
docs/design/room-power-levels.md.人类成员房间权限授予(room join 时)
摘要
human 被 human reconciler 邀请进 worker/team 房间、被 Manager 拉进项目群——但没有任何步骤授予他们 Matrix 权限等级。他们在所有房间里恒为隐式 0 级,任何房间操作(重命名、邀请、基本维护)都 403。更早创建、从未设过 power levels 的房间则回退到 homeserver 严格默认值。房间对拥有它的 bot 好用,对真正要用的人不可用。
本 PR 把授权变成声明式且自愈:
spec.permissionLevel:level 1(admin 等价)→ 100(co-owner);level 2/3(团队/worker 范围)→ 50(Matrix 默认成员权限)。这包括 rename、invite,以及对严格低于 50 级成员(即 worker;100 级的 manager/leader 永远不可被其操作)的 kick/ban/redact——设计文档已明确记录并接受该权限,而非抬高阈值。Provisioner.EnsureRoomPowerLevel读当前m.room.power_levels(新matrix.Client.GetRoomState,admin 身份;state 端点直接返回 content 对象、按此解码;404 = 从未设置 → 空态),只把目标用户的条目对账到精确的映射级别(降级 100 → 50 就是真的降),写回时保留完整既有内容、仅改动该条目——events、invite、notifications、users_default、state_default、ban/kick/redact及任何扩展字段原样存活。用户已在该精确级别时不写。稳态 = 每房间每周期 1 次 GET、0 写。项目群由 Manager 而非 Controller 创建。
create-project.sh本就写power_level_content_override(manager + admin 100、worker 0),但没有给 human 操作员的通道;现新增可选参数--grant-admin <u1,u2,...>,把指定用户(localpart 或完整 Matrix ID)以 100 级加入建群时的 override。包含内容
internal/matrix/client.go(+测试)—Client.GetRoomState:admin 身份读 state event 的 content,直接解码 wire 响应(state 端点返回 content 对象、非 event 信封);state 缺失返回(nil, nil)而非错误。internal/service/provisioner.go—EnsureRoomPowerLevel(精确级别对账、全量内容保留写、幂等)。internal/service/interfaces.go— human provisioner 接口方法。internal/controller/human_reconcile_rooms.go— desired 房间循环对每个房间(新 + 已观察)授权。internal/controller/human_controller_test.go— 级别映射(1→100、2→50)、存量房间自愈、失败非致命。internal/service/provisioner_power_test.go(新)— legacy 房间、合并保留其他用户、扩展字段(events/invite/notifications)写后存活、精确匹配不写、100 → 50 降级真的把用户降下来、读错误、无 users 表的 state、第二次授权不覆盖第一次。test/testutil/mocks/— 两个 provisioner mock 的接口方法与调用记录;team-room 测试 fake 增加 stateful、JSON 往返仿真的 power-level 存储。manager/agent/skills/project-management/scripts/create-project.sh— 可选--grant-admin。manager/agent/skills/project-management/references/create-project.md— runtime 面向的参考文档现在指示 Manager 为需要房间管理的 human 操作员传--grant-admin。tests/check-create-project-grant-admin.sh(+.github/workflows/helm-lint.yml注册)— 契约检查:该 flag 在脚本中保持实现、在 runtime 参考文档中保持有文档。docs/design/room-power-levels.md— 设计契约,含对 50 级权限的明确接受说明。数据边界
spec.permissionLevel。--grant-admin出现前创建)由 Manager 端一次性操作修复(设计文档有记录)——不在此 PR 自动化,因为 Controller 不创建也不拥有项目群。测试
go test ./internal/matrix/ ./internal/service/ ./internal/controller/全绿。go test ./internal/... ./cmd/...:除internal/executor预存环境问题(测试容器缺 unzip,与本 diff 无关)外全部 ok。diff 内gofmt/go vet干净;bash -n与新增契约检查通过。相关
docs/design/room-power-levels.md。