hosting: lint fixes + address #677 review comments#678
Merged
Conversation
- server.py, gateway/main.py: note that _limit_body_size only checks Content-Length; chunked bodies pass through (review #1) - gateway/main.py: strip token:tenant pairs and skip malformed entries when parsing GATEWAY_TENANTS; log loaded count (review #2) - requirements.txt, server.py _remember, gateway/proxy.py: clarifying comments (review #3, #6, #7) - gateway/k8s.py: validate AGENT_IMAGE in initialize_standby_pool() instead of at import time (review #8) - kind-quickstart.sh: auto-restart gateway on re-runs so regenerated GATEWAY_TENANTS takes effect (review #9)
Notebook ChangesThis PR modifies the following notebooks: 📓
|
There was a problem hiding this comment.
PR Review
Recommendation: COMMENT
Summary
Follow-up to #677 that lands the lint-and-format check on main and addresses the non-blocking review comments — moves the AGENT_IMAGE check to runtime, hardens GATEWAY_TENANTS parsing, improves the kind-quickstart re-run UX, and adds clarifying comments. No blocking issues; all findings below are observability/wording nits.
Actionable Feedback (3 items)
-
claude_agent_sdk/hosting/kubernetes/gateway/main.py:74— The module-levellogger.info("gateway: loaded %d tenant token(s)", ...)runs at import. uvicorn configures logging after importing the app, and Python's default root logger is WARNING with no handler. This line almost certainly emits nothing. Either move it intolifespan()or addlogging.basicConfig(level=logging.INFO). Observability-only, not functional. -
claude_agent_sdk/hosting/kubernetes/gateway/k8s.py:47-49— Comment claims the relocated check "fails its readiness probe." An exception in FastAPI'slifespan()startup actually causes uvicorn to exit the process → pod willCrashLoopBackOff, not fail readiness. The real win is a clear traceback in pod logs and a clean shutdown path. Consider rewording (e.g. "logs a clear startup error in pod logs"). -
claude_agent_sdk/hosting/kubernetes/gateway/main.py:66-73— Subtle behavior change: the rewrite now.strip()s tokens. A misconfig like" abc:alice"previously created a token that no bearer header would ever match (HTTP strips whitespace); now it works. Probably the intended improvement, but worth a one-line comment so a future reader doesn't "fix" it.
Detailed Review
Code Quality
- The tenant-parsing rewrite is much clearer than the previous dict-comprehension and correctly drops empty-token pairs. The
len(_parts) != 2guard is dead-defensive (split(":", 1)returns at most 2 elements), but it's harmless and theif not _tokenguard does the real work. REUSING_CLUSTERscoping inkind-quickstart.shis correctly handled underset -u: assigned only in theelsebranch and consumed with${REUSING_CLUSTER:-}.- Notebook output-cell key reorders (
output_type/name) are nbformat-normalized; they'll flip back on the nextRun Allin some environments. Consider adding nbformat normalization to pre-commit if you want it to stick. # noqa: S106on the k8s Secret name"egress-proxy-tls"is correct — it's a Secret object name, not credential content.
Security
- No security regressions. Tenant parsing remains strict about empty tokens (the original concern from #677). Auth flow and ownership checks are unchanged.
_limit_body_sizeContent-Length-only behavior is now documented (good); the existing guidance to put a real size limit on the LB/gateway in production is the right framing.
Suggestions
- The import reorder in
server.pymoves the "agent we built in notebook 00…" comment so it visually sits inside the third-party group. Cosmetic — Ruff's isort treatedresearch_agentas first-party correctly.
Positive Notes
proxy.py:43— the "Not async with on purpose — the client must outlivesend(..., stream=True)" comment is exactly the kind of why comment that prevents a well-meaning future refactor from breaking the stream.server.py:166-168— cross-reference from_remember()to the RedisSET NXequivalent inkubernetes/gateway/k8s.pycouples the two tiers' approaches at the comment level. Nice.requirements.txt— the note that the Dockerfile npm pin moves together with the Python pin set is a great anti-footgun for whoever bumps these next.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to #677. Fixes the lint-and-format check on main, and addresses the non-blocking review items (chunked-encoding note, tenant-parse robustness, kind-quickstart re-run UX, AGENT_IMAGE check timing, plus a few clarifying comments). Items 4/5/10/11 from the review are intentionally left — they're already covered by the README's 'What this doesn't give you' section.