feat(agents): lead-agent identities + canvas access — full #1800 set (slices 1-7)#1838
Conversation
Add canvas_read and canvas_write to VALID_SCOPES and _ALLOWED_SCOPES. Canvas scopes require an explicit project_id at approval time, matching the project_tasks guard. Approver cannot widen scopes beyond what the agent requested. Docs-Reviewed: scope vocab additions per design doc lead-agent-identity-and-canvas-access.md
The scope label map was added but never rendered (no scope-label site in this component; that belongs to the Members UI in a later slice), which failed the strict SPA build with an unused-declaration error. Remove it; the canvas scopes already drive the project picker via needsProject. Docs-Reviewed: build fix only, no behaviour change, scope vocab per lead-agent-identity-and-canvas-access.md
…ce 2) Add _AGENT_CANVAS_ROUTES allowlist and _is_agent_canvas_path helper so registry Bearer tokens reach the canvas element list/create/delete, snapshot PNG/TLDR, and SSE stream endpoints. PATCH elements and PATCH permissions remain session-only. Docs-Reviewed: middleware canvas-route allowlist per design doc lead-agent-identity-and-canvas-access.md
…entity claim Derive the registry handle via _slugify from the already-sanitized identity claim in the approve path. Before minting the canonical identity, check get_by_handle(status='active'); if an active identity owns that handle, return 409 and leave the request PENDING so the approver can try a different variant. A SUSPENDED holder does not block reuse. After activating the new agent, update its handle via the store update setter. Tests: - TestHandleSetOnApprove: handle is set, agent passes the a2a no-handle gate - TestHandleCollisionActiveRejects: 409 + request stays pending when an active identity already has the handle - TestHandleCollisionSuspendedAllowsReuse: approval succeeds once the previous holder is suspended Docs-Reviewed: consent approve sets registry handle per lead-agent-identity-and-canvas-access.md
Adversarial review flagged that the project-binding guard only checked 'is None', so an empty or whitespace project_id passed through and minted a token with a falsy project_id that downstream truthy checks treat as unbound, opening a cross-project canvas write. Fail closed on None, empty, and whitespace, and add a regression test for both blank forms. Docs-Reviewed: defense-in-depth on the canvas scope project binding per lead-agent-identity-and-canvas-access.md
Gate the canvas routes by agent scope and per-project canvas permission
(slice 3 of the lead-agent identity epic), fix element attribution so a
session user is recorded by id and an agent principal by its agent id, and
stamp a uniform {kind, id} actor on every canvas event.
Docs-Reviewed: canvas route gating + attribution + event actor per lead-agent-identity-and-canvas-access.md
Adversarial review caught that snapshot.png and snapshot.tldr used an unescaped dot, so the regex treated it as a wildcard and a near-miss like snapshotXpng would match the agent-token allowlist. Escape both dots so the match is literal, and add regression tests for the near-miss forms plus the single-element GET boundary. Docs-Reviewed: harden canvas allowlist regex anchoring per lead-agent-identity-and-canvas-access.md
- Finding 1: add the PATCH /canvas/elements/{eid} route to the agent canvas
allowlist so a canvas_write token can update elements; the PATCH permissions
route stays owner/admin only. Flip the middleware allowlist test accordingly.
- Finding 2: gate the session branch of _authorize_canvas_actor on project
visibility (owner/admin). A non-owner collapses to the same existence-hiding
404 the agent path uses; owner/admin and user attribution are unchanged.
- Finding 4: add a can_read_canvas store gate and enforce it on the in-process
MCP read path (canvas_list_elements) so a write-only agent cannot read the
board, mirroring the write flag gate.
Docs-Reviewed: fold adversarial review findings per lead-agent-identity-and-canvas-access.md
…ce 6)
Implement slice 6 of the lead-agent identity and canvas access epic.
Backend:
- projects gains lead_member_id; backfill it from the single is_lead flag
and retire set_member_lead.
- Add PATCH /api/projects/{project_id}/lead to set or clear the exclusive
lead; remove the old per-member lead route.
- The a2a quiet-filter and settings.leads sync read lead_member_id.
Frontend:
- ProjectMembers gains a second Can read canvas checkbox and an exclusive
Lead selector driven by lead_member_id.
- projects.setLead is now project-level and canvas-api.setPermission takes a
read vs edit flag.
Tests cover exclusive lead promotion and clearing, 404 on non-member,
session-only gating, a2a quiet-filter sync, and the selector/read checkbox.
Docs-Reviewed: members canvas checkboxes + exclusive lead per lead-agent-identity-and-canvas-access.md
Docs-Reviewed: canvas payload cap + agent write rate limit per lead-agent-identity-and-canvas-access.md
Require canvas_read (scope + can_read_canvas member flag) for agent principals on GET /canvas/stream, /canvas/snapshot.png and /canvas/snapshot.tldr, reusing _authorize_canvas_actor in read mode. Snapshots stay read-scope only: a canvas_write-only token is 403. On the SSE keepalive tick, re-verify an agent principal's can_read_canvas flag and close the stream if it was cleared, so a revoked agent cannot hold a long-lived stream open. Tests: SSE connect as agent with and without scope+flag, snapshot fetch as agent (read allowed, write-only 403), stream closes after the read flag is cleared; session owner/admin behavior unchanged. Docs-Reviewed: stream + snapshot gating with keepalive recheck per lead-agent-identity-and-canvas-access.md
…e orphan window Register the handle at birth in the approve path so an agent is never left ACTIVE with an empty handle, and add a partial unique index on agent_registry(handle) WHERE status = 'active' AND handle != '' so the DB atomically rejects a concurrent approval of the same identity_claim instead of granting two active agents the identical handle (a2a "from" spoofing). The concurrent loser now fails with 409 and its half-registered row is rolled back and removed. Docs-Reviewed: close consent handle-set TOCTOU + orphan window per lead-agent-identity-and-canvas-access.md
… 5 per lead-agent-identity-and-canvas-access.md
Slice 1 renamed the picker label from 'Grant project_tasks for project' to 'Grant project access for' (the label now covers canvas scopes too), but the component test still looked the picker up by the old label, so three cases failed in the SPA build vitest run. Point them at the new label. Test-only change. Docs-Reviewed: test label sync for the project-picker rename in slice 1 per lead-agent-identity-and-canvas-access.md
…into epic-integrate
… into epic-integrate
… into epic-integrate
…' into epic-integrate
…epic-integrate # Conflicts: # tests/test_routes_project_canvas.py
…nto epic-integrate
…e' into epic-integrate
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? |
|
Warning Review limit reached
Next review available in: 26 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe PR replaces per-member lead flags with an exclusive project lead pointer and synchronizes it with A2A settings and the desktop selector. It also adds project-bound canvas scopes, agent authorization, read/edit permissions, event attribution, rate limits, payload limits, and MCP permission enforcement. ChangesExclusive project lead pointer
Project-scoped canvas access
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Agent
participant AuthMiddleware
participant CanvasRoutes
participant ProjectCanvasStore
Agent->>AuthMiddleware: Send Bearer canvas request
AuthMiddleware->>CanvasRoutes: Mark registry JWT candidate
CanvasRoutes->>ProjectCanvasStore: Check project permission
ProjectCanvasStore-->>CanvasRoutes: Return authorization result
CanvasRoutes-->>Agent: Return canvas response
sequenceDiagram
participant ProjectMembers
participant ProjectsAPI
participant ProjectStore
participant A2AChannel
ProjectMembers->>ProjectsAPI: Set or clear member_id
ProjectsAPI->>ProjectStore: Persist lead_member_id
ProjectStore-->>ProjectsAPI: Return updated project state
ProjectsAPI->>A2AChannel: Synchronize settings.leads
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
Code Review SummaryStatus: No Issues Found (incremental) | Recommendation: Merge Overview
Incremental AnalysisThe only change since the previous review (commit Previous finding resolved: The prior review flagged that the partial index — whose Files Reviewed (1 file, incremental)
Previous Review Summary (commit aa44876)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit aa44876)Status: 5 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)CRITICAL(none) WARNING
SUGGESTION
Files Reviewed (27 files)
Reviewed by hy3:free · Input: 36.5K · Output: 1.8K · Cached: 145.4K |
There was a problem hiding this comment.
Actionable comments posted: 16
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/projects/test_canvas_mcp_tools.py (1)
202-210: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy liftDo not let edit-only agents read snapshots.
This test grants only
can_edit_canvas, then invokes the MCP snapshot tool without anagent_id. That bypasses the independentcan_read_canvasfloor enforced by listing and HTTP snapshots. Pass the agent identity into the tool, check read permission, and add a denied case.🤖 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 `@tests/projects/test_canvas_mcp_tools.py` around lines 202 - 210, Update test_canvas_get_snapshot_png_writes_file and the snapshot tool invocation to pass agent_id explicitly, then enforce the independent can_read_canvas permission before generating snapshots. Add a denied-case test using an edit-only agent that verifies the snapshot request is rejected, while preserving the existing successful read-authorized path.
🧹 Nitpick comments (3)
tests/projects/test_routes_a2a.py (1)
162-184: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExercise replacement without clearing the first lead.
This test only clears and restores the same member, so it would still pass if the endpoint rejected replacing an existing lead. Add a second distinct member and PATCH directly from the first lead to the second.
🤖 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 `@tests/projects/test_routes_a2a.py` around lines 162 - 184, The test_set_lead_exclusive_replaces_previous test must verify direct replacement rather than clearing and restoring the same member. Add a second distinct project member using the available test-agent setup, then PATCH the lead from a1 directly to that member and assert the response and fetched project contain only the second member as lead.tests/test_routes_agent_auth_requests.py (1)
372-375: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winAssert that every canvas grant retains the selected project binding.
Checking only scope names allows
project_id=Noneto regress unnoticed.Proposed assertion
agent_grants = await grants.list_grants(agents[0]["canonical_id"]) assert {g["scope"] for g in agent_grants} == {"canvas_read", "canvas_write"} + assert {g["project_id"] for g in agent_grants} == {"prj-canvas-1"}🤖 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 `@tests/test_routes_agent_auth_requests.py` around lines 372 - 375, Extend the assertions following registry.list_all() and grants.list_grants() to verify that each canvas grant retains the selected project binding, not just its scope. Assert the relevant project_id value for both canvas_read and canvas_write grants, while preserving the existing scope-set assertion.desktop/src/components/ConsentActions.test.tsx (1)
100-100: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winExercise both new canvas scopes in the project-picker tests.
These cases still cover
project_tasks. Add cases forcanvas_readandcanvas_writethat verify project selection is required and included in the approval payload.Also applies to: 128-128, 154-154
🤖 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 `@desktop/src/components/ConsentActions.test.tsx` at line 100, Add project-picker test cases for the new canvas scopes `canvas_read` and `canvas_write` alongside the existing `project_tasks` cases in ConsentActions tests. For each scope, verify that selecting a project is required and that the selected project is included in the approval payload, preserving the existing test structure and assertions.
🤖 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 `@desktop/src/apps/ProjectsApp/ProjectMembers.tsx`:
- Around line 199-208: Update handleLeadChange in ProjectMembers so lead changes
cannot issue concurrent setLead requests: disable the selector while saving or
serialize updates through a queue. Track the project identity and requested
value for each save, and only apply success or rollback state when the
completion still belongs to the current project and latest request; ensure
onChanged is likewise not triggered by stale completions.
In `@tests/test_routes_agent_auth_requests.py`:
- Around line 805-808: Update the second activation assertion around
registry.set_status to expect the database IntegrityError type instead of the
generic Exception, while preserving the existing check that the failure
references ux_agent_active_handle or UNIQUE.
In `@tests/test_routes_project_canvas.py`:
- Around line 1072-1076: Update the tests that modify _canvas_write_limiter._log
in the affected test blocks to save the existing cid entry before overwriting
it, then restore or remove that entry in a finally block. Apply the same cleanup
around both referenced limiter setups so timestamp-derived IDs cannot leak
rate-limit state between tests.
- Around line 410-439: The session authorization flow must reject nonexistent
projects before evaluating admin or owner access. Update _authorize_canvas_actor
to return the established 404 response when get_project() returns None, and add
a regression test in TestSessionCanvasOwnerGating covering an arbitrary missing
project ID.
In `@tinyagentos/agent_registry_store.py`:
- Around line 51-59: The partial unique index must not be created in SCHEMA
before legacy migration completes. Remove ux_agent_active_handle from SCHEMA,
then add it in _post_init() after status backfill and duplicate active-handle
cleanup, ensuring legacy databases migrate successfully and index creation
cannot fail on existing duplicates.
In `@tinyagentos/projects/a2a.py`:
- Around line 107-128: Update _resolve_lead_names to accept project_members,
locate the row matching project["lead_member_id"], and resolve that row through
the same lookup logic used by _resolve_member_names instead of looking up the
lead ID directly in config. Update all call sites, including the additional
location around the reported later range, to pass project_members so clone lead
IDs resolve correctly while preserving the no-lead behavior.
In `@tinyagentos/projects/canvas/mcp_tools.py`:
- Around line 95-100: Update canvas_add_link to preflight the caller’s edit
permission before invoking fetch_link_metadata, using the existing authorization
mechanism associated with project_id and agent_id. Keep _add_agent_element’s
authorization check during insertion unchanged to preserve TOCTOU protection,
and only fetch metadata after the preflight succeeds.
In `@tinyagentos/projects/project_store.py`:
- Around line 106-125: The set_lead and member-removal paths must be mutually
atomic to prevent a removed member from becoming the project lead. Update
set_lead and the corresponding removal method to use the same store-level lock
around membership validation, lead assignment, and member deletion, or enforce
the invariant with an atomic conditional update and delete trigger. Preserve the
existing KeyError behavior for missing projects or members.
- Around line 76-81: Update the migration execution around _db.execute and
_db.commit to ignore only SQLite’s duplicate-column error; re-raise all other
exceptions, including lock, disk, permission, and malformed-schema failures.
Preserve the existing behavior for an already-existing column without
suppressing unexpected migration errors.
In `@tinyagentos/routes/agent_auth_requests.py`:
- Around line 362-398: Extend the approval flow surrounding registry.set_status
in the external-selfjoin path to compensate any later grant, relationship, or
decision failure after activation. Ensure downstream failure removes partial
grants and edges and deletes or revokes the newly registered identity, while
leaving the request retryable; alternatively restructure transaction boundaries
so activation cannot be committed before all approval work succeeds.
In `@tinyagentos/routes/project_canvas.py`:
- Around line 57-68: Replace the separate is_limited() and record() calls with a
single locked try_acquire() operation that prunes, checks capacity, and records
the attempt atomically. Update every affected request path to call try_acquire()
before awaited processing, preserving the existing 30-attempt limit and
rejecting requests when acquisition fails.
- Around line 398-407: The SSE stream authorization in the queue-consumption
loop must be revalidated periodically even when events arrive continuously.
Update the logic surrounding the asyncio.wait_for call to run a monotonic
scheduled check independent of timeout activity, and reuse the full
project-scope authorization verification—including agent status, scope/grant
revocation, project binding, token expiry, and can_read_canvas—before continuing
to stream; terminate the stream immediately when any check fails.
- Around line 117-121: Update the project authorization flow around
ps.get_project in the relevant route helper to return a 404 response whenever
project is None, before applying admin or ownership checks. Preserve the
existing 404 behavior for non-admin users accessing another user’s project and
only return the session actor tuple for an existing authorized project.
- Around line 208-215: Update the creation handler around cs.add_element in the
project canvas route to catch CanvasPermissionError and return the same 403
response used by the update/delete handlers. Preserve the existing ValueError
handling and successful creation flow.
In `@tinyagentos/routes/projects.py`:
- Around line 329-330: Update the project mirroring flow after set_lead to
publish the refreshed project data, ensuring the lead_member_id reflects the
newly selected lead instead of the stale p row. Use the result of the project
reload or otherwise update p before calling _mirror, while preserving the
current members payload.
- Around line 299-300: Make ProjectLeadIn.member_id required while retaining its
nullable type, so callers must explicitly provide None to clear the lead. In
set_project_lead(), re-read the project after store.set_lead() and pass the
refreshed project to _mirror() instead of the pre-update p.
---
Outside diff comments:
In `@tests/projects/test_canvas_mcp_tools.py`:
- Around line 202-210: Update test_canvas_get_snapshot_png_writes_file and the
snapshot tool invocation to pass agent_id explicitly, then enforce the
independent can_read_canvas permission before generating snapshots. Add a
denied-case test using an edit-only agent that verifies the snapshot request is
rejected, while preserving the existing successful read-authorized path.
---
Nitpick comments:
In `@desktop/src/components/ConsentActions.test.tsx`:
- Line 100: Add project-picker test cases for the new canvas scopes
`canvas_read` and `canvas_write` alongside the existing `project_tasks` cases in
ConsentActions tests. For each scope, verify that selecting a project is
required and that the selected project is included in the approval payload,
preserving the existing test structure and assertions.
In `@tests/projects/test_routes_a2a.py`:
- Around line 162-184: The test_set_lead_exclusive_replaces_previous test must
verify direct replacement rather than clearing and restoring the same member.
Add a second distinct project member using the available test-agent setup, then
PATCH the lead from a1 directly to that member and assert the response and
fetched project contain only the second member as lead.
In `@tests/test_routes_agent_auth_requests.py`:
- Around line 372-375: Extend the assertions following registry.list_all() and
grants.list_grants() to verify that each canvas grant retains the selected
project binding, not just its scope. Assert the relevant project_id value for
both canvas_read and canvas_write grants, while preserving the existing
scope-set assertion.
🪄 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: 8da9dda9-8d1a-479a-9148-5c6fc9759ed8
📒 Files selected for processing (27)
desktop/src/apps/ProjectsApp/ProjectMembers.test.tsxdesktop/src/apps/ProjectsApp/ProjectMembers.tsxdesktop/src/apps/ProjectsApp/canvas/canvas-api.tsdesktop/src/components/ConsentActions.test.tsxdesktop/src/components/ConsentActions.tsxdesktop/src/lib/projects.test.tsdesktop/src/lib/projects.tstests/projects/test_a2a.pytests/projects/test_canvas_integration.pytests/projects/test_canvas_mcp_tools.pytests/projects/test_canvas_store.pytests/projects/test_routes_a2a.pytests/projects/test_routes_canvas.pytests/test_auth_middleware.pytests/test_routes_agent_auth_requests.pytests/test_routes_project_canvas.pytests/test_routes_projects.pytinyagentos/agent_registry_store.pytinyagentos/auth_middleware.pytinyagentos/projects/a2a.pytinyagentos/projects/canvas/mcp_tools.pytinyagentos/projects/canvas/store.pytinyagentos/projects/project_store.pytinyagentos/routes/agent_auth_requests.pytinyagentos/routes/agent_registry.pytinyagentos/routes/project_canvas.pytinyagentos/routes/projects.py
| const handleLeadChange = async (value: string) => { | ||
| const next = value || null; | ||
| setLeadMemberId(next); | ||
| try { | ||
| await projectsApi.setLead(project.id, next); | ||
| } catch { | ||
| // Revert on failure so the UI stays consistent with the server. | ||
| setLeadMemberId(project.lead_member_id ?? null); | ||
| } | ||
| onChanged(); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Serialize lead updates and ignore stale completions.
Rapid changes issue concurrent PATCH requests, so response/write ordering can leave the server with an earlier selection. Disable the selector while saving or queue writes, and guard completion handling against project.id changes.
🤖 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 `@desktop/src/apps/ProjectsApp/ProjectMembers.tsx` around lines 199 - 208,
Update handleLeadChange in ProjectMembers so lead changes cannot issue
concurrent setLead requests: disable the selector while saving or serialize
updates through a queue. Track the project identity and requested value for each
save, and only apply success or rollback state when the completion still belongs
to the current project and latest request; ensure onChanged is likewise not
triggered by stale completions.
| # The second activation must be rejected by the partial unique index. | ||
| with pytest.raises(Exception) as exc: | ||
| await registry.set_status(b["canonical_id"], "active") | ||
| assert "ux_agent_active_handle" in str(exc.value) or "UNIQUE" in str(exc.value) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '== test file context ==\n'
sed -n '760,830p' tests/test_routes_agent_auth_requests.py
printf '\n== search for set_status implementation ==\n'
rg -n "def set_status|async def set_status|set_status\(" -S .
printf '\n== search for IntegrityError usage/imports ==\n'
rg -n "IntegrityError|sqlite3\.IntegrityError|aiosqlite\.IntegrityError|UNIQUE" -S tests . | head -n 200Repository: jaylfc/taOS
Length of output: 21034
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '== relevant test file around the assertion ==\n'
nl -ba tests/test_routes_agent_auth_requests.py | sed -n '790,820p'
printf '\n== candidate implementation files ==\n'
rg -n "class .*Registry|def set_status|async def set_status" -S . --glob '!tests/*'
printf '\n== exceptions and unique constraint handling ==\n'
rg -n "IntegrityError|UNIQUE constraint failed|ux_agent_active_handle|partial unique index" -S . --glob '!tests/*'Repository: jaylfc/taOS
Length of output: 235
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# If the previous searches didn't surface enough, inspect the registry implementation directly.
fd -a "registry" .Repository: jaylfc/taOS
Length of output: 1649
🏁 Script executed:
#!/bin/bash
set -euo pipefail
nl -ba tests/test_routes_agent_auth_requests.py | sed -n '790,820p'
echo
rg -n "def set_status|async def set_status" -S . --glob '!tests/*'
echo
rg -n "IntegrityError|UNIQUE constraint failed|ux_agent_active_handle|partial unique index" -S . --glob '!tests/*'Repository: jaylfc/taOS
Length of output: 189
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '== set_status definitions ==\n'
rg -n "async def set_status|def set_status" -S . --glob '!tests/*'
printf '\n== nearby implementation context for candidate files ==\n'
for f in $(rg -l "async def set_status|def set_status" -S . --glob '!tests/*'); do
echo "--- $f ---"
nl -ba "$f" | sed -n '1,220p'
done
printf '\n== unique constraint handling ==\n'
rg -n "IntegrityError|UNIQUE constraint|ux_agent_active_handle|raise .*Exception|except .*IntegrityError" -S . --glob '!tests/*'Repository: jaylfc/taOS
Length of output: 743
🌐 Web query:
aiosqlite IntegrityError re-export sqlite3 exception class
💡 Result:
Yes, aiosqlite re-exports standard sqlite3 exception classes, including IntegrityError, to maintain compatibility with the standard library [1][2]. You can import and use IntegrityError directly from the aiosqlite package, for example: from aiosqlite import IntegrityError try: await db.execute("...") except IntegrityError: # Handle constraint violation pass The library's init.py file explicitly imports these exception classes from the sqlite3 module and includes them in its all list, making them available as part of the aiosqlite public API [1]. This design ensures that code using standard sqlite3 exception handling can be migrated to aiosqlite with minimal changes [2].
Citations:
- 1: https://github.com/omnilib/aiosqlite/blob/main/aiosqlite/__init__.py
- 2: https://deepwiki.com/omnilib/aiosqlite/3-public-api-reference
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
path = Path("tinyagentos/agent_registry_store.py")
lines = path.read_text().splitlines()
for start in [1, 40, 540, 620]:
end = min(len(lines), start + 90)
print(f"== {path} lines {start}-{end} ==")
for i in range(start-1, end):
print(f"{i+1:4d}: {lines[i]}")
print()
PY
printf '== route handler around IntegrityError ==\n'
python3 - <<'PY'
from pathlib import Path
path = Path("tinyagentos/routes/agent_auth_requests.py")
lines = path.read_text().splitlines()
for start in [1, 350, 370]:
end = min(len(lines), start + 50)
print(f"== {path} lines {start}-{end} ==")
for i in range(start-1, end):
print(f"{i+1:4d}: {lines[i]}")
print()
PYRepository: jaylfc/taOS
Length of output: 25179
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
path = Path("tests/test_agent_registry_store.py")
text = path.read_text().splitlines()
for needle in ["IntegrityError", "ux_agent_active_handle", "pytest.raises(Exception)", "pytest.raises(IntegrityError)"]:
print(f"== matches for {needle!r} ==")
for i, line in enumerate(text, 1):
if needle in line:
print(f"{i:4d}: {line}")
print()
PYRepository: jaylfc/taOS
Length of output: 326
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
path = Path("tinyagentos/agent_registry_store.py")
lines = path.read_text().splitlines()
for start in [1, 40, 540, 620]:
end = min(len(lines), start + 90)
print(f"== {path} lines {start}-{end} ==")
for i in range(start-1, end):
print(f"{i+1:4d}: {lines[i]}")
print()
PY
printf '== route handler around IntegrityError ==\n'
python3 - <<'PY'
from pathlib import Path
path = Path("tinyagentos/routes/agent_auth_requests.py")
lines = path.read_text().splitlines()
for start in [1, 350, 370]:
end = min(len(lines), start + 50)
print(f"== {path} lines {start}-{end} ==")
for i in range(start-1, end):
print(f"{i+1:4d}: {lines[i]}")
print()
PYRepository: jaylfc/taOS
Length of output: 25179
Assert the unique-index IntegrityError. Use pytest.raises(IntegrityError) here; Exception can hide unrelated failures and doesn’t prove ux_agent_active_handle fired.
🤖 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 `@tests/test_routes_agent_auth_requests.py` around lines 805 - 808, Update the
second activation assertion around registry.set_status to expect the database
IntegrityError type instead of the generic Exception, while preserving the
existing check that the failure references ux_agent_active_handle or UNIQUE.
| @pytest.mark.asyncio | ||
| class TestSessionCanvasOwnerGating: | ||
| async def test_owner_session_write_allowed(self, ctx): | ||
| pid = await _new_project(ctx, "owner-gate-write") | ||
| resp = await ctx.client.post( | ||
| f"/api/projects/{pid}/canvas/elements", | ||
| json={"kind": "note", "x": 0, "y": 0, "w": 1, "h": 1, | ||
| "payload": {"text": "owned"}}, | ||
| ) | ||
| assert resp.status_code == 201, resp.text | ||
|
|
||
| async def test_non_owner_session_list_is_404(self, ctx): | ||
| pid = await _new_project(ctx, "nonowner-gate") | ||
| async with _non_owner_client(ctx.app) as other: | ||
| resp = await other.get(f"/api/projects/{pid}/canvas/elements") | ||
| assert resp.status_code == 404 | ||
| assert resp.status_code != 403 | ||
| assert resp.status_code != 200 | ||
|
|
||
| async def test_non_owner_session_write_is_404(self, ctx): | ||
| pid = await _new_project(ctx, "nonowner-gate-write") | ||
| async with _non_owner_client(ctx.app) as other: | ||
| resp = await other.post( | ||
| f"/api/projects/{pid}/canvas/elements", | ||
| json={"kind": "note", "x": 0, "y": 0, "w": 1, "h": 1, | ||
| "payload": {"text": "x"}}, | ||
| ) | ||
| assert resp.status_code == 404 | ||
| assert resp.status_code != 403 | ||
|
|
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Add the missing-project case to the session gate.
_authorize_canvas_actor still returns a user actor when get_project() returns None. These tests therefore leave arbitrary project IDs able to reach canvas handlers instead of returning the promised 404. Add a regression test and reject None before the admin/owner branch.
🤖 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 `@tests/test_routes_project_canvas.py` around lines 410 - 439, The session
authorization flow must reject nonexistent projects before evaluating admin or
owner access. Update _authorize_canvas_actor to return the established 404
response when get_project() returns None, and add a regression test in
TestSessionCanvasOwnerGating covering an arbitrary missing project ID.
| limiter = canvas_mod._canvas_write_limiter | ||
| max_attempts = canvas_mod._CANVAS_WRITE_MAX_ATTEMPTS | ||
| now = time.monotonic() | ||
| with limiter._lock: | ||
| limiter._log[cid] = [now - 1.0] * max_attempts |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Restore the global rate limiter after each test.
These tests overwrite _canvas_write_limiter._log[cid] without cleanup. Separate test registries can generate the same timestamp-derived canonical ID, causing later or randomized tests to inherit a full window and receive unexpected 429s. Restore the previous entry in finally.
Also applies to: 1105-1109
🤖 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 `@tests/test_routes_project_canvas.py` around lines 1072 - 1076, Update the
tests that modify _canvas_write_limiter._log in the affected test blocks to save
the existing cid entry before overwriting it, then restore or remove that entry
in a finally block. Apply the same cleanup around both referenced limiter setups
so timestamp-derived IDs cannot leak rate-limit state between tests.
| -- Partial unique index: at most ONE active agent may own any given non-empty | ||
| -- handle. SQLite enforces this atomically, so two concurrent consent approvals | ||
| -- of the same identity_claim cannot both flip to 'active' with the same handle. | ||
| -- Pending / suspended / revoked rows and empty handles are excluded from the | ||
| -- index, so (a) a handle can be reused once its owner leaves 'active', and (b) | ||
| -- pre-existing empty-handle active agents never block the index's creation. | ||
| CREATE UNIQUE INDEX IF NOT EXISTS ux_agent_active_handle | ||
| ON agent_registry(handle) | ||
| WHERE status = 'active' AND handle != ''; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
# Confirm schema versus post-init migration ordering.
rg -n -C5 'class BaseStore|async def init|executescript|_post_init' tinyagentos --type py
# Check whether legacy status and duplicate-handle migration coverage exists.
rg -n -C4 '_migration_v1_add_status|ux_agent_active_handle|duplicate.*handle' \
tinyagentos tests --type pyRepository: jaylfc/taOS
Length of output: 50367
🏁 Script executed:
#!/bin/bash
set -eu
# Inspect the registry store schema, migrations, and init path.
sed -n '1,220p' tinyagentos/agent_registry_store.py
printf '\n--- base_store ---\n'
sed -n '1,120p' tinyagentos/base_store.py
printf '\n--- db_migrations notes ---\n'
sed -n '1,120p' tinyagentos/db_migrations.pyRepository: jaylfc/taOS
Length of output: 15166
🏁 Script executed:
#!/bin/bash
set -eu
# Find any registry-specific migrations or duplicate-handle reconciliation.
rg -n -C4 'MIGRATIONS|ux_agent_active_handle|_migration_v[0-9]+|duplicate|handle' tinyagentos/agent_registry_store.py tinyagentos tests --type py
# Inspect the rest of agent_registry_store for lifecycle methods that may enforce uniqueness.
sed -n '220,520p' tinyagentos/agent_registry_store.pyRepository: jaylfc/taOS
Length of output: 50368
🏁 Script executed:
#!/bin/bash
set -eu
# Show the registry store methods that touch status or handle.
rg -n -C3 'UPDATE agent_registry|INSERT INTO agent_registry|handle|status|reconcile|duplicate|revoked_at' \
tinyagentos/agent_registry_store.py
# Read the parts around the core lifecycle/update methods.
sed -n '220,520p' tinyagentos/agent_registry_store.pyRepository: jaylfc/taOS
Length of output: 25210
Move the partial unique index out of SCHEMA tinyagentos/agent_registry_store.py:51-59 — BaseStore.init() runs SCHEMA before _post_init(), so legacy databases without status will fail here, and any pre-existing duplicate active handles will still abort index creation. Add the index only after the status backfill and a duplicate-handle cleanup step.
🤖 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 `@tinyagentos/agent_registry_store.py` around lines 51 - 59, The partial unique
index must not be created in SCHEMA before legacy migration completes. Remove
ux_agent_active_handle from SCHEMA, then add it in _post_init() after status
backfill and duplicate active-handle cleanup, ensuring legacy databases migrate
successfully and index creation cannot fail on existing duplicates.
| project = await ps.get_project(project_id) | ||
| is_admin = bool(getattr(request.state, "is_admin", False)) | ||
| if project is not None and not is_admin and project.get("user_id") != uid: | ||
| return JSONResponse({"error": "not found"}, status_code=404) | ||
| return ("user", _user_id(request)) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Return 404 when the project does not exist.
Line 119 only rejects non-owners when project is non-null. A nonexistent project therefore authorizes the session actor, causing reads to return an empty canvas and writes to continue against an invalid project ID.
Proposed fix
project = await ps.get_project(project_id)
is_admin = bool(getattr(request.state, "is_admin", False))
-if project is not None and not is_admin and project.get("user_id") != uid:
+if project is None:
+ return JSONResponse({"error": "not found"}, status_code=404)
+if not is_admin and project.get("user_id") != uid:
return JSONResponse({"error": "not found"}, status_code=404)
-return ("user", _user_id(request))
+return ("user", uid)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| project = await ps.get_project(project_id) | |
| is_admin = bool(getattr(request.state, "is_admin", False)) | |
| if project is not None and not is_admin and project.get("user_id") != uid: | |
| return JSONResponse({"error": "not found"}, status_code=404) | |
| return ("user", _user_id(request)) | |
| project = await ps.get_project(project_id) | |
| is_admin = bool(getattr(request.state, "is_admin", False)) | |
| if project is None: | |
| return JSONResponse({"error": "not found"}, status_code=404) | |
| if not is_admin and project.get("user_id") != uid: | |
| return JSONResponse({"error": "not found"}, status_code=404) | |
| return ("user", uid) |
🤖 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 `@tinyagentos/routes/project_canvas.py` around lines 117 - 121, Update the
project authorization flow around ps.get_project in the relevant route helper to
return a 404 response whenever project is None, before applying admin or
ownership checks. Preserve the existing 404 behavior for non-admin users
accessing another user’s project and only return the session actor tuple for an
existing authorized project.
| try: | ||
| new_el = await cs.add_element( | ||
| project_id=project_id, element=element, | ||
| author_kind="user", author_id=_user_id(request), | ||
| author_kind=actor_kind, author_id=actor_id, | ||
| element_id=element_id, | ||
| ) | ||
| except ValueError as e: | ||
| return JSONResponse({"error": str(e)}, status_code=400) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Handle the store-level permission rejection during creation.
add_element() now raises CanvasPermissionError. If permission is revoked between route authorization and the store call, creation returns 500, unlike update/delete which correctly return 403.
Proposed fix
try:
new_el = await cs.add_element(...)
+ except CanvasPermissionError as e:
+ return JSONResponse(
+ {"error": "permission_denied", "message": str(e)},
+ status_code=403,
+ )
except ValueError as e:
return JSONResponse({"error": str(e)}, status_code=400)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| try: | |
| new_el = await cs.add_element( | |
| project_id=project_id, element=element, | |
| author_kind="user", author_id=_user_id(request), | |
| author_kind=actor_kind, author_id=actor_id, | |
| element_id=element_id, | |
| ) | |
| except ValueError as e: | |
| return JSONResponse({"error": str(e)}, status_code=400) | |
| try: | |
| new_el = await cs.add_element( | |
| project_id=project_id, element=element, | |
| author_kind=actor_kind, author_id=actor_id, | |
| element_id=element_id, | |
| ) | |
| except CanvasPermissionError as e: | |
| return JSONResponse( | |
| {"error": "permission_denied", "message": str(e)}, | |
| status_code=403, | |
| ) | |
| except ValueError as e: | |
| return JSONResponse({"error": str(e)}, status_code=400) |
🤖 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 `@tinyagentos/routes/project_canvas.py` around lines 208 - 215, Update the
creation handler around cs.add_element in the project canvas route to catch
CanvasPermissionError and return the same 403 response used by the update/delete
handlers. Preserve the existing ValueError handling and successful creation
flow.
| try: | ||
| ev = await asyncio.wait_for(queue.get(), timeout=10.0) | ||
| except asyncio.TimeoutError: | ||
| # Keepalive tick: re-verify a live agent principal's read | ||
| # access so a revoked/removed agent cannot keep a long-lived | ||
| # SSE open (slice 4). Session users are unaffected. | ||
| if actor_kind == "agent": | ||
| member = await ps.get_member(project_id, actor_id) | ||
| if not (member or {}).get("can_read_canvas"): | ||
| return |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Reauthorize streams periodically, not only when idle.
Continuous events prevent asyncio.TimeoutError, so a revoked agent can keep receiving data indefinitely. The timeout check also verifies only can_read_canvas, not agent status, scope/grant revocation, project binding, or token expiry.
Run a monotonic periodic check independent of queue activity and repeat the full project-scope verification plus the member-flag check.
🤖 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 `@tinyagentos/routes/project_canvas.py` around lines 398 - 407, The SSE stream
authorization in the queue-consumption loop must be revalidated periodically
even when events arrive continuously. Update the logic surrounding the
asyncio.wait_for call to run a monotonic scheduled check independent of timeout
activity, and reuse the full project-scope authorization verification—including
agent status, scope/grant revocation, project binding, token expiry, and
can_read_canvas—before continuing to stream; terminate the stream immediately
when any check fails.
| class ProjectLeadIn(BaseModel): | ||
| member_id: "str | None" = None |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify the repository's FastAPI/Pydantic versions and existing required-nullable conventions.
fd -a '^(pyproject\.toml|requirements.*\.txt|uv\.lock|poetry\.lock)$' . \
-x rg -n -i 'fastapi|pydantic' {}
rg -n -C3 --type=py 'Field\(\.\.\.\)|:\s*["'\'']?.+\|\s*None["'\'']?\s*=\s*Field' .Repository: jaylfc/taOS
Length of output: 24591
🏁 Script executed:
sed -n '260,340p' tinyagentos/routes/projects.pyRepository: jaylfc/taOS
Length of output: 3190
🏁 Script executed:
sed -n '1,220p' tinyagentos/routes/projects.py && printf '\n---\n' && rg -n "def _mirror|async def set_lead|lead_member_id|class ProjectStore|def get_project" tinyagentos -nRepository: jaylfc/taOS
Length of output: 9291
🏁 Script executed:
rg -n "async def get_project|async def set_lead|lead_member_id|class .*ProjectStore|def _mirror" tinyagentos -n && printf '\n---\n' && fd -a 'project.*\.py|store.*\.py' tinyagentosRepository: jaylfc/taOS
Length of output: 5052
🏁 Script executed:
sed -n '100,190p' tinyagentos/projects/project_store.pyRepository: jaylfc/taOS
Length of output: 3984
🏁 Script executed:
sed -n '100,190p' tinyagentos/projects/project_store.py && printf '\n---\n' && sed -n '300,350p' tinyagentos/routes/projects.pyRepository: jaylfc/taOS
Length of output: 5831
Make member_id required and mirror fresh project state.
ProjectLeadIn.member_iddefaults toNone, so{}silently clears the lead. Keep it nullable, but make it required so clearing is explicit.set_project_lead()calls_mirror()with the pre-updatep, so the exported project can keep the oldlead_member_id. Re-read the project afterstore.set_lead()before mirroring.
🤖 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 `@tinyagentos/routes/projects.py` around lines 299 - 300, Make
ProjectLeadIn.member_id required while retaining its nullable type, so callers
must explicitly provide None to clear the lead. In set_project_lead(), re-read
the project after store.set_lead() and pass the refreshed project to _mirror()
instead of the pre-update p.
| members = await store.list_members(project_id) | ||
| _mirror(request, {**p, "members": members}) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Mirror the updated lead pointer, not the stale project row.
p was loaded before set_lead, so _mirror publishes the previous lead_member_id. The desktop selector can revert or remain stale until the project is fetched again.
Proposed fix
- _mirror(request, {**p, "members": members})
+ _mirror(
+ request,
+ {**p, "lead_member_id": body.member_id, "members": members},
+ )📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| members = await store.list_members(project_id) | |
| _mirror(request, {**p, "members": members}) | |
| members = await store.list_members(project_id) | |
| _mirror( | |
| request, | |
| {**p, "lead_member_id": body.member_id, "members": members}, | |
| ) |
🤖 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 `@tinyagentos/routes/projects.py` around lines 329 - 330, Update the project
mirroring flow after set_lead to publish the refreshed project data, ensuring
the lead_member_id reflects the newly selected lead instead of the stale p row.
Use the result of the project reload or otherwise update p before calling
_mirror, while preserving the current members payload.
The slice-7 partial unique index ux_agent_active_handle has a WHERE status = 'active' clause, but it lived in SCHEMA, which BaseStore runs via executescript BEFORE migrations. On an existing pre-status DB the CREATE UNIQUE INDEX crashed with 'no such column: status' before _migration_v1_add_status could add the column, breaking the registry migration path (caught by test_migration_backfills_revoked_at_rows on CI, not in the local subset). Move the index out of SCHEMA into _post_init, created after the status migration so the WHERE clause always resolves. Idempotent (IF NOT EXISTS); handle-uniqueness enforcement unchanged.
Consolidated landing of the 7-slice lead-agent identity + canvas-access epic (#1800), held as one set per the plan. Supersedes the individual slice PRs #1824-1830.
What is in it
Review
Every slice got an adversarial security pass (nemotron / maintainer); five real CRITICALs were caught and folded before this assembly:
empty/whitespace project_id bypass, unescaped-dot allowlist regex, missing PATCH route + session gate + MCP read bypass, and a handle-spoofing TOCTOU race. Full assembled suite: 494 passed, 2 skipped locally.
Slice 8 (mint lead identities, delete the interim account, rotate the maintainer password) is executed on the live controller after this lands.
Summary by CodeRabbit
New Features
Bug Fixes