review: #424 + follow-ups - #427
Open
rejojer wants to merge 3 commits into
Open
Conversation
… home (#424) * feat: the client grows two sides — documents and chat each pick their home One client, two independent switches: api_key decides where documents live (the PageIndex cloud, or the local store); a configured chat model decides who answers (your own model in your process, or the managed cloud chat). Their free combination opens the bridge — cloud documents, your model — and the fourth cell stays unspellable. - index=/chat= slots: string shorthand or grouped dict, 1:1 with the flat arguments; one spelling per side, sides mix freely - optional "type" everywhere (top-level and in either dict): always omittable, checked against the content, meaningful alone — type="cloud" is a keyless cloud spelling - PAGEINDEX_API_KEY is read only when the code explicitly says cloud (PageIndexCloudClient(), type="cloud", "pageindex-cloud", {"type": "cloud"}); a bare PageIndexClient() stays local - bare mode words ("cloud", "local", …) are reserved: they error with the real spellings instead of silently parsing as model names - bridge chat runs the in-process agent over the live cloud MCP tools and instructions; doc_id targets at the prompt level; citations stay managed-only; an auth-shaped backend failure explains whose credentials run the model - typed shapes (IndexConfig, ChatConfig) ship as optional annotations Every previously working program is byte-for-byte unchanged: the only behavioral deltas are error paths — reworded guidance, and the api_key+chat_model combination graduating from an error into the bridge. * fix: the constructor refuses empty and mistyped values on every spelling - .env keys reach all four keyless-cloud spellings: utils' import-time load_dotenv now runs before every PAGEINDEX_API_KEY read - an empty chat-side value ("", {}) errors instead of silently selecting own-model chat on the default model; None-valued slot keys mean absent, exactly like the flat arguments - _local_chat derives from chat_model, so a post-construction assignment switches the whole client, never half of it - model= beside a slot gets the split guidance (index_model=/chat_model=) instead of "two spellings of the same thing" - the messages door wraps provider failures through _model_backend_error, and 401s count as auth-shaped even without "api key" in the text - keyless-cloud hints name the spelling that actually combines; slot strings are stripped; wrong-typed values raise PageIndexAPIError - retrieve_model/chat_backend docs drop the stale "Local mode only"; the local-scope refusal no longer claims bridge tools are server-scoped * fix: type= cross-checks the index slot; the cloud pinned class frees its chat side - type= beside index= now does what the docstring promises: agreement passes, disagreement errors, and a mistyped value reports the vocabulary error instead of a spelling collision - PageIndexCloudClient grows the chat-side arguments (chat=, chat_model, retrieve_model, chat_backend), so "pin the index side" is literally true and the chat surfaces' construct-with-chat_model guidance is followable on it - the four chat doors' doc_id entries carry the enforcement split the config helpers already state (local: tool-layer allowlist; cloud: prompt-level / server-side) - types.py stops claiming slot keys share the flat names — the side prefix is factored out, index={"model"} is index_model= * docs: bridge-reachable wording — dependency errors say own-model chat, hints name a chat= model - the three framework-missing errors said "in local mode", which is wrong on a bridge client (cloud documents + own model) — they now explain the dependency the way the surfaces do: your own chat model - the construct-with guidance reads "(or a chat= model)": a bare chat="pageindex-cloud" is also chat= but selects the managed side - the mechanical Local-only → Own-model-chat-only substitution left orphan fragments and two overlong lines; those paragraphs re-flowed * fix: managed chat reads None; the bridge stops paying per-turn tool lists - a managed-chat cloud client stores chat_model/chat_backend as None, so the documented attribute reads instead of raising AttributeError; _local_chat derives from "is a chat model configured" - McpBridge caches tools/list per session — every chat turn rebuilds the tool set, and the round trip was pure latency; the 404 session-expiry reset drops the cache with the session - run_messages builds tools before the transport: on a bridge client that build is network I/O, and a failure there stranded a per-call anthropic client ahead of the try/finally * refactor: the side declaration is spelled mode=, not type= "type" is Python's own word — a builtin, and "data type" beside the TypedDict shapes; "mode" is what the SDK already calls the two sides ("local mode", "cloud mode"). Same grammar everywhere the declaration appears: the top-level argument, the index dict, the chat dict, the typed shapes. The rename also frees the builtin inside the constructor, so the shape-check error names the offending class through type() again. "type" in a slot dict is now an ordinary unknown key. * fix: the reserved-word errors stop calling "cloud" not a mode word With the declaration key spelled mode=, 'index="cloud" is not a mode word' contradicted its own remedy, index={"mode": "cloud"} — "cloud" is exactly a mode value. The four bare strings are reserved words; the message now says so. * fix: a blank tools/list is not cached; the auth note's managed exit is chat-lane only - McpBridge.list_tools caches only a non-empty list — a transient blank (a deploy blip, a gate misconfiguration) would otherwise run every later turn with zero tools while the instructions still name them, and only a 404 session reset could clear it - the 401 architecture note appends "drop the chat model configuration" only on the chat lane: responses() and messages() refuse a client without an own model, so on those lanes the exit sent the caller in a circle - CloudIndexConfig says api_key is omittable only while mode: "cloud" stays — index={} refuses as an empty dict rather than reading the env * fix: the bridge fetches tools/list per call again; .env resolves from the cwd - McpBridge.list_tools no longer caches: the tool set is built once per SDK call (Agent(tools=...) ahead of Runner.run; build_anthropic_tools ahead of tool_runner), not per model turn, so the cache saved one round trip per later call while a mid-pagination 404 replayed a dead cursor into a duplicated (and cached) list, and the list went out by reference across a lock dropped between miss and store - utils.load_dotenv searches upward from the cwd: a bare load_dotenv() walked up from utils.py, which is site-packages for an installed SDK, so the four keyless-cloud spellings never saw a project-root .env; the package-relative walk stays as the fallback - the emptiness guard strips strings: chat_model=" " selected own-model chat, the silent flip the guard's own comment rules out - the _local_chat comment stops advertising post-construction assignment as a full mode switch * fix: the pinned classes take index=/chat=; "cloud"/"local" are mode words; a blank chat_model stays managed - PageIndexLocalClient takes index= and chat=, PageIndexCloudClient takes index= — the grouped spelling of the flat vocabulary each already took; their refusals name the class and an exit that class can take, and the mode cross-check runs before any environment read - "cloud" and "local" are accepted wherever "pageindex-cloud" was (index=, chat=, mode=, {"mode": ...}), case- and whitespace-insensitive; "hosted" and "managed" still refuse, pointing at the real word - every spelling strips its strings, and the slot spellings' type/empty errors name the slot key (index["model"]), not the flat argument - _local_chat treats a blank chat_model as managed: the constructor refuses "", so assignment agrees instead of opening the bridge on a nameless model; openai_agent_config carries no model then either - an empty MCP tools/list raises like empty instructions does — a zero-tool agent would answer from the model's own knowledge silently - enable_citations names the real gate (managed vs own chat), not "cloud-only", on a cloud own-model client - pageindex/py.typed: the exported config TypedDicts reach installed type-checked callers * test: the two framework-door tests skip without openai-agents as_openai_tools() and openai_agent_config() need the agents package, which the "without frameworks" CI legs do not install — the same importorskip every other test on those doors already carries.
…k chat_model refuses at the chat door; storage_path is typed PathLike (#428) * fix: .env stays unset when the cwd tree has none; a local client with a blank chat_model refuses at the chat door; storage_path is typed PathLike find_dotenv(usecwd=True) returns '' when nothing is reachable from the cwd, and `or None` turned that into load_dotenv's own upward walk from utils.py — the install-dir leak the cwd search was added to replace. A pip-installed SDK could load another project's .env from above site-packages, silently. _local_chat treats a blank chat_model as "managed chat", which a client without an api_key does not have: chat_completions() then reached for LocalAPI.chat_completions and raised a bare AttributeError. The managed branch now refuses as a PageIndexAPIError naming chat_model. py.typed made the annotations authoritative while storage_path was typed str; _ARG_TYPES accepts os.PathLike, so Path(...) ran fine and failed the user's type check. Both signatures and LocalIndexConfig now say so. Claude-Session: https://claude.ai/code/session_017Fd7jVm366S2Xamzhxv6yb * fix: the exported config shapes pass into index=/chat=; a comment and two docstrings stop overclaiming The slots were annotated dict[str, Any]. A TypedDict is consistent with Mapping[str, object], never with dict (PEP 589: a dict-typed receiver could write arbitrary keys through it), so the four shapes types.py exports — and py.typed advertises to installed callers' checkers — could not be passed to the one place they describe. pyright on a probe that does exactly that: 9 errors before, 0 after. The constructor only reads the slot (items(), then a fresh conf dict), so Mapping is the honest bound; a plain dict is a Mapping, and TypedDict instances are plain dicts at runtime, so nothing moves at runtime. The _ARG_TYPES comment said "every value" is shape-checked; api_key is not in the table (its empty check is separate, its type check stays unchecked by ruling), so the comment now speaks for the table only. _local_doc_scope and _require_local_scope still explained the cloud drop as "scoping is server-side" — true of the managed chat, which never reaches either function. What reaches them on a cloud client is own-model chat and the config helpers, whose cloud tools take no allowlist: targeting there is prompt-level only, as the error message between them already said. 434 passed; pyright on pageindex/ unchanged at 235 (0 in the touched files, before and after). Claude-Session: https://claude.ai/code/session_01TxG8u8x29XRnK4yscZVCch * test: the install-dir .env test is named for what it asserts Claude-Session: https://claude.ai/code/session_017Fd7jVm366S2Xamzhxv6yb
rejojer
force-pushed
the
fix/client-config-followups
branch
from
August 26, 2026 07:09
90d6289 to
b9a9a3b
Compare
Member
Author
Code reviewNo issues found. Checked for bugs and CLAUDE.md compliance. 🤖 Generated with Claude Code |
…alling cloud tool scoping server-side (#429) fix: the slots accept any Mapping at runtime, as their annotation admits; eight docstrings stop calling cloud tool scoping server-side 4e9c56c widened index=/chat= to Mapping[str, Any] so the exported TypedDicts pass a checker, but _resolve_index_slot/_resolve_chat_slot still dispatched on isinstance(..., dict): a MappingProxyType or ChainMap was pyright-clean and raised "must be a string or a dict" at construction. The resolvers now narrow on Mapping — the comprehension already copies, so a read-only proxy proves the caller's mapping is never mutated. 4e9c56c corrected three of eleven "scoping is server-side" sites; the remaining eight said the same untrue thing about doc_id on cloud (its tools carry no allowlist — targeting is prompt-level, as the runtime error already explains). Deleted rather than reworded. local_chat.py's module docstring predates own-model chat over the cloud bridge; storage_path's prose now names the PathLike 5e2dc9b typed. Claude-Session: https://claude.ai/code/session_01VQ6mruXZBgw9Hjii8KPbQP
rejojer
force-pushed
the
fix/client-config-followups
branch
from
August 26, 2026 09:04
2e606b8 to
174f95f
Compare
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
1 participant
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.
Review view only — do not merge. Base is
pre-424-main, pinned at416e304(main right before #424 landed), so the diff is everything #424 shipped plus the follow-ups on top, the way #400 pinspre-389-main.Follow-ups since v0.2.11:
5e2dc9b—.envsearch ends at the cwd tree (find_dotenvreturns'', andor Nonehanded dotenv its own walk up fromutils.py); a local client with a blankedchat_modelrefuses at the chat door instead ofAttributeError;storage_pathtypedstr | os.PathLike[str]to match_ARG_TYPESnow thatpy.typedships.4e9c56c—index=/chat=typedMapping[str, Any]so the exported config shapes pass; a comment and two docstrings stop overclaiming.90d6289— test renamed for what it asserts.The mergeable PR for the same commits targets
mainseparately.https://claude.ai/code/session_017Fd7jVm366S2Xamzhxv6yb