feat(e2e): AGX1-325 — agent_api_keys authz black-box suite by dm36 · Pull Request #276 · scaleapi/scale-agentex

dm36 · 2026-06-04T18:46:21Z

Summary

Layers the agent_api_keys test cases (AGX1-325) on top of the scaffolding established in #277. This PR is purely the new test files + MANUAL_SMOKE.md; the clients, conftest, cleanup tracker, parent_agent fixture, and graceful-skip wiring all live in #277.

Linear: AGX1-325. Depends on: #277.

What this PR adds

tests/test_api_key_create.py (2 tests) — dual-write registers owner with parent_agent edge; non-owner has no permissions.
tests/test_api_key_get.py (4 tests) — owner 200 (id + name); non-owner 404 (collapsed from 403) on both id and name routes.
tests/test_api_key_list.py (2 tests) — owner sees own; non-owner doesn't see user_a's api_key in the filtered list.
tests/test_api_key_delete.py (3 tests) — delete deregisters from SpiceDB; non-owner delete returns 404 and leaves the row intact.
MANUAL_SMOKE.md — 17 copy-paste curl cases for verifying api_key routes against any deployed env, no venv or pytest required. Doubles as a rollout-verification checklist.
Small Makefile / README.md adjustments to re-expose the api_key targets and scope section now that the test files exist.

Status against pubsec-dev

Today: 0 passed / 11 failed. All 11 fail with the same shape:

POST /agent_api_keys            -> 200 (DB row lands)
GET  /agent_api_keys/{just_id}  -> 422 wrapping 403 from agentex-auth

Root cause: pubsec-dev's agentex-auth deployment has AUTH_PROVIDER=sgp. Every register_resource / check call routes to legacy SGP-authz, which doesn't know about the api_key resource type. Tracked separately in scaleapi/sgp#3334 (parameterizes AUTH_PROVIDER in the agentex-auth Helm chart + system-manager pack so a per-env override can flip pubsec-dev to spark).

