Add task edge read endpoint - #185
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 new task-graph edges query capability spanning the data layer ( ChangesTask Edges Listing Feature
Estimated code review effort: 2 (Simple) | ~12 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant HTTPServer
participant Service
participant RemoteClient
participant TasksDB
Client->>HTTPServer: GET /tasks/edges?from_id&to_id&type&limit
HTTPServer->>HTTPServer: _handle_task_list_edges(query)
HTTPServer->>Service: task_list_edges(from_id, to_id, edge_type, limit)
alt remote server configured
Service->>RemoteClient: task_list_edges(...)
RemoteClient->>HTTPServer: GET /tasks/edges (remote)
RemoteClient-->>Service: edges list
else local mode
Service->>TasksDB: list_edges(...)
TasksDB-->>Service: filtered active edges
end
Service-->>HTTPServer: edges list
HTTPServer-->>Client: {"edges": [...]}
🚥 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 |
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| ) | ||
| self._send_json(200, {"tasks": tasks}) | ||
|
|
||
| def _handle_task_list_edges(self, qs: dict) -> None: |
There was a problem hiding this comment.
WARNING: _handle_task_list_edges skips the token-binding step that every other task read endpoint applies.
The sibling handlers _handle_task_list (line 1374), _handle_task_ready (line 1419), and _handle_task_prime (line 1435) all call self._apply_token_binding(assignee, project) before dispatching to the service layer, so a registry verifier / grants check can scope results to the caller's project_id. The new /tasks/edges handler returns the full active edge graph to any caller that reaches it, which leaks task-graph topology (parent/blocks/relates/duplicates relationships) for every project, not just the caller's. Apply _apply_token_binding (or an analogous project-scoping filter) here for consistency and to prevent cross-project information disclosure.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| from_id = (qs.get("from_id") or [None])[0] | ||
| to_id = (qs.get("to_id") or [None])[0] | ||
| edge_type = (qs.get("type") or [None])[0] | ||
| limit_raw = (qs.get("limit") or [500])[0] |
There was a problem hiding this comment.
WARNING: limit is accepted as a raw integer with no upper bound and no positivity check.
int(limit_raw) only rejects non-integer values. A client can send ?limit=10000000 (or ?limit=-1) and force an unbounded scan / huge response from task_edges. After int(...) succeeds, add a guard such as if limit_i <= 0 or limit_i > 500: raise _BadRequest(...) so the limit is clamped to a sane range and the SQLite LIMIT ? binding can't receive negative values (SQLite raises sqlite3.InterfaceError on a non-positive LIMIT, which would surface as a 500 here). The same gap exists for sibling handlers (e.g. _handle_task_list at line 1366) but since this handler is new code it should ship with the validation in place.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| conditions.append("type = ?") | ||
| params.append(edge_type) | ||
|
|
||
| params.append(limit) |
There was a problem hiding this comment.
SUGGESTION: Validate limit in the core list_edges function (and reject limit <= 0).
_get_db(...).execute(... LIMIT ?, params) will raise sqlite3.InterfaceError if limit is 0 or negative, and there is no upper-bound cap. A defensive check before the SQL — e.g. if limit <= 0 or limit > MAX_EDGE_LIMIT: raise ValueError(...) — keeps the contract local to the tasks module and prevents the HTTP layer from translating a sqlite3.InterfaceError into an opaque 500. The same params.append(limit) pattern in list_tasks (line 287) has the same gap; mentioning it here for awareness, but the fix should land in this new function first.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
Code Review SummaryStatus: 3 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed (7 files)
Fix these issues in Kilo Cloud Reviewed by minimax-m3 · Input: 42.5K · Output: 4.6K · Cached: 366.4K |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@taosmd/http_server.py`:
- Around line 1388-1410: The `_handle_task_list_edges` handler is missing
project scoping, so it should apply the same token binding/isolation as the
other task routes. Update `_handle_task_list_edges` to thread the current
project context through `service.task_list_edges` and ensure the query is
filtered by `tasks.project`, or alternatively make this endpoint admin-only if
that better matches the access model. Use `_apply_token_binding` and the
existing task handlers as the reference points for where to enforce the project
restriction.
In `@taosmd/tasks.py`:
- Around line 295-328: The list_edges function forwards limit directly into the
SQLite LIMIT clause without enforcing a minimum, so add lower-bound validation
before executing the query. In list_edges, reject values less than 1 with a
ValueError (or clamp only if that matches the API contract) alongside the
existing edge_type validation, so negative limits cannot return unbounded rows.
Keep the fix localized to list_edges and preserve the current filtering/query
behavior for valid inputs.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 72ec0a02-33d4-4106-9daa-819833e49ddd
📒 Files selected for processing (7)
.gitignoretaosmd/http_server.pytaosmd/remote.pytaosmd/service.pytaosmd/tasks.pytests/test_http_server.pytests/test_tasks.py
| def _handle_task_list_edges(self, qs: dict) -> None: | ||
| from_id = (qs.get("from_id") or [None])[0] | ||
| to_id = (qs.get("to_id") or [None])[0] | ||
| edge_type = (qs.get("type") or [None])[0] | ||
| limit_raw = (qs.get("limit") or [500])[0] | ||
| try: | ||
| limit_i = int(limit_raw) | ||
| except (TypeError, ValueError) as exc: | ||
| raise _BadRequest("'limit' must be an integer") from exc | ||
| try: | ||
| edges = runner.run( | ||
| service.task_list_edges( | ||
| from_id=from_id, | ||
| to_id=to_id, | ||
| edge_type=edge_type, | ||
| limit=limit_i, | ||
| data_dir=data_dir, | ||
| ) | ||
| ) | ||
| except ValueError as exc: | ||
| raise _BadRequest(str(exc)) from exc | ||
| self._send_json(200, {"edges": edges}) | ||
|
|
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect relevant handler methods and token-binding helper usage
python3 - <<'PY'
from pathlib import Path
path = Path('taosmd/http_server.py')
text = path.read_text()
lines = text.splitlines()
def show(start, end):
for i in range(start, end+1):
print(f"{i:5d}: {lines[i-1]}")
for start, end in [(650, 710), (1340, 1435)]:
print(f"\n--- taosmd/http_server.py {start}-{end} ---")
show(start, end)
PY
# Locate task_list_edges implementation and related schema/docs
rg -n "def task_list_edges|task_edges|_apply_token_binding|project" taosmd -g '!**/__pycache__/**'Repository: jaylfc/taosmd
Length of output: 50370
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
path = Path('taosmd/http_server.py')
lines = path.read_text().splitlines()
def show(start, end):
print(f"\n--- taosmd/http_server.py {start}-{end} ---")
for i in range(start, end + 1):
if 1 <= i <= len(lines):
print(f"{i:5d}: {lines[i-1]}")
for rng in [(650, 710), (1340, 1435)]:
show(*rng)
PY
rg -n "def task_list_edges|task_edges|_apply_token_binding|project" taosmd -g '!**/__pycache__/**'Repository: jaylfc/taosmd
Length of output: 50370
🏁 Script executed:
python3 - <<'PY'
from pathlib import Path
p = Path('taosmd/http_server.py')
lines = p.read_text().splitlines()
for s,e in [(650,710),(1340,1435)]:
print(f"\n--- {p} {s}-{e} ---")
for i in range(s,e+1):
if i<=len(lines):
print(f"{i:5d}: {lines[i-1]}")
PY
rg -n "def task_list_edges|task_edges|_apply_token_binding|project" taosmdRepository: jaylfc/taosmd
Length of output: 50370
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
p = Path('taosmd/http_server.py')
lines = p.read_text().splitlines()
for s,e in [(650,710),(1340,1435)]:
print(f"\n--- {p} {s}-{e} ---")
for i in range(s,e+1):
if i <= len(lines):
print(f"{i:5d}: {lines[i-1]}")
PY
rg -n "def task_list_edges|task_edges|_apply_token_binding|project" taosmdRepository: jaylfc/taosmd
Length of output: 50370
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
def show(path_str, start, end):
path = Path(path_str)
lines = path.read_text().splitlines()
print(f"\n--- {path_str} {start}-{end} ---")
for i in range(start, end + 1):
if i <= len(lines):
print(f"{i:5d}: {lines[i-1]}")
show('taosmd/tasks.py', 55, 110)
show('taosmd/tasks.py', 250, 340)
show('taosmd/tasks.py', 620, 705)
show('taosmd/service.py', 640, 695)
PYRepository: jaylfc/taosmd
Length of output: 11185
Scope task-edge reads to project
taosmd/http_server.py:1388-1409 — _handle_task_list_edges skips _apply_token_binding, so a project-scoped registry token can read edges server-wide. Thread project through service.task_list_edges and filter via tasks.project (or restrict this route to admin-only) to match the other task handlers' isolation.
🤖 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 `@taosmd/http_server.py` around lines 1388 - 1410, The
`_handle_task_list_edges` handler is missing project scoping, so it should apply
the same token binding/isolation as the other task routes. Update
`_handle_task_list_edges` to thread the current project context through
`service.task_list_edges` and ensure the query is filtered by `tasks.project`,
or alternatively make this endpoint admin-only if that better matches the access
model. Use `_apply_token_binding` and the existing task handlers as the
reference points for where to enforce the project restriction.
| async def list_edges( | ||
| *, | ||
| from_id: str | None = None, | ||
| to_id: str | None = None, | ||
| edge_type: str | None = None, | ||
| limit: int = 500, | ||
| data_dir: str | None = None, | ||
| ) -> list[dict]: | ||
| """Return active task edges matching the given filters, newest first.""" | ||
| if edge_type is not None and edge_type not in VALID_EDGE_TYPES: | ||
| raise ValueError(f"edge_type must be one of {sorted(VALID_EDGE_TYPES)}") | ||
|
|
||
| conn = _get_db(data_dir) | ||
| conditions: list[str] = ["removed_ts IS NULL"] | ||
| params: list[Any] = [] | ||
|
|
||
| if from_id is not None: | ||
| conditions.append("from_id = ?") | ||
| params.append(from_id) | ||
| if to_id is not None: | ||
| conditions.append("to_id = ?") | ||
| params.append(to_id) | ||
| if edge_type is not None: | ||
| conditions.append("type = ?") | ||
| params.append(edge_type) | ||
|
|
||
| params.append(limit) | ||
| rows = conn.execute( | ||
| "SELECT from_id, to_id, type, created_ts, created_by, removed_ts " | ||
| f"FROM task_edges WHERE {' AND '.join(conditions)} " | ||
| "ORDER BY created_ts DESC LIMIT ?", | ||
| params, | ||
| ).fetchall() | ||
| return [dict(r) for r in rows] |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
No lower-bound validation on limit.
limit is forwarded straight into LIMIT ? without checking it's positive. SQLite treats a negative LIMIT as "no limit," so callers passing e.g. limit=-1 (a valid Python int, and the HTTP handler only checks int(limit_raw) succeeds) get every matching row back unbounded — a potential resource/DoS concern for a table that can grow without bound.
🛡️ Proposed validation
if edge_type is not None and edge_type not in VALID_EDGE_TYPES:
raise ValueError(f"edge_type must be one of {sorted(VALID_EDGE_TYPES)}")
+ if limit < 1:
+ raise ValueError("limit must be a positive integer")📝 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.
| async def list_edges( | |
| *, | |
| from_id: str | None = None, | |
| to_id: str | None = None, | |
| edge_type: str | None = None, | |
| limit: int = 500, | |
| data_dir: str | None = None, | |
| ) -> list[dict]: | |
| """Return active task edges matching the given filters, newest first.""" | |
| if edge_type is not None and edge_type not in VALID_EDGE_TYPES: | |
| raise ValueError(f"edge_type must be one of {sorted(VALID_EDGE_TYPES)}") | |
| conn = _get_db(data_dir) | |
| conditions: list[str] = ["removed_ts IS NULL"] | |
| params: list[Any] = [] | |
| if from_id is not None: | |
| conditions.append("from_id = ?") | |
| params.append(from_id) | |
| if to_id is not None: | |
| conditions.append("to_id = ?") | |
| params.append(to_id) | |
| if edge_type is not None: | |
| conditions.append("type = ?") | |
| params.append(edge_type) | |
| params.append(limit) | |
| rows = conn.execute( | |
| "SELECT from_id, to_id, type, created_ts, created_by, removed_ts " | |
| f"FROM task_edges WHERE {' AND '.join(conditions)} " | |
| "ORDER BY created_ts DESC LIMIT ?", | |
| params, | |
| ).fetchall() | |
| return [dict(r) for r in rows] | |
| async def list_edges( | |
| *, | |
| from_id: str | None = None, | |
| to_id: str | None = None, | |
| edge_type: str | None = None, | |
| limit: int = 500, | |
| data_dir: str | None = None, | |
| ) -> list[dict]: | |
| """Return active task edges matching the given filters, newest first.""" | |
| if edge_type is not None and edge_type not in VALID_EDGE_TYPES: | |
| raise ValueError(f"edge_type must be one of {sorted(VALID_EDGE_TYPES)}") | |
| if limit < 1: | |
| raise ValueError("limit must be a positive integer") | |
| conn = _get_db(data_dir) | |
| conditions: list[str] = ["removed_ts IS NULL"] | |
| params: list[Any] = [] | |
| if from_id is not None: | |
| conditions.append("from_id = ?") | |
| params.append(from_id) | |
| if to_id is not None: | |
| conditions.append("to_id = ?") | |
| params.append(to_id) | |
| if edge_type is not None: | |
| conditions.append("type = ?") | |
| params.append(edge_type) | |
| params.append(limit) | |
| rows = conn.execute( | |
| "SELECT from_id, to_id, type, created_ts, created_by, removed_ts " | |
| f"FROM task_edges WHERE {' AND '.join(conditions)} " | |
| "ORDER BY created_ts DESC LIMIT ?", | |
| params, | |
| ).fetchall() | |
| return [dict(r) for r in rows] |
🧰 Tools
🪛 Ruff (0.15.20)
[error] 323-325: Possible SQL injection vector through string-based query construction
(S608)
🤖 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 `@taosmd/tasks.py` around lines 295 - 328, The list_edges function forwards
limit directly into the SQLite LIMIT clause without enforcing a minimum, so add
lower-bound validation before executing the query. In list_edges, reject values
less than 1 with a ValueError (or clamp only if that matches the API contract)
alongside the existing edge_type validation, so negative limits cannot return
unbounded rows. Keep the fix localized to list_edges and preserve the current
filtering/query behavior for valid inputs.
|
Thanks for this, it's a genuinely tidy PR. The layering matches how the rest of the codebase is built, the SQL is parameterized with the type allowlist checked up front, and the tombstone handling lines up with remove_edge, so nice work reading the conventions. One blocker before I can merge: the handler needs to go through _apply_token_binding the way _handle_task_list does, so a verified registry token gets its grant checked and a project-bound token can't read edges belonging to other projects. Since task_edges has no project column you'll want to join to tasks to scope by the bound project. The edge write endpoints have the same gap on our side and a fix is planned, so I'd rather not add another instance via a read path. Could you also add a short CHANGELOG entry, and ideally a bounds check on limit (negative means unlimited in SQLite) plus an HTTP test for an invalid type? The .venv ignore line is fine to keep. |
|
Quick update: the scoping infrastructure just landed on master (#186 for the edge write endpoints, #187 for task update), so there's now a ready-made path for this PR. After a rebase you can route the handler through _apply_token_binding and use service.task_projects to filter the returned edges to the token's bound project (see _enforce_edge_project_scope in http_server.py for the non-enumerating pattern). Happy to merge once that's in along with the CHANGELOG line. |
|
Thank you for this, and apologies for the long wait — the delay is on us, not on the contribution. The endpoint is a genuine gap and the implementation is clean: sensible filters, an explicit One blocking issue, and it is a matter of timing rather than of your code. This PR was opened on 2026-07-06. Two security fixes landed shortly afterwards that changed the contract for exactly this family of endpoints:
Since those, every task-edge surface derives its scope from the token, and a token bound to a project may only see that project's tasks. Foreign and nonexistent ids deliberately return the identical 403, so that task existence cannot be probed by an unauthorised caller. On master today that is enforced through
To be clear about blame: nothing here was careless. The endpoint simply predates the posture, and there was no way for you to write against a contract that did not exist yet. What it needs
How to proceed Please open this as a new PR from a fresh branch rather than pushing to this one. Our build tooling constructs a fresh worktree and cannot operate on an existing branch, so a follow-up on this branch would be unworkable on our side. Reference this PR number so the history stays followable, and I will close this one with a pointer once the replacement is up. Happy to review promptly this time — and if you would rather not carry the scoping work, say so and I will card it internally with full credit to you for the endpoint design and the tests. |
|
Correction to my review above — please disregard the "open a new PR" request. Keep this branch. I asked you to abandon this branch and open a fresh PR, explaining that "our build tooling constructs a fresh worktree and cannot operate on an existing branch." That limitation is real, but it applies only to our internal automated build lanes. You are pushing your own commits from your own branch and never touch that tooling, so none of it constrains you. I imposed an internal constraint on an outside contributor for no benefit. Push the scoping changes to this branch as you normally would, and this PR merges as-is once they land. Everything else in the review stands unchanged — the token-derived scoping, the non-enumerating My thanks for the patience; you have now waited on us twice. |
2 participants
Summary
Tests
Summary by CodeRabbit
New Features
GET /tasks/edgesendpoint.Bug Fixes
Chores