tsk-mtds32 [OPEN] S4e: device pair-requests + grant Decision (consen - #2233
tsk-mtds32 [OPEN] S4e: device pair-requests + grant Decision (consen#2233jaylfc wants to merge 1 commit into
Conversation
📝 WalkthroughWalkthroughAdds SQLite-backed device-pairing request storage and unauthenticated routes. The flow supports validation, expiry, admin decisions, capability-based polling, sanitized device data, and one-time scoped tokens. ChangesDevice pairing
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant PairingRoutes
participant DevicePairRequestsStore
participant DecisionService
participant DeviceRegistry
Client->>PairingRoutes: Create pairing request
PairingRoutes->>DevicePairRequestsStore: Persist request
PairingRoutes->>DecisionService: Create admin decision
PairingRoutes-->>Client: Return request ID and verification code
Client->>PairingRoutes: Poll request ID
PairingRoutes->>DevicePairRequestsStore: Read request and claim token
PairingRoutes->>DeviceRegistry: Retrieve accepted device data
PairingRoutes-->>Client: Return status, device data, and scoped token
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 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 device pairing requests store + Decisions-gated poll API
AI Description
Diagram
High-Level Assessment
Files changed (2)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (7)
tinyagentos/routes/device_pair_requests.py (3)
180-181: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winLog the swallowed notification failure.
The comment at lines 167-168 states that the bell notification is best-effort, so swallowing the exception is intentional. Discarding it without a log leaves no trace when the notification subsystem fails. Ruff reports this as S110 and BLE001.
♻️ Proposed change
- except Exception: - pass + except Exception: + logger.warning( + "device_pair_requests: bell notification failed", exc_info=True + )🤖 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/device_pair_requests.py` around lines 180 - 181, Update the notification exception handler in the device-pair request flow to retain best-effort behavior while logging the swallowed exception. Replace the bare pass in the visible except block with the project’s established logging mechanism, preserving the existing broad exception handling and avoiding propagation.Source: Linters/SAST tools
48-57: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the unused constant and the unused request model.
_MAX_DEVICES_PER_USERis never referenced in this file. The docstring at line 23 states thatDeviceStore.registerowns the per-user cap, so this copy can drift from the real limit.
PairRequestInis not used by either route. It also declares averify_codefield, which contradicts the security note at lines 19-20 stating that no endpoint acceptsverify_codeas input. A future author may wire it up and break that property.♻️ Proposed change
_VALID_PLATFORMS = frozenset({"ios", "watchos", "android"}) _VERIFY_CODE_DIGITS = 6 -_MAX_DEVICES_PER_USER = 50 class CreatePairRequest(BaseModel): platform: str display_name: str = "" - - -class PairRequestIn(BaseModel): - verify_code: str | None = None🤖 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/device_pair_requests.py` around lines 48 - 57, Remove the unused _MAX_DEVICES_PER_USER constant and the unused PairRequestIn model from this module. Leave DeviceStore.register as the sole owner of the per-user device limit and ensure no request model exposes a verify_code input.
98-99: 📐 Maintainability & Code Quality | 🔵 TrivialTests are missing for this unauthenticated flow.
The PR description states that the card requests tests and that this must be resolved before merging. No test file is changed in this PR. This flow is unauthenticated and issues device credentials, so it needs coverage for the platform whitelist rejection, the pending cap 429, the 404 on an unknown id, the expired-pending status, and the single release of
scoped_tokenacross two polls.Do you want me to generate the test module, or open an issue to track it?
🤖 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/device_pair_requests.py` around lines 98 - 99, Add tests for the unauthenticated create_pair_request flow covering platform whitelist rejection, the pending-cap 429 response, unknown-id 404 handling, expired-pending status, and exactly one scoped_token release across two polls. Place the coverage in a test module for the device pairing routes and exercise the /api/devices/pair-requests endpoint without authentication.tinyagentos/device_pair_requests_store.py (4)
29-32: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePromote the names that other modules import.
tinyagentos/routes/device_pair_requests.pyimports_PENDING_CAPand_live_statusfrom this module. Both names use the underscore prefix, which signals module-private use. Rename them toPENDING_CAPandlive_status(or re-export public aliases) so the cross-module contract is explicit.Also applies to: 87-93
🤖 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/device_pair_requests_store.py` around lines 29 - 32, Promote the cross-module symbols in device_pair_requests_store by exposing public names PENDING_CAP and live_status instead of relying on _PENDING_CAP and _live_status. Update all definitions and references, including imports in routes/device_pair_requests.py, while preserving the existing cap and live-status behavior; public aliases are acceptable if compatibility with internal references is needed.
220-234: 🔒 Security & Privacy | 🔵 TrivialConsider a purge path for expired pending requests.
count_pendingandlist_pendingboth exclude rows pastexpires_at_ts. Those rows are never deleted or transitioned, so the table grows without bound and retainsrequester_ipandverify_codeindefinitely. Add a periodic delete or a transition-to-expiredsweep with a retention window. This also limits PII retention forrequester_ip.🤖 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/device_pair_requests_store.py` around lines 220 - 234, Add an expiration sweep to DevicePairRequestsStore, invoked from the pending-request flow such as count_pending and list_pending, that deletes expired pending rows or transitions them to expired and removes them after a defined retention window. Ensure the sweep uses expires_at_ts and preserves counting/listing only live pending requests while preventing indefinite retention of requester_ip and verify_code.
208-211: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSilence the SQL-injection lint findings with a justification.
_SAFE_COLSis a module-level literal, so the f-string cannot carry caller input. Ruff (S608) and OpenGrep both report this line, which can fail the lint stage. Add a scoped suppression and state why the interpolation is safe.♻️ Proposed change
cur = await self._db.execute( - f"SELECT {_SAFE_COLS} FROM device_pair_requests WHERE id = ?", + # _SAFE_COLS is a module-level column-name literal, never caller input. + f"SELECT {_SAFE_COLS} FROM device_pair_requests WHERE id = ?", # noqa: S608 (pair_request_id,), )🤖 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/device_pair_requests_store.py` around lines 208 - 211, Add a narrowly scoped suppression for Ruff S608 and the OpenGrep SQL-injection finding on the SELECT statement in the device-pair request lookup, with an adjacent justification that `_SAFE_COLS` is a module-level literal containing no caller-controlled input. Keep parameterization of `pair_request_id` unchanged.Source: Linters/SAST tools
241-252: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winUse
_SAFE_COLSinstead ofSELECT *inlist_pending.
SELECT *readsverify_codeinto memory, and correctness then depends on thepopat line 251.getalready projects_SAFE_COLSin SQL. Apply the same projection here so the nonce never leaves the database on this path.♻️ Proposed change
cur = await self._db.execute( - "SELECT * FROM device_pair_requests " + f"SELECT {_SAFE_COLS} FROM device_pair_requests " # noqa: S608 "WHERE status = 'pending' AND expires_at_ts > ? " "ORDER BY created_ts", (now_iso,), ) rows = await cur.fetchall() out = [] for row in rows: d = {k: row[k] for k in row.keys()} - d.pop("verify_code", None) out.append(d)🤖 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/device_pair_requests_store.py` around lines 241 - 252, Update the SQL query in list_pending to select the existing _SAFE_COLS projection instead of SELECT *. Keep the pending-status, expiration, ordering, and result conversion behavior unchanged; the query must exclude verify_code before rows are fetched, and the post-fetch pop can be removed if no longer needed.
🤖 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/device_pair_requests_store.py`:
- Around line 1-19: Restore the module docstrings by placing each documentation
string before the future import. In tinyagentos/device_pair_requests_store.py
(lines 1-19), move the docstring above from __future__ import annotations; apply
the same ordering change in tinyagentos/routes/device_pair_requests.py (lines
1-28), with no other changes.
- Around line 161-177: Update set_decision’s pending-row UPDATE to allow
acceptance only when expires_at_ts has not passed, preserving atomicity and
returning None for expired requests. Ensure expired decisions cannot mint
device_id, and revise the method’s docstring comment to describe this expired
decision behavior.
In `@tinyagentos/routes/device_pair_requests.py`:
- Around line 88-95: The pairing route currently returns an unapprovable request
when no admin exists or Decision creation fails. In
tinyagentos/routes/device_pair_requests.py lines 88-95, update _admin_user_id to
log a warning when no admin is found and make create_pair_request return HTTP
503 instead of a request ID; at lines 164-165, raise
HTTPException(status_code=503, detail="pairing unavailable") immediately after
logging the Decision failure.
- Around line 88-95: Update _admin_user_id to select the primary admin
deterministically using an explicit rule such as the configured primary-admin ID
or earliest created_at, and handle the no-admin case explicitly. Ensure the
pairing route does not return a successful pair_request_id while silently
skipping approval: log a warning and/or reject the request with HTTP 503 before
creating the request when no admin resolves.
- Around line 113-136: Move the pending-cap enforcement from the separate
count/create flow in the route to an atomic store operation: update the store
method used by the pairing endpoint to perform a guarded INSERT that only
succeeds when pending rows are below _PENDING_CAP, returning the created record
or None. Update the route around store.create to map a None result to the
existing 429 response, while preserving the current success response and request
fields.
- Around line 81-85: Update _requester_ip to avoid returning the raw
request.client.host: when the immediate client belongs to a configured trusted
proxy range, extract the original client IP from the appropriate forwarded
header; otherwise use the direct peer address. Preserve the empty result when no
client is present and ensure forwarded headers are ignored for untrusted
clients.
- Around line 51-53: Update CreatePairRequest to enforce a maximum length for
display_name and constrain platform using a Literal derived from the existing
_VALID_PLATFORMS source. Then remove the redundant platform validation in the
route, or retain it only if the model constraint remains the single
authoritative definition.
---
Nitpick comments:
In `@tinyagentos/device_pair_requests_store.py`:
- Around line 29-32: Promote the cross-module symbols in
device_pair_requests_store by exposing public names PENDING_CAP and live_status
instead of relying on _PENDING_CAP and _live_status. Update all definitions and
references, including imports in routes/device_pair_requests.py, while
preserving the existing cap and live-status behavior; public aliases are
acceptable if compatibility with internal references is needed.
- Around line 220-234: Add an expiration sweep to DevicePairRequestsStore,
invoked from the pending-request flow such as count_pending and list_pending,
that deletes expired pending rows or transitions them to expired and removes
them after a defined retention window. Ensure the sweep uses expires_at_ts and
preserves counting/listing only live pending requests while preventing
indefinite retention of requester_ip and verify_code.
- Around line 208-211: Add a narrowly scoped suppression for Ruff S608 and the
OpenGrep SQL-injection finding on the SELECT statement in the device-pair
request lookup, with an adjacent justification that `_SAFE_COLS` is a
module-level literal containing no caller-controlled input. Keep
parameterization of `pair_request_id` unchanged.
- Around line 241-252: Update the SQL query in list_pending to select the
existing _SAFE_COLS projection instead of SELECT *. Keep the pending-status,
expiration, ordering, and result conversion behavior unchanged; the query must
exclude verify_code before rows are fetched, and the post-fetch pop can be
removed if no longer needed.
In `@tinyagentos/routes/device_pair_requests.py`:
- Around line 180-181: Update the notification exception handler in the
device-pair request flow to retain best-effort behavior while logging the
swallowed exception. Replace the bare pass in the visible except block with the
project’s established logging mechanism, preserving the existing broad exception
handling and avoiding propagation.
- Around line 48-57: Remove the unused _MAX_DEVICES_PER_USER constant and the
unused PairRequestIn model from this module. Leave DeviceStore.register as the
sole owner of the per-user device limit and ensure no request model exposes a
verify_code input.
- Around line 98-99: Add tests for the unauthenticated create_pair_request flow
covering platform whitelist rejection, the pending-cap 429 response, unknown-id
404 handling, expired-pending status, and exactly one scoped_token release
across two polls. Place the coverage in a test module for the device pairing
routes and exercise the /api/devices/pair-requests endpoint without
authentication.
🪄 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: 61062f39-71c7-4950-b8ca-c03f2e3e94a6
📒 Files selected for processing (2)
tinyagentos/device_pair_requests_store.pytinyagentos/routes/device_pair_requests.py
| from __future__ import annotations | ||
|
|
||
| """Store for device pairing (consent) requests. | ||
|
|
||
| Each record tracks one inbound pairing request from an external device/app. | ||
| Pending requests wait for the instance user to approve or deny (surfaced | ||
| through a device_pairing Decision). Accepted requests carry the minted | ||
| ``device_id`` so the caller can poll and retrieve the issued device token. | ||
|
|
||
| The state machine is: pending -> accepted | denied | expired (terminal). | ||
| ``set_decision`` is atomic -- it uses a conditional UPDATE that only matches | ||
| rows still in ``pending`` status, so two concurrent approve/deny calls cannot | ||
| both win a read-check-then-write race. | ||
|
|
||
| The ``verify_code`` is a human-comparison nonce (F3): it is persisted only so | ||
| the Decision text can display it for the approving user, and is NEVER returned | ||
| by ``get``/poll -- the route layer strips it. It is never server-checked and no | ||
| endpoint accepts it as input. | ||
| """ |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Both new modules lose their docstring to the from __future__ import. Python assigns __doc__ only when the string literal is the first statement in the module. In both files from __future__ import annotations comes first, so the documentation block becomes a discarded expression.
tinyagentos/device_pair_requests_store.py#L1-L19: move the docstring at lines 3-19 above line 1, and placefrom __future__ import annotationsdirectly after it.tinyagentos/routes/device_pair_requests.py#L1-L28: move the docstring at lines 3-28 above line 1, and placefrom __future__ import annotationsdirectly after it.
📍 Affects 2 files
tinyagentos/device_pair_requests_store.py#L1-L19(this comment)tinyagentos/routes/device_pair_requests.py#L1-L28
🤖 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/device_pair_requests_store.py` around lines 1 - 19, Restore the
module docstrings by placing each documentation string before the future import.
In tinyagentos/device_pair_requests_store.py (lines 1-19), move the docstring
above from __future__ import annotations; apply the same ordering change in
tinyagentos/routes/device_pair_requests.py (lines 1-28), with no other changes.
| now_iso = _iso(_now()) | ||
| cur = await self._db.execute( | ||
| """ | ||
| UPDATE device_pair_requests | ||
| SET status = ?, | ||
| device_id = ?, | ||
| decided_ts = ?, | ||
| decided_by = ? | ||
| WHERE id = ? AND status = 'pending' | ||
| """, | ||
| (status, device_id if status == "accepted" else None, | ||
| now_iso, decided_by, pair_request_id), | ||
| ) | ||
| await self._db.commit() | ||
| if cur.rowcount == 0: | ||
| return None | ||
| return await self.get(pair_request_id) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Inspect the device pairing grant path for an expiry check.
set -euo pipefail
fd -t f 'decisions.py' -x rg -n -C 12 '_apply_device_pairing_grant|device_pairing' {}
# Any other caller of set_decision / _is_expired across the repo.
rg -nP -C 6 '\bset_decision\s*\(|\b_is_expired\s*\('Repository: jaylfc/taOS
Length of output: 149
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Candidate files:\n'
git ls-files | rg '(^|/)device_pair_requests_store\.py$|(^|/)decisions\.py$|(^|/)device_pair_requests\.py$|(^|/)routes/'
printf '\nFind matching source filenames:\n'
fd -t f 'device_pair_requests_store.py|decisions.py|device_pair_requests.py' || true
printf '\nStore file outline:\n'
ast-grep outline tinyagentos/device_pair_requests_store.py || true
printf '\nRelevant store sections:\n'
sed -n '1,220p' tinyagentos/device_pair_requests_store.py | cat -n
printf '\nSearch for grant/request routes and decision handlers:\n'
rg -n -C 8 'device_pair|pair_request|_apply_device_pairing_grant|set_decision|_is_expired|decides|pairing' . || trueRepository: jaylfc/taOS
Length of output: 50370
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Store outline:\n'
ast-grep outline tinyagentos/device_pair_requests_store.py || true
printf '\nRelevant store file sections:\n'
sed -n '1,220p' tinyagentos/device_pair_requests_store.py | cat -n
printf '\nRoute decision handler references:\n'
rg -n -C 10 '_apply_device_pairing_grant|def .*device.*pair|kind.*device_pairing|pair_request_id' tinyagentos/routes/decisions.py || true
printf '\nAll small focused callers in tinyagentos/tests:\n'
rg -n -C 8 '\.set_decision|DevicePairRequestsStore|device_pair|pair_request_id' tinyagentos/tests tinyagentos/routes tinyagentos || true
printf '\nSQLite UPDATE behavior probe:\n'
python3 - <<'PY'
import sqlite3
con = sqlite3.connect(":memory:")
con.executescript("""
CREATE TABLE device_pair_requests (
id TEXT PRIMARY KEY,
status TEXT,
device_id TEXT,
decided_ts TEXT,
decided_by TEXT,
expires_at_ts TEXT,
claim_request TEXT,
created_ts TEXT
);
""")
con.execute("INSERT INTO device_pair_requests VALUES (?, ?, ?, ?, ?, ?, ?, ?)",
["a","pending","d","2026-01-01T12:00:00Z","admin","2026-01-01T11:55:00Z","c","created"])
now = "2026-01-01T12:15:00Z"
cur = con.execute("""
UPDATE device_pair_requests
SET status = ?, device_id = ?, decided_ts = ?, decided_by = ?
WHERE id = ? AND status = 'pending'
""", ("accepted", "d", now, "admin", "a"))
print("rowcount_no_expiry_guard=", cur.rowcount)
print("status_after=", con.execute("SELECT status,device_id,expires_at_ts FROM device_pair_requests WHERE id='a'").fetchone())
con.execute("INSERT INTO device_pair_requests VALUES (?, ?, ?, ?, ?, ?, ?, ?)",
["b","accepted","d2","2026-01-01T11:00:00Z","admin","2026-01-01T11:05:00Z","c","created"])
cur2 = con.execute("""
UPDATE device_pair_requests
SET status = ?, device_id = ?, decided_ts = ?, decided_by = ?
WHERE id = ?
AND status = 'pending'
AND (? != 'accepted' OR expires_at_ts > ?)
""", ("accepted", "d2", now, "admin", "b", "accepted", now))
print("rowcount_already_decided=", cur2.rowcount)
PYRepository: jaylfc/taOS
Length of output: 50367
Enforce the pairing TTL atomically in set_decision.
create() sets _TTL_SECS for expires_at_ts, and _live_status() treats overdue pending rows as expired, but set_decision() only checks status = 'pending'. An approve after the 10-minute TTL can set status = 'accepted' and mint device_id; the poll route then releases the scoped token. Add the expiry predicate to the accept UPDATE and update the docstring comment so it describes the expired decision 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 `@tinyagentos/device_pair_requests_store.py` around lines 161 - 177, Update
set_decision’s pending-row UPDATE to allow acceptance only when expires_at_ts
has not passed, preserving atomicity and returning None for expired requests.
Ensure expired decisions cannot mint device_id, and revise the method’s
docstring comment to describe this expired decision behavior.
| class CreatePairRequest(BaseModel): | ||
| platform: str | ||
| display_name: str = "" |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
Bound display_name and platform at the model.
display_name has no length limit. The route stores it, embeds it in the Decision question at line 149, and embeds it in the notification message at line 174. An unauthenticated caller can therefore write an arbitrarily large string into the database and into the admin inbox.
platform is validated against _VALID_PLATFORMS at line 107, so a Literal type moves that check into the model and produces a 422 with a precise error.
🛡️ Proposed change
-from pydantic import BaseModel
+from typing import Literal
+
+from pydantic import BaseModel, Field
@@
class CreatePairRequest(BaseModel):
- platform: str
- display_name: str = ""
+ platform: Literal["ios", "watchos", "android"]
+ display_name: str = Field(default="", max_length=64)If you adopt Literal, keep _VALID_PLATFORMS as the single source and derive both from it, or remove the now-redundant check at lines 107-111.
🤖 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/device_pair_requests.py` around lines 51 - 53, Update
CreatePairRequest to enforce a maximum length for display_name and constrain
platform using a Literal derived from the existing _VALID_PLATFORMS source. Then
remove the redundant platform validation in the route, or retain it only if the
model constraint remains the single authoritative definition.
| def _requester_ip(request: Request) -> str: | ||
| client = request.client | ||
| if client is None: | ||
| return "" | ||
| return client.host or "" |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Look for forwarded-header or proxy middleware handling.
set -euo pipefail
rg -nP -C 4 'X-Forwarded-For|x-forwarded-for|ProxyHeaders|forwarded_allow_ips|client\.host'Repository: jaylfc/taOS
Length of output: 149
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Tracked files under routes/configs:\n'
git ls-files | rg '(^|/)(routes|.*proxy|middleware|config|settings|docker|compose|nginx|caddy|traefik|haproxy|gunicorn|uvicorn|starlette|fastapi)' || true
printf '\nTarget file excerpt:\n'
sed -n '1,230p' tinyagentos/routes/device_pair_requests.py
printf '\nSearch request/client/address extraction patterns:\n'
rg -n -C 3 'request\.client|client\.host|X-Forwarded-For|x-forwarded-for|ProxyHeaders|forwarded_allow_ips|RemoteAddr|trust|headers\.get|miniconfig|config' . || trueRepository: jaylfc/taOS
Length of output: 50368
Avoid storing/proxying the raw client IP.
_requester_ip() still returns request.client.host, so this value can show the proxy/LB address instead of the peer address behind a reverse proxy. Check the deployment topology; if a trusted proxy is in front, read the original client IP from forwarded headers only for configured trusted proxy ranges.
🤖 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/device_pair_requests.py` around lines 81 - 85, Update
_requester_ip to avoid returning the raw request.client.host: when the immediate
client belongs to a configured trusted proxy range, extract the original client
IP from the appropriate forwarded header; otherwise use the direct peer address.
Preserve the empty result when no client is present and ensure forwarded headers
are ignored for untrusted clients.
| def _admin_user_id(request: Request) -> str: | ||
| """Resolve the instance user the pairing Decision is addressed to (the | ||
| primary admin), mirroring the OS-level decider in routes/decisions.py.""" | ||
| users = request.app.state.auth.list_users() | ||
| admins = [u for u in users if u.get("is_admin")] | ||
| if not admins: | ||
| return "" | ||
| return admins[0].get("id") or "" |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
create_pair_request returns 200 even when no admin Decision was raised. Two paths reach the same outcome: no admin exists, or the Decision store call fails. In both cases the route still returns a pair_request_id, so the device polls a request that no admin can approve and no error surfaces to either party.
tinyagentos/routes/device_pair_requests.py#L88-L95: log a warning when_admin_user_idfinds no admin, and make the caller fail with 503 instead of returning an unapprovable request id.tinyagentos/routes/device_pair_requests.py#L164-L165: after logging the Decision failure, raiseHTTPException(status_code=503, detail="pairing unavailable")rather than continuing to the success response.
📍 Affects 1 file
tinyagentos/routes/device_pair_requests.py#L88-L95(this comment)tinyagentos/routes/device_pair_requests.py#L164-L165
🤖 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/device_pair_requests.py` around lines 88 - 95, The pairing
route currently returns an unapprovable request when no admin exists or Decision
creation fails. In tinyagentos/routes/device_pair_requests.py lines 88-95,
update _admin_user_id to log a warning when no admin is found and make
create_pair_request return HTTP 503 instead of a request ID; at lines 164-165,
raise HTTPException(status_code=503, detail="pairing unavailable") immediately
after logging the Decision failure.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
_admin_user_id can return an empty string and silently disable approval.
If list_users() returns no admin, this helper returns "". The guard at line 144 then skips the Decision and the notification. The route still returns 200 with a pair_request_id, so the device polls a request that no admin will ever see.
Log a warning when no admin resolves, or reject the request with 503 so the caller learns that pairing is unavailable.
admins[0] also depends on the ordering of list_users(), which this file does not control. If ordering is not stable, the Decision can be addressed to a different admin between deployments. Select the admin by an explicit rule, for example the lowest created_at or a configured primary-admin id.
🤖 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/device_pair_requests.py` around lines 88 - 95, Update
_admin_user_id to select the primary admin deterministically using an explicit
rule such as the configured primary-admin ID or earliest created_at, and handle
the no-admin case explicitly. Ensure the pairing route does not return a
successful pair_request_id while silently skipping approval: log a warning
and/or reject the request with HTTP 503 before creating the request when no
admin resolves.
| store = _get_pair_requests_store(request) | ||
|
|
||
| # F4: cap TOTAL pending (not per-IP) -- mirrors the agent auth-request cap. | ||
| pending_count = await store.count_pending() | ||
| if pending_count >= _PENDING_CAP: | ||
| raise HTTPException( | ||
| status_code=429, | ||
| detail=( | ||
| f"too many pending pair requests ({pending_count} pending; " | ||
| f"resolve existing requests first)" | ||
| ), | ||
| ) | ||
|
|
||
| verify_code = _generate_verify_code() | ||
| requester_ip = _requester_ip(request) | ||
| display = (body.display_name or "").strip() or body.platform | ||
|
|
||
| record = await store.create( | ||
| platform=body.platform, | ||
| display_name=display, | ||
| verify_code=verify_code, | ||
| requester_ip=requester_ip, | ||
| ) | ||
| pair_request_id = record["id"] |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
The pending cap is a check-then-write race on an unauthenticated endpoint.
count_pending at line 116 and store.create at line 130 are separate statements with an await between them. Concurrent POSTs all observe a count below _PENDING_CAP and all insert. The endpoint requires no authentication, so an attacker controls the concurrency directly and can push the live pending count well above 5.
Each excess row raises a blocking Decision to the admin at lines 146-163, so the failure mode is admin-inbox flooding, which is exactly the condition F4 is meant to prevent.
Enforce the cap inside a single SQL statement in the store, for example an INSERT ... SELECT guarded by a count subquery, and return the created row or None. The route then maps None to 429. A per-process asyncio.Lock around the count-and-create pair is a smaller fix, but it does not hold across multiple workers.
🤖 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/device_pair_requests.py` around lines 113 - 136, Move the
pending-cap enforcement from the separate count/create flow in the route to an
atomic store operation: update the store method used by the pairing endpoint to
perform a guarded INSERT that only succeeds when pending rows are below
_PENDING_CAP, returning the created record or None. Update the route around
store.create to map a None result to the existing 429 response, while preserving
the current success response and request fields.
Code Review by Qodo
1. Router not in register_all_routers
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| from tinyagentos.device_pair_requests_store import ( | ||
| DevicePairRequestsStore, | ||
| _PENDING_CAP, | ||
| _live_status, | ||
| ) |
There was a problem hiding this comment.
1. devicepairrequestsstore imported in route 📜 Skill insight ⌂ Architecture
tinyagentos/routes/device_pair_requests.py directly imports the store module/class instead of accessing stores only via request.app.state, violating the route-store access rule. This increases coupling/circular-import risk and bypasses the intended lifespan-managed store wiring pattern.
Agent Prompt
## Issue description
Routes must not directly import store modules/classes; they must access stores via `request.app.state` only.
## Issue Context
`tinyagentos/routes/device_pair_requests.py` imports `DevicePairRequestsStore`, `_PENDING_CAP`, and `_live_status` from `tinyagentos/device_pair_requests_store.py`.
## Fix Focus Areas
- tinyagentos/routes/device_pair_requests.py[36-65]
- tinyagentos/device_pair_requests_store.py[29-93]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| router = APIRouter() | ||
| logger = logging.getLogger(__name__) |
There was a problem hiding this comment.
2. Router not in register_all_routers 📜 Skill insight ⌂ Architecture
The new device_pair_requests router that defines the /api/devices/pair-requests endpoints is not registered in tinyagentos/routes/__init__.py’s register_all_routers(), so FastAPI will never mount or serve these paths. This violates the requirement that new route modules be registered centrally (rather than wired inline elsewhere), making the pairing feature unavailable.
Agent Prompt
## Issue description
Ensure the new pairing router (`tinyagentos.routes.device_pair_requests.router`) is included in `register_all_routers()` so its `/api/devices/pair-requests` endpoints are actually mounted and reachable, and so it follows the central registration requirement (including standard CSRF dependency wiring).
## Issue Context
- PR Compliance ID 2185138 requires new routers to be registered in `tinyagentos/routes/__init__.py:register_all_routers()`.
- `tinyagentos/routes/device_pair_requests.py` defines `router = APIRouter()` with the pair-request endpoints, but `register_all_routers()` does not include it, so FastAPI never serves these paths.
- Routers are mounted centrally in `tinyagentos/routes/__init__.py` and should receive the standard CSRF dependency wiring (`dependencies=_csrf`) unless explicitly justified otherwise.
## Fix Focus Areas
- tinyagentos/routes/__init__.py[1-120]
- tinyagentos/routes/device_pair_requests.py[42-105]
- tinyagentos/routes/device_pair_requests.py[98-218]
## Proposed fix
1. In `tinyagentos/routes/__init__.py`, import the router:
`from tinyagentos.routes.device_pair_requests import router as device_pair_requests_router`.
2. Add it to `register_all_routers()` with the standard dependencies, e.g.:
`app.include_router(device_pair_requests_router, dependencies=_csrf)` (or explicitly justify using a different dependency set).
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| def _get_pair_requests_store(request: Request) -> DevicePairRequestsStore: | ||
| store = getattr(request.app.state, "device_pair_requests", None) | ||
| if store is None: | ||
| raise RuntimeError("device_pair_requests store not on app.state") | ||
| return store |
There was a problem hiding this comment.
3. Store not wired to app.state 📜 Skill insight ⌂ Architecture
The pair-request routes depend on request.app.state.device_pair_requests, but the application wiring in tinyagentos/app.py does not create or attach a DevicePairRequestsStore to that state field. As a result, calling these endpoints will raise a runtime error and return 500s, violating the requirement that new stores be attached via the lifespan/app wiring.
Agent Prompt
## Issue description
`_get_pair_requests_store()` hard-requires `request.app.state.device_pair_requests`, but the app startup wiring in `tinyagentos/app.py` never constructs, initializes (`init()`), or assigns a `DevicePairRequestsStore` to that field. Update the app wiring so the store is created, attached to `app.state`, initialized during `lifespan()`, and shut down/closed if your store pattern requires it.
## Issue Context
- PR Compliance ID 2185458 requires new stores to be attached to `app.state` in the lifespan/app wiring.
- The route helper `_get_pair_requests_store()` expects `request.app.state.device_pair_requests` and raises at runtime if it is absent, which would surface as 500s for requests.
- `tinyagentos/app.py` already wires many SQLite-backed stores by instantiating them in `create_app()`, assigning them onto `app.state` (e.g., `app.state.device_store = device_store`), and calling `await ...init()` during `lifespan()`, but there is no equivalent wiring for `device_pair_requests`.
## Fix Focus Areas
- tinyagentos/app.py[401-606]
- tinyagentos/app.py[500-700]
- tinyagentos/app.py[1589-1645]
- tinyagentos/app.py[1608-1630]
- tinyagentos/routes/device_pair_requests.py[60-65]
- tinyagentos/device_pair_requests_store.py[96-110]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| @router.post("/api/devices/pair-requests") | ||
| async def create_pair_request(request: Request, body: CreatePairRequest): | ||
| """Submit a pairing request from an external device/app. |
There was a problem hiding this comment.
6. Missing tests for new route 📜 Skill insight ⚙ Maintainability
A new route module tinyagentos/routes/device_pair_requests.py was added without corresponding tests that mirror the module structure and cover the new endpoints. This risks untested auth/expiry/token-claim behavior in a security-sensitive flow.
Agent Prompt
## Issue description
New route modules must have a corresponding test file that mirrors the module name.
## Issue Context
This PR adds `tinyagentos/routes/device_pair_requests.py`. Existing device route tests (`tests/routes/test_devices.py`) do not cover the new `/api/devices/pair-requests` endpoints.
## Fix Focus Areas
- tinyagentos/routes/device_pair_requests.py[98-218]
- tests/routes/test_devices.py[1-49]
- tests/test_device_pair_requests.py[1-200]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| """Routes for the device pairing consent loop (taOS S4e). | ||
|
|
||
| POST /api/devices/pair-requests -- unauthenticated; create a request + Decision | ||
| GET /api/devices/pair-requests/{id} -- unauthenticated; poll status / retrieve token | ||
|
|
There was a problem hiding this comment.
7. Docs not updated for routes 📜 Skill insight § Compliance
A new route module was added under tinyagentos/routes/ but docs/agent-coordination.md does not document the new /api/devices/pair-requests endpoints. This violates the doc-gate requirement for route-module changes.
Agent Prompt
## Issue description
When adding/removing route modules under `tinyagentos/routes/`, update `docs/agent-coordination.md` to reflect the new endpoints, or include the required `Docs-Reviewed:` commit trailer when no doc change is needed.
## Issue Context
This PR adds `/api/devices/pair-requests` endpoints but `docs/agent-coordination.md` contains device-token flow documentation that does not mention these endpoints.
## Fix Focus Areas
- tinyagentos/routes/device_pair_requests.py[3-28]
- docs/agent-coordination.md[238-253]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| metadata={ | ||
| "kind": "device_pairing", | ||
| "pair_request_id": pair_request_id, | ||
| }, |
There was a problem hiding this comment.
8. No device_pairing handler 🐞 Bug ≡ Correctness
create_pair_request creates a Decision with metadata kind="device_pairing", but the decisions answer path only runs side effects for execution_gate/delegation_gate/app_grant. As a result, approving/denying the Decision will not transition the pair request or mint a device, so polling cannot ever reach "accepted" with a token.
Agent Prompt
### Issue description
Decisions answers are not wired to the new device-pairing workflow: there is no handler that interprets `metadata.kind == "device_pairing"` to mint a device and update `device_pair_requests`.
### Issue Context
`create_pair_request()` writes `metadata={"kind":"device_pairing","pair_request_id":...}` and the module docstring claims an `_apply_device_pairing_grant` exists, but `answer_decision()` only invokes three handlers.
### Fix Focus Areas
- tinyagentos/routes/device_pair_requests.py[138-163]
- tinyagentos/routes/decisions.py[393-417]
- tinyagentos/device_pair_requests_store.py[140-178]
- tinyagentos/device_store.py[43-66]
### Proposed fix
1. Add a new handler in `routes/decisions.py` (e.g. `_apply_device_pairing_grant`) dispatched from `answer_decision()`.
2. Validate metadata contains `pair_request_id` and that the decision is approve/deny.
3. On approve: mint a device via `DeviceStore.register(user_id=decision.user_id, platform=..., ...)`, then atomically set pair request to accepted with `set_decision(..., status="accepted", device_id=..., decided_by=answered_by)`.
4. On deny: `set_decision(..., status="denied", decided_by=answered_by)`.
5. Enforce TTL at approve time (reject or mark expired) before minting/accepting.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| device_safe = {k: v for k, v in device.items() if k != "scoped_token"} | ||
| result["device"] = device_safe | ||
| # scoped_token is released exactly once (F3 / design "ONCE"). |
There was a problem hiding this comment.
9. Unauth device data leak 🐞 Bug ⛨ Security
The unauthenticated poll endpoint returns the entire device row except scoped_token, which includes user_id and push_token from DeviceStore.get(). Anyone who obtains the pair_request_id capability can learn these unnecessary internal/sensitive fields.
Agent Prompt
### Issue description
`get_pair_request()` returns `device_safe` built by removing only `scoped_token`, but the device row includes fields that should not be exposed on an unauthenticated capability endpoint (notably `user_id` and `push_token`).
### Issue Context
`DeviceStore.get()` returns `_FULL_COLS` including `user_id` and `push_token`.
### Fix Focus Areas
- tinyagentos/routes/device_pair_requests.py[203-217]
- tinyagentos/device_store.py[10-19]
### Proposed fix
Replace the current `device_safe = {k: v for k, v in device.items() if k != "scoped_token"}` with an explicit allowlist appropriate for pairing, e.g. `{ "device_id": ..., "platform": ..., "display_name": ..., "registered_at": ..., "last_seen": ..., "revoked": ... }`, and exclude `user_id` and `push_token` (and any other nonessential fields).
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| class PairRequestIn(BaseModel): | ||
| verify_code: str | None = None | ||
|
|
There was a problem hiding this comment.
10. Dead code in routes 🐞 Bug ⚙ Maintainability
PairRequestIn plus constants _VERIFY_CODE_DIGITS and _MAX_DEVICES_PER_USER are defined but unused in tinyagentos/routes/device_pair_requests.py. This adds misleading surface area and increases maintenance risk as the constants drift from real behavior.
Agent Prompt
### Issue description
Unused symbols in the new routes module: `PairRequestIn`, `_VERIFY_CODE_DIGITS`, `_MAX_DEVICES_PER_USER`.
### Issue Context
The verify-code generator hardcodes 6 digits and device caps are enforced elsewhere.
### Fix Focus Areas
- tinyagentos/routes/device_pair_requests.py[45-58]
- tinyagentos/routes/device_pair_requests.py[74-79]
### Proposed fix
Either delete the unused model/constants, or use them consistently (e.g., use `_VERIFY_CODE_DIGITS` in `_generate_verify_code()` and only keep `_MAX_DEVICES_PER_USER` if it is enforced in the pairing approval handler).
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
|
nemotron-ultra-kilo review VERDICT: Code is largely correct with solid atomic transitions and security-by-design (verify_code never server-checked, one-time token claim, expiry enforced at approve time). Minor issues below.
Automated first-pass review by the nemotron-ultra-kilo lane. The lead still reviews before merge. |
|
nemotron-ultra-orB review Now I'll provide the code review of the git diff.
Automated first-pass review by the nemotron-ultra-orB lane. The lead still reviews before merge. |
|
Mechanical note on the doc-gate red: the gate gained on_modify rules today (#2236), and this PR modifies files under tinyagentos/routes/, so it now needs either a CHANGELOG.md line / agent-doc update in the changeset or a 'Docs-Reviewed: ' commit trailer. The red is the new gate working as intended, not a flake. Kilo's red IS the known rate-limit noise. The 14 bot findings still need adjudication before this merges. |
|
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-mtds32.
Files:
docs/design/postgres-bounded-adoption.md | 143 ----------------
tinyagentos/device_pair_requests_store.py | 253 +++++++++++++++++++++++++++++
tinyagentos/routes/device_pair_requests.py | 218 +++++++++++++++++++++++++
3 files changed, 471 insertions(+), 143 deletions(-)
Summary by Gitar
tinyagentos/routes/device_pair_requests.pyDevicePairRequestsStorefor persistent tracking and state management intinyagentos/device_pair_requests_store.pyThis will update automatically on new commits.
Summary by CodeRabbit