fix(schedules): grant legacy auth for run schedules (#344)

jromualdez-scale · cursoragent · web-flow · commit 648e81cf6d95 · 2026-07-01T16:12:06.000-04:00
## Summary - Preserve Spark-style schedule resource registration while also granting the schedule directly for legacy SGP auth. - Ensure schedule create rollback and delete cleanup revoke legacy permissions as well as deregister Spark resources. - Cover create, rollback, grant-failure compensation, no-creator, and delete cleanup behavior in focused unit tests. ## Context Legacy `agentex-auth` currently runs with `AUTH_PROVIDER=sgp`, where `register_resource` is a no-op. Run schedules were only writing Spark-style registration, so schedule rows and Temporal clocks could exist while legacy `schedule.read` search/check returned no permission, causing list endpoints to return `200 []`. This bridges current legacy SGP auth while keeping `register_resource` / `deregister_resource` in place for the eventual Spark migration. ## Test plan - `uv run --group test pytest tests/unit/services/test_agent_run_schedule_service.py` - `uv run ruff check src/domain/services/agent_run_schedule_service.py tests/unit/services/test_agent_run_schedule_service.py` ## Notes Existing schedules created before this change may need a one-time legacy grant backfill or recreation to become visible through schedule list/get under legacy SGP auth. Made with [Cursor](https://cursor.com)  <h3>Greptile Summary</h3> This PR bridges the gap where `register_resource` is a no-op under the legacy SGP auth provider (`AUTH_PROVIDER=sgp`), causing `schedule.read` to return no permission and list endpoints to return `200 []`. It adds a `grant` call after `register_resource` in the creation path and a `revoke` call before `deregister_resource` in the cleanup path, keeping both Spark-style and SGP-style auth in sync. - `_register_schedule_in_auth` now calls `grant` after `register_resource`, with a compensation block that calls `deregister_resource` on grant failure before re-raising; the `registered` flag in `create_schedule` remains `False` on any failure so the outer rollback doesn't double-deregister. - `_deregister_schedule_from_auth` now prepends a best-effort `revoke` call (in its own try/except) before the existing `deregister_resource`, so both auth paths are cleaned up on delete or rollback. - Tests cover the happy path, grant-failure compensation, registration-failure abort, Temporal-failure rollback, no-creator skip, and delete cleanup — with precise `assert_called_once_with` assertions on the new `grant` and `revoke` calls. <details><summary><h3>Confidence Score: 5/5</h3></summary> Safe to merge — the dual-auth bridge is correctly wired, the `registered` flag prevents double-cleanup, and the new test cases verify all compensation paths. The rollback semantics are sound: `registered` stays `False` if `_register_schedule_in_auth` raises (whether from `register_resource` or `grant`), so the outer cleanup never calls `_deregister_schedule_from_auth` when the inner compensation already handled it. The grant-failure path correctly skips `revoke` (nothing was granted). The delete path calls `revoke` then `deregister_resource` in independent try/except blocks, so a failure in one doesn't prevent the other. Tests are precise and cover the newly introduced failure modes. No files require special attention. </details> <details><summary><h3>Important Files Changed</h3></summary> | Filename | Overview | |----------|----------| | agentex/src/domain/services/agent_run_schedule_service.py | Adds legacy SGP grant/revoke calls alongside the existing Spark register/deregister calls; compensation logic and rollback flag semantics are correct — grant failure triggers deregister (not revoke), and the `registered` flag stays False so the outer rollback never double-cleans. | | agentex/tests/unit/services/test_agent_run_schedule_service.py | New tests cover grant-failure compensation, no-creator skip, and upgraded assertions on existing rollback/delete tests to use `assert_called_once_with` for both `grant` and `revoke`; all scenarios are consistent with implementation behavior. | </details> <details><summary><h3>Sequence Diagram</h3></summary> <a href="#gh-light-mode-only"> ```mermaid %%{init: {'theme': 'neutral'}}%% sequenceDiagram participant CS as create_schedule participant RA as _register_schedule_in_auth participant AS as authorization_service participant TA as temporal_adapter participant DR as _deregister_schedule_from_auth CS->>RA: _register_schedule_in_auth(authz_selector, agent_id) RA->>AS: "register_resource(schedule_resource, parent=agent_resource)" alt register_resource fails AS-->>RA: raises Exception RA-->>CS: "raises (registered=False)" CS->>CS: _best_effort_delete_row() else register_resource succeeds RA->>AS: grant(schedule_resource) alt grant fails AS-->>RA: raises Exception RA->>AS: deregister_resource(schedule_resource) [compensation] RA-->>CS: "raises (registered=False)" CS->>CS: _best_effort_delete_row() else grant succeeds RA-->>CS: "returns True (registered=True)" CS->>TA: create_schedule(...) alt Temporal fails TA-->>CS: raises Exception CS->>DR: _deregister_schedule_from_auth(authz_selector) DR->>AS: revoke(schedule_resource) DR->>AS: deregister_resource(schedule_resource) CS->>CS: _best_effort_delete_row() else Temporal succeeds TA-->>CS: success CS-->>CS: return response end end end ``` </a> <a href="#gh-dark-mode-only"> ```mermaid %%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%% sequenceDiagram participant CS as create_schedule participant RA as _register_schedule_in_auth participant AS as authorization_service participant TA as temporal_adapter participant DR as _deregister_schedule_from_auth CS->>RA: _register_schedule_in_auth(authz_selector, agent_id) RA->>AS: "register_resource(schedule_resource, parent=agent_resource)" alt register_resource fails AS-->>RA: raises Exception RA-->>CS: "raises (registered=False)" CS->>CS: _best_effort_delete_row() else register_resource succeeds RA->>AS: grant(schedule_resource) alt grant fails AS-->>RA: raises Exception RA->>AS: deregister_resource(schedule_resource) [compensation] RA-->>CS: "raises (registered=False)" CS->>CS: _best_effort_delete_row() else grant succeeds RA-->>CS: "returns True (registered=True)" CS->>TA: create_schedule(...) alt Temporal fails TA-->>CS: raises Exception CS->>DR: _deregister_schedule_from_auth(authz_selector) DR->>AS: revoke(schedule_resource) DR->>AS: deregister_resource(schedule_resource) CS->>CS: _best_effort_delete_row() else Temporal succeeds TA-->>CS: success CS-->>CS: return response end end end ``` </a> </details> <sub>Reviews (2): Last reviewed commit: ["test(schedules): tighten legacy auth cle..."](7202557) | [Re-trigger Greptile](https://app.greptile.com/api/retrigger?id=41188765)</sub>  --------- Co-authored-by: Cursor <cursoragent@cursor.com>
diff --git a/agentex/src/domain/services/agent_run_schedule_service.py b/agentex/src/domain/services/agent_run_schedule_service.py
@@ -502,16 +502,57 @@ async def _register_schedule_in_auth(
                 extra={"authz_selector": authz_selector, "agent_id": agent_id},
             )
             return False
+        schedule_resource = AgentexResource.schedule(authz_selector)
         await self.authorization_service.register_resource(
-            resource=AgentexResource.schedule(authz_selector),
+            resource=schedule_resource,
             parent=AgentexResource.agent(agent_id),
         )
+        try:
+            # Legacy SGP auth treats register_resource as a no-op. Keep the
+            # Spark registration above for the future path, and write the legacy
+            # grant so current list/check calls can see the schedule.
+            await self.authorization_service.grant(schedule_resource)
+        except Exception as grant_exc:
+            logger.warning(
+                "Auth grant failed for run schedule; compensating with deregister",
+                extra={
+                    "authz_selector": authz_selector,
+                    "error_type": type(grant_exc).__name__,
+                },
+                exc_info=True,
+            )
+            try:
+                await self.authorization_service.deregister_resource(
+                    resource=schedule_resource,
+                )
+            except Exception as cleanup_exc:
+                logger.warning(
+                    "Auth deregister failed after run schedule grant failure",
+                    extra={
+                        "authz_selector": authz_selector,
+                        "error_type": type(cleanup_exc).__name__,
+                    },
+                    exc_info=True,
+                )
+            raise
         return True
 
     async def _deregister_schedule_from_auth(self, *, authz_selector: str) -> None:
+        schedule_resource = AgentexResource.schedule(authz_selector)
+        try:
+            await self.authorization_service.revoke(resource=schedule_resource)
+        except Exception as exc:
+            logger.warning(
+                "Auth revoke failed for run schedule; entry may be orphaned",
+                extra={
+                    "authz_selector": authz_selector,
+                    "error_type": type(exc).__name__,
+                },
+                exc_info=True,
+            )
         try:
             await self.authorization_service.deregister_resource(
-                resource=AgentexResource.schedule(authz_selector),
+                resource=schedule_resource
             )
         except Exception as exc:
             logger.warning(
diff --git a/agentex/tests/unit/services/test_agent_run_schedule_service.py b/agentex/tests/unit/services/test_agent_run_schedule_service.py
@@ -11,6 +11,7 @@
     ScheduleInitialInput,
     UpdateAgentRunScheduleRequest,
 )
+from src.api.schemas.authorization_types import AgentexResource
 from src.domain.entities.agent_run_schedules import AgentRunScheduleEntity
 from src.domain.entities.agents import ACPType, AgentEntity, AgentStatus
 from src.domain.exceptions import ClientError
@@ -112,8 +113,16 @@ async def test_create_persists_and_schedules(self, service, agent):
         )
         assert create_kwargs["time_zone_name"] == "America/New_York"
 
