feat: replace 7 read tools with generic api_get tool

forgejo_admin commented

2026-03-31 00:50:35 +00:00

Author

Owner

PR #24 Review

DOMAIN REVIEW

Tech stack: Python / FastAPI / Anthropic SDK / httpx / pytest. Domain checklist applied: Python conventions, OWASP awareness, async patterns.

Architecture assessment: Clean refactor. 7 hand-coded read methods and their corresponding tool definitions are replaced by a single generic api_get tool backed by BasketballClient.get(path). The design is sound -- the generic path approach means new basketball-api endpoints are immediately accessible without code changes.

GET-only enforcement -- verified at two layers:

BasketballClient.get() (basketball.py:134-145) hardcodes "GET" in the call to self._request("GET", path). The method signature only accepts path -- no parameter for HTTP method, no json body parameter. Structurally impossible to override.
_execute_api_get() (ai.py:214-225) exclusively calls client.get(path). No other client method is reachable from this code path.
The dispatcher in _handle_response (ai.py:504-507) routes operation == "read" to _execute_api_get and operation == "write" to the confirmation flow. The only read tool is api_get. Clean separation.

Write tools and confirmation flow -- preserved exactly:

All 7 write tool definitions in TOOLS unchanged (ai.py:76-183)
_execute_write_tool, _build_write_confirmation, _handle_confirmation, _ACTION_DESCRIPTIONS all unchanged
confirmation.py untouched by this PR
Write operation tests in TestWriteOperations untouched

System prompt updated with endpoint examples including new endpoints (/api/jerseys, /api/schedules, /api/billing/tiers) that demonstrate the extensibility benefit.

Test coverage -- thorough:

TestToolDefinitions: 8 tools (1 read + 7 write), schema validation, metadata lookup
TestApiGetExecution: client.get() dispatch, multiple paths, error handling, None result, GET-only assertion
TestGenericGet: HTTP-level verification that GET method is used, no json body, slash prepending, novel endpoints
TestProcessMessage: Two integration tests for api_get through the full process_message pipeline
GET-only enforcement tested at both the ai.py layer (test_api_get_enforces_get_only) and basketball.py layer (test_get_always_uses_get_method)

BLOCKERS

None.

NITS

SSRF hardening on path parameter (ai.py:506, basketball.py:142-144): The slash-prepend logic (if not path.startswith("/"): path = f"/{path}") does not reject protocol-relative URLs. If Claude were to emit path="//evil.com/foo", this starts with / and would pass through. In httpx with a base_url, RFC 3986 URL resolution of //evil.com/foo relative to http://basketball-api:8000 resolves to http://evil.com/foo -- a potential SSRF vector. The risk is low here because (a) the path originates from Claude's tool_use output, not raw user input, and (b) the client runs on an internal network with Keycloak auth headers that would be sent to the wrong host. Still, a defensive check like rejecting paths containing :// or starting with // would be a worthwhile hardening:
```
if path.startswith("//") or "://" in path:
    raise ValueError(f"Invalid path (must be relative): {path}")
```
Minor: test_api_get_enforces_get_only assertion pattern (test_ai.py:236-239): The not hasattr(client, "post") or not client.post.called assertions are weak because MagicMock() auto-creates attributes on access. hasattr will always return True for a MagicMock. The assertions still pass but only because of the or not client.post.called fallback. Consider using spec=BasketballClient on the mock to make the hasattr check meaningful, or just assert client.post.called is False directly.
Minor: hardcoded default path (ai.py:506): tool_input.get("path", "/") falls back to "/" if the path key is missing. Since "path" is "required" in the tool schema, Claude should always provide it. The fallback to root / is safe but could mask a bug. Consider logging a warning when the fallback triggers, or raising an error.

SOP COMPLIANCE

