Skip to content

review: #424 + follow-ups - #427

Open
rejojer wants to merge 3 commits into
pre-424-mainfrom
fix/client-config-followups
Open

review: #424 + follow-ups#427
rejojer wants to merge 3 commits into
pre-424-mainfrom
fix/client-config-followups

Conversation

@rejojer

Copy link
Copy Markdown
Member

Review view only — do not merge. Base is pre-424-main, pinned at 416e304 (main right before #424 landed), so the diff is everything #424 shipped plus the follow-ups on top, the way #400 pins pre-389-main.

Follow-ups since v0.2.11:

  • 5e2dc9b.env search ends at the cwd tree (find_dotenv returns '', and or None handed dotenv its own walk up from utils.py); a local client with a blanked chat_model refuses at the chat door instead of AttributeError; storage_path typed str | os.PathLike[str] to match _ARG_TYPES now that py.typed ships.
  • 4e9c56cindex= / chat= typed Mapping[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 main separately.

https://claude.ai/code/session_017Fd7jVm366S2Xamzhxv6yb

… 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
rejojerforce-pushed the fix/client-config-followups branch from 90d6289 to b9a9a3bCompareAugust 26, 2026 07:09
@rejojer

Copy link
Copy Markdown
MemberAuthor

Code review

No 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
rejojerforce-pushed the fix/client-config-followups branch from 2e606b8 to 174f95fCompareAugust 26, 2026 09:04
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant

@rejojer
, 'i'); if (__m === '*' || __re.test(location.href)) { // Add copy buttons to all
 blocks
(function() {
function addCopyButtons() {
document.querySelectorAll('pre code').forEach(function(codeBlock) {
if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;
codeBlock.parentElement.setAttribute('data-copy-added', 'true');
var btn = document.createElement('button');
btn.textContent = 'Copy';
btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';
btn.onmouseover = function() { this.style.opacity = '1'; };
btn.onmouseout = function() { this.style.opacity = '0.7'; };
btn.onclick = function() {
navigator.clipboard.writeText(codeBlock.textContent).then(function() {
btn.textContent = 'Copied!';
setTimeout(function() { btn.textContent = 'Copy'; }, 1500);
});
};
codeBlock.parentElement.style.position = 'relative';
codeBlock.parentElement.appendChild(btn);
});
}
addCopyButtons();
// Re-run on dynamic content
var observer = new MutationObserver(addCopyButtons);
observer.observe(document.body, { childList: true, subtree: true });
})();
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '
review: #424 + follow-ups by rejojer · Pull Request #427 · VectifyAI/PageIndex · GitHub
Skip to content

review: #424 + follow-ups - #427

Open
rejojer wants to merge 3 commits into
pre-424-mainfrom
fix/client-config-followups
Open

review: #424 + follow-ups#427
rejojer wants to merge 3 commits into
pre-424-mainfrom
fix/client-config-followups

Conversation

@rejojer

Copy link
Copy Markdown
Member

Review view only — do not merge. Base is pre-424-main, pinned at 416e304 (main right before #424 landed), so the diff is everything #424 shipped plus the follow-ups on top, the way #400 pins pre-389-main.

Follow-ups since v0.2.11:

  • 5e2dc9b.env search ends at the cwd tree (find_dotenv returns '', and or None handed dotenv its own walk up from utils.py); a local client with a blanked chat_model refuses at the chat door instead of AttributeError; storage_path typed str | os.PathLike[str] to match _ARG_TYPES now that py.typed ships.
  • 4e9c56cindex= / chat= typed Mapping[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 main separately.

https://claude.ai/code/session_017Fd7jVm366S2Xamzhxv6yb

… 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
rejojerforce-pushed the fix/client-config-followups branch from 90d6289 to b9a9a3bCompareAugust 26, 2026 07:09
@rejojer

Copy link
Copy Markdown
MemberAuthor

Code review

No 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
rejojerforce-pushed the fix/client-config-followups branch from 2e606b8 to 174f95fCompareAugust 26, 2026 09:04
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant

@rejojer
, 'i'); if (__m === '*' || __re.test(location.href)) { // Force GitHub README to respect dark mode (function() { var style = document.createElement('style'); style.textContent = ' .markdown-body { color-scheme: dark light; } .markdown-body pre { background: #161b22 !important; } .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; } .markdown-body table th, .markdown-body table td { border-color: #30363d !important; } .markdown-body img { background: #0d1117; } .markdown-body blockquote { border-left-color: #8b949e; } .markdown-body hr { border-color: #30363d; } '; document.head.appendChild(style); })(); } } catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + ' review: #424 + follow-ups by rejojer · Pull Request #427 · VectifyAI/PageIndex · GitHub
Skip to content

review: #424 + follow-ups - #427

Open
rejojer wants to merge 3 commits into
pre-424-mainfrom
fix/client-config-followups
Open

review: #424 + follow-ups#427
rejojer wants to merge 3 commits into
pre-424-mainfrom
fix/client-config-followups

Conversation

@rejojer

Copy link
Copy Markdown
Member

Review view only — do not merge. Base is pre-424-main, pinned at 416e304 (main right before #424 landed), so the diff is everything #424 shipped plus the follow-ups on top, the way #400 pins pre-389-main.

Follow-ups since v0.2.11:

  • 5e2dc9b.env search ends at the cwd tree (find_dotenv returns '', and or None handed dotenv its own walk up from utils.py); a local client with a blanked chat_model refuses at the chat door instead of AttributeError; storage_path typed str | os.PathLike[str] to match _ARG_TYPES now that py.typed ships.
  • 4e9c56cindex= / chat= typed Mapping[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 main separately.

https://claude.ai/code/session_017Fd7jVm366S2Xamzhxv6yb

… 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
rejojerforce-pushed the fix/client-config-followups branch from 90d6289 to b9a9a3bCompareAugust 26, 2026 07:09
@rejojer

Copy link
Copy Markdown
MemberAuthor

Code review

No 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
rejojerforce-pushed the fix/client-config-followups branch from 2e606b8 to 174f95fCompareAugust 26, 2026 09:04
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant

@rejojer
, 'i'); if (__m === '*' || __re.test(location.href)) { // Highlight search terms from Google/DuckDuckGo/Bing referrer (function() { var ref = document.referrer; var terms = []; if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) { var url = new URL(ref); var q = url.searchParams.get('q') || url.searchParams.get('p'); if (q) { terms = q.split(/\s+/).filter(function(t) { return t.length > 2; }); } } if (terms.length === 0) return; var style = document.createElement('style'); style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }'; document.head.appendChild(style); function highlight(node) { if (node.nodeType === 3) { // text node var text = node.textContent; var found = false; terms.forEach(function(term) { var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\]\\]/g, '\\') + ')', 'gi'); if (regex.test(text)) { found = true; var frag = document.createDocumentFragment(); var parts = text.split(regex); parts.forEach(function(part, i) { if (i % 2 === 0) { frag.appendChild(document.createTextNode(part)); } else { var span = document.createElement('span'); span.className = 'userscript-highlight'; span.textContent = part; frag.appendChild(span); } }); node.parentNode.replaceChild(frag, node); } }); } else if (node.nodeType === 1 && node.childNodes) { // element var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT']; if (!skipTags.includes(node.tagName)) { Array.from(node.childNodes).forEach(highlight); } } } highlight(document.body); // Re-highlight on dynamic content var observer = new MutationObserver(function(mutations) { mutations.forEach(function(m) { m.addedNodes.forEach(function(node) { if (node.nodeType === 1 || node.nodeType === 3) highlight(node); }); }); }); observer.observe(document.body, { childList: true, subtree: true }); })(); } } catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + ' review: #424 + follow-ups by rejojer · Pull Request #427 · VectifyAI/PageIndex · GitHub
Skip to content

review: #424 + follow-ups - #427

Open
rejojer wants to merge 3 commits into
pre-424-mainfrom
fix/client-config-followups
Open

review: #424 + follow-ups#427
rejojer wants to merge 3 commits into
pre-424-mainfrom
fix/client-config-followups

Conversation

@rejojer

Copy link
Copy Markdown
Member

Review view only — do not merge. Base is pre-424-main, pinned at 416e304 (main right before #424 landed), so the diff is everything #424 shipped plus the follow-ups on top, the way #400 pins pre-389-main.

Follow-ups since v0.2.11:

  • 5e2dc9b.env search ends at the cwd tree (find_dotenv returns '', and or None handed dotenv its own walk up from utils.py); a local client with a blanked chat_model refuses at the chat door instead of AttributeError; storage_path typed str | os.PathLike[str] to match _ARG_TYPES now that py.typed ships.
  • 4e9c56cindex= / chat= typed Mapping[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 main separately.

https://claude.ai/code/session_017Fd7jVm366S2Xamzhxv6yb

… 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
rejojerforce-pushed the fix/client-config-followups branch from 90d6289 to b9a9a3bCompareAugust 26, 2026 07:09
@rejojer

Copy link
Copy Markdown
MemberAuthor

Code review

No 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
rejojerforce-pushed the fix/client-config-followups branch from 2e606b8 to 174f95fCompareAugust 26, 2026 09:04
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant

@rejojer
, 'i'); if (__m === '*' || __re.test(location.href)) { // Strip utm_, fbclid, gclid, etc. from all links on page (function() { var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content', 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid', 'ref', 'ref_src', 'source', 'medium', 'campaign']; function cleanUrl(url) { try { var u = new URL(url, window.location.origin); var changed = false; trackingParams.forEach(function(p) { if (u.searchParams.has(p)) { u.searchParams.delete(p); changed = true; } }); return changed ? u.toString() : url; } catch (e) { return url; } } function cleanLinks() { document.querySelectorAll('a[href]').forEach(function(a) { var clean = cleanUrl(a.href); if (clean !== a.href) a.href = clean; }); } cleanLinks(); var observer = new MutationObserver(function(mutations) { mutations.forEach(function(m) { m.addedNodes.forEach(function(node) { if (node.nodeType === 1) { if (node.tagName === 'A') cleanLinks(); node.querySelectorAll('a[href]').forEach(function(a) { var clean = cleanUrl(a.href); if (clean !== a.href) a.href = clean; }); } }); }); }); observer.observe(document.body, { childList: true, subtree: true }); })(); } } catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); } })(); (function(){ try { var __m = "youtube.com"; var __re = new RegExp('^' + "youtube\\.com" + ' review: #424 + follow-ups by rejojer · Pull Request #427 · VectifyAI/PageIndex · GitHub
Skip to content

review: #424 + follow-ups - #427

Open
rejojer wants to merge 3 commits into
pre-424-mainfrom
fix/client-config-followups
Open

review: #424 + follow-ups#427
rejojer wants to merge 3 commits into
pre-424-mainfrom
fix/client-config-followups

Conversation

@rejojer

Copy link
Copy Markdown
Member

Review view only — do not merge. Base is pre-424-main, pinned at 416e304 (main right before #424 landed), so the diff is everything #424 shipped plus the follow-ups on top, the way #400 pins pre-389-main.

Follow-ups since v0.2.11:

  • 5e2dc9b.env search ends at the cwd tree (find_dotenv returns '', and or None handed dotenv its own walk up from utils.py); a local client with a blanked chat_model refuses at the chat door instead of AttributeError; storage_path typed str | os.PathLike[str] to match _ARG_TYPES now that py.typed ships.
  • 4e9c56cindex= / chat= typed Mapping[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 main separately.

https://claude.ai/code/session_017Fd7jVm366S2Xamzhxv6yb

… 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
rejojerforce-pushed the fix/client-config-followups branch from 90d6289 to b9a9a3bCompareAugust 26, 2026 07:09
@rejojer

Copy link
Copy Markdown
MemberAuthor

Code review

No 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
rejojerforce-pushed the fix/client-config-followups branch from 2e606b8 to 174f95fCompareAugust 26, 2026 09:04
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant

@rejojer
, 'i'); if (__m === '*' || __re.test(location.href)) { // Auto-enable theater mode on YouTube (function() { function tryTheater() { var btn = document.querySelector('button[aria-label="Theater mode"], ytd-player #player button[title="Theater mode"]'); if (btn && !btn.classList.contains('activated')) { btn.click(); } } // Try immediately tryTheater(); // Try after navigation (SPA) var lastUrl = location.href; setInterval(function() { if (location.href !== lastUrl) { lastUrl = location.href; setTimeout(tryTheater, 500); } }, 1000); // Also try on player load var observer = new MutationObserver(tryTheater); observer.observe(document.body, { childList: true, subtree: true }); })(); } } catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + ' review: #424 + follow-ups by rejojer · Pull Request #427 · VectifyAI/PageIndex · GitHub
Skip to content

review: #424 + follow-ups - #427

Open
rejojer wants to merge 3 commits into
pre-424-mainfrom
fix/client-config-followups
Open

review: #424 + follow-ups#427
rejojer wants to merge 3 commits into
pre-424-mainfrom
fix/client-config-followups

Conversation

@rejojer

Copy link
Copy Markdown
Member

Review view only — do not merge. Base is pre-424-main, pinned at 416e304 (main right before #424 landed), so the diff is everything #424 shipped plus the follow-ups on top, the way #400 pins pre-389-main.

Follow-ups since v0.2.11:

  • 5e2dc9b.env search ends at the cwd tree (find_dotenv returns '', and or None handed dotenv its own walk up from utils.py); a local client with a blanked chat_model refuses at the chat door instead of AttributeError; storage_path typed str | os.PathLike[str] to match _ARG_TYPES now that py.typed ships.
  • 4e9c56cindex= / chat= typed Mapping[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 main separately.

https://claude.ai/code/session_017Fd7jVm366S2Xamzhxv6yb

… 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
rejojerforce-pushed the fix/client-config-followups branch from 90d6289 to b9a9a3bCompareAugust 26, 2026 07:09
@rejojer

Copy link
Copy Markdown
MemberAuthor

Code review

No 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
rejojerforce-pushed the fix/client-config-followups branch from 2e606b8 to 174f95fCompareAugust 26, 2026 09:04
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant

@rejojer
, 'i'); if (__m === '*' || __re.test(location.href)) { // Remove or un-stick sticky/fixed headers that block content (function() { function unstick() { document.querySelectorAll('header, nav, [role="banner"], .header, .navbar, .sticky, .fixed-top, [style*="position: fixed"], [style*="position:sticky"]').forEach(function(el) { if (el.style.position === 'fixed' || el.style.position === 'sticky' || getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') { el.style.position = 'static'; el.style.top = 'auto'; el.style.zIndex = 'auto'; } }); } unstick(); var observer = new MutationObserver(unstick); observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] }); })(); } } catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + ' review: #424 + follow-ups by rejojer · Pull Request #427 · VectifyAI/PageIndex · GitHub
Skip to content

review: #424 + follow-ups - #427

Open
rejojer wants to merge 3 commits into
pre-424-mainfrom
fix/client-config-followups
Open

review: #424 + follow-ups#427
rejojer wants to merge 3 commits into
pre-424-mainfrom
fix/client-config-followups

Conversation

@rejojer

Copy link
Copy Markdown
Member

Review view only — do not merge. Base is pre-424-main, pinned at 416e304 (main right before #424 landed), so the diff is everything #424 shipped plus the follow-ups on top, the way #400 pins pre-389-main.

Follow-ups since v0.2.11:

  • 5e2dc9b.env search ends at the cwd tree (find_dotenv returns '', and or None handed dotenv its own walk up from utils.py); a local client with a blanked chat_model refuses at the chat door instead of AttributeError; storage_path typed str | os.PathLike[str] to match _ARG_TYPES now that py.typed ships.
  • 4e9c56cindex= / chat= typed Mapping[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 main separately.

https://claude.ai/code/session_017Fd7jVm366S2Xamzhxv6yb

… 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
rejojerforce-pushed the fix/client-config-followups branch from 90d6289 to b9a9a3bCompareAugust 26, 2026 07:09
@rejojer

Copy link
Copy Markdown
MemberAuthor

Code review

No 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
rejojerforce-pushed the fix/client-config-followups branch from 2e606b8 to 174f95fCompareAugust 26, 2026 09:04
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant

@rejojer
, 'i'); if (__m === '*' || __re.test(location.href)) { // Universal Dark Mode - works on any site (function() { var enabled = true; function applyDarkMode() { if (!enabled) return; // Create style element if it doesn't exist var style = document.getElementById('universal-dark-mode-style'); if (!style) { style = document.createElement('style'); style.id = 'universal-dark-mode-style'; document.head.appendChild(style); } // Dark mode CSS - inverts colors but preserves images/video style.textContent = ' /* Invert everything except media */ html { filter: invert(1) hue-rotate(180deg) !important; background: #1a1a2e !important; } /* Restore images, videos, iframes, canvas */ img, video, iframe, canvas, svg, picture, [style*="background-image"] { filter: invert(1) hue-rotate(180deg) !important; } /* Preserve specific elements that should not be inverted */ .no-dark-mode, .no-dark-mode *, [data-theme="light"], [data-theme="light"], .ace_editor, .ace_editor *, .CodeMirror, .CodeMirror *, .monaco-editor, .monaco-editor *, .markdown-body pre, .markdown-body pre *, .highlight, .highlight *, pre code, pre code * { filter: none !important; } /* Fix common UI elements */ .modal, .popup, .dropdown-menu, .tooltip, .popover { filter: invert(1) hue-rotate(180deg) !important; background: #2d2d44 !important; border-color: #444 !important; } /* Scrollbars */ ::-webkit-scrollbar { background: #1a1a2e !important; } ::-webkit-scrollbar-thumb { background: #444 !important; } ::-webkit-scrollbar-thumb:hover { background: #555 !important; } /* Selection */ ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; } ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; } '; } function removeDarkMode() { var style = document.getElementById('universal-dark-mode-style'); if (style) style.remove(); } // Toggle with Alt+Shift+D document.addEventListener('keydown', function(e) { if (e.altKey && e.shiftKey && e.key === 'D') { e.preventDefault(); enabled = !enabled; if (enabled) { applyDarkMode(); console.log('[Universal Dark Mode] Enabled'); } else { removeDarkMode(); console.log('[Universal Dark Mode] Disabled'); } } }); // Apply on load applyDarkMode(); // Re-apply on dynamic content var observer = new MutationObserver(function(mutations) { if (enabled && !document.getElementById('universal-dark-mode-style')) { applyDarkMode(); } }); observer.observe(document.head, { childList: true }); console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle'); })(); } } catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); } })(); })(); review: #424 + follow-ups by rejojer · Pull Request #427 · VectifyAI/PageIndex · GitHub
Skip to content

review: #424 + follow-ups - #427

Open
rejojer wants to merge 3 commits into
pre-424-mainfrom
fix/client-config-followups
Open

review: #424 + follow-ups#427
rejojer wants to merge 3 commits into
pre-424-mainfrom
fix/client-config-followups

Conversation

@rejojer

Copy link
Copy Markdown
Member

Review view only — do not merge. Base is pre-424-main, pinned at 416e304 (main right before #424 landed), so the diff is everything #424 shipped plus the follow-ups on top, the way #400 pins pre-389-main.

Follow-ups since v0.2.11:

  • 5e2dc9b.env search ends at the cwd tree (find_dotenv returns '', and or None handed dotenv its own walk up from utils.py); a local client with a blanked chat_model refuses at the chat door instead of AttributeError; storage_path typed str | os.PathLike[str] to match _ARG_TYPES now that py.typed ships.
  • 4e9c56cindex= / chat= typed Mapping[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 main separately.

https://claude.ai/code/session_017Fd7jVm366S2Xamzhxv6yb

… 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
rejojerforce-pushed the fix/client-config-followups branch from 90d6289 to b9a9a3bCompareAugust 26, 2026 07:09
@rejojer

Copy link
Copy Markdown
MemberAuthor

Code review

No 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
rejojerforce-pushed the fix/client-config-followups branch from 2e606b8 to 174f95fCompareAugust 26, 2026 09:04
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant

@rejojer