-        # Ownership registered before the Temporal write.
-        service.authorization_service.register_resource.assert_called_once()
+        # Ownership is written to both auth paths before the Temporal write:
+        # Spark resource registration for the future path, and a legacy grant
+        # for current SGP list/check behavior.
+        authz_selector = build_run_schedule_authz_selector(agent.id, persisted.name)
+        schedule_resource = AgentexResource.schedule(authz_selector)
+        service.authorization_service.register_resource.assert_called_once_with(
+            resource=schedule_resource,
+            parent=AgentexResource.agent(agent.id),
+        )
+        service.authorization_service.grant.assert_called_once_with(schedule_resource)
 
         assert response.name == "daily-summary"
         assert response.initial_input_method == "event/send"  # async agent
@@ -141,8 +150,13 @@ async def test_create_rolls_back_row_on_temporal_failure(self, service, agent):
         with pytest.raises(RuntimeError):
             await service.create_schedule(agent, request, {"user_id": "u1"})
 
-        # The orphaned row and auth entry are compensated.
+        # The orphaned row and both auth entries are compensated.
+        authz_selector = build_run_schedule_authz_selector(agent.id, persisted.name)
+        schedule_resource = AgentexResource.schedule(authz_selector)
         service.schedule_repository.delete.assert_called_once_with(id=persisted.id)
