Add full Lanis MCP server - #5
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (8)
lanis_mcp/client.py (3)
107-112: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
download_filedoes not require a token, unlikerequest.
requestcallsresolve_tokenand raises a clearValueErrorwhen no token exists.download_filesends the request without the header instead. The caller then receives an opaque 401, andlanis_download_course_fileinlanis_mcp/server.pytries a token refresh for a case that never had a token. Callresolve_tokenhere for consistent behavior.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lanis_mcp/client.py` around lines 107 - 112, Update download_file to resolve the access token through the existing resolve_token method before constructing the request headers, preserving the optional access_token override and clear ValueError behavior when no token is available. Use the resolved token for X-Session-Token instead of silently sending an unauthenticated request.
134-137: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winSanitize the parsed filename.
The parser accepts any value the server sends. A response with
filename="../../etc/passwd"returns that string unchanged. A consumer that writes the file to disk then writes outside the target directory. The parser also ignores the RFC 5987filename*form, so UTF-8 names fall back to"download".Reduce the value to its base name and reject empty results.
♻️ Proposed fix
disposition = response.headers.get("content-disposition", "") filename = "download" if "filename=" in disposition: - filename = disposition.split("filename=", 1)[1].strip().strip('"') + candidate = disposition.split("filename=", 1)[1].strip().strip('"') + candidate = os.path.basename(candidate.replace("\\", "/")).strip() + if candidate not in ("", ".", ".."): + filename = candidate🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lanis_mcp/client.py` around lines 134 - 137, Update the filename parsing around the response content-disposition handling to support RFC 5987 filename* values, sanitize the selected filename to its base name, and fall back to "download" when sanitization produces an empty result. Preserve the existing default for missing or unusable filename parameters.
68-73: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winReuse one
httpx.AsyncClientinstead of creating one per request.
requestanddownload_fileeach build and close a newAsyncClient. This discards connection pooling and forces a new TCP and TLS handshake for every tool call. The server also constructs a newLanisClientper call (_client()inlanis_mcp/server.pyline 161), so no connection is ever reused.Hold one lazily created client on the instance and close it explicitly, or expose the class as an async context manager.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lanis_mcp/client.py` around lines 68 - 73, Update the LanisClient request and download_file flows to reuse one lazily initialized httpx.AsyncClient stored on the instance instead of creating and closing a client per call. Add explicit async cleanup or async-context-manager support, and update the server’s _client() lifecycle so the same LanisClient and underlying client are reused across tool calls and closed when the server shuts down.tests/test_lanis_mcp_client.py (2)
12-76: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSilence the Ruff S105 and S106 hits with a per-file ignore.
Ruff reports hardcoded-password findings for the test tokens at lines 32, 44, 73, 107, and 109. These are fixtures, not real secrets. Add a per-file ignore in
pyproject.tomlso the lint stage stays green without weakening the rule for production code.♻️ Proposed Ruff configuration
[tool.ruff.lint.per-file-ignores] "tests/**" = ["S105", "S106"]🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_lanis_mcp_client.py` around lines 12 - 76, Add a Ruff per-file ignore in the existing pyproject.toml lint configuration for tests/** covering S105 and S106, while leaving these security rules enabled for production code.Source: Linters/SAST tools
113-154: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a test for the 401 refresh-and-retry path.
The tests cover login, session isolation, and token reuse. They do not cover
_api_requestinlanis_mcp/server.pylines 242-257, which catchesLanisAPIError, refreshes the token once, and retries. That branch carries the highest risk in this PR: a wrong retry can repeat a mutating request.Extend
FakeClientto raiseLanisAPIError(401, ...)on the first authenticated call. Then assert that/auth/refreshis called exactly once and that the retry uses the new access token.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_lanis_mcp_client.py` around lines 113 - 154, Extend test_login_token_is_reused_inside_the_same_mcp_session with a FakeClient 401 response on the first authenticated request, track /auth/refresh calls, and return a new access token from the refresh response. Assert the refresh endpoint is called exactly once and the retried request uses the refreshed token, while preserving the existing login and successful tool-call assertions.lanis_mcp/server.py (2)
838-844: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMark the
ssetransport as legacy, or remove the option.The two-endpoint HTTP+SSE transport is deprecated. The two-endpoint HTTP+SSE transport has been deprecated since the 2025-03-26 spec revision; Streamable HTTP (a single endpoint, usually /mcp) replaced it. Documentation for the newer transport also states "SSE exists only for backward compatibility and shouldn't be used in new projects."
Keep
sseonly if an old client requires it, and state that in the argument help text so operators do not select it by default.♻️ Proposed change
parser.add_argument( "--transport", choices=("stdio", "streamable-http", "sse"), default="stdio", + help=( + "Transport to serve. Use stdio for local clients and streamable-http " + "for remote clients. sse is deprecated and kept only for legacy clients." + ), )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lanis_mcp/server.py` around lines 838 - 844, Update the transport argument definition near mcp.run to remove the deprecated sse option, or retain it only with help text explicitly marking it as legacy and for backward compatibility. Keep streamable-http as the preferred HTTP transport and preserve the existing default of stdio.
643-657: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReuse one retry helper for the download path.
This block duplicates the 401-refresh flow of
_api_request. The two copies will drift. The token is also passed twice, once to_client()and once todownload_file().Extract a helper that takes a callable and applies the single refresh-and-retry policy, then use it here and in
_api_request.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lanis_mcp/server.py` around lines 643 - 657, Extract the shared 401 refresh-and-retry logic from _api_request into a helper that accepts the API callable, and update both _api_request and the download path to use it. In the helper, reuse refreshed credentials for the retry and avoid passing the access token redundantly to both _client() and download_file().requirements.txt (1)
14-15: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winKeep the MCP extra optional in the API-server requirements path.
README.mdpresentsrequirements.txtas the API-server install, but these lines install MCP dependencies for every API-server environment. Move them to an MCP-specific requirements file, or document this file as a full-stack installation.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@requirements.txt` around lines 14 - 15, Remove the MCP dependency entries httpx and mcp from the API-server requirements represented by requirements.txt, and place them in an MCP-specific requirements file or otherwise make the MCP installation optional. Keep requirements.txt limited to dependencies needed for the API server, unless README.md is updated to clearly describe it as a full-stack installation.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@lanis_mcp/client.py`:
- Around line 119-132: Update the download logic in the client method containing
the shown response handling to stream the response body instead of using the
fully buffered client.get result. Accumulate chunks only while max_bytes
permits, abort as soon as the limit is exceeded, and preserve HTTP error
handling; then construct the result from the collected content, content_type,
and disposition.
- Around line 64-73: Update the request flow around the AsyncClient in the
client method to prevent X-Session-Token from being forwarded to redirect
targets whose host differs from base_url. Either implement manual redirect
handling that removes the header before cross-host requests, or configure
equivalent host-aware redirect behavior while preserving authenticated same-host
redirects and existing request functionality.
In `@lanis_mcp/server.py`:
- Around line 211-218: Add an asyncio.Lock field to SessionCredentials and
serialize the refresh request and credential updates under it, rechecking or
preserving the current token state for concurrent callers. In the refresh
handling around new_access_token, update credentials.refresh_token from result’s
refresh_token when it is present, falling back to the request token only when
absent; keep access-token validation unchanged.
- Around line 102-135: Update CredentialStore to avoid sharing the process-wide
_fallback for non-stdio transports: add an allow_shared_fallback configuration,
pass it from main() as true only for stdio and false for other transports, and
have get() return a new empty SessionCredentials when session resolution fails
and sharing is disabled. Preserve the existing _fallback behavior for stdio.
In `@pyproject.toml`:
- Around line 35-37: Align package discovery with the documented skill link by
removing skills* from the exclude list so skills/use-lanis-api is included in
the distribution, or update the README.md and generated PKG-INFO links to use an
absolute repository or hosted-documentation URL instead.
- Around line 21-27: Make the lanis-mcp entry point conditional on the mcp
optional dependency instead of exposing it from the unconditional
project.scripts metadata; place it in the dependency group that installs both
mcp and httpx, or otherwise ensure the entry point cannot load without those
dependencies and reports a clear unavailable-dependency error.
In `@README.md`:
- Line 14: Correct the spelling errors in the changed README text: replace
“straigth” with “straight” and “theire” with “their” at both referenced
occurrences, without changing the surrounding wording.
- Around line 95-98: Update the README section describing the default
`https://lanis-backend.joancode.dev` backend to identify its operator, disclose
what account and school data it retains, and explain how users can self-host or
force a local backend. If remote processing is not explicitly opt-in, change the
documented default to local execution and clearly describe how to opt into the
hosted service.
- Around line 100-101: Update the MCP installation instructions in README.md to
include a PyPI-compatible command that installs the published sph-client package
with its mcp extra, while retaining the existing editable/source installation
command if it serves local development.
---
Nitpick comments:
In `@lanis_mcp/client.py`:
- Around line 107-112: Update download_file to resolve the access token through
the existing resolve_token method before constructing the request headers,
preserving the optional access_token override and clear ValueError behavior when
no token is available. Use the resolved token for X-Session-Token instead of
silently sending an unauthenticated request.
- Around line 134-137: Update the filename parsing around the response
content-disposition handling to support RFC 5987 filename* values, sanitize the
selected filename to its base name, and fall back to "download" when
sanitization produces an empty result. Preserve the existing default for missing
or unusable filename parameters.
- Around line 68-73: Update the LanisClient request and download_file flows to
reuse one lazily initialized httpx.AsyncClient stored on the instance instead of
creating and closing a client per call. Add explicit async cleanup or
async-context-manager support, and update the server’s _client() lifecycle so
the same LanisClient and underlying client are reused across tool calls and
closed when the server shuts down.
In `@lanis_mcp/server.py`:
- Around line 838-844: Update the transport argument definition near mcp.run to
remove the deprecated sse option, or retain it only with help text explicitly
marking it as legacy and for backward compatibility. Keep streamable-http as the
preferred HTTP transport and preserve the existing default of stdio.
- Around line 643-657: Extract the shared 401 refresh-and-retry logic from
_api_request into a helper that accepts the API callable, and update both
_api_request and the download path to use it. In the helper, reuse refreshed
credentials for the retry and avoid passing the access token redundantly to both
_client() and download_file().
In `@requirements.txt`:
- Around line 14-15: Remove the MCP dependency entries httpx and mcp from the
API-server requirements represented by requirements.txt, and place them in an
MCP-specific requirements file or otherwise make the MCP installation optional.
Keep requirements.txt limited to dependencies needed for the API server, unless
README.md is updated to clearly describe it as a full-stack installation.
In `@tests/test_lanis_mcp_client.py`:
- Around line 12-76: Add a Ruff per-file ignore in the existing pyproject.toml
lint configuration for tests/** covering S105 and S106, while leaving these
security rules enabled for production code.
- Around line 113-154: Extend
test_login_token_is_reused_inside_the_same_mcp_session with a FakeClient 401
response on the first authenticated request, track /auth/refresh calls, and
return a new access token from the refresh response. Assert the refresh endpoint
is called exactly once and the retried request uses the refreshed token, while
preserving the existing login and successful tool-call assertions.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 1ecdc340-30fe-43d9-9f1b-07c264cc3273
📒 Files selected for processing (14)
README.mdchatgpt-app-submission.jsonlanis_mcp/__init__.pylanis_mcp/__main__.pylanis_mcp/client.pylanis_mcp/server.pypyproject.tomlrequirements.txtsph_client.egg-info/PKG-INFOsph_client.egg-info/SOURCES.txtsph_client.egg-info/entry_points.txtsph_client.egg-info/requires.txtsph_client.egg-info/top_level.txttests/test_lanis_mcp_client.py
| headers: dict[str, str] = {"Accept": "application/json"} | ||
| if authenticated: | ||
| headers["X-Session-Token"] = self.resolve_token(access_token) | ||
|
|
||
| async with httpx.AsyncClient( | ||
| base_url=self.base_url, | ||
| timeout=self.timeout, | ||
| follow_redirects=True, | ||
| transport=self.transport, | ||
| ) as client: |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
httpx 0.27 redirect custom headers stripped cross-origin Authorization
💡 Result:
In httpx (including version 0.27 and later), when follow_redirects is enabled, the library automatically strips the Authorization header (along with the Cookie header) when a redirect involves a cross-origin transition [1][2][3]. This is a deliberate security feature designed to prevent the accidental leakage of sensitive credentials to a different host, scheme, or port [2][4][5]. Key behaviors include: - Cross-Origin Stripping: If the redirect target has a different origin (host, scheme, or port) than the original request, the Authorization header is removed from the subsequent redirected request [2][3]. - Same-Origin Persistence: If the redirect remains within the same origin, the Authorization header is preserved [6][4]. - Custom Headers: Httpx only explicitly strips standard sensitive headers like Authorization and Cookie [5][3]. Any custom authentication headers (e.g., X-API-Key, Authorization-Token) are not automatically stripped by httpx during cross-origin redirects, which may lead to credential exposure if those custom headers are used [5]. If you need to maintain Authorization headers across cross-origin redirects, you must handle the redirection manually by setting follow_redirects=False and performing the subsequent requests explicitly using the same client, which ensures you can control the header headers applied to each step of the redirect chain [1]. For security, it is highly recommended to only follow redirects to trusted domains if you are handling sensitive authentication information [4].
Citations:
- 1: follow_redirects dropping Bearer token header encode/httpx#3291
- 2: https://deepwiki.com/encode/httpx/3.4-redirects-and-history
- 3: https://github.com/encode/httpx/blob/4f8068a7ade1f4d10403e325ed2354109df1724f/httpx/_client.py
- 4: https://github.com/andrewstellman/quality-playbook/blob/main/repos/docs_gathered/httpx/07_AUTHENTICATION.md
- 5: Azure
api-keyheader is forwarded across cross-origin redirects (httpx stripsAuthorizationbut not custom auth headers) openai/openai-python#3516 - 6: https://github.com/encode/httpx/blob/b5addb64/tests/client/test_redirects.py
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Tracked files matching client.py:\n'
fd -a 'client\.py$' . | sed 's#^\./##'
printf '\nlanis_mcp/client.py excerpt:\n'
if [ -f lanis_mcp/client.py ]; then
nl -ba lanis_mcp/client.py | sed -n '1,140p'
else
fd -a 'client\.py$' .
fi
printf '\nSearch for X-Session-Token and base_url handling:\n'
rg -n "X-Session-Token|base_url|resolve_token|LANIS_API_BASE_URL|follow_redirects" -S .Repository: joan-code6/lanis_api
Length of output: 351
🌐 Web query:
httpx follow_redirects behavior preserves custom headers Authorization stripped source
💡 Result:
In HTTPX, the Authorization header is automatically stripped when a request follows a redirect to a different origin [1][2][3]. This is a security measure implemented to prevent the accidental leakage of sensitive credentials across different domains [4][2]. Specifically, the library performs the following check during a redirect: 1. Same-Origin Check: If the redirect target is on the same origin as the original request, the Authorization header is preserved [1][5]. 2. Cross-Origin Check: If the redirect is to a different origin, the Authorization header is removed [1][3]. There is a notable exception: if the redirect is an "upgrade" from an HTTP request to an HTTPS request (specifically when using default ports 80 and 443), the Authorization header is preserved even if the domain changes [1][6]. If you require the Authorization header to be sent across different origins during a redirect, you must disable automatic redirects (set follow_redirects=False) and manually handle the redirected request yourself [4]. By manually following the redirect with the same client instance, you can ensure your headers are included in the subsequent request [4].
Citations:
- 1: https://github.com/encode/httpx/blob/master/httpx/_client.py
- 2: https://deepwiki.com/encode/httpx/3.4-redirects-and-history
- 3: https://github.com/encode/httpx/blob/71ee50b27770b461a5d2aaba9fca1fbc261bede1/httpx/_client.py
- 4: follow_redirects dropping Bearer token header encode/httpx#3291
- 5: https://github.com/encode/httpx/blob/b5addb64/tests/client/test_redirects.py
- 6: Preserve Authorization header on HTTPS redirect (#1850) encode/httpx#2074
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'lanis_mcp/client.py lines 1-160:\n'
awk '{printf "%6d: %s\n", NR, $0}' lanis_mcp/client.py | sed -n '1,160p'
printf '\nRelevant searches:\n'
rg -n "X-Session-Token|base_url|resolve_token|LANIS_API_BASE_URL|follow_redirects|AsyncClient|transport" lanis_mcp/client.py tests/test_lanis_mcp_client.py 2>/dev/null || trueRepository: joan-code6/lanis_api
Length of output: 8445
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Dependency/version references:\n'
git ls-files | rg '(^|/)(pyproject\.toml|requirements.*\.txt|poetry\.lock|uv\.lock|Pipfile\.lock|.*requirements.*\.txt)$' || true
for f in $(git ls-files | rg '(^|/)(pyproject\.toml|requirements.*\.txt|poetry\.lock|uv\.lock|Pipfile\.lock|.*requirements.*\.txt)$' 2>/dev/null); do
printf '\n--- %s ---\n' "$f"
rg -n "httpx" "$f" || true
done
printf '\nInspect test redirect/auth coverage:\n'
awk '{printf "%6d: %s\n", NR, $0}' tests/test_lanis_mcp_client.py | sed -n '1,120p'
printf '\nInspect dependency constraints:\n'
rg -n "httpx" pyproject.toml requirements.txt requirements*.txt poetry.lock uv.lock 2>/dev/null || trueRepository: joan-code6/lanis_api
Length of output: 5441
Send X-Session-Token only to redirects where the host matches base_url.
httpx strips Authorization on cross-origin redirects, but X-Session-Token is a custom credential header and is forwarded when the redirect target has a different host. This path runs with follow_redirects=True, and base_url can come from LANIS_API_BASE_URL, so a redirect can send the session token to an untrusted origin. Use manual redirect handling, or drop the token for cross-host redirects.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@lanis_mcp/client.py` around lines 64 - 73, Update the request flow around the
AsyncClient in the client method to prevent X-Session-Token from being forwarded
to redirect targets whose host differs from base_url. Either implement manual
redirect handling that removes the header before cross-host requests, or
configure equivalent host-aware redirect behavior while preserving authenticated
same-host redirects and existing request functionality.
| response = await client.get(path, headers=headers) | ||
|
|
||
| if response.is_error: | ||
| try: | ||
| detail = response.json().get("detail", response.text) | ||
| except ValueError: | ||
| detail = response.text[:1000] | ||
| raise LanisAPIError(response.status_code, "GET", path, str(detail)) | ||
| if max_bytes is not None and max_bytes < 1: | ||
| raise ValueError("max_bytes must be positive or null") | ||
| if max_bytes is not None and len(response.content) > max_bytes: | ||
| raise ValueError( | ||
| f"File is {len(response.content)} bytes, exceeding max_bytes={max_bytes}." | ||
| ) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
max_bytes does not limit memory use because the body is fully buffered first.
client.get reads the complete response into memory. The size check at line 129 runs after that. A large course file therefore causes the same memory pressure with or without max_bytes, and the base64 encoding at line 144 adds about one third more. Callers cannot bound the download.
Stream the response and stop as soon as the limit is exceeded.
🛡️ Proposed fix using streaming with an early abort
- path = f"/meinunterricht/file/{quote(file_hash, safe='')}"
- async with httpx.AsyncClient(
- base_url=self.base_url,
- timeout=self.timeout,
- follow_redirects=True,
- transport=self.transport,
- ) as client:
- response = await client.get(path, headers=headers)
-
- if response.is_error:
- try:
- detail = response.json().get("detail", response.text)
- except ValueError:
- detail = response.text[:1000]
- raise LanisAPIError(response.status_code, "GET", path, str(detail))
- if max_bytes is not None and max_bytes < 1:
- raise ValueError("max_bytes must be positive or null")
- if max_bytes is not None and len(response.content) > max_bytes:
- raise ValueError(
- f"File is {len(response.content)} bytes, exceeding max_bytes={max_bytes}."
- )
+ if max_bytes is not None and max_bytes < 1:
+ raise ValueError("max_bytes must be positive or null")
+
+ path = f"/meinunterricht/file/{quote(file_hash, safe='')}"
+ async with httpx.AsyncClient(
+ base_url=self.base_url,
+ timeout=self.timeout,
+ follow_redirects=True,
+ transport=self.transport,
+ ) as client:
+ async with client.stream("GET", path, headers=headers) as response:
+ if response.is_error:
+ await response.aread()
+ try:
+ detail = response.json().get("detail", response.text)
+ except ValueError:
+ detail = response.text[:1000]
+ raise LanisAPIError(
+ response.status_code, "GET", path, str(detail)
+ )
+ chunks: list[bytes] = []
+ size = 0
+ async for chunk in response.aiter_bytes():
+ size += len(chunk)
+ if max_bytes is not None and size > max_bytes:
+ raise ValueError(
+ f"File exceeds max_bytes={max_bytes}."
+ )
+ chunks.append(chunk)
+ content = b"".join(chunks)
+ disposition = response.headers.get("content-disposition", "")
+ content_type = response.headers.get(
+ "content-type", "application/octet-stream"
+ )Then build the result from content, content_type, and disposition.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@lanis_mcp/client.py` around lines 119 - 132, Update the download logic in the
client method containing the shown response handling to stream the response body
instead of using the fully buffered client.get result. Accumulate chunks only
while max_bytes permits, abort as soon as the limit is exceeded, and preserve
HTTP error handling; then construct the result from the collected content,
content_type, and disposition.
| class CredentialStore: | ||
| """Keep login state isolated between stateful MCP client sessions.""" | ||
|
|
||
| def __init__(self) -> None: | ||
| self._sessions: WeakKeyDictionary[object, SessionCredentials] = ( | ||
| WeakKeyDictionary() | ||
| ) | ||
| self._fallback = self._new_credentials() | ||
|
|
||
| @staticmethod | ||
| def _new_credentials() -> SessionCredentials: | ||
| return SessionCredentials( | ||
| access_token=os.getenv("LANIS_ACCESS_TOKEN"), | ||
| refresh_token=os.getenv("LANIS_REFRESH_TOKEN"), | ||
| ) | ||
|
|
||
| def get(self, ctx: Context) -> SessionCredentials: | ||
| try: | ||
| session = ctx.session | ||
| credentials = self._sessions.get(session) | ||
| if credentials is None: | ||
| credentials = self._new_credentials() | ||
| self._sessions[session] = credentials | ||
| return credentials | ||
| except (TypeError, ValueError): | ||
| return self._fallback | ||
|
|
||
| def clear(self, ctx: Context) -> None: | ||
| credentials = self.get(ctx) | ||
| credentials.access_token = None | ||
| credentials.refresh_token = None | ||
|
|
||
|
|
||
| _credentials = CredentialStore() |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Do not share one fallback credential object across sessions.
_fallback is a single process-wide SessionCredentials. get() returns it whenever ctx.session access raises TypeError or ValueError. Under the streamable-http transport, one process serves many users. If session resolution fails, user A's access token and refresh token are then used for user B's request.
Restrict the process-wide fallback to stdio, and return empty credentials for HTTP transports.
🔒 Proposed change
class CredentialStore:
"""Keep login state isolated between stateful MCP client sessions."""
- def __init__(self) -> None:
+ def __init__(self, *, allow_shared_fallback: bool = True) -> None:
self._sessions: WeakKeyDictionary[object, SessionCredentials] = (
WeakKeyDictionary()
)
- self._fallback = self._new_credentials()
+ self._allow_shared_fallback = allow_shared_fallback
+ self._fallback = self._new_credentials()
@@
- except (TypeError, ValueError):
- return self._fallback
+ except (TypeError, ValueError):
+ if self._allow_shared_fallback:
+ return self._fallback
+ return SessionCredentials()Set allow_shared_fallback=False in main() when the selected transport is not stdio.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@lanis_mcp/server.py` around lines 102 - 135, Update CredentialStore to avoid
sharing the process-wide _fallback for non-stdio transports: add an
allow_shared_fallback configuration, pass it from main() as true only for stdio
and false for other transports, and have get() return a new empty
SessionCredentials when session resolution fails and sharing is disabled.
Preserve the existing _fallback behavior for stdio.
| new_access_token = result.get("access_token") | ||
| if not isinstance(new_access_token, str) or not new_access_token: | ||
| raise TypeError( | ||
| "The LANIS API refresh response did not contain an access token" | ||
| ) | ||
| credentials.access_token = new_access_token | ||
| credentials.refresh_token = token | ||
| return result |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Serialize refresh calls and keep a rotated refresh token.
Two concerns exist in this block:
- Concurrent tool calls in one session can both receive 401 and both call
POST /auth/refreshwith the same refresh token. If the API rotates refresh tokens, the second call fails and the session loses authentication. - Line 217 re-stores the request token. If the response contains a new
refresh_token, that value is discarded.
Add an asyncio.Lock to SessionCredentials and refresh under that lock. Store the response refresh_token when the API returns one.
♻️ Proposed change for the token handling
credentials.access_token = new_access_token
- credentials.refresh_token = token
+ rotated = result.get("refresh_token")
+ credentials.refresh_token = rotated if isinstance(rotated, str) and rotated else token
return result🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@lanis_mcp/server.py` around lines 211 - 218, Add an asyncio.Lock field to
SessionCredentials and serialize the refresh request and credential updates
under it, rechecking or preserving the current token state for concurrent
callers. In the refresh handling around new_access_token, update
credentials.refresh_token from result’s refresh_token when it is present,
falling back to the request token only when absent; keep access-token validation
unchanged.
| [project.scripts] | ||
| lanis-mcp = "lanis_mcp.server:main" | ||
|
|
||
| [project.optional-dependencies] | ||
| mcp = [ | ||
| "httpx>=0.27.0", | ||
| "mcp>=1.27,<2", |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -nP '^\s*(from|import)\s+(mcp|httpx)\b' lanis_mcp
rg -n 'lanis-mcp|optional-dependencies' pyproject.tomlRepository: joan-code6/lanis_api
Length of output: 400
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== files =="
git ls-files | sed -n '1,120p'
echo "== lanis_mcp/server.py outline =="
ast-grep outline lanis_mcp/server.py || true
echo "== lanis_mcp/server.py =="
cat -n lanis_mcp/server.py
echo "== pyproject relevant =="
cat -n pyproject.toml | sed -n '1,80p'
echo "== behavioral parse/import probe =="
python3 - <<'PY'
from pathlib import Path
import ast
for path in ["lanis_mcp/server.py", "lanis_mcp/client.py"]:
text = Path(path).read_text()
tree = ast.parse(text)
imports = []
for node in ast.walk(tree):
if isinstance(node, ast.Import):
for alias in node.names:
imports.append((node.lineno, "import", alias.name))
elif isinstance(node, ast.ImportFrom):
module = node.module or ""
for alias in node.names:
imports.append((node.lineno, "from", module, alias.name))
print(f"== {path} ==")
for item in imports:
print(item)
print("module-level mcp/httpx imports:", [item for item in imports if any(part in {"mcp","httpx"} for part in item)])
PYRepository: joan-code6/lanis_api
Length of output: 45684
Fix the unconditional lanis-mcp entry point.
lanis-mcp = "lanis_mcp.server:main" is created for every pip install sph-client, but mcp and httpx are only in the optional mcp extra. lanis_mcp.server imports mcp at module load, and lanis_mcp.client imports httpx at module load, so a base install cannot load the CLI without installing the extra. Keep the CLI in metadata that requires mcp, or use lazy imports with a clear unavailable-dependency error.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pyproject.toml` around lines 21 - 27, Make the lanis-mcp entry point
conditional on the mcp optional dependency instead of exposing it from the
unconditional project.scripts metadata; place it in the dependency group that
installs both mcp and httpx, or otherwise ensure the entry point cannot load
without those dependencies and reports a clear unavailable-dependency error.
| [tool.setuptools.packages.find] | ||
| include = ["api*", "lanis_mcp*", "schulportal_hessen*", "sph_client*"] | ||
| exclude = ["docs*", "scripts*", "skills*", "tests*", "tui*"] |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Keep the package exclusion aligned with the documented skill link.
skills* is excluded from package discovery, but README.md and generated PKG-INFO link to skills/use-lanis-api. sph_client.egg-info/SOURCES.txt contains no corresponding skill file. Include the skill in the distribution or use an absolute repository or hosted-documentation link.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pyproject.toml` around lines 35 - 37, Align package discovery with the
documented skill link by removing skills* from the exclude list so
skills/use-lanis-api is included in the distribution, or update the README.md
and generated PKG-INFO links to use an absolute repository or
hosted-documentation URL instead.
| I provide a hosted server of this for anyone to use at | ||
| [https://lanis-backend.joancode.dev/](https://lanis-backend.joancode.dev/) | ||
| If you wanna jump straigth in and not read too much code you can just have a look at the [api documentation](https://lanis-backend.joancode.dev/documentation) where you can see all endpoints and how to use them. | ||
| If you wanna jump straigth in and not read too much code you can just have a look at the [api documentation](https://lanis-backend.joancode.dev/documentation) where you can see all endpoints and how to use them. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Fix the spelling errors in the changed README text.
Use straight instead of straigth, and their instead of theire.
Proposed wording fix
-... jump straigth in ...
+... jump straight in ...
-... names to theire ids ...
+... names to their IDs ...Also applies to: 39-39
🧰 Tools
🪛 LanguageTool
[grammar] ~14-~14: Ensure spelling is correct
Context: ...ackend.joancode.dev/) If you wanna jump straigth in and not read too much code you can j...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[locale-violation] ~14-~14: In American English, “take a look” is more commonly used.
Context: ...and not read too much code you can just have a look at the [api documentation](https://lani...
(HAVE_A_LOOK)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@README.md` at line 14, Correct the spelling errors in the changed README
text: replace “straigth” with “straight” and “theire” with “their” at both
referenced occurrences, without changing the surrounding wording.
Source: Linters/SAST tools
| Lanis lets an AI assistant interact with the user's Schulportal Hessen account through typed MCP | ||
| tools. Users do not need to know the LANIS REST API or pass an access token to every tool. The | ||
| server covers every user-facing REST operation and defaults to | ||
| `https://lanis-backend.joancode.dev`. |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Document the hosted-server trust boundary before using it as the default.
The documentation makes a hosted deployment the default and describes login, access-token, and refresh-token handling. A local lanis-mcp process can therefore send account and school data to that hosted service. State who operates the service, what data it retains, and how users can self-host or force a local backend. Change the default if remote processing is not an explicit opt-in.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@README.md` around lines 95 - 98, Update the README section describing the
default `https://lanis-backend.joancode.dev` backend to identify its operator,
disclose what account and school data it retains, and explain how users can
self-host or force a local backend. If remote processing is not explicitly
opt-in, change the documented default to local execution and clearly describe
how to opt into the hosted service.
| ```bash | ||
| pip install -e '.[mcp]' |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Files named README.md:\n'
fd -a '^README\.md$' . | sed 's#^\./##'
printf '\nREADME lines 80-115:\n'
READMES=$(fd '^README\.md$' .)
if [ -n "$READMES" ]; then
for f in $READMES; do
echo "--- $f"
sed -n '80,115p' "$f" | nl -ba -v80
done
fi
printf '\nPackage metadata candidates:\n'
fd '(^setup\.py$|setup\.cfg|pyproject\.toml|PKG-INFO$|pyproject\.toml|.*egg-info/.*PKG-INFO)' . | sed 's#^\./##' | sort
printf '\nSearch sph-client/mcp/pip install -e:\n'
rg -n "pip install -e|sph-client|mcp|Install|installation" pyproject.toml setup.py setup.cfg README.md . 2>/dev/null | head -n 200Repository: joan-code6/lanis_api
Length of output: 322
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'README.md lines 80-115:\n'
sed -n '80,115p' README.md | awk '{printf "%5d: %s\n", NR+79, $0}'
printf '\nPackage metadata candidates:\n'
find . \( -name 'pyproject.toml' -o -name 'setup.py' -o -name 'setup.cfg' -o -name 'PKG-INFO' \) -print | sort
printf '\nRelevant package metadata:\n'
for f in $(find . \( -name 'pyproject.toml' -o -name 'setup.py' -o -name 'setup.cfg' -o -name 'PKG-INFO' \) -print | sort); do
echo "--- $f"
rg -n "name|version|dependencies|optional-dependencies|mcp|requires" "$f" || true
done
printf '\nSearch sph-client/mcp/pip install -e:\n'
rg -n "pip install -e|sph-client|mcp|Install|installation" README.md pyproject.toml setup.cfg setup.py . 2>/dev/null | head -n 200Repository: joan-code6/lanis_api
Length of output: 11603
Provide a PyPI installation command for the MCP extra.
sph-client declares the mcp optional dependency, but the only MCP install command is editable/source-only. PyPI users also get this pip install -e '.[mcp]', which cannot run from PyPI.
Proposed installation documentation
+# From PyPI
+python -m pip install 'sph-client[mcp]'
+
+# From a source checkout
python -m pip install -e '.[mcp]'🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@README.md` around lines 100 - 101, Update the MCP installation instructions
in README.md to include a PyPI-compatible command that installs the published
sph-client package with its mcp extra, while retaining the existing
editable/source installation command if it serves local development.
1 participant
What changed
Why
Lanis needs an MCP transport that gives assistants the same Schulportal Hessen capabilities as the REST API without requiring users to manually pass an access token to every tool call.
User and developer impact
Users can log in once per MCP session and use the full Lanis feature set through their assistant. Developers get an explicit REST-to-MCP route map, safer tool metadata, and packaging that installs the
lanis-mcpcommand.Validation
ruff check lanis_mcp testspytest -q— 8 passedjq empty chatgpt-app-submission.jsonpip install -e '.[mcp,dev]'Summary by CodeRabbit
New Features
lanis-mcpcommand-line entry point and Python module execution.Documentation