docs(design): lead-agent identities and canvas access for external agents (#98)#1800
docs(design): lead-agent identities and canvas access for external agents (#98)#1800jaylfc wants to merge 2 commits into
Conversation
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
📝 WalkthroughWalkthroughAdds a design document for project-scoped lead-agent identities, canvas authorization and attribution, credential migration, write limits, implementation slices, edge cases, decisions, and non-goals. ChangesLead Agent Canvas Access
Estimated code review effort: 1 (Trivial) | ~5 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/design/lead-agent-identity-and-canvas-access.md`:
- Around line 255-262: Resolve the v1 human-member authorization policy
consistently across the session-path helper, authorization matrix, and tests:
allow any HUMAN project member to write while preserving owner/admin access and
existence-hiding behavior for unauthorized users. Update the relevant
`_get_owned_project`-based logic and matrix expectations, including the
corresponding section around the forward-rule documentation, so member access no
longer returns 404.
- Around line 346-355: Update the migration design around the supersession flow
to define an atomic transaction that releases the seeded handle and assigns it
to the consent-minted successor without collision. Explicitly rebind live
task/element assignments, project-member rows, and grants to the successor;
revoke old tokens and credentials; and retain historical attribution against the
original canonical ID. Ensure all identity, handle, reference, and credential
changes commit or roll back together.
- Around line 387-403: Clarify the “D6. Limits on agent canvas writes” section
by defining separate rate-limit keys/windows for agent canvas writes and
snapshot.png render requests. Explicitly state whether agent and human
principals are included in each limit, removing the current contradiction that
limits apply to both principals while human sessions are exempt and renders
share the write window.
- Around line 199-203: The canvas_write API contract must not expose full
element bodies to write-only callers. Update the POST/PATCH /canvas/elements
response behavior described in the canvas_write section to return only a minimal
acknowledgment, or gate the full element response on canvas_read, and add
coverage for the write-only path.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: b25012e8-491d-4ee1-b6d8-cf17b105dc4e
📒 Files selected for processing (1)
docs/design/lead-agent-identity-and-canvas-access.md
| - `canvas_read`: list elements, receive the SSE stream, fetch snapshots, for | ||
| the token's bound project only. | ||
| - `canvas_write`: create, update, delete canvas elements on the bound | ||
| project, subject to the membership flag in D3. Write does not imply read; | ||
| leads request both. |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
Avoid returning full element bodies from canvas_write routes. POST/PATCH /canvas/elements return {"element": ...}, which makes write-only access read-capable on update. Return only a minimal acknowledgment here, or require canvas_read for the full element response, and add a test for the write-only path.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/design/lead-agent-identity-and-canvas-access.md` around lines 199 - 203,
The canvas_write API contract must not expose full element bodies to write-only
callers. Update the POST/PATCH /canvas/elements response behavior described in
the canvas_write section to return only a minimal acknowledgment, or gate the
full element response on canvas_read, and add coverage for the write-only path.
| - **Session path**: resolve `request.state.user_id`; apply | ||
| `_get_owned_project` semantics (owner or admin, existence-hiding 404 for | ||
| everyone else). Decision for USER sessions: a human needs no checkbox and | ||
| no scope; the gate is project visibility. Today that means owner/admin, | ||
| which is unchanged behavior for jay and simultaneously closes the | ||
| any-authenticated-user hole documented above. The forward rule, recorded | ||
| now for multi-user: any HUMAN project member may write; the | ||
| `can_edit_canvas` flag governs agent principals only. |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Resolve the human-member authorization contradiction.
Lines [255-262] restrict human access to project owners/admins, while the stated forward rule allows any human project member to write; the matrix still returns 404 for those members. Make the v1 policy explicit and align the helper, matrix, and tests.
Also applies to: 291-296
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/design/lead-agent-identity-and-canvas-access.md` around lines 255 - 262,
Resolve the v1 human-member authorization policy consistently across the
session-path helper, authorization matrix, and tests: allow any HUMAN project
member to write while preserving owner/admin access and existence-hiding
behavior for unauthorized users. Update the relevant `_get_owned_project`-based
logic and matrix expectations, including the corresponding section around the
forward-rule documentation, so member access no longer returns 404.
| One verified wrinkle the migration must handle: consent-minted records have | ||
| `handle=""`, and a2a send 403s an agent with no handle | ||
| (`tinyagentos/routes/a2a_bus.py:171-175`). Additionally, the lead handles may | ||
| ALREADY exist as seeded internal identities | ||
| (`tinyagentos/routes/agent_registry.py:170-175`). The migration therefore | ||
| supersedes: suspend the old seeded row (freeing the handle), then set the | ||
| handle on the new consent-minted identity (the store supports it, | ||
| `tinyagentos/agent_registry_store.py:573-605`; slice 6 adds the small admin | ||
| surface). Supersede-not-mutate matches the reattach rule in | ||
| `docs/design/external-agent-onboarding.md`. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Make identity supersession atomic and rebind persisted references.
The successor has a different canonical ID, so the migration must explicitly enumerate what happens to existing task/element assignments, project-member rows, grants, tokens, and historical attribution. Reassign live references while preserving historical authorship, revoke the old credentials, and perform handle release/reassignment atomically; otherwise leads can lose assignments or the new handle can collide during migration.
Also applies to: 365-385
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/design/lead-agent-identity-and-canvas-access.md` around lines 346 - 355,
Update the migration design around the supersession flow to define an atomic
transaction that releases the seeded handle and assigns it to the consent-minted
successor without collision. Explicitly rebind live task/element assignments,
project-member rows, and grants to the successor; revoke old tokens and
credentials; and retain historical attribution against the original canonical
ID. Ensure all identity, handle, reference, and credential changes commit or
roll back together.
| ### D6. Limits on agent canvas writes | ||
|
|
||
| Verified: no payload size cap and no write rate limit exist today. Agents | ||
| are exactly the callers that can emit a pathological loop, so writes get | ||
| bounds (applied to BOTH principals, humans included, generously): | ||
|
|
||
| - Serialized `payload` cap of 64 KiB per element, checked in the route before | ||
| the store call, 413 on breach. Covers create and patch. | ||
| - Per-principal fixed-window rate limit on agent writes: 60 writes per | ||
| minute per canonical_id per project, 429 on breach, reusing the fixed | ||
| window pattern already in the tree (login limiter in `routes/auth.py`, | ||
| pairing claim limiter in `routes/cluster.py`). Human sessions are not rate | ||
| limited in v1. | ||
| - `snapshot.png` renders and writes a file server-side on every call | ||
| (`tinyagentos/routes/project_canvas.py:127-133`); agent calls share the | ||
| same 60/min window so a polling agent cannot turn the renderer into a disk | ||
| hose. |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Define separate, consistent limits for writes and rendering.
The plan says limits apply to both principals, then says human sessions are not rate-limited. It also defines an agent write window but says snapshot.png reads share it. Specify separate keys/windows for agent writes and render requests, and state whether humans are included in each.
🧰 Tools
🪛 LanguageTool
[style] ~390-~390: Consider an alternative for the overused word “exactly”.
Context: ...rite rate limit exist today. Agents are exactly the callers that can emit a pathologica...
(EXACTLY_PRECISELY)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/design/lead-agent-identity-and-canvas-access.md` around lines 387 - 403,
Clarify the “D6. Limits on agent canvas writes” section by defining separate
rate-limit keys/windows for agent canvas writes and snapshot.png render
requests. Explicitly state whether agent and human principals are included in
each limit, removing the current contradiction that limits apply to both
principals while human sessions are exempt and renders share the write window.
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
|
|
||
| - `projects` gains `lead_member_id TEXT NULL` referencing a member of that | ||
| project (validated in the route, not by FK, matching the store's style in | ||
| `tinyagentos/projects/project_store.py`). |
There was a problem hiding this comment.
WARNING: lead_member_id can dangle after member removal — no cleanup path defined
The column is validated only at SET time (the new PATCH 404s on a member id not in the project), but nothing requires lead_member_id to be cleared when that member is later removed from the project or its project_members row is deleted. Consumers reading lead_member_id would then dereference a non-existent member, so a project could retain a "lead" who is no longer a participant, and the a2a quiet-filter / default-assignee pointer could point at a ghost. Specify the cleanup: clear lead_member_id when a member is removed, or have consumers validate the id against live members on read.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| their UI call site. | ||
| - Consumers move: the a2a quiet-filter and channel `settings.leads` sync | ||
| currently select members `WHERE is_lead = 1` | ||
| (`tinyagentos/projects/a2a.py:108-146`); they read `lead_member_id` |
There was a problem hiding this comment.
SUGGESTION: Regenerate any denormalized settings.leads copy during the is_lead → lead_member_id migration
The design moves consumers to read lead_member_id, but settings.leads (the persisted channel/quiet-filter list) is described only as "read the column" — it does not state who rewrites the stored settings.leads value when lead_member_id changes. If settings.leads is a denormalized snapshot written by a sync job, that job must be updated to source from lead_member_id; otherwise the a2a channel membership and quiet-filter can silently diverge from the new pointer until something re-syncs it. Call out the sync-write side explicitly in slice 6.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
Code Review SummaryStatus: 2 New Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed (1 file)Fix these issues in Kilo Cloud Reviewed by hy3-20260706:free · Input: 75.1K · Output: 13.7K · Cached: 222.5K |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/design/lead-agent-identity-and-canvas-access.md`:
- Around line 465-478: Update the lead-member design to require clearing
projects.lead_member_id when its referenced member is removed or suspended,
preventing a dangling lead pointer. Specify that the PATCH
/api/projects/{project_id}/lead route rejects suspended or inactive members,
while still allowing null to unset the lead.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 77dd7f62-1baf-43d2-aff7-4af07c8a5411
📒 Files selected for processing (1)
docs/design/lead-agent-identity-and-canvas-access.md
| - `projects` gains `lead_member_id TEXT NULL` referencing a member of that | ||
| project (validated in the route, not by FK, matching the store's style in | ||
| `tinyagentos/projects/project_store.py`). | ||
| - New endpoint `PATCH /api/projects/{project_id}/lead` with body | ||
| `{"member_id": "<id>" | null}`, session-only (owner or admin, same gate as | ||
| the members routes), 404 for a member id not in the project. It replaces | ||
| the per-member route `PATCH /api/projects/{pid}/members/{mid}/lead` | ||
| (`tinyagentos/routes/projects.py:298`) and `set_member_lead` | ||
| (`tinyagentos/projects/project_store.py:80-84`), which are removed with | ||
| their UI call site. | ||
| - Consumers move: the a2a quiet-filter and channel `settings.leads` sync | ||
| currently select members `WHERE is_lead = 1` | ||
| (`tinyagentos/projects/a2a.py:108-146`); they read `lead_member_id` | ||
| instead (a list of at most one). |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Specify unsetting lead_member_id on lead-member removal. The pointer is TEXT NULL with no FK and is only route-validated, but membership revocation deletes the project_members row (per D3, "A revoked membership deletes the row," and the mid-session edge case). Nothing here clears lead_member_id when the pointed member is removed, so the column can dangle at a non-member id. The a2a quiet-filter / settings.leads sync then read a stale lead (a suspended/removed agent still designated), or silently drop it depending on the join. Add an explicit rule: removing a member (or suspending the lead agent) must null lead_member_id, and state whether the set-lead route rejects a suspended/inactive member.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/design/lead-agent-identity-and-canvas-access.md` around lines 465 - 478,
Update the lead-member design to require clearing projects.lead_member_id when
its referenced member is removed or suspended, preventing a dangling lead
pointer. Specify that the PATCH /api/projects/{project_id}/lead route rejects
suspended or inactive members, while still allowing null to unset the lead.
feat(agents): lead-agent identities + canvas access — full #1800 set (slices 1-7)
1 participant
What
Founding design document for lead-agent identities and external-agent canvas access (project board task 98):
docs/design/lead-agent-identity-and-canvas-access.md(541 lines). Plan only, no implementation.The problem it encodes
Jay's org model is a three-lead structure (@taOS-dev, @taOSmd-dev, @taOS-website-dev lead the three elements of the taOS project; grok and kilo are tools the leads use). In reality only jay's admin account exists: two leads have been logging in AS JAY with copied credentials (website-dev broke on a stale password), the Members-screen canvas checkbox writes
project_members.can_edit_canvasbut nothing enforces it over HTTP, and the auth middleware does not accept agent tokens on canvas routes at all. The 2026-07-12 interim (a website-dev non-admin USER account via invite code) is explicitly retired by this design.What the design covers (all claims verified in code, cited file:line at dev@9d006f47)
canvas_read+canvas_writeadded to the closed vocab in bothagent_auth_requests.pyandagent_registry.py, project-bound likeproject_tasks, narrow-not-widen preserved._is_agent_task_path; the permissions PATCH stays human-only.canvas_writegrant ANDproject_members.can_edit_canvas = 1; the checkbox becomes the live kill switch (default OFF). Reads requirecanvas_read+ membership. User sessions get_get_owned_projectsemantics, closing the current any-authenticated-user hole.author_kind="agent"+ canonical id; includes the verified_user_idbug (readsrequest.state.user, which nothing sets, so every element today is authored as "system").Slice plan: 5 bounded slices for external CLI agents (scopes, middleware, gating + attribution, stream/snapshots, limits) with exact file paths and verify commands, plus one operational maintainer slice (minting + retirement runbook).
Review focus
Do not merge without Jay review; this is a founding design doc.
Summary by CodeRabbit