Repository navigation
Conversation
mabry1985
enabled auto-merge (squash)
October 13, 2026 20:13
mabry1985
disabled auto-merge
October 13, 2026 20:13
Member
Author
|
@vera review |
mabry1985
enabled auto-merge (squash)
October 13, 2026 20:36
mabry1985
disabled auto-merge
October 13, 2026 20:43
mabry1985
enabled auto-merge (squash)
October 13, 2026 20:44
|
@mabry1985 — a review round for this PR is already running, so this summon did not start a second panel on top of it. Its verdict posts here when it finishes; if you still dispute that verdict, summon again then. |
![protoreview[bot]](https://avatars.githubusercontent.com/in/4233969?s=60&v=4){kind=small}
There was a problem hiding this comment.
QA panel review — FAIL
code-review-structural · head c517e67a5e12 · formal
⚠️ Coverage incomplete — this is not a clean pass. 1 of 5 review lane(s) did not complete a full pass this round:find_structural(structural pass cut short: 16 of 17 features reviewed — feature cap reached: the 16 features with the most changed lines were reviewed, 1 were not (structuralmaxfeatures=16)). Findings from the lanes that ran stand, but a defect only the missing lanes would catch may be missed, so a PASS is capped at WARN. Where the brief below implies full coverage, this line supersedes it. The next push re-runs the full panel.
All 18 findings confirmed by the verifier against code at head; none refuted, none uncertain. The SSRF on the delegate test endpoint (delegates.py:238) is the only major and the clear fix-first item — an authenticated user can point the probe at internal addresses. The verifier noted finding #5 (line 129) is slightly overstated: create_delegate_endpoint is actually protected by its try block, so the defect is confined to update_delegate_endpoint. The cluster of missing exception handlers (lines 35, 97, 129, 177, 249, 25, 50) is one class of gap addressable in a single pass. No panel disagreement; no coverage gaps beyond what the prior brief already flagged (web/src/lib/api.ts under-read relative to backend).
Findings
| Severity | Location | Finding | Verified | |
|---|---|---|---|---|
| 🟠 | major | server/routers/delegates.py:238 |
SSRF via user-controlled URL in the delegate test probe: an authenticated user can specify an internal/loopback URL and the endpoint will make an outbound requ… | confirmed |
| 🟡 | minor | server/routers/delegates.py:35 |
reload_delegates_endpoint calls app._DELEGATES.reload() without any exception handling; a malformed YAML file produces an unstructured 500 instead of a structu… | confirmed · nearby, not gating |
| 🟡 | minor | server/routers/delegates.py:97 |
GET /api/delegates only catches DelegateValidationError; other exceptions from read_delegates() (FileNotFoundError, yaml.YAMLError, OSError) produce unstructur… | confirmed · nearby, not gating |
| 🟡 | minor | server/routers/delegates.py:128 |
TOCTOU race on delegate name uniqueness: the read-then-write pattern (check existence via read_delegates, then upsert) allows two concurrent POST requests with… | confirmed · nearby, not gating |
| 🟡 | minor | server/routers/delegates.py:129 |
Both create_delegate_endpoint and update_delegate_endpoint call read_delegates() without wrapping it in a try/except for DelegateValidationError, so a malforme… | confirmed · nearby, not gating |
| 🟡 | minor | server/routers/delegates.py:177 |
The delete endpoint only catches KeyError for the not-found case; a DelegateValidationError from a corrupted YAML entry produces an unstructured 500, inconsist… | confirmed · nearby, not gating |
| 🟡 | minor | server/routers/delegates.py:207 |
No tests exist for the /api/delegates/discover endpoint, which has non-trivial logic (env var parsing, private-function call to discovery._local_ip(), URL pars… | |
| 🟡 | minor | server/routers/delegates.py:231 |
Already-configured delegates with implicit default ports (no explicit port in URL) are not excluded from discovery results because parsed.port is None, so the … | confirmed |
| 🟡 | minor | server/routers/delegates.py:252 |
The final await probe(delegate) call in _probe_delegate is unguarded; any exception that escapes probe() (e.g., an unexpected non-httpx error) produces an un… |
confirmed |
| 🟡 | minor | agent/delegate_adapters.py:282 |
The A2AClient cache (_clients dict keyed by (url, auth_headers)) is unbounded: when a delegate's URL or credentials change, a new client is created for the new… | confirmed |
| 🟡 | minor | agent/delegate_adapters.py:563 |
The probe's success condition was narrowed from 'any HTTP 200 from the agent card' to 'JSON-RPC body echoes the request id AND error.code == -32001', so a reac… | confirmed |
| 🟡 | minor | agent/delegate_config_store.py:103 |
Standard-library module re is imported inside the redact_entry function body instead of at the top of the file, violating the repo's import-at-top conventi… |
confirmed |
| 🟡 | minor | web/src/lib/api.ts:404 |
No timeout on API requests: if the Python sidecar becomes unresponsive mid-request, every subsequent UI action that touches the API will hang indefinitely. | confirmed · nearby, not gating |
| ⚪ | nit | server/routers/delegates.py:25 |
Unstructured 500 on user_registry.reload() failure breaks API contract consistency: if the reload raises (network timeout, malformed file, permission error), F… | confirmed · nearby, not gating |
| ⚪ | nit | server/routers/delegates.py:50 |
Hot-refresh partial failure is reported as full success: _hot_refresh_delegate_sessions catches all exceptions from individual session refresh calls and logs t… | confirmed |
| ⚪ | nit | agent/delegate_adapters.py:494 |
No test coverage for the streaming-to-sync fallback in A2A dispatch: the non-trivial fallback path (catch A2ADispatchError/TimeoutError/HTTPError on streaming,… | |
| ⚪ | nit | tests/test_delegate_health.py:15 |
Standard-library import json is placed after third-party import httpx, violating PEP 8 / isort import ordering (stdlib before third-party). |
confirmed |
| ⚪ | nit | tests/test_delegates_api.py:17 |
Standard-library import json is placed after third-party import httpx, violating PEP 8 / isort import ordering (stdlib before third-party). |
confirmed |
findings JSON (machine-readable)
[
{
"file": "server/routers/delegates.py",
"line": 238,
"severity": "major",
"category": "security",
"claim": "SSRF via user-controlled URL in the delegate test probe: an authenticated user can specify an internal/loopback URL and the endpoint will make an outbound request to it, potentially leaking internal service responses.",
"evidence": "The POST /api/delegates/test endpoint accepts an arbitrary user-supplied dict as the probe target. _probe_delegate calls resolve_saved_credentials(entry) which can inject stored auth credentials (API keys, tokens) into the entry based on its name field, then probe(delegate) makes an outbound HTTP request to the URL in the entry.",
"source": "protopatch",
"verdict": "confirmed",
"note": "Read delegates.py at head: test_delegate_endpoint (line 238) passes user dict to _probe_delegate \u2192 validate_entry \u2192 _parse_entry \u2192 probe(delegate). probe() in delegate_adapters.py:549 does `await client.post(delegate.url, ...)` with no SSRF guard (no loopback/private-IP check). Authenticated user can point probe at 127.0.0.1 or internal service URLs."
},
{
"file": "server/routers/delegates.py",
"line": 35,
"severity": "minor",
"category": "correctness",
"claim": "reload_delegates_endpoint calls app._DELEGATES.reload() without any exception handling; a malformed YAML file produces an unstructured 500 instead of a structured error response.",
"evidence": "reload_delegates_endpoint calls app._DELEGATES.reload() without any exception handling. If the YAML file is malformed (e.g., a user saves a partially-edited file, or a concurrent write leaves it in an inconsistent state), the reload will raise a parse error. The sibling endpoint list_delegates_endpoint (lines 97-104) explicitly catches DelegateValidationError and returns a structured JSONResponse.",
"source": "protopatch",
"verdict": "confirmed",
"note": "Read reload_delegates_endpoint at head (line 33-38): `names = app._DELEGATES.reload()` with no try/except. Sibling list_delegates_endpoint (line 97) does `except DelegateValidationError as e: return JSONResponse(status_code=500, content={\"error\": str(e)})`. Asymmetric error handling confirmed. \u2014 nearby: in code this PR did not change (outside its changed lines and the functions they sit in) \u2014 reported, not gated (#232)",
"nearby": true
},
{
"file": "server/routers/delegates.py",
"line": 97,
"severity": "minor",
"category": "correctness",
"claim": "GET /api/delegates only catches DelegateValidationError; other exceptions from read_delegates() (FileNotFoundError, yaml.YAMLError, OSError) produce unstructured 500s that break the expected error-response contract.",
"evidence": "read_delegates() performs a file read and YAML parse. Besides DelegateValidationError it can raise FileNotFoundError, PermissionError, yaml.YAMLError, OSError, etc. None of these are caught, so they propagate as an unhandled exception. FastAPI will return a generic 500 with no JSON body, breaking the expected {\"error\": \"...\"} contract that the Settings UI relies on for display.",
"source": "protopatch",
"verdict": "confirmed",
"note": "Read list_delegates_endpoint at head: only `except DelegateValidationError` is caught. read_delegates() in config_store.py catches yaml.YAMLError and re-raises as DelegateValidationError, but PermissionError/OSError from p.read_text() would escape unhandled. FileNotFoundError is mitigated by the p.exists() check but PermissionError is not. \u2014 nearby: in code this PR did not change (outside its changed lines and the functions they sit in) \u2014 reported, not gated (#232)",
"nearby": true
},
{
"file": "server/routers/delegates.py",
"line": 128,
"severity": "minor",
"category": "correctness",
"claim": "TOCTOU race on delegate name uniqueness: the read-then-write pattern (check existence via read_delegates, then upsert) allows two concurrent POST requests with the same name to both pass the existence check, silently overwriting one delegate.",
"evidence": "The endpoint performs a read-then-write pattern: it calls read_delegates() to check whether the requested name already exists (line 129), then calls upsert_delegate(body) to persist it (line 134). In a multi-worker deployment (e.g., uvicorn with --workers N, which is common for production FastAPI apps), two concurrent POST /api/delegates requests with the same name can both pass the existence check.",
"source": "protopatch",
"verdict": "confirmed",
"note": "Read create_delegate_endpoint at head: `if isinstance(name, str) and any(e.get(\"name\") == name for e in read_delegates()): return 409` then `entries = upsert_delegate(body)`. No lock, no atomic check-and-set. In multi-worker uvicorn, two concurrent POSTs with same name can both pass the existence check. Valid TOCTOU. \u2014 nearby: in code this PR did not change (outside its changed lines and the functions they sit in) \u2014 reported, not gated (#232)",
"nearby": true
},
{
"file": "server/routers/delegates.py",
"line": 129,
"severity": "minor",
"category": "correctness",
"claim": "Both create_delegate_endpoint and update_delegate_endpoint call read_delegates() without wrapping it in a try/except for DelegateValidationError, so a malformed YAML file causes an unstructured 500.",
"evidence": "Both create_delegate_endpoint (line 129) and update_delegate_endpoint (line 164) call read_delegates() without wrapping it in a try/except for DelegateValidationError. If the delegates YAML file is malformed (e.g., a user manually edits it and introduces a syntax or schema error), these endpoints raise an unhandled exception, causing FastAPI to return an opaque 500 instead of a structured error response.",
"source": "protopatch",
"verdict": "confirmed",
"note": "Partially overstated: create_delegate_endpoint DOES have read_delegates() inside its try block (line 129: `any(e.get(\"name\") == name for e in read_delegates())` is within the try). However, update_delegate_endpoint calls read_delegates() OUTSIDE its try block (line 164: `if not any(e.get(\"name\") == name for e in read_delegates()):` precedes the try). The defect is real for update; create is protected. \u2014 nearby: in code this PR did not change (outside its changed lines and the functions they sit in) \u2014 reported, not gated (#232)",
"nearby": true
},
{
"file": "server/routers/delegates.py",
"line": 177,
"severity": "minor",
"category": "correctness",
"claim": "The delete endpoint only catches KeyError for the not-found case; a DelegateValidationError from a corrupted YAML entry produces an unstructured 500, inconsistent with sibling create/update endpoints.",
"evidence": "The create and update endpoints both catch DelegateValidationError from the config store and return a 400 with the error message. The delete endpoint only catches KeyError for the not-found case. If delete_delegate encounters a malformed or corrupted YAML entry (e.g., the file was edited externally between list and delete, or a concurrent write left it in an invalid state), a DelegateValidationError is raised unhandled.",
"source": "protopatch",
"verdict": "confirmed",
"note": "Read delete_delegate_endpoint at head: `try: entries = delete_delegate(name) except KeyError: ...`. delete_delegate() in config_store.py calls read_delegates() which raises DelegateValidationError on YAML parse errors. Only KeyError is caught; DelegateValidationError would propagate as unstructured 500. \u2014 nearby: in code this PR did not change (outside its changed lines and the functions they sit in) \u2014 reported, not gated (#232)",
"nearby": true
},
{
"file": "server/routers/delegates.py",
"line": 207,
"severity": "minor",
"category": "tests",
"claim": "No tests exist for the /api/delegates/discover endpoint, which has non-trivial logic (env var parsing, private-function call to discovery._local_ip(), URL parsing with exclusion set) that is entirely untested.",
"evidence": "The feature metadata lists tests: []. The endpoint has non-trivial logic (env var parsing, private-function call to discovery._local_ip(), URL parsing with exclusion set) that is entirely untested. The default-port exclusion bug above would have been caught by even a basic test.",
"source": "protopatch",
"verdict": "uncertain",
"note": "Searched both PR test files (tests/test_delegates_api.py 420 lines, tests/test_delegate_health.py 555 lines) for 'discover': no test references the /api/delegates/discover endpoint. The test_delegates_api.py docstring lists coverage as 'GET list, POST create, PUT update, DELETE, POST /test' \u2014 discover is absent. \u2014 absence refuted by a repo-wide search at the reviewed head: referenced at `tests/test_delegates_api.py:1` \u2014 downgraded to uncertain, cannot gate a merge (issue #259)",
"ungrounded": true,
"absence_refuted": "tests/test_delegates_api.py:1"
},
{
"file": "server/routers/delegates.py",
"line": 231,
"severity": "minor",
"category": "correctness",
"claim": "Already-configured delegates with implicit default ports (no explicit port in URL) are not excluded from discovery results because parsed.port is None, so the guard skips adding them to the known set.",
"evidence": "When a configured a2a delegate has a URL without an explicit port (e.g. \"http://myagent.local\"), `parsed.port` is `None`, so the `if parsed.hostname and parsed.port` guard on line 231 skips adding it to `known`. The discovery scan will find that agent on its actual listening port (80 or 443) and return it as a 'discovered' candidate even though it is already configured.",
"source": "protopatch",
"verdict": "confirmed",
"note": "Read discover_delegates_endpoint at head: `if parsed.hostname and parsed.port:` \u2014 for URL 'http://myagent.local', urlparse gives port=None, so the guard is False and the entry is never added to `known`. Confirmed by tracing the exact code path.",
"line_original": 229,
"line_corrected": true
},
{
"file": "server/routers/delegates.py",
"line": 252,
"severity": "minor",
"category": "correctness",
"claim": "The final `await probe(delegate)` call in _probe_delegate is unguarded; any exception that escapes probe() (e.g., an unexpected non-httpx error) produces an unstructured 500 instead of the structured {ok: False, error: ...} response the UI expects.",
"evidence": "The _probe_delegate function handles DelegateValidationError and a None return from _parse_entry with structured {ok: False, error: ...} responses, but the final call `await probe(delegate)` is unguarded. Network-level failures (connection refused, DNS failure, TLS error, timeout) that escape probe() will propagate as unhandled exceptions, producing a bare 500 with no structured error body.",
"source": "protopatch",
"verdict": "confirmed",
"note": "Read _probe_delegate at head: `return await probe(delegate)` with no try/except. probe() catches httpx.HTTPError (covers connect/read timeouts, TLS), but non-httpx exceptions (e.g., from delegate.auth_headers() or a bug in the JSON-RPC body construction) would escape. The finding's specific examples (connection refused, DNS, TLS) are actually caught by httpx.HTTPError, but the general claim about unguarded call is correct.",
"line_original": 249,
"line_corrected": true
},
{
"file": "agent/delegate_adapters.py",
"line": 282,
"severity": "minor",
"category": "correctness",
"claim": "The A2AClient cache (_clients dict keyed by (url, auth_headers)) is unbounded: when a delegate's URL or credentials change, a new client is created for the new key while the old client (wrapping an httpx.AsyncClient with a connection pool) remains in the dict indefinitely, leaking connection pools in a long-running server.",
"evidence": "The A2AAdapter singleton holds a _clients dict keyed by (url, auth_headers). When a delegate's URL or credentials change (an explicitly supported scenario \u2014 config_changed returns True on those fields), a new client is created for the new key while the old client remains in the dict indefinitely. Each A2AClient wraps an httpx.AsyncClient with a connection pool.",
"source": "protopatch",
"verdict": "confirmed",
"note": "Read A2AAdapter.__init__ and client_for at head: `self._clients: dict[tuple, A2AClient] = {}` and `client_for` does `self._clients[key] = client` with no eviction. config_changed() returns True on url/auth_scheme/a2a_credential/a2a_headers changes, confirming the scenario. No LRU, no TTL, no cleanup on reload. Unbounded growth confirmed."
},
{
"file": "agent/delegate_adapters.py",
"line": 563,
"severity": "minor",
"category": "removed-behavior",
"claim": "The probe's success condition was narrowed from 'any HTTP 200 from the agent card' to 'JSON-RPC body echoes the request id AND error.code == -32001', so a reachable open (no-auth) A2A agent whose tasks/get returns a 200 with a result or a different error code now reports 'not connected' even though it was previously reported healthy.",
"evidence": "Old: `return {\"ok\": True, \"latency_ms\": latency_ms}` after any 200 from `card_url`. New: `if isinstance(body, dict) and body.get(\"jsonrpc\") == \"2.0\" and body.get(\"id\") == request_id:` ... `if isinstance(error, dict) and error.get(\"code\") == -32001: return {\"ok\": True, ...}` else `return {\"ok\": False, \"error\": \"The endpoint did not confirm an authenticated A2A connection...\"}`.",
"verdict": "confirmed",
"note": "Diff confirms: old probe returned {\"ok\": True} on any HTTP 200 from agent-card.json. New probe requires body.jsonrpc=='2.0' AND body.id==request_id AND body.error.code==-32001. An open agent returning a 200 with a result (not an error) or a different error code will now report 'not connected'. This is a deliberate design change (the comment says 'A public agent card proves discovery, not authentication') but the behavioral narrowing is factual.",
"line_original": 561,
"line_corrected": true
},
{
"file": "agent/delegate_config_store.py",
"line": 103,
"severity": "minor",
"category": "conventions",
"claim": "Standard-library module `re` is imported inside the `redact_entry` function body instead of at the top of the file, violating the repo's import-at-top convention.",
"evidence": "# Legacy invalid references may themselves be pasted secrets.\n import re\n ref = auth.get(\"credentialsEnv\")",
"source": "protopatch",
"verdict": "confirmed",
"note": "Read delegate_config_store.py at head: top-level imports are `copy, logging, os, tempfile, pathlib, yaml` \u2014 no `re`. The `import re` appears inside redact_entry() at line 102. Note: delegate_adapters.py (same PR) does import re at top level, making the inconsistency more visible.",
"line_original": 102,
"line_corrected": true
},
{
"file": "web/src/lib/api.ts",
"line": 404,
"severity": "minor",
"category": "correctness",
"claim": "No timeout on API requests: if the Python sidecar becomes unresponsive mid-request, every subsequent UI action that touches the API will hang indefinitely.",
"evidence": "The `request` function awaits `backendBase()` (which correctly gates until the sidecar URL is known), but once the base URL is established there is no timeout on the `invoke('api_request', \u2026)` call. If the Python sidecar becomes unresponsive mid-request (e.g. a blocking LLM call, a deadlocked thread, or a network partition to an external API), every subsequent UI action that touches the API will hang.",
"source": "protopatch",
"verdict": "confirmed",
"note": "Read api.ts at head lines 404-420: `res = await invoke<{ status: number; body: string }>('api_request', { method, path, body, headers })` \u2014 no timeout parameter, no AbortController, no Promise.race with a timer. The invoke call will hang indefinitely if the Rust IPC proxy or Python sidecar is unresponsive. \u2014 nearby: in code this PR did not change (outside its changed lines and the functions they sit in) \u2014 reported, not gated (#232)",
"nearby": true
},
{
"file": "server/routers/delegates.py",
"line": 25,
"severity": "nit",
"category": "correctness",
"claim": "Unstructured 500 on user_registry.reload() failure breaks API contract consistency: if the reload raises (network timeout, malformed file, permission error), FastAPI returns an unstructured 500 instead of the structured error response used by sibling endpoints.",
"evidence": "The endpoint calls user_registry.reload() which (per its docstring) re-fetches from Infisical or a YAML file. If that raises (network timeout, malformed file, permission error), FastAPI returns an unstructured 500 ({\"detail\":\"Internal Server Error\"}). Every other fallible endpoint in this file wraps its core operation in try/except and returns a structured JSONResponse with a machine-readable \"error\" field.",
"source": "protopatch",
"verdict": "confirmed",
"note": "Read reload_users_endpoint at head (line 24-28): `names = user_registry.reload(); return {\"ok\": True, \"users\": names, ...}` with no try/except. The docstring says it 're-fetches from Infisical (if configured) or the YAML file' \u2014 network I/O that can raise. No structured error handling. \u2014 nearby: in code this PR did not change (outside its changed lines and the functions they sit in) \u2014 reported, not gated (#232)",
"nearby": true
},
{
"file": "server/routers/delegates.py",
"line": 50,
"severity": "nit",
"category": "correctness",
"claim": "Hot-refresh partial failure is reported as full success: _hot_refresh_delegate_sessions catches all exceptions from individual session refresh calls and logs them as warnings, but the caller unconditionally returns {\"ok\": True}.",
"evidence": "_hot_refresh_delegate_sessions catches all exceptions from individual session refresh calls and logs them as warnings, but the caller (reload_delegates_endpoint) unconditionally returns {\"ok\": True}. If one or more sessions fail to refresh (e.g., a session is mid-teardown and its refresh_delegates raises AttributeError), the API still reports full success.",
"source": "protopatch",
"verdict": "confirmed",
"note": "Read _hot_refresh_delegate_sessions at head: `try: fn() except Exception as e: logger.warning(...)`. Caller reload_delegates_endpoint: `names = app._DELEGATES.reload(); _hot_refresh_delegate_sessions(); return {\"ok\": True, \"delegates\": names}`. No error propagation from the hot-refresh to the response."
},
{
"file": "agent/delegate_adapters.py",
"line": 494,
"severity": "nit",
"category": "tests",
"claim": "No test coverage for the streaming-to-sync fallback in A2A dispatch: the non-trivial fallback path (catch A2ADispatchError/TimeoutError/HTTPError on streaming, fall back to synchronous message/send) is untested.",
"evidence": "The _dispatch_inner method contains a non-trivial streaming-to-sync fallback: it attempts a streaming send (gated on env var A2A_STREAM or a progress_callback + card support), catches (A2ADispatchError, asyncio.TimeoutError, httpx.HTTPError), and falls back to a synchronous message/send. This is the reliability-critical path for long-running A2A tasks. The included test file exercises only the regression path.",
"source": "protopatch",
"verdict": "uncertain",
"note": "Searched both PR test files (test_delegate_health.py, test_delegates_api.py) for streaming/dispatch/fallback: no test exercises _dispatch_inner with A2A_STREAM=1 or progress_callback. The tests cover probe() and CRUD endpoints only. The streaming-to-sync fallback path is untested in this PR. \u2014 absence refuted by a repo-wide search at the reviewed head: referenced at `tests/test_acp_adapter.py:15` \u2014 downgraded to uncertain, cannot gate a merge (issue #259)",
"ungrounded": true,
"absence_refuted": "tests/test_acp_adapter.py:15"
},
{
"file": "tests/test_delegate_health.py",
"line": 15,
"severity": "nit",
"category": "conventions",
"claim": "Standard-library `import json` is placed after third-party `import httpx`, violating PEP 8 / isort import ordering (stdlib before third-party).",
"evidence": "import httpx\n+import json\nimport pytest",
"verdict": "confirmed",
"note": "Read test_delegate_health.py at head lines 14-16: `import httpx` / `import json` / `import pytest`. json is stdlib, httpx is third-party. PEP 8/isort requires stdlib before third-party. The diff shows `+import json` was added after the existing `import httpx` line."
},
{
"file": "tests/test_delegates_api.py",
"line": 17,
"severity": "nit",
"category": "conventions",
"claim": "Standard-library `import json` is placed after third-party `import httpx`, violating PEP 8 / isort import ordering (stdlib before third-party).",
"evidence": "import httpx\n+import json\nimport pytest",
"verdict": "confirmed",
"note": "Read test_delegates_api.py at head lines 16-18: `import httpx` / `import json` / `import pytest`. Same ordering violation as test_delegate_health.py. json (stdlib) after httpx (third-party)."
}
]2 absence finding(s) (“no test” / “dead” / “never called” / “does not exist”) downgraded to uncertain: an absence is a claim about the whole repo, so it is checked by a search of the repo at this head, and a reference found there refutes it (issue #259). Each still stands for a human to judge.
server/routers/delegates.py(minor) — referenced attests/test_delegates_api.py:1agent/delegate_adapters.py(nit) — referenced attests/test_acp_adapter.py:15
7 structural finding(s) are nearby notes, not part of the verdict: they sit in code this PR did not change (outside its changed lines and the functions those lines are in). Worth a look; not a request for this PR.
server/routers/delegates.py:35(minor) — reload_delegates_endpoint calls app._DELEGATES.reload() without any exception handling; a malformed YAML file produces an unstructured 500 instead of a structurserver/routers/delegates.py:97(minor) — GET /api/delegates only catches DelegateValidationError; other exceptions from read_delegates() (FileNotFoundError, yaml.YAMLError, OSError) produce unstructureserver/routers/delegates.py:128(minor) — TOCTOU race on delegate name uniqueness: the read-then-write pattern (check existence via read_delegates, then upsert) allows two concurrent POST requests withserver/routers/delegates.py:129(minor) — Both create_delegate_endpoint and update_delegate_endpoint call read_delegates() without wrapping it in a try/except for DelegateValidationError, so a malformedserver/routers/delegates.py:177(minor) — The delete endpoint only catches KeyError for the not-found case; a DelegateValidationError from a corrupted YAML entry produces an unstructured 500, inconsisteweb/src/lib/api.ts:404(minor) — No timeout on API requests: if the Python sidecar becomes unresponsive mid-request, every subsequent UI action that touches the API will hang indefinitely.server/routers/delegates.py:25(nit) — Unstructured 500 on user_registry.reload() failure breaks API contract consistency: if the reload raises (network timeout, malformed file, permission error), Fa
Member
Author
|
Review follow-up at d925e14: Fixed default-port discovery exclusions, structured unexpected probe failures without exposing exception text, malformed settings errors on update/delete, non-mapping YAML handling, read-permission errors, and import ordering. Added regression tests for these paths. The full Python suite, focused API/health tests, ruff, and diff checks pass. The localhost/private-address finding needs the desktop trust model considered: this is ORBIS's single-owner settings API ( The stricter health probe is intentional: a public agent card previously reported green while the protected RPC returned 401. The read-only The existing client-cache lifecycle and session-refresh reporting notes remain follow-up work; they have not been represented as fixed here. Please re-review the latest head, including the previously incomplete lane. |
There was a problem hiding this comment.
QA panel review — WARN
code-review-structural · head d925e14e19ab · formal
⚠️ Coverage incomplete — this is not a clean pass. 1 of 5 review lane(s) did not complete a full pass this round:find_structural(structural pass cut short: 16 of 17 features reviewed — feature cap reached: the 16 features with the most changed lines were reviewed, 1 were not (structuralmaxfeatures=16)). Findings from the lanes that ran stand, but a defect only the missing lanes would catch may be missed, so a PASS is capped at WARN. Where the brief below implies full coverage, this line supersedes it. The next push re-runs the full panel.
Overall risk is low-to-moderate. The one major is the SSRF vector in the delegate test probe (carried from round 1, still open — no URL validation against internal/loopback targets was added). Verification refuted 5 of 13 findings: the "no tests" claims for PUT/POST/DELETE endpoints were wrong (tests exist in test_delegates_api.py), the "no timeout on probe" was wrong (httpx enforces an 8s default), and the "no test for credential carryover" was wrong (test_access_token_lifecycle_is_private_and_live covers it). The 8 surviving findings are all confirmed. No panel disagreement. No verification gap — all 13 were annotated.
Prior requests
| Prior finding | Disposition | Why | |
|---|---|---|---|
| 🔴 | server/routers/delegates.py:238 |
open | No URL validation against internal/loopback addresses was added to the /test endpoint; the verifier's refutation of the 'no timeout' finding addresses timing, … |
Findings
| Severity | Location | Finding | Verified | |
|---|---|---|---|---|
| 🟠 | major | server/routers/delegates.py:238 |
SSRF via user-controlled URL in the delegate test probe: an authenticated user can specify an internal/loopback URL and the endpoint will make an outbound requ… | confirmed |
| 🟡 | minor | agent/delegate_adapters.py:282 |
The A2AClient cache (_clients dict keyed by (url, auth_headers)) is unbounded: when a delegate's URL or credentials change, a new client is created for the new… | confirmed |
| 🟡 | minor | web/src/lib/api.ts:448 |
Unhandled SyntaxError when a 2xx response body is empty or non-JSON: JSON.parse(res.body) throws a raw SyntaxError that bypasses the module's ApiError/Unauthor… | confirmed · nearby, not gating |
| 🟡 | minor | server/routers/delegates.py:234 |
Hostname/IP mismatch in configured-agent exclusion may cause duplicates in discovery results: the known set uses parsed hostnames while discovery resolves to I… | |
| ⚪ | nit | server/routers/delegates.py:35 |
Unstructured 500 on reload failure: app._DELEGATES.reload() is not wrapped in try/except, inconsistent with every other CRUD endpoint in the same file that ret… | confirmed · nearby, not gating |
| ⚪ | nit | server/routers/delegates.py:165 |
TOCTOU race on PUT: existence check can be stale, causing PUT to create instead of update if a concurrent DELETE removes the delegate between the check and the… | confirmed |
| ⚪ | nit | server/routers/delegates.py:193 |
No error handling around delegate_type_specs() call, inconsistent with sibling endpoints that wrap their core operations in try/except and return structured JS… | confirmed |
| ⚪ | nit | server/routers/delegates.py:129 |
TOCTOU race on create: existence check and upsert are not atomic; two concurrent requests with the same delegate name can both pass the check before either wri… | confirmed · nearby, not gating |
| ⚪ | nit | server/routers/delegates.py:25 |
reload_users_endpoint lacks structured error handling inconsistent with sibling routes: if user_registry.reload() raises, the caller gets FastAPI's default uns… | confirmed · nearby, not gating |
findings JSON (machine-readable)
[
{
"file": "agent/delegate_adapters.py",
"line": 282,
"severity": "minor",
"category": "correctness",
"claim": "The A2AClient cache (_clients dict keyed by (url, auth_headers)) is unbounded: when a delegate's URL or credentials change, a new client is created for the new key while the old client (wrapping an httpx.AsyncClient with a connection pool) remains in the dict indefinitely, leaking connection pools in a long-running server.",
"evidence": "self._clients: dict[tuple, A2AClient] = {} \u2014 client_for does self._clients[key] = client with no eviction. config_changed() returns True on url/auth_scheme/a2a_credential/a2a_headers changes, confirming the scenario.",
"verdict": "confirmed",
"note": "Read agent/delegate_adapters.py:282-287 and 378-388: `self._clients: dict[tuple, A2AClient] = {}` in __init__, `client_for` does `self._clients[key] = client` with no eviction path. `config_changed` (line 372-377) confirms url/credential changes produce new keys. No removal/eviction code found in the 941-line file."
},
{
"file": "web/src/lib/api.ts",
"line": 448,
"severity": "minor",
"category": "correctness",
"claim": "Unhandled SyntaxError when a 2xx response body is empty or non-JSON: JSON.parse(res.body) throws a raw SyntaxError that bypasses the module's ApiError/UnauthorizedError error contract.",
"evidence": "The `request` function is the single funnel for all API calls. At line 448 it executes `JSON.parse(res.body)` after confirming the status is 2xx. If the backend returns a 200/204 with an empty body (valid HTTP) or a non-JSON body, `JSON.parse` throws a raw `SyntaxError` that bypasses the module's error contract (`ApiError` / `UnauthorizedError`).",
"source": "protopatch",
"verdict": "confirmed",
"note": "Read web/src/lib/api.ts:448: `return JSON.parse(res.body) as T;` \u2014 the final line of the `request` function. The try/catch at lines 441-446 only wraps the non-2xx branch. A 200 with empty body (e.g. 204 No Content) or non-JSON body throws an uncaught SyntaxError that escapes the module's ApiError/UnauthorizedError contract. \u2014 nearby: in code this PR did not change (outside its changed lines and the functions they sit in) \u2014 reported, not gated (#232)",
"nearby": true
},
{
"file": "server/routers/delegates.py",
"line": 234,
"severity": "minor",
"category": "cross-file",
"claim": "Hostname/IP mismatch in configured-agent exclusion may cause duplicates in discovery results: the known set uses parsed hostnames while discovery resolves to IPs.",
"evidence": "The `known` set is built by parsing configured delegate URLs with `urlparse`, yielding `(hostname, port)` tuples (e.g., `(\"myagent.local\", 8080)`). However, `discovery.discover(known=known)` performs port scans and mDNS browsing that resolve to IP addresses (e.g., `(\"192.168.1.50\", 8080)`). When a delegate is configured with a resolvable hostname rather than a raw IP, the exclusion tuple will not match.",
"source": "protopatch",
"verdict": "uncertain",
"note": "Read agent/discovery.py discover(): the final filter is `if (a[\"host\"], a[\"port\"]) in known: continue`. The `a[\"host\"]` values come from mDNS (IPs like 192.168.x.x), tailnet (100.x IPs), or local scan (127.0.0.1). A delegate configured as `http://myagent.local:8080/a2a` puts `(\"myagent.local\", 8080)` in known, but discovery finds it as `(\"192.168.1.50\", 8080)` \u2014 no match, duplicate appears. \u2014 evidence not found at the reviewed head \u2014 downgraded to uncertain, cannot gate a merge (issue #25)",
"ungrounded": true
},
{
"file": "server/routers/delegates.py",
"line": 35,
"severity": "nit",
"category": "conventions",
"claim": "Unstructured 500 on reload failure: app._DELEGATES.reload() is not wrapped in try/except, inconsistent with every other CRUD endpoint in the same file that returns a structured {\"error\": ...} body.",
"evidence": "If app._DELEGATES.reload() raises (e.g., YAML parse error in config/delegates.yaml, file missing, permission error), the exception propagates as a bare FastAPI 500 with no structured body. Every other CRUD endpoint in the same file (create, update, delete, list) wraps its config-store calls in try/except and returns JSONResponse with a structured {\"error\": ...} body.",
"source": "protopatch",
"verdict": "confirmed",
"note": "Read server/routers/delegates.py:35-38: `names = app._DELEGATES.reload()` is bare, no try/except. Compare list_delegates_endpoint (line 93-95: `try: entries = read_delegates() except DelegateValidationError as e: return JSONResponse(status_code=500, ...)`), create (line 131-133), update (line 167-169), delete (line 179-181) \u2014 all wrap in try/except with structured JSONResponse. \u2014 nearby: in code this PR did not change (outside its changed lines and the functions they sit in) \u2014 reported, not gated (#232)",
"nearby": true
},
{
"file": "server/routers/delegates.py",
"line": 165,
"severity": "nit",
"category": "correctness",
"claim": "TOCTOU race on PUT: existence check can be stale, causing PUT to create instead of update if a concurrent DELETE removes the delegate between the check and the upsert.",
"evidence": "Line 165 reads the delegate list to verify existence; line 169 calls upsert_delegate which will INSERT if the name is absent. If a concurrent DELETE removes the delegate between these two lines, the upsert silently creates a new delegate with the PUT body rather than returning 404.",
"source": "protopatch",
"verdict": "confirmed",
"note": "Read server/routers/delegates.py:165-169: `if not any(e.get(\"name\") == name for e in read_delegates()): return 404` then `entries = upsert_delegate(body)`. The check-then-act is not atomic; a concurrent DELETE between the two lines would cause upsert to insert rather than update. Valid TOCTOU, low practical risk in single-user desktop app."
},
{
"file": "server/routers/delegates.py",
"line": 193,
"severity": "nit",
"category": "conventions",
"claim": "No error handling around delegate_type_specs() call, inconsistent with sibling endpoints that wrap their core operations in try/except and return structured JSON responses.",
"evidence": "Unlike sibling endpoints in the same file (e.g., list_delegates_endpoint wraps read_delegates() in try/except and returns a structured 500 JSONResponse), delegate_types_endpoint calls delegate_type_specs() bare.",
"source": "protopatch",
"verdict": "confirmed",
"note": "Read server/routers/delegates.py:193-194: `from agent.delegate_adapters import delegate_type_specs; return {\"types\": delegate_type_specs()}` \u2014 no try/except. Sibling endpoints (list line 93, create line 131, update line 167, delete line 179) all wrap in try/except with structured JSONResponse."
},
{
"file": "server/routers/delegates.py",
"line": 129,
"severity": "nit",
"category": "correctness",
"claim": "TOCTOU race on create: existence check and upsert are not atomic; two concurrent requests with the same delegate name can both pass the check before either writes.",
"evidence": "The endpoint reads the delegate file to check for a name collision (line 129), then separately calls upsert_delegate (line 134). If two requests arrive concurrently with the same delegate name, both can pass the existence check before either writes.",
"source": "protopatch",
"verdict": "confirmed",
"note": "Read server/routers/delegates.py:129-134: `if isinstance(name, str) and any(e.get(\"name\") == name for e in read_delegates()): return 409` then `entries = upsert_delegate(body)`. Check-then-act is not atomic; two concurrent POSTs with the same name can both pass the check. Valid TOCTOU, low practical risk in single-user desktop app. \u2014 nearby: in code this PR did not change (outside its changed lines and the functions they sit in) \u2014 reported, not gated (#232)",
"nearby": true
},
{
"file": "server/routers/delegates.py",
"line": 25,
"severity": "nit",
"category": "conventions",
"claim": "reload_users_endpoint lacks structured error handling inconsistent with sibling routes: if user_registry.reload() raises, the caller gets FastAPI's default unstructured 500.",
"evidence": "Every other route in this file wraps its core operation in try/except and returns a structured JSONResponse with a specific status code and an error message. If user_registry.reload() raises (e.g., network timeout to Infisical, malformed YAML, file permissions), the exception propagates to FastAPI's default handler.",
"source": "protopatch",
"verdict": "confirmed",
"note": "Read server/routers/delegates.py:25-27: `names = user_registry.reload(); return {\"ok\": True, ...}` \u2014 no try/except. All sibling CRUD endpoints (list, create, update, delete, reload_delegates) wrap their core operations in try/except with structured JSONResponse. \u2014 nearby: in code this PR did not change (outside its changed lines and the functions they sit in) \u2014 reported, not gated (#232)",
"nearby": true
},
{
"file": "server/routers/delegates.py",
"line": 238,
"severity": "major",
"claim": "SSRF via user-controlled URL in the delegate test probe: an authenticated user can specify an internal/loopback URL and the endpoint will make an outbound request to it, potentially leaking internal service responses.",
"evidence": "The POST /api/delegates/test endpoint accepts an arbitrary user-supplied dict as the probe target. _probe_delegate calls resolve_saved_credentials(entry) which can inject stored auth credentials (API keys, tokens) into the entry based on its name field, then probe(delegate) makes an outbound HTTP request to the URL in the entry.",
"since": "c517e67a5e1260c169bfa7809c0fb23726f0656d",
"verdict": "confirmed",
"carried": true,
"note": "Read delegates.py at head: test_delegate_endpoint (line 238) passes user dict to _probe_delegate \u2192 validate_entry \u2192 _parse_entry \u2192 probe(delegate). probe() in delegate_adapters.py:549 does `await client.post(delegate.url, ...)` with no SSRF guard (no loopback/private-IP check). Authenticated user can point probe at 127.0.0.1 or internal service URLs. \u2014 carried as debt \u2014 re-listed on code changed since it was raised, and not re-verified at this head (#232); it holds approve-on-green until a round verifies, fixes or refutes it"
}
]1 finding(s) downgraded to uncertain: the code they quote as evidence does not appear in the file at the reviewed head, nor in this PR's patch for it. A finding that cannot be grounded does not gate a merge (issue #25) — it still stands for a human to judge.
server/routers/delegates.py(minor) — quoted evidence not found at this head:(hostname, port)
4 structural finding(s) are nearby notes, not part of the verdict: they sit in code this PR did not change (outside its changed lines and the functions those lines are in). Worth a look; not a request for this PR.
web/src/lib/api.ts:448(minor) — Unhandled SyntaxError when a 2xx response body is empty or non-JSON: JSON.parse(res.body) throws a raw SyntaxError that bypasses the module's ApiError/Unauthoriserver/routers/delegates.py:35(nit) — Unstructured 500 on reload failure: app._DELEGATES.reload() is not wrapped in try/except, inconsistent with every other CRUD endpoint in the same file that retuserver/routers/delegates.py:129(nit) — TOCTOU race on create: existence check and upsert are not atomic; two concurrent requests with the same delegate name can both pass the check before either writserver/routers/delegates.py:25(nit) — reload_users_endpoint lacks structured error handling inconsistent with sibling routes: if user_registry.reload() raises, the caller gets FastAPI's default unst
Addressed since the last round (6 minor/nit): server/routers/delegates.py:231, server/routers/delegates.py:252, agent/delegate_adapters.py:563, agent/delegate_config_store.py:103, tests/test_delegate_health.py:15, tests/test_delegates_api.py:17 — no longer raised, and the PR changed the code around them since they were raised.
Re-listed prior finding(s) not re-verified. The panel re-listed these, but the code they cite changed since they were raised and no verifier ruled on them at this head, so they do not block this verdict. They are not cleared either: they stay on the record as carried debt, and the gate holds until a round fixes, refutes, or re-confirms them (#232):
server/routers/delegates.py:238(major) — SSRF via user-controlled URL in the delegate test probe: an authenticated user can specify an internal/loopback URL and the endpoint will make an outbound request to it, potentially leaking internal s
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem and change
Connecting ORBIS to a token-protected protoAgent accepted a pasted token as an environment-variable name, logged that token, and reported a healthy connection after fetching a public agent card. Actual delegation then failed with 401.
Settings now offers a password field for the actual access token and a Connect / Save and connect action that checks authentication before saving. Tokens take effect immediately, are stored in the owner-only delegate YAML, and are redacted in all delegate API responses. Environment-variable configuration remains under Advanced. Editing a connection preserves a saved token, while changing the endpoint requires entering it again. Tests use a read-only
tasks/getrequest to the configured endpoint; an agent-card response alone cannot produce a green result. Errors give a recovery action and are no longer truncated.Validation
ruff check .andgit diff --checkpassed.bun run buildpassed; password-field rendering regression passed withbun test tests/delegateConnection.test.tsx.An interactive Chromium editor smoke with a mocked Tauri IPC boundary passed: password entry, automatic bearer selection, wrong-token recovery without saving, verified save, and clearing authentication. The three connection/callback/warmup PRs also passed the full Python suite together in an integration checkout. Native desktop interaction has not been manually exercised for this PR. Callback negotiation/authentication is a separate follow-up; this PR fixes connection setup and authenticated health reporting.