After that rollout: expect ~11/12 passing (the 1 skipped is test_get_event_with_view_returns_200 from #277, which depends on event seeding out of scope).

When spark-authz isn't reachable

The two create-time tests and test_owner_delete_deregisters_in_spicedb assert directly against spark-authz. They skip cleanly when spark_authz.host doesn't answer /healthz with 2xx — see #277's authz_reachable fixture for the graceful-skip logic. To run them, port-forward spark-authz from the target cluster:

kubectl --context <cluster> -n spark port-forward svc/spark-authz 8090:8090

Set spark_authz.host = "localhost:8090" in config.json.

Why this is split from #277

Honest call: AGX1-331 (events) ships clean against pubsec-dev today — 3/3 of its assertions pass. The api_key suite is correct code but blocked on a deployment-config change in another repo. Splitting them lets the scaffolding + events ship as a green PR; the api_key cases sit here ready to flip green the moment the SGP rollout lands. Same review surface, smaller blast radius per PR.

Greptile Summary

This PR adds a black-box FGAC end-to-end test suite (scripts/spark-authz-e2e-tests) for agent_api_keys routes, modeled after the equivalent KB suite. Every test hits the real scale-agentex HTTP API and verifies state in SpiceDB via a separate SparkAuthzClient.

Four test files cover the AGX1-325 deliverables: POST dual-write registration, owner GET (200) vs. non-owner GET (404), FGAC-filtered LIST, and DELETE deregistration — plus the AGX1-331 denied-path event tests from feat(e2e): AGX1-331 — event route authz black-box suite #277.
Shared scaffolding includes session-scoped HTTP clients, a LIFO CleanupTracker with REST-then-SpiceDB fallback, a parent_agent fixture that falls back to a config-provided agent ID, and a healthz-gated authz_client fixture that gracefully skips SpiceDB-asserting tests when spark-authz isn't reachable.

Confidence Score: 5/5

Safe to merge — this is a new scripts-only test suite with no changes to production code.

All changes are confined to scripts/spark-authz-e2e-tests. The suite is well-structured: the healthz probe correctly uses resp.is_success, the prior silent-return issue in the list test has been fixed with pytest.skip, and the cleanup tracker handles errors without masking test results. No production code is touched.

No files require special attention for merge safety.

Important Files Changed

Filename	Overview
scripts/spark-authz-e2e-tests/conftest.py	Session-scoped fixtures for config, identities, and HTTP clients; the healthz probe now correctly uses `resp.is_success` (addressing the prior review concern); cleanup logic correctly guards on `rep_call` and respects `on_success`/`on_failure` knobs.
scripts/spark-authz-e2e-tests/tests/test_api_key_delete.py	Tests owner-deregister and non-owner 404 collapse; the `test_owner_delete_deregisters_in_spicedb` test manually creates an api_key without a cleanup entry — if the pre-delete SpiceDB assertion fails the api_key leaks (noted in the prior review's outside-diff comment).
scripts/spark-authz-e2e-tests/tests/test_api_key_list.py	Owner sees own key; non-owner 404-on-agent now correctly uses `pytest.skip` instead of a silent `return` (prior feedback addressed); no new issues.
scripts/spark-authz-e2e-tests/tests/test_event_authz.py	Denied-path event tests pass; the skipped happy-path test references "Linear: TODO follow-up" without a ticket number.
scripts/spark-authz-e2e-tests/Makefile	Well-structured with logical groupings; `make test-sub-resources` / `make test-event` reference `tests/test_event_authz.py` which does not exist in this PR — noted in prior review.

Sequence Diagram

sequenceDiagram
    participant T as Test
    participant CA as AgentexClient (user_a)
    participant CB as AgentexClient (user_b)
    participant SAX as scale-agentex API
    participant AUTHZ as spark-authz (SpiceDB)

    Note over T,AUTHZ: create test
    T->>CA: POST /agents/register-build
    CA->>SAX: HTTP POST
    SAX-->>CA: "200 {id: agent_id}"
    T->>CA: "POST /agent_api_keys {agent_id}"
    CA->>SAX: HTTP POST
    SAX->>AUTHZ: "register_resource(api_key, parent=agent)"
    SAX-->>CA: "200 {id, api_key}"
    T->>AUTHZ: check_permission(user_a, api_key, read)
    AUTHZ-->>T: true

    Note over T,AUTHZ: get/delete negative path
    T->>CB: "GET /agent_api_keys/{id}"
    CB->>SAX: HTTP GET
    SAX->>AUTHZ: check(user_b, api_key.read)
    AUTHZ-->>SAX: denied
    SAX-->>CB: 404 (collapsed from 403)
    CB-->>T: assert 404

    Note over T,AUTHZ: delete dual-write
    T->>CA: "DELETE /agent_api_keys/{id}"
    CA->>SAX: HTTP DELETE
    SAX->>AUTHZ: deregister_resource(api_key)
    SAX-->>CA: 200
    T->>AUTHZ: check_permission(user_a, api_key, read)
    AUTHZ-->>T: false

Comments Outside Diff (1)

scripts/spark-authz-e2e-tests/tests/test_api_key_delete.py, line 1207-1212 (link)

Unregistered api_key leaks if pre-delete assertion fails

The test intentionally bypasses the create_api_key factory to avoid a teardown race, but that means the freshly-created api_key has no cleanup entry. If the pre-delete check_permission_bool assertion fails, the api_key is never deleted and leaks in both the DB and SpiceDB, potentially contaminating subsequent runs. The factory teardown actually handles the 404 case gracefully (it checks not in (200, 204, 404)), so the race concern is minimal. If keeping the factory out is intentional, registering a raw cleanup.add(...) call after the create would prevent the leak without the race issue.

Prompt To Fix With AI

This is a comment left during a code review.
Path: scripts/spark-authz-e2e-tests/tests/test_api_key_delete.py
Line: 1207-1212

Comment:
**Unregistered api_key leaks if pre-delete assertion fails**

The test intentionally bypasses the `create_api_key` factory to avoid a teardown race, but that means the freshly-created api_key has no cleanup entry. If the pre-delete `check_permission_bool` assertion fails, the api_key is never deleted and leaks in both the DB and SpiceDB, potentially contaminating subsequent runs. The factory teardown actually handles the 404 case gracefully (it checks `not in (200, 204, 404)`), so the race concern is minimal. If keeping the factory out is intentional, registering a raw `cleanup.add(...)` call after the create would prevent the leak without the race issue.

How can I resolve this? If you propose a fix, please make it concise.

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
scripts/spark-authz-e2e-tests/tests/test_event_authz.py:1854-1860
**TODO without a Linear ticket number**

The skip reason includes "Linear: TODO follow-up" but no actual ticket number. Without a linked ticket, this placeholder is hard to search for across the repo and easy to forget. Per the team convention, the follow-up should reference a specific Linear ID (e.g., `Linear: AGX1-XXX`).

_{Reviews (5): Last reviewed commit: "feat(e2e): AGX1-325 — agent_api_key auth..." | Re-trigger Greptile}

Context used:

Rule used - Include ticket numbers in TODO comments to make cl... (source)

Learned From
scaleapi/scaleapi#126926

Layered on top of the AGX1-325 api_keys suite (PR #276). Same conftest, same SparkAuthzClient, same cleanup pattern; just extends AgentexClient with /events methods and adds one test file. AGX1-331 scope: event get/list delegated to parent agent view (read-only): no agent view, 404 on get and filtered/empty list. Tests (test_event_authz.py): - test_get_event_nonexistent_returns_404 — sanity: ItemDoesNotExist short-circuits before any authz fires. - test_list_events_without_agent_view_returns_404 — user_b lacks read on user_a's agent → DAuthorizedQuery collapses denial to 404 on agent_id. - test_list_events_without_task_view_returns_404 — symmetric on task_id → both query-param gates are exercised. - test_get_event_with_view_returns_200 — SKIPPED. No public POST /events, events come from ACP streaming. Skip reason makes the gap explicit so whoever later wires up an event-seeding harness can flip it to a real assertion. Out of scope: positive-path coverage that needs a test-only event seeding helper. The ticket asks for the denied paths, and that's what this delivers.

Per Greptile review on #277: the test's previous name + docstring claimed to verify "BOTH query params are gated" by passing zero-UUIDs for both task_id and agent_id, but FastAPI evaluates the ``task_id`` Depends first and short-circuits before the agent_id gate runs. The test would still pass if the agent_id gate were removed entirely — it doesn't isolate either gate. Rename and rewrite the docstring to be honest about what it actually verifies: end-to-end route gating, no per-gate isolation. Independent isolation would require granting one resource but not the other in SpiceDB, which depends on a reachable spark-authz (out of scope today per Gap 1 in #276 — pubsec-dev runs raw SpiceDB without spark-authz).

Per Greptile review: ``make test-sub-resources`` and ``make test-event`` referenced ``tests/test_event_authz.py``, which only exists on #277. Running either target on this branch in isolation produced ``ERROR: not found: tests/test_event_authz.py`` and a non-zero exit. Remove ``EVENT_TESTS``, ``SUB_RESOURCE_TESTS``, ``test-sub-resources``, ``test-event``, plus their references in ``.PHONY``, the ``help`` block, and the README ``Run`` section. The Makefile's header comment retains the convention so #277 (and follow-up sub-resource tickets) can re-add the targets when their test files actually land.

Layered on top of the AGX1-325 api_keys suite (PR #276). Same conftest, same SparkAuthzClient, same cleanup pattern; just extends AgentexClient with /events methods and adds one test file. AGX1-331 scope: event get/list delegated to parent agent view (read-only): no agent view, 404 on get and filtered/empty list. Tests (test_event_authz.py): - test_get_event_nonexistent_returns_404 — sanity: ItemDoesNotExist short-circuits before any authz fires. - test_list_events_without_agent_view_returns_404 — user_b lacks read on user_a's agent → DAuthorizedQuery collapses denial to 404 on agent_id. - test_list_events_without_task_view_returns_404 — symmetric on task_id → both query-param gates are exercised. - test_get_event_with_view_returns_200 — SKIPPED. No public POST /events, events come from ACP streaming. Skip reason makes the gap explicit so whoever later wires up an event-seeding harness can flip it to a real assertion. Out of scope: positive-path coverage that needs a test-only event seeding helper. The ticket asks for the denied paths, and that's what this delivers.

Per Greptile review on #277: the test's previous name + docstring claimed to verify "BOTH query params are gated" by passing zero-UUIDs for both task_id and agent_id, but FastAPI evaluates the ``task_id`` Depends first and short-circuits before the agent_id gate runs. The test would still pass if the agent_id gate were removed entirely — it doesn't isolate either gate. Rename and rewrite the docstring to be honest about what it actually verifies: end-to-end route gating, no per-gate isolation. Independent isolation would require granting one resource but not the other in SpiceDB, which depends on a reachable spark-authz (out of scope today per Gap 1 in #276 — pubsec-dev runs raw SpiceDB without spark-authz).

…y exists #276's Greptile-driven cleanup removed test-event / test-sub-resources from the Makefile to avoid dangling targets that referenced a file which doesn't exist on that PR's branch. This PR adds the test file, so it now re-adds the matching targets + README references. After this commit (#277 in isolation): make test-direct-resources # api_key tests make test-sub-resources # event tests make test-api-key # all AGX1-325 cases make test-event # all AGX1-331 cases

Per Greptile review: ``test_non_owner_list_excludes_user_a_api_key`` silently returned on 404, making the test appear PASSED even though the FGAC list-filter assertion was never reached. If user_b consistently 404s in an environment (e.g. lacks read on the parent agent), the test would show all-green while never exercising the filter behavior it claims to verify. Replace the silent ``return`` with ``pytest.skip`` and a reason that points at the actual gap: user_b lacks ``read`` on the parent agent, so the route 404s before the list filter runs. Reports now show this as SKIPPED instead of PASSED, which is the honest outcome. To actually exercise the list filter, user_b would need read on the agent but no read on user_a's api_keys — which requires a working spark-authz round-trip we don't have today (see Gap 1 in #276's PR description for the deployment gap).