Skip to content

refactor(events): inline agent collapse-to-404 check; drop one-off helper#267

Open
dm36 wants to merge 1 commit into
mainfrom
dhruv/inline-events-agent-authz-check
Open

refactor(events): inline agent collapse-to-404 check; drop one-off helper#267
dm36 wants to merge 1 commit into
mainfrom
dhruv/inline-events-agent-authz-check

Conversation

@dm36
Copy link
Copy Markdown
Contributor

@dm36 dm36 commented Jun 2, 2026

Summary

Addresses @rpatel-scale's review comment on #257:

Shouldn't this already be implemented somewhere? How do we make this check for tasks?

check_agent_or_collapse_to_404 had exactly one caller (GET /events/{id}) and structurally duplicated:

Rather than ship a third per-resource helper, inline the 4-line try/except in events.py and delete agent_authorization.py.

Wire behavior

Unchanged:

  • One authorization.check(resource=AgentexResource.agent(event.agent_id), operation=read).
  • On AuthorizationError, raise ItemDoesNotExist → FastAPI returns 404.
  • Cross-tenant existence still hidden (403 vs 404 indistinguishable).

Follow-up

When #249 lands, all three of (task, api_key, events) will have the same try/except shape inline or in their own one-off helper. A small cleanup PR can then fold them into a single generic check_or_collapse_to_404(authorization, resource, operation) — with full context across all three callers in one place rather than piecemeal.

Diff

 agentex/src/api/routes/events.py         | 19 +++++++++++++++----
 agentex/src/utils/agent_authorization.py | 26 ----------------------------
 2 files changed, 12 insertions(+), 31 deletions(-)
 delete mode 100644 agentex/src/utils/agent_authorization.py

Test plan

  • uv run pytest agentex/tests/integration/api/events/test_events_authz_api.py — the existing 5 cases assert wire behavior (resource type, operation, 200/404 status), so they should pass unchanged.

Greptile Summary

This PR inlines the 4-line try/except from check_agent_or_collapse_to_404 directly into GET /events/{id} and deletes agent_authorization.py, which had exactly one caller and duplicated the pattern already established by the api_key and task helpers.

  • The inlined logic is functionally equivalent and also silently fixes a minor issue where the deleted helper was raising a 404 message referencing agent_id instead of the event_id visible to the caller.
  • The TODO(AGX1-290) comment (restore 403/404 split once agents carry tenant scope) was present in the deleted file but was not carried forward into the inlined block; it still lives in agent_api_key_authorization.py for the api_key path.

Confidence Score: 5/5

Safe to merge — the inlined logic is a direct, correct transcription of the deleted helper with no behavioral regressions.

The change is a straightforward inline refactor. The only meaningful delta is the 404 error message now correctly uses event_id rather than agent_id, which is an improvement. The one minor thing worth cleaning up is the dropped TODO(AGX1-290) tracking comment.

No files require special attention; both touched files are small and self-contained.

Important Files Changed

Filename Overview
agentex/src/api/routes/events.py Inlines the 4-line try/except from the deleted helper; also corrects the 404 error message to use event_id instead of the old agent_id. The TODO(AGX1-290) tracking the 403/404 restore was not carried forward.
agentex/src/utils/agent_authorization.py Deleted — had exactly one caller and structurally duplicated the api_key and task helpers. No other references remain in the codebase.

Sequence Diagram

sequenceDiagram
    participant Client
    participant EventsRouter
    participant EventUseCase
    participant AuthorizationService

    Client->>EventsRouter: "GET /events/{event_id}"
    EventsRouter->>EventUseCase: get(event_id)
    EventUseCase-->>EventsRouter: event_entity
    EventsRouter->>AuthorizationService: "check(resource=agent(event_entity.agent_id), operation=read)"
    alt Authorization OK
        AuthorizationService-->>EventsRouter: OK
        EventsRouter-->>Client: 200 Event
    else AuthorizationError
        AuthorizationService-->>EventsRouter: AuthorizationError
        EventsRouter-->>Client: 404 ItemDoesNotExist(event_id)
    end
Loading

Fix All in Cursor Fix All in Claude Code Fix All in Codex

Prompt To Fix All With AI
Fix the following 1 code review issue. Work through them one at a time, proposing concise fixes.

---

### Issue 1 of 1
agentex/src/api/routes/events.py:33-39
The `TODO(AGX1-290)` that tracked restoring the 403/404 split once agents carry tenant scope was present in the deleted `agent_authorization.py` helper but wasn't carried into the inlined code. The same ticket is still referenced in `agent_api_key_authorization.py`, so the work item lives on in one place but is now silently dropped for the agent path here.

```suggestion
    try:
        await authorization.check(
            resource=AgentexResource.agent(event_entity.agent_id),
            operation=AuthorizedOperationType.read,
        )
    except AuthorizationError:
        # TODO(AGX1-290): restore 403/404 split once agents carry tenant scope.
        raise ItemDoesNotExist(f"Item with id '{event_id}' does not exist.") from None
```

Reviews (1): Last reviewed commit: "refactor(events): inline agent collapse-..." | Re-trigger Greptile

Context used:

  • Rule used - Create Linear tasks for TODO comments in code and ... (source)

Learned From
scaleapi/scaleapi#127117

…lper

Per @rpatel-scale's review on #257: ``check_agent_or_collapse_to_404`` had
exactly one caller (``GET /events/{id}``) and duplicated the shape of
``_check_api_key_or_collapse_to_404`` and the in-flight
``check_task_or_collapse_to_404`` from #249.

Rather than ship a third per-resource helper, inline the 4-line try/except
in ``events.py`` and delete ``agent_authorization.py`` outright. Wire
behavior is unchanged: one ``authz.check`` on the parent agent; denial
surfaces as 404 (``ItemDoesNotExist``) so callers can't probe cross-tenant
existence by comparing 403 vs 404.

When #249 lands, a follow-up can fold the (then-three) inline/named copies
into a single generic ``check_or_collapse_to_404(authz, resource, op)``
with full context across tasks / api_keys / events.
@dm36 dm36 requested a review from a team as a code owner June 2, 2026 21:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant