From d6e615d32484e462dc235de6ba44eab9052f7d47 Mon Sep 17 00:00:00 2001 From: Patrick Buckley Date: Sun, 26 Apr 2026 21:06:25 -0700 Subject: [PATCH] fix: apply /review feedback on legacy URL cleanup MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Reviewer caught real misses on the consumer-swap claim: - TypeScript SDK still defined and re-exported `CloseWorkstreamRequest` (types.ts + index.ts) — drop both. Now matches the Python-side removal. - Four `tests/test_auth.py` cases (`test_write_full_token_ok`, `test_approve_full_token_ok`, `test_bearer_takes_precedence_over_cookie`, `test_cookie_full_on_write_ok`) were tautological after the legacy URL removal: they posted to `/api/send` / `/api/approve` and asserted `allowed is True`, but those paths now classify as `read` so a read token would also pass — they no longer tested the write/approve scope enforcement. Swap to path-keyed URLs to restore the original intent. - `is_public_path("/api/send")` test renamed + retargeted to a path-keyed URL. Doc-table drift the previous commit missed: - `docs/security.md` path-to-scope mapping rewritten for the path-keyed verb family (write set, DELETE-on-/send dequeue, per-ws_id approve). - `docs/architecture.md` scope-model row text swap from `/api/send` / `/api/approve` to the path-keyed equivalents. - `docs/diagrams/01-system-context.puml` channel→server edge label swap. - `docs/diagrams/15-auth-architecture.puml` scope class swap. Cosmetic comment-only stragglers: - `tests/test_session_worker.py` module docstring URL update. - `tests/test_ratelimit.py` ~11 `/api/send` fixture-key strings retargeted to `/api/workstreams/abc/send` so the URL fixtures reflect the post-1.5 surface (rate limiter is path-agnostic; the swap is purely cosmetic). 4557 tests still passing under -m "not live"; ruff + mypy clean. --- docs/architecture.md | 4 ++-- docs/diagrams/01-system-context.puml | 2 +- docs/diagrams/15-auth-architecture.puml | 2 +- docs/security.md | 9 +++++---- sdk/typescript/src/index.ts | 1 - sdk/typescript/src/types.ts | 4 ---- tests/test_auth.py | 12 ++++++------ tests/test_ratelimit.py | 22 +++++++++++----------- tests/test_session_worker.py | 2 +- 9 files changed, 27 insertions(+), 31 deletions(-) diff --git a/docs/architecture.md b/docs/architecture.md index dd1a8840..a17c8be1 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -1100,8 +1100,8 @@ Three hierarchical scopes control endpoint access: | Scope | Grants | Endpoints | |-------|--------|-----------| | `read` | SSE streams, workstream listing, history | GET endpoints | -| `write` | `read` + send, command, workstream create/close | POST to `/api/send`, `/api/command`, etc. | -| `approve` | `write` + tool approval, admin operations | POST to `/api/approve`, `/api/admin/*` | +| `write` | `read` + send, command, workstream create/close | POST to `/api/workstreams/{ws_id}/send`, `/api/command`, etc. | +| `approve` | `write` + tool approval, admin operations | POST to `/api/workstreams/{ws_id}/approve`, `/api/admin/*` | ### Middleware Flow diff --git a/docs/diagrams/01-system-context.puml b/docs/diagrams/01-system-context.puml index 2c039561..e0f8a26f 100644 --- a/docs/diagrams/01-system-context.puml +++ b/docs/diagrams/01-system-context.puml @@ -43,7 +43,7 @@ eval --> sqlite : SQLite console --> server : HTTP proxy\n(hash-ring bucket lookup,\nproxy /node/{id}/* traffic) -channel --> server : HTTP + SSE\n(POST /v1/api/send,\nGET /v1/api/events) +channel --> server : HTTP + SSE\n(POST /v1/api/workstreams/{ws_id}/send,\nGET /v1/api/workstreams/{ws_id}/events) ' Notes note right of console diff --git a/docs/diagrams/15-auth-architecture.puml b/docs/diagrams/15-auth-architecture.puml index 6f5487b1..45e404e8 100644 --- a/docs/diagrams/15-auth-architecture.puml +++ b/docs/diagrams/15-auth-architecture.puml @@ -79,7 +79,7 @@ class "Scope Hierarchy" as SH <> { -- GET → read POST write paths → write - POST /api/approve → approve + POST /api/workstreams/{ws_id}/approve → approve /api/admin/* → approve } diff --git a/docs/security.md b/docs/security.md index fa2c68c6..ba111e93 100644 --- a/docs/security.md +++ b/docs/security.md @@ -67,10 +67,11 @@ Scopes are hierarchical — higher scopes imply all lower ones. | Method | Path pattern | Required scope | |--------|-------------|----------------| | GET | Any protected path | `read` | -| POST | `/api/send`, `/api/plan`, `/api/command` | `write` | -| POST | `/api/workstreams/new`, `/api/workstreams/close` | `write` | -| POST | `/api/cluster/workstreams/new` | `write` | -| POST | `/api/approve` | `approve` | +| POST | `/api/plan`, `/api/command` | `write` | +| POST | `/api/workstreams/new`, `/api/cluster/workstreams/new` | `write` | +| POST | `/api/workstreams/{ws_id}/{send,cancel,close,delete,open,refresh-title,title,attachments}` | `write` | +| DELETE | `/api/workstreams/{ws_id}/send` (dequeue), `/api/workstreams/{ws_id}/attachments/{attachment_id}` | `write` | +| POST | `/api/workstreams/{ws_id}/approve` | `approve` | | Any | `/api/admin/*` | `approve` | Public paths bypass authentication entirely: `/`, `/health`, `/metrics`, diff --git a/sdk/typescript/src/index.ts b/sdk/typescript/src/index.ts index 971ea9a7..81fa800c 100644 --- a/sdk/typescript/src/index.ts +++ b/sdk/typescript/src/index.ts @@ -84,7 +84,6 @@ export type { CommandRequest, CreateWorkstreamRequest, CreateWorkstreamResponse, - CloseWorkstreamRequest, WorkstreamInfo, ListWorkstreamsResponse, DashboardWorkstream, diff --git a/sdk/typescript/src/types.ts b/sdk/typescript/src/types.ts index d22d1b89..fa28313c 100644 --- a/sdk/typescript/src/types.ts +++ b/sdk/typescript/src/types.ts @@ -161,10 +161,6 @@ export interface CreateWorkstreamResponse { attachment_ids?: string[]; } -export interface CloseWorkstreamRequest { - ws_id: string; -} - export interface WorkstreamInfo { // Renamed `id` → `ws_id` and added kind/parent_ws_id/user_id in // the Stage 2 list-verb lift. Pre-1.5 readers branching on diff --git a/tests/test_auth.py b/tests/test_auth.py index 571ee31e..826d4d7b 100644 --- a/tests/test_auth.py +++ b/tests/test_auth.py @@ -53,8 +53,8 @@ class TestIsPublicPath: def test_api_workstreams_not_public(self): assert is_public_path("/api/workstreams") is False - def test_api_send_not_public(self): - assert is_public_path("/api/send") is False + def test_api_workstreams_send_not_public(self): + assert is_public_path("/api/workstreams/abc/send") is False def test_api_cluster_overview_not_public(self): assert is_public_path("/api/cluster/overview") is False @@ -419,7 +419,7 @@ class TestCheckRequest: def test_write_full_token_ok(self, full_jwt): allowed, status, msg, _result = check_request( - "POST", "/api/send", full_jwt, jwt_secret=self._SECRET + "POST", "/api/workstreams/abc/send", full_jwt, jwt_secret=self._SECRET ) assert allowed is True assert status == 200 @@ -520,7 +520,7 @@ class TestCheckRequest: def test_approve_full_token_ok(self, full_jwt): allowed, status, msg, _result = check_request( - "POST", "/api/approve", full_jwt, jwt_secret=self._SECRET + "POST", "/api/workstreams/abc/approve", full_jwt, jwt_secret=self._SECRET ) assert allowed is True @@ -562,7 +562,7 @@ class TestCheckRequestWithCookie: def test_bearer_takes_precedence_over_cookie(self, read_jwt, full_jwt): allowed, status, _, _r = check_request( "POST", - "/api/send", + "/api/workstreams/abc/send", f"Bearer {full_jwt}", cookie_header=f"turnstone_auth={read_jwt}", jwt_secret=self._SECRET, @@ -594,7 +594,7 @@ class TestCheckRequestWithCookie: def test_cookie_full_on_write_ok(self, full_jwt): allowed, status, _, _r = check_request( "POST", - "/api/send", + "/api/workstreams/abc/send", None, cookie_header=f"turnstone_auth={full_jwt}", jwt_secret=self._SECRET, diff --git a/tests/test_ratelimit.py b/tests/test_ratelimit.py index a5849e4b..c0e37d19 100644 --- a/tests/test_ratelimit.py +++ b/tests/test_ratelimit.py @@ -61,15 +61,15 @@ class TestRateLimiter: def test_disabled_allows_everything(self): limiter = RateLimiter(enabled=False, rate=1.0, burst=1) for _ in range(100): - allowed, retry = limiter.check("1.2.3.4", "/api/send") + allowed, retry = limiter.check("1.2.3.4", "/api/workstreams/abc/send") assert allowed is True assert retry == 0.0 def test_exempt_paths_bypass(self): limiter = RateLimiter(enabled=True, rate=1.0, burst=1) # Exhaust the bucket on a normal path - limiter.check("1.2.3.4", "/api/send") - limiter.check("1.2.3.4", "/api/send") + limiter.check("1.2.3.4", "/api/workstreams/abc/send") + limiter.check("1.2.3.4", "/api/workstreams/abc/send") # Exempt paths should still pass allowed, retry = limiter.check("1.2.3.4", "/health") @@ -84,18 +84,18 @@ class TestRateLimiter: limiter = RateLimiter(enabled=True, rate=1.0, burst=1) # Exhaust IP A - allowed_a, _ = limiter.check("10.0.0.1", "/api/send") + allowed_a, _ = limiter.check("10.0.0.1", "/api/workstreams/abc/send") assert allowed_a is True - allowed_a, _ = limiter.check("10.0.0.1", "/api/send") + allowed_a, _ = limiter.check("10.0.0.1", "/api/workstreams/abc/send") assert allowed_a is False # IP B should still have its own bucket - allowed_b, _ = limiter.check("10.0.0.2", "/api/send") + allowed_b, _ = limiter.check("10.0.0.2", "/api/workstreams/abc/send") assert allowed_b is True def test_burst_then_reject(self): limiter = RateLimiter(enabled=True, rate=10.0, burst=3) - results = [limiter.check("1.2.3.4", "/api/send")[0] for _ in range(5)] + results = [limiter.check("1.2.3.4", "/api/workstreams/abc/send")[0] for _ in range(5)] assert results == [True, True, True, False, False] def test_cleanup_removes_stale(self): @@ -104,8 +104,8 @@ class TestRateLimiter: limiter = RateLimiter(enabled=True, rate=10.0, burst=5) # Create buckets for two IPs - limiter.check("10.0.0.1", "/api/send") - limiter.check("10.0.0.2", "/api/send") + limiter.check("10.0.0.1", "/api/workstreams/abc/send") + limiter.check("10.0.0.2", "/api/workstreams/abc/send") # Advance time past max_age for both mock_time.return_value = 5000.0 @@ -120,11 +120,11 @@ class TestRateLimiter: mock_time.return_value = 1000.0 limiter = RateLimiter(enabled=True, rate=10.0, burst=5) - limiter.check("10.0.0.1", "/api/send") + limiter.check("10.0.0.1", "/api/workstreams/abc/send") # Only 60s later — well within max_age mock_time.return_value = 1060.0 - limiter.check("10.0.0.2", "/api/send") + limiter.check("10.0.0.2", "/api/workstreams/abc/send") mock_time.return_value = 1060.0 removed = limiter.cleanup(max_age=3600.0) diff --git a/tests/test_session_worker.py b/tests/test_session_worker.py index 442fec65..7decf3ef 100644 --- a/tests/test_session_worker.py +++ b/tests/test_session_worker.py @@ -1,7 +1,7 @@ """Unit tests for ``turnstone.core.session_worker``. The shared worker dispatch is load-bearing for both the interactive -``/v1/api/send`` HTTP handler and the coordinator +``/v1/api/workstreams/{ws_id}/send`` HTTP handler and the coordinator ``CoordinatorAdapter.send`` path. Tests cover the four invariants the module must hold: