tsk-x46zyb [OPEN] Observatory queue/throttle routes reject the agent - #2201
tsk-x46zyb [OPEN] Observatory queue/throttle routes reject the agent#2201jaylfc wants to merge 1 commit into
Conversation
📝 WalkthroughWalkthroughObservatory pause and throttle routes now use scoped agent or admin authorization, and agent-token passthrough includes Observatory paths. A persisted queue-control API was added with global and per-lane limits. Tests now cover agent permissions, persistence, authenticated reads, and shared client helpers. ChangesObservatory controls
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
PR Summary by QodoEnable scoped agent access to Observatory controls
AI Description
Diagram
High-Level Assessment
Files changed (3)
|
Code Review by Qodo
1. observatory_control is not grantable
|
![qodo-code-review[bot]](https://avatars.github.com/in/484649?s=60&v=4){kind=small}
|
|
||
| async def _require_observatory_write(request: Request) -> str: | ||
| from tinyagentos.agent_token_auth import check_agent_scope | ||
| cid = await check_agent_scope(request, "observatory_control") |
There was a problem hiding this comment.
1. observatory_control is not grantable 📜 Skill insight ≡ Correctness
The new Observatory write authorization requires observatory_control, but that scope is absent from the closed vocabularies used by both consent/scope-request approval and internal-agent minting. Production agents therefore cannot obtain the required permission through supported provisioning flows, while regression tests conceal the issue by inserting the grant directly into the store.
Agent Prompt
## Issue description
The Observatory write routes require an `observatory_control` grant that none of the supported production provisioning APIs currently recognize, preventing agents from acquiring the permission through normal minting or consent flows.
## Issue Context
Add `observatory_control` to both synchronized, authoritative grantable-scope vocabularies and any user-facing scope metadata. Retain API-level tests that request, approve, and mint the scope through a public provisioning flow rather than relying only on direct grant-store insertion.
## Fix Focus Areas
- tinyagentos/routes/observatory.py[104-112]
- tinyagentos/routes/agent_registry.py[87-100]
- tinyagentos/routes/agent_auth_requests.py[43-81]
- tests/test_routes_observatory.py[10-33]
- tests/test_routes_observatory.py[157-180]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| if not p.exists(): | ||
| return {"global": None, "lanes": {}} | ||
| try: | ||
| data = json.loads(p.read_text()) |
There was a problem hiding this comment.
2. _read_queue performs synchronous i/o 📜 Skill insight ≡ Correctness
_read_queue calls Path.exists() and Path.read_text() synchronously and is invoked directly by async route handlers. These filesystem operations can block the event loop for concurrent requests.
Agent Prompt
## Issue description
The new async queue routes perform blocking filesystem reads through the synchronous `_read_queue` helper.
## Issue Context
Move the filesystem work to an asynchronous file API or `asyncio.to_thread`, make the helper awaitable, and update every queue-route caller.
## Fix Focus Areas
- tinyagentos/routes/observatory.py[218-231]
- tinyagentos/routes/observatory.py[239-254]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| state["lanes"][scope] = limit | ||
| else: | ||
| state["lanes"].pop(scope, None) | ||
| _atomic_write(_queue_path(request), state) |
There was a problem hiding this comment.
3. set_queue writes synchronously 📜 Skill insight ≡ Correctness
The async set_queue handler invokes _atomic_write directly, which performs directory creation, temporary-file writes, replacement, and cleanup synchronously. A slow filesystem can block the application event loop during queue updates.
Agent Prompt
## Issue description
The new queue mutation route performs its atomic filesystem write synchronously inside an async handler.
## Issue Context
Run `_atomic_write` through an awaited asynchronous boundary such as `asyncio.to_thread`, while preserving the existing write lock and atomic replacement behavior.
## Fix Focus Areas
- tinyagentos/routes/observatory.py[245-262]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
|
|
||
| async def _new_project(client, name="alpha", slug="alpha"): | ||
| resp = await client.post("/api/projects", json={"name": name, "slug": slug}) | ||
| assert resp.status_code in (200, 201), resp.text |
There was a problem hiding this comment.
5. _new_project accepts ambiguous statuses 📜 Skill insight ≡ Correctness
The helper accepts either HTTP 200 or 201 even though project creation deterministically returns 200. This weakens the regression tests by allowing an unintended status-code change to pass.
Agent Prompt
## Issue description
The new project helper uses a multi-value status assertion instead of exact equality.
## Issue Context
The project creation route currently returns HTTP 200, so assert that exact value and retain the returned project identifier as the resulting-state check.
## Fix Focus Areas
- tests/test_routes_observatory.py[58-61]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
|
|
||
| async def _require_observatory_read(request: Request) -> None: | ||
| from tinyagentos.agent_token_auth import check_agent_identity | ||
| cid = await check_agent_identity(request) |
There was a problem hiding this comment.
6. Local token rejected 🐞 Bug ≡ Correctness
_require_observatory_read validates every Bearer credential as a registry JWT before honoring middleware-established authentication, so valid local admin tokens used by taosctl receive 401 instead of reading Observatory state. The write resolver has the same ordering, also breaking pause, throttle, and queue updates made with the local token.
Agent Prompt
## Issue description
Observatory authentication attempts registry-JWT validation before honoring the admin identity already established by middleware. A valid local admin Bearer token is consequently parsed as a registry JWT and rejected.
## Issue Context
Check `request.state.is_admin` or the authenticated human identity before invoking agent-token validation, following the established A2A authorization pattern. Add coverage using a valid local admin token for both reads and writes.
## Fix Focus Areas
- tinyagentos/routes/observatory.py[94-112]
- tinyagentos/routes/a2a_bus.py[55-69]
- tests/test_routes_observatory.py[64-192]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| except (OSError, ValueError): | ||
| return {"global": None, "lanes": {}} | ||
| lanes = {} | ||
| for k, v in (data.get("lanes") or {}).items(): |
There was a problem hiding this comment.
7. Malformed queue state crashes 🐞 Bug ☼ Reliability
_read_queue assumes successfully decoded JSON and its lanes member are dictionaries, so valid
malformed state such as [] or {"lanes": []} raises AttributeError. Both queue GET and POST
then return 500 until the persisted file is manually repaired.
Agent Prompt
## Issue description
The queue-state reader handles invalid JSON but not valid JSON with an unexpected top-level or nested shape.
## Issue Context
Fall back to the default state when the decoded value is not a dictionary, and treat a non-dictionary `lanes` value as an empty mapping. Add regression tests for scalar/list top-level state and non-mapping lanes.
## Fix Focus Areas
- tinyagentos/routes/observatory.py[218-231]
- tinyagentos/routes/observatory.py[289-313]
- tests/test_routes_observatory.py[198-229]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
|
nemotron-ultra-orB review VERDICT: Significant test coverage gap for pause endpoint agent authorization; potential test isolation issues with duplicate agent handles; minor auth return type inconsistency.
Automated first-pass review by the nemotron-ultra-orB lane. The lead still reviews before merge. |
|
nemotron-ultra-kilo review VERDICT: Multiple correctness bugs, security gaps, and missing tests.
Automated first-pass review by the nemotron-ultra-kilo lane. The lead still reviews before merge. |
![coderabbitai[bot]](https://avatars.github.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
tinyagentos/routes/observatory.py (2)
104-139: 🔒 Security & Privacy | 🔵 TrivialConsider logging the acting identity on privileged writes.
_require_observatory_writeresolves and returns the caller's canonical_id/user_id, butset_pause/set_throttle/set_queuediscard it (_actor). Since these endpoints steer the whole fleet (pause, concurrency caps, queue caps), recording who made the change (agent vs. admin, and which id) would help incident review/audit trails without much added code.🤖 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/observatory.py` around lines 104 - 139, Add audit logging for privileged observatory mutations by using the canonical actor returned from _require_observatory_write in set_pause, set_throttle, and set_queue instead of discarding it; record whether the caller is an agent or admin and include its identifier alongside the change details, using the module’s existing logging mechanism.
208-264: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winQueue read/write duplicates throttle's logic almost verbatim.
_queue_path/_read_queue/set_queueare structurally identical to_throttle_path/_read_throttle/set_throttle(same "global vs. lane cap with_coerce_limit" shape, same lock/atomic-write dance). Since this pattern now exists twice, consider extracting a shared helper (e.g._read_capped_state(path)/_apply_scope_limit(state, scope, limit)) parameterized by path and field name, used by both throttle and queue.♻️ Sketch
def _read_capped_state(path: Path) -> dict: if not path.exists(): return {"global": None, "lanes": {}} try: data = json.loads(path.read_text()) except (OSError, ValueError): return {"global": None, "lanes": {}} if not isinstance(data, dict): return {"global": None, "lanes": {}} lanes = {} for k, v in (data.get("lanes") or {}).items(): lim = _coerce_limit(v) if lim is not None: lanes[str(k)] = lim return {"global": _coerce_limit(data.get("global")), "lanes": lanes}Both
_read_throttleand_read_queuebecome one-line wrappers around this, and thescope == "global" / elif limit is not None / else popblock inset_throttle/set_queuecan similarly be factored into a shared mutator.🤖 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/observatory.py` around lines 208 - 264, Extract the duplicated capped-state handling from `_read_throttle` and `_read_queue` into a shared reader such as `_read_capped_state`, preserving invalid-data normalization and `_coerce_limit` behavior. Also factor the shared global/lane update logic used by `set_throttle` and `set_queue` into a mutator, then have both endpoints retain their existing paths, locking, and atomic writes while delegating state transformation to the helpers.
🤖 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/routes/observatory.py`:
- Around line 214-231: Update _read_queue to validate that the parsed JSON value
is a dictionary before accessing data.get; return the documented empty queue
default for valid non-object JSON such as lists or null, while preserving the
existing lane and limit coercion behavior for dictionary data.
---
Nitpick comments:
In `@tinyagentos/routes/observatory.py`:
- Around line 104-139: Add audit logging for privileged observatory mutations by
using the canonical actor returned from _require_observatory_write in set_pause,
set_throttle, and set_queue instead of discarding it; record whether the caller
is an agent or admin and include its identifier alongside the change details,
using the module’s existing logging mechanism.
- Around line 208-264: Extract the duplicated capped-state handling from
`_read_throttle` and `_read_queue` into a shared reader such as
`_read_capped_state`, preserving invalid-data normalization and `_coerce_limit`
behavior. Also factor the shared global/lane update logic used by `set_throttle`
and `set_queue` into a mutator, then have both endpoints retain their existing
paths, locking, and atomic writes while delegating state transformation to the
helpers.
🪄 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: 467b868a-ef4e-4db3-b10d-ac28ccd63843
📒 Files selected for processing (3)
tests/test_routes_observatory.pytinyagentos/auth_middleware.pytinyagentos/routes/observatory.py
| def _queue_path(request: Request) -> Path: | ||
| return Path(request.app.state.data_dir) / "observatory_queue.json" | ||
|
|
||
|
|
||
| def _read_queue(request: Request) -> dict: | ||
| p = _queue_path(request) | ||
| if not p.exists(): | ||
| return {"global": None, "lanes": {}} | ||
| try: | ||
| data = json.loads(p.read_text()) | ||
| except (OSError, ValueError): | ||
| return {"global": None, "lanes": {}} | ||
| lanes = {} | ||
| for k, v in (data.get("lanes") or {}).items(): | ||
| lim = _coerce_limit(v) | ||
| if lim is not None: | ||
| lanes[str(k)] = lim | ||
| return {"global": _coerce_limit(data.get("global")), "lanes": lanes} |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Malformed queue file crashes instead of falling back.
_read_queue catches (OSError, ValueError) around json.loads, but if the persisted file contains valid JSON that isn't a dict (e.g. a hand-edited [] or null), data.get(...) raises AttributeError, which is uncaught and will 500 the request instead of degrading to the documented default {"global": None, "lanes": {}}. This mirrors the same latent gap already present in _read_throttle/_read_state.
🛡️ Proposed fix
try:
data = json.loads(p.read_text())
- except (OSError, ValueError):
+ except (OSError, ValueError):
+ return {"global": None, "lanes": {}}
+ if not isinstance(data, dict):
return {"global": None, "lanes": {}}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def _queue_path(request: Request) -> Path: | |
| return Path(request.app.state.data_dir) / "observatory_queue.json" | |
| def _read_queue(request: Request) -> dict: | |
| p = _queue_path(request) | |
| if not p.exists(): | |
| return {"global": None, "lanes": {}} | |
| try: | |
| data = json.loads(p.read_text()) | |
| except (OSError, ValueError): | |
| return {"global": None, "lanes": {}} | |
| lanes = {} | |
| for k, v in (data.get("lanes") or {}).items(): | |
| lim = _coerce_limit(v) | |
| if lim is not None: | |
| lanes[str(k)] = lim | |
| return {"global": _coerce_limit(data.get("global")), "lanes": lanes} | |
| def _queue_path(request: Request) -> Path: | |
| return Path(request.app.state.data_dir) / "observatory_queue.json" | |
| def _read_queue(request: Request) -> dict: | |
| p = _queue_path(request) | |
| if not p.exists(): | |
| return {"global": None, "lanes": {}} | |
| try: | |
| data = json.loads(p.read_text()) | |
| except (OSError, ValueError): | |
| return {"global": None, "lanes": {}} | |
| if not isinstance(data, dict): | |
| return {"global": None, "lanes": {}} | |
| lanes = {} | |
| for k, v in (data.get("lanes") or {}).items(): | |
| lim = _coerce_limit(v) | |
| if lim is not None: | |
| lanes[str(k)] = lim | |
| return {"global": _coerce_limit(data.get("global")), "lanes": lanes} |
🤖 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/observatory.py` around lines 214 - 231, Update _read_queue
to validate that the parsed JSON value is a dictionary before accessing
data.get; return the documented empty queue default for valid non-object JSON
such as lists or null, while preserving the existing lane and limit coercion
behavior for dictionary data.
|
Closing and recutting the card rather than leaving this open: the lane cannot push again to its own branch, and the fixes are specific enough to spec precisely. No criticism of the work, the shape is right and the negative tests are genuinely load-bearing (I sabotaged the write check and 5 tests went red, so the refusal path is real). BLOCKER 1, the write path is dead in production. observatory.py:106 requires the scope observatory_control, but it is absent from BOTH VALID_SCOPES (agent_auth_requests.py:45-81) and _ALLOWED_SCOPES (agent_registry.py:90-102). Probed against the real provisioning routes: consent-request returns 400 unknown scopes, mint-internal returns 422. So no agent can ever hold the grant and POST /api/observatory/{pause,throttle,queue} stays exactly as unreachable as before. CI passed anyway because the test helper calls grants.add_grant directly and bypasses every validation layer, which is why the suite proves nothing about grantability. This is the third time today a green suite has certified a feature nobody can reach. BLOCKER 2, a regression that breaks taosctl. observatory.py:94-112 runs the registry-JWT verifier BEFORE honouring the admin identity the middleware already established, so a local admin token now 401s. Proven head against dev on the same probe: GET and POST /api/observatory/pause and GET /throttle all 200 on dev and 401 here, with an untouched control route still 200 both sides. cli/taosctl/client.py:70 sends the local token as Bearer, so taosctl observatory pause/unpause/throttle/status is fully broken, as is any local-token poller. The correct pattern is already in this repo at a2a_bus.py:55-69: check request.state.is_admin FIRST, then fall through to the scope check. Also unresolved, folded into the new card: the write scope is checked globally rather than per project, so a project-bound grant silently confers fleet-wide pause; the read gate uses check_agent_identity, which is any registered agent with zero grants rather than the project-scoped agent the card asked for, and it discloses the full lane roster; POST /api/observatory/pause has no agent-token test at all; docs/agent-coordination.md was not swept. Adjudicated so nobody chases them: nemotron's claim that _non_admin_client creates an admin is WRONG (auth.py:357 hardcodes is_admin False, and the sabotage run proves those tests are live). The sync-I/O and non-dict-JSON findings are real but PRE-EXISTING, copied from _read_throttle; they do not belong in this card. |
1 participant
Autonomous build of board card tsk-x46zyb.
Files:
tests/test_routes_observatory.py | 222 +++++++++++++++++++++++++++++++++-----
tinyagentos/auth_middleware.py | 11 +-
tinyagentos/routes/observatory.py | 106 +++++++++++++++---
3 files changed, 302 insertions(+), 37 deletions(-)
Summary by CodeRabbit
New Features
Bug Fixes