tsk-i6hggr [OPEN] S7a: watch projection endpoint for canvas state - #2204
tsk-i6hggr [OPEN] S7a: watch projection endpoint for canvas state#2204jaylfc wants to merge 1 commit into
Conversation
📝 WalkthroughWalkthroughAdds a renderer-neutral canvas watch projection builder, exposes it through an authenticated GET endpoint, and permits registry-JWT passthrough for the new route. ChangesCanvas watch projection
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant AuthMiddleware
participant watch_project_canvas
participant project_canvas_store
participant build_watch_projection
Client->>AuthMiddleware: GET /api/projects/{project_id}/canvas/watch-projection
AuthMiddleware->>watch_project_canvas: authorize and pass request
watch_project_canvas->>project_canvas_store: load canvas elements
project_canvas_store-->>watch_project_canvas: return elements
watch_project_canvas->>build_watch_projection: build projection
build_watch_projection-->>Client: return versioned JSON projection
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
PR Summary by QodoAdd renderer-neutral canvas watch projection endpoint
AI Description
Diagram
High-Level Assessment
Files changed (3)
|
|
nemotron-ultra-orB review Code ReviewCritical Issues1. 2. kind = _str_or(el.get("kind"), "unknown") if isinstance(el, dict) else "unknown"
entry = _base_entry(el, kind, "placeholder") # passes non-dict el to _base_entry
3. Correctness Bugs4. for key in ("text", "title", "label", "source", "url", "caption"):Keys 5. projected.sort(key=lambda e: (e.get("z_index") or 0))
6. Security Issues7. except Exception:
return ""Swallowing Missing/Weak Test Cases8. No tests for
9. No tests for
Style Issues10. Missing return type hints — 11.
12. Inconsistent 13. Unused import — Summary Priority
Fix #1-3 before merge. Add tests for #8-9. Automated first-pass review by the nemotron-ultra-orB lane. The lead still reviews before merge. |
![coderabbitai[bot]](https://avatars.github.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tinyagentos/projects/canvas/watch_projection.py (1)
28-43: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd the requested endpoint-contract tests.
No tests accompany this new versioned API, projection behavior, or registry-JWT path.
tinyagentos/projects/canvas/watch_projection.py#L28-L43: test supported rows, malformed payload degradation, and deterministicz_indexordering.tinyagentos/routes/project_canvas.py#L201-L208: test authorized success and denied read access.tinyagentos/auth_middleware.py#L87-L87: test exact GET-only JWT passthrough and insufficient-scope/project rejection.🤖 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/projects/canvas/watch_projection.py` around lines 28 - 43, Add endpoint-contract tests covering supported rows, malformed-element placeholder degradation, and deterministic z_index ordering in tinyagentos/projects/canvas/watch_projection.py:28-43; authorized success and denied read access in tinyagentos/routes/project_canvas.py:201-208; and exact GET-only JWT passthrough plus insufficient-scope/project rejection in tinyagentos/auth_middleware.py:87-87. Use the existing build_watch_projection and relevant route/auth symbols, with no production-code changes unless required to make these contracts testable.
🤖 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 `@tinyagentos/projects/canvas/watch_projection.py`:
- Around line 134-140: Update _domain_of to parse the URL once and return
parsed.hostname or an empty string, avoiding netloc so userinfo and ports are
excluded; catch the URL parsing error raised for malformed input while
preserving the existing invalid or blank input behavior.
---
Nitpick comments:
In `@tinyagentos/projects/canvas/watch_projection.py`:
- Around line 28-43: Add endpoint-contract tests covering supported rows,
malformed-element placeholder degradation, and deterministic z_index ordering in
tinyagentos/projects/canvas/watch_projection.py:28-43; authorized success and
denied read access in tinyagentos/routes/project_canvas.py:201-208; and exact
GET-only JWT passthrough plus insufficient-scope/project rejection in
tinyagentos/auth_middleware.py:87-87. Use the existing build_watch_projection
and relevant route/auth symbols, with no production-code changes unless required
to make these contracts testable.
🪄 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: 44050b55-44da-4b83-9c70-d33c7b9b8a1c
📒 Files selected for processing (3)
tinyagentos/auth_middleware.pytinyagentos/projects/canvas/watch_projection.pytinyagentos/routes/project_canvas.py
| def _domain_of(url: Any) -> str: | ||
| if not isinstance(url, str) or not url.strip(): | ||
| return "" | ||
| try: | ||
| return urlparse(url).netloc or "" | ||
| except Exception: | ||
| return "" |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Files matching watch_projection.py:\n'
fd -a 'watch_projection\.py$' . || true
file="$(fd 'watch_projection\.py$' . | head -n1 || true)"
if [ -n "${file:-}" ]; then
printf '\nLine count:\n'
wc -l "$file"
printf '\nRelevant source:\n'
sed -n '1,220p' "$file"
fi
printf '\nSearch for _domain_of usages:\n'
rg -n "_domain_of|watch_projection" .Repository: jaylfc/taOS
Length of output: 5933
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Behavioral probe for urlsplit/urlparse semantics on userinfo/ports.
python3 - <<'PY'
from urllib.parse import urlsplit, urlparse
urls = [
"http://example.com/path",
"http://example.com:8080/path",
"http://user:pass@example.com/path",
"http://user@example.com:8080/path",
"ftp://user:secret@example.com:21/dir/file",
"",
"not a url",
]
for url in urls:
try:
parsed = urlsplit(url)
except Exception as e:
print(f"{url!r} -> urlsplit exception {type(e).__name__} {e}")
continue
fields = ({"scheme": parsed.scheme, "netloc": parsed.netloc, "hostname": parsed.hostname, "port": parsed.port})
print(f"{url!r} -> {fields}")
print("\nUsing urlparse:")
for url in urls:
try:
parsed = urlparse(url)
except Exception as e:
print(f"{url!r} -> urlparse exception {type(e).__name__} {e}")
continue
fields = ({"scheme": parsed.scheme, "netloc": parsed.netloc, "hostname": parsed.hostname, "port": parsed.port})
print(f"{url!r} -> {fields}")
if isinstance(url, str) and not url.strip():
continue
PYRepository: jaylfc/taOS
Length of output: 1752
Do not expose URL userinfo as the domain.
netloc includes embedded user:pass@, usernames, and ports, which can be stored in the watch projection. Return parsed.hostname instead and catch the URL parsing error that can be raised for malformed input.
🧰 Tools
🪛 Ruff (0.16.0)
[warning] 139-139: Do not catch blind exception: Exception
(BLE001)
🤖 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/projects/canvas/watch_projection.py` around lines 134 - 140,
Update _domain_of to parse the URL once and return parsed.hostname or an empty
string, avoiding netloc so userinfo and ports are excluded; catch the URL
parsing error raised for malformed input while preserving the existing invalid
or blank input behavior.
Code Review by Qodo
1. Thumbnail source is discarded
|
![qodo-code-review[bot]](https://avatars.github.com/in/484649?s=60&v=4){kind=small}
| ("GET", re.compile(rf"^/api/projects/{_SEG}/canvas/snapshot\.png$")), | ||
| ("GET", re.compile(rf"^/api/projects/{_SEG}/canvas/snapshot\.tldr$")), | ||
| ("GET", re.compile(rf"^/api/projects/{_SEG}/canvas/stream$")), | ||
| ("GET", re.compile(rf"^/api/projects/{_SEG}/canvas/watch-projection$")), |
There was a problem hiding this comment.
2. Allowlist documentation not updated 📜 Skill insight § Compliance
The agent-token allowlist now exposes the watch projection endpoint, but the PR does not update docs/agent-coordination.md or provide the required Docs-Reviewed: declaration. The allowlisted agent-facing API can therefore drift from its coordination documentation.
Agent Prompt
## Issue description
The agent-token route allowlist was changed without the documentation update required by the compliance gate.
## Issue Context
Document the new watch projection route in `docs/agent-coordination.md`. If no user-facing documentation is needed, provide the required `Docs-Reviewed:` commit trailer instead.
## Fix Focus Areas
- tinyagentos/auth_middleware.py[87-87]
- docs/agent-coordination.md[52-79]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| entry = _base_entry(el, "image", "thumbnail") | ||
| entry["alt"] = _payload_str(el, "alt") | ||
| return entry |
There was a problem hiding this comment.
3. Thumbnail source is discarded 🐞 Bug ≡ Correctness
_thumbnail drops payload.file_id and supplies no alternative image source, so every projected image labeled as a thumbnail is non-renderable. Existing canvas image producers and renderers use file_id as the image identity.
Agent Prompt
## Issue description
Image projections discard the stored `file_id`, leaving watch clients unable to render the advertised thumbnail.
## Issue Context
Canvas image elements store their image identity in `payload.file_id`, and the existing UI constructs the image endpoint from that identifier. Preserve a validated identifier or provide an authenticated renderer-neutral thumbnail URL.
## Fix Focus Areas
- tinyagentos/projects/canvas/watch_projection.py[80-83]
- tinyagentos/projects/canvas/mcp_tools.py[109-119]
- desktop/src/apps/ProjectsApp/canvas/shapes/ImageShape.tsx[39-46]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| if not isinstance(url, str) or not url.strip(): | ||
| return "" | ||
| try: | ||
| return urlparse(url).netloc or "" |
There was a problem hiding this comment.
5. Url credentials exposed as domain 🐞 Bug ⛨ Security
_domain_of returns urlparse(url).netloc, which includes userinfo and ports; for example, https://alice:******@example.com exposes alice:secret@example.com as the projected domain. This can leak embedded URL credentials into watch displays, logs, or downstream projection consumers.
Agent Prompt
## Issue description
The projected `domain` field includes URL userinfo because it is populated from `netloc`.
## Issue Context
Stored link payloads retain the original URL, and URL validation uses `hostname` without rejecting userinfo. Populate `domain` from the parsed hostname instead, applying any desired normalized port handling separately, and add credential-bearing URL coverage.
## Fix Focus Areas
- tinyagentos/projects/canvas/watch_projection.py[134-140]
- tinyagentos/projects/canvas/unfurl.py[40-47]
- tinyagentos/projects/canvas/unfurl.py[126-132]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| @router.get("/api/projects/{project_id}/canvas/watch-projection") | ||
| async def watch_project_canvas(project_id: str, request: Request): | ||
| auth = await _authorize_canvas_actor(request, project_id, "read") | ||
| if isinstance(auth, JSONResponse): | ||
| return auth | ||
| cs = request.app.state.project_canvas_store | ||
| elements = await cs.list_elements(project_id) | ||
| return build_watch_projection(elements) |
There was a problem hiding this comment.
6. Projection changes lack tests 🐞 Bug ⚙ Maintainability
The PR adds a new projection contract, route, and bearer-token allowlist entry without any tests, despite the card’s explicit test acceptance criterion. Element mappings, malformed-payload behavior, project errors, and authorization boundaries are consequently unverified.
Agent Prompt
## Issue description
The watch-projection implementation and its authorization exposure have no automated test coverage, contrary to the stated acceptance criterion.
## Issue Context
Add pure projection tests for every supported kind, ordering, malformed/freeform payloads, image sources, and safe domain extraction. Add route and middleware tests covering owner access, agent scope/member flags, wrong-project tokens, nonexistent projects, and exact allowlist matching.
## Fix Focus Areas
- tinyagentos/projects/canvas/watch_projection.py[28-155]
- tests/test_routes_project_canvas.py[342-399]
- tests/test_auth_middleware.py[273-320]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
![kilo-code-bot[bot]](https://avatars.github.com/in/2193792?s=60&v=4){kind=small}
|
|
||
|
|
||
| def _placeholder_entry(el: dict, label: str) -> dict: | ||
| kind = _str_or(el.get("kind"), "unknown") if isinstance(el, dict) else "unknown" |
There was a problem hiding this comment.
WARNING: _placeholder_entry crashes on non-dict el
_str_or(el.get("kind"), "unknown") if isinstance(el, dict) else "unknown" evaluates el.get("kind") before the isinstance check. If el is a list/string/int, this raises AttributeError and crashes the entire projection.
| kind = _str_or(el.get("kind"), "unknown") if isinstance(el, dict) else "unknown" | |
| kind = _str_or(el.get("kind") if isinstance(el, dict) else None, "unknown") |
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| if isinstance(auth, JSONResponse): | ||
| return auth | ||
| cs = request.app.state.project_canvas_store | ||
| elements = await cs.list_elements(project_id) |
There was a problem hiding this comment.
WARNING: Missing project existence check returns 200 for nonexistent projects
watch_project_canvas loads elements without verifying project existence. An authorized request for a nonexistent project returns HTTP 200 with an empty projection instead of 404, making it impossible for clients to distinguish an invalid project from an empty canvas. The existing get_canvas_png route checks project existence before proceeding.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
Code Review SummaryStatus: 2 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
Files Reviewed (3 files)
Fix these issues in Kilo Cloud Reviewed by step-3.7-flash · Input: 131.9K · Output: 29K · Cached: 646.9K |
|
nemotron-ultra-kilo review VERDICT: Approved with minor concerns
Automated first-pass review by the nemotron-ultra-kilo lane. The lead still reviews before merge. |
|
Reviewed at code level with empirical probes of the projection module and a full bot adjudication. BLOCKED on two items, both cheap. @hognek fix-forward on this branch; the authoring lane released the card and will not return. What holds up, do not rework: the projection module's defensive plumbing genuinely works (probed: non-dict rows, list payloads, bare strings, bool z_index all degrade to correct placeholders, sorted by z_index), and the auth wiring is correct end to end: the middleware entry is registry-JWT reachability, not a public bypass; the handler enforces canvas_read + project binding with existence-hiding 404 + the member flag, identical to the sibling canvas routes. BLOCKING:
FIX-WITH (small, same commit): Bot adjudication so you skip the noise: CodeRabbit's netloc finding and Qodo's no-tests, file_id and docs findings are real (items above). Kilo's _placeholder_entry-crashes-on-non-dict is FALSE, the ternary guards it and I ran the case. Kilo's missing-project-200 inconsistency matches the sibling GET /canvas/elements exactly, pre-existing idiom, ignore here. The nemotron comment claiming an auth bypass via _PUBLIC_ROUTES is FALSE, the list is _AGENT_TASK_ROUTES and both middleware auth and handler authorization still run. Re-request review after pushing and I will re-verify with the same probes. |
|
nemotron-super review VERDICT: No blocking issues found Automated first-pass review by the nemotron-super lane. The lead still reviews before merge. |
1 participant
Autonomous build of board card tsk-i6hggr.
Files:
tinyagentos/auth_middleware.py | 1 +
tinyagentos/projects/canvas/watch_projection.py | 155 ++++++++++++++++++++++++
tinyagentos/routes/project_canvas.py | 11 ++
3 files changed, 167 insertions(+)
Summary by Gitar
watch_projection.py/api/projects/{project_id}/canvas/watch-projectionThis will update automatically on new commits.
Summary by CodeRabbit