+        service.authorization_service.revoke.assert_called_once_with(
+            resource=schedule_resource
+        )
         service.authorization_service.deregister_resource.assert_called_once()
 
     async def test_create_rolls_back_row_on_auth_registration_failure(
@@ -163,6 +177,39 @@ async def test_create_rolls_back_row_on_auth_registration_failure(
         # must not create a Temporal schedule.
         service.schedule_repository.delete.assert_called_once_with(id=persisted.id)
         service.temporal_adapter.create_schedule.assert_not_called()
+        service.authorization_service.grant.assert_not_called()
+
+    async def test_create_compensates_register_when_grant_fails(self, service, agent):
+        request = _request()
+        persisted = _persisted(agent.id, request)
+        service.schedule_repository.get_by_agent_id_and_name.return_value = None
+        service.schedule_repository.create.return_value = persisted
+        service.authorization_service.grant.side_effect = RuntimeError("authz down")
+
+        with pytest.raises(RuntimeError):
+            await service.create_schedule(agent, request, {"user_id": "u1"})
+
+        # The Spark registration is compensated, the row is removed, and the
+        # Temporal clock is never created if the legacy grant cannot be written.
+        service.authorization_service.register_resource.assert_called_once()
+        service.authorization_service.deregister_resource.assert_called_once()
+        service.authorization_service.revoke.assert_not_called()
+        service.schedule_repository.delete.assert_called_once_with(id=persisted.id)
+        service.temporal_adapter.create_schedule.assert_not_called()
+
+    async def test_create_skips_auth_when_no_creator(self, service, agent):
+        type(service.authorization_service).principal_context = PropertyMock(
+            return_value={}
+        )
+        request = _request()
+        persisted = _persisted(agent.id, request)
+        service.schedule_repository.get_by_agent_id_and_name.return_value = None
+        service.schedule_repository.create.return_value = persisted
+
+        await service.create_schedule(agent, request, {})
+
+        service.authorization_service.register_resource.assert_not_called()
+        service.authorization_service.grant.assert_not_called()
 
 
 @pytest.mark.unit
@@ -255,6 +302,11 @@ async def test_delete_tolerates_missing_temporal_schedule(self, service, agent):
         service.schedule_repository.update.assert_called_once()
         tombstoned = service.schedule_repository.update.call_args.args[0]
         assert tombstoned.deleted_at is not None
+        authz_selector = build_run_schedule_authz_selector(agent.id, row.name)
+        schedule_resource = AgentexResource.schedule(authz_selector)
+        service.authorization_service.revoke.assert_called_once_with(
+            resource=schedule_resource
+        )
         service.authorization_service.deregister_resource.assert_called_once()
 
 