Branch named after issue (23-generic-api-get references issue #23)
PR body follows template (Summary, Changes, Test Plan, Review Checklist, Related)
Related references issue (Closes #23)
No secrets committed
No unnecessary file changes (exactly 4 files, all related to the refactor)
Commit messages are descriptive (inferred from PR title)

PROCESS OBSERVATIONS

Deployment frequency: This PR improves deployment frequency for the AI assistant -- new basketball-api endpoints no longer require code changes to the assistant. Good architectural decision.
Change failure risk: Low. The refactor is a clean 1:1 replacement with comprehensive test coverage at both unit and integration levels. The write path is completely untouched.
Test count: PR body claims 110 tests. Test files show thorough coverage of the new generic path with multiple integration scenarios.
Documentation: System prompt serves as living documentation of available endpoints. The inline docstrings on get() and _execute_api_get() clearly document the GET-only guarantee.

VERDICT: APPROVED

Clean, well-tested refactor. GET-only enforcement is structurally sound at both layers. Write tools and confirmation flow are preserved exactly. The SSRF hardening nit (protocol-relative URL rejection) should be tracked as a follow-up issue but is not a blocker given the threat model (LLM-intermediated path, internal network, auth headers scoped to basketball-api).

## PR #24 Review ### DOMAIN REVIEW **Tech stack**: Python / FastAPI / Anthropic SDK / httpx / pytest. Domain checklist applied: Python conventions, OWASP awareness, async patterns. **Architecture assessment**: Clean refactor. 7 hand-coded read methods and their corresponding tool definitions are replaced by a single generic `api_get` tool backed by `BasketballClient.get(path)`. The design is sound -- the generic path approach means new basketball-api endpoints are immediately accessible without code changes. **GET-only enforcement -- verified at two layers:** 1. `BasketballClient.get()` (basketball.py:134-145) hardcodes `"GET"` in the call to `self._request("GET", path)`. The method signature only accepts `path` -- no parameter for HTTP method, no `json` body parameter. Structurally impossible to override. 2. `_execute_api_get()` (ai.py:214-225) exclusively calls `client.get(path)`. No other client method is reachable from this code path. 3. The dispatcher in `_handle_response` (ai.py:504-507) routes `operation == "read"` to `_execute_api_get` and `operation == "write"` to the confirmation flow. The only read tool is `api_get`. Clean separation. **Write tools and confirmation flow -- preserved exactly:** - All 7 write tool definitions in TOOLS unchanged (ai.py:76-183) - `_execute_write_tool`, `_build_write_confirmation`, `_handle_confirmation`, `_ACTION_DESCRIPTIONS` all unchanged - `confirmation.py` untouched by this PR - Write operation tests in `TestWriteOperations` untouched **System prompt updated** with endpoint examples including new endpoints (/api/jerseys, /api/schedules, /api/billing/tiers) that demonstrate the extensibility benefit. **Test coverage -- thorough:** - `TestToolDefinitions`: 8 tools (1 read + 7 write), schema validation, metadata lookup - `TestApiGetExecution`: client.get() dispatch, multiple paths, error handling, None result, GET-only assertion - `TestGenericGet`: HTTP-level verification that GET method is used, no json body, slash prepending, novel endpoints - `TestProcessMessage`: Two integration tests for api_get through the full process_message pipeline - GET-only enforcement tested at both the ai.py layer (`test_api_get_enforces_get_only`) and basketball.py layer (`test_get_always_uses_get_method`) ### BLOCKERS None. ### NITS 1. **SSRF hardening on path parameter** (ai.py:506, basketball.py:142-144): The slash-prepend logic (`if not path.startswith("/"): path = f"/{path}"`) does not reject protocol-relative URLs. If Claude were to emit `path="//evil.com/foo"`, this starts with `/` and would pass through. In httpx with a `base_url`, RFC 3986 URL resolution of `//evil.com/foo` relative to `http://basketball-api:8000` resolves to `http://evil.com/foo` -- a potential SSRF vector. The risk is low here because (a) the path originates from Claude's tool_use output, not raw user input, and (b) the client runs on an internal network with Keycloak auth headers that would be sent to the wrong host. Still, a defensive check like rejecting paths containing `://` or starting with `//` would be a worthwhile hardening: ```python if path.startswith("//") or "://" in path: raise ValueError(f"Invalid path (must be relative): {path}") ``` 2. **Minor: `test_api_get_enforces_get_only` assertion pattern** (test_ai.py:236-239): The `not hasattr(client, "post") or not client.post.called` assertions are weak because `MagicMock()` auto-creates attributes on access. `hasattr` will always return True for a MagicMock. The assertions still pass but only because of the `or not client.post.called` fallback. Consider using `spec=BasketballClient` on the mock to make the hasattr check meaningful, or just assert `client.post.called is False` directly. 3. **Minor: hardcoded default path** (ai.py:506): `tool_input.get("path", "/")` falls back to `"/"` if the path key is missing. Since `"path"` is `"required"` in the tool schema, Claude should always provide it. The fallback to root `/` is safe but could mask a bug. Consider logging a warning when the fallback triggers, or raising an error. ### SOP COMPLIANCE - [x] Branch named after issue (`23-generic-api-get` references issue #23) - [x] PR body follows template (Summary, Changes, Test Plan, Review Checklist, Related) - [x] Related references issue (`Closes #23`) - [x] No secrets committed - [x] No unnecessary file changes (exactly 4 files, all related to the refactor) - [x] Commit messages are descriptive (inferred from PR title) ### PROCESS OBSERVATIONS - **Deployment frequency**: This PR improves deployment frequency for the AI assistant -- new basketball-api endpoints no longer require code changes to the assistant. Good architectural decision. - **Change failure risk**: Low. The refactor is a clean 1:1 replacement with comprehensive test coverage at both unit and integration levels. The write path is completely untouched. - **Test count**: PR body claims 110 tests. Test files show thorough coverage of the new generic path with multiple integration scenarios. - **Documentation**: System prompt serves as living documentation of available endpoints. The inline docstrings on `get()` and `_execute_api_get()` clearly document the GET-only guarantee. ### VERDICT: APPROVED Clean, well-tested refactor. GET-only enforcement is structurally sound at both layers. Write tools and confirmation flow are preserved exactly. The SSRF hardening nit (protocol-relative URL rejection) should be tracked as a follow-up issue but is not a blocker given the threat model (LLM-intermediated path, internal network, auth headers scoped to basketball-api).

2026-03-31 00:47:41 +00:00

Summary

Replaces 7 hand-coded read tools with a single generic api_get tool that makes GET-only requests to any basketball-api endpoint. Claude picks the endpoint path from system prompt examples. New endpoints (jerseys, schedules, billing) work automatically without code changes.

Changes

app/ai.py — Replaced 7 read tool definitions with 1 api_get tool (accepts path parameter). Removed _execute_read_tool dispatcher, added _execute_api_get that delegates to client.get(path). Updated SYSTEM_PROMPT with example endpoint list. Updated _handle_response to extract path from tool input. All 7 write tools + confirmation flow unchanged.
app/basketball.py — Replaced 7 individual read methods with a single get(path) method. HTTP method hardcoded to GET at code level. No request body ever sent. Prepends / if missing.
tests/test_ai.py — Updated tool definition tests (8 tools: 1 read + 7 write). Replaced TestReadToolExecution with TestApiGetExecution. Added integration tests for api_get with dynamic paths. Added GET-only enforcement test.
tests/test_basketball.py — Replaced TestReadOperations with TestGenericGet. Added tests for novel endpoints, slash prepending, and GET-method-only enforcement. Updated token/error tests to use client.get().

Test Plan

pytest tests/ — all 110 tests pass
pytest tests/ -k test_api_get — targeted api_get tests pass
Verify write tools + confirmation flow unchanged (existing confirmation tests pass)
Verify GET-only enforcement at both _execute_api_get and BasketballClient.get() levels

Review Checklist

No unrelated changes
All 110 tests pass
Write tools and confirmation flow unchanged
GET-only enforced at code level (hardcoded in BasketballClient.get() and _execute_api_get)
System prompt updated with example endpoints
No new dependencies

Closes #23
basketball-api #257 (jersey fields) — once merged, Nemo auto-gains access via api_get

## Summary Replaces 7 hand-coded read tools with a single generic `api_get` tool that makes GET-only requests to any basketball-api endpoint. Claude picks the endpoint path from system prompt examples. New endpoints (jerseys, schedules, billing) work automatically without code changes. ## Changes - `app/ai.py` — Replaced 7 read tool definitions with 1 `api_get` tool (accepts `path` parameter). Removed `_execute_read_tool` dispatcher, added `_execute_api_get` that delegates to `client.get(path)`. Updated `SYSTEM_PROMPT` with example endpoint list. Updated `_handle_response` to extract `path` from tool input. All 7 write tools + confirmation flow unchanged. - `app/basketball.py` — Replaced 7 individual read methods with a single `get(path)` method. HTTP method hardcoded to `GET` at code level. No request body ever sent. Prepends `/` if missing. - `tests/test_ai.py` — Updated tool definition tests (8 tools: 1 read + 7 write). Replaced `TestReadToolExecution` with `TestApiGetExecution`. Added integration tests for `api_get` with dynamic paths. Added GET-only enforcement test. - `tests/test_basketball.py` — Replaced `TestReadOperations` with `TestGenericGet`. Added tests for novel endpoints, slash prepending, and GET-method-only enforcement. Updated token/error tests to use `client.get()`. ## Test Plan - `pytest tests/` — all 110 tests pass - `pytest tests/ -k test_api_get` — targeted api_get tests pass - Verify write tools + confirmation flow unchanged (existing confirmation tests pass) - Verify GET-only enforcement at both `_execute_api_get` and `BasketballClient.get()` levels ## Review Checklist - [x] No unrelated changes - [x] All 110 tests pass - [x] Write tools and confirmation flow unchanged - [x] GET-only enforced at code level (hardcoded in `BasketballClient.get()` and `_execute_api_get`) - [x] System prompt updated with example endpoints - [x] No new dependencies ## Related Notes - Closes #23 - basketball-api #257 (jersey fields) — once merged, Nemo auto-gains access via api_get

forgejo_admin added 1 commit

feat: replace 7 read tools with generic api_get tool

ci/woodpecker/pr/woodpecker Pipeline was successful

Details

ecbb449397

Single api_get tool accepts any basketball-api path and enforces
GET-only at the code level. Claude picks the endpoint from the
system prompt examples. New endpoints (jerseys, schedules, billing)
work automatically without code changes. All 7 write tools and
confirmation flow unchanged.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>