Targeting moves to the first user message: document_context() replaces doc_id on the agent surfaces; folder_context() and chat(folder_id=) add folders - #495
Conversation
rejojer
changed the title
|
@codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d439b19292
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| "Use this document's name to retrieve its content with " | ||
| "get_document_structure() and get_page_content()." |
There was a problem hiding this comment.
When a cloud library contains same-name documents in different folders, this directive tells the model to retrieve by name alone even though the cloud tool contract explicitly requires folder_id to disambiguate such names (agent_tools.py:47-55). The removed shadow check previously prevented silently targeting an ambiguous name; now document_context(doc_id) and own-model chat(doc_id=...) can read the wrong document or fail despite get_document() supplying its folderId. Include the target folder ID in the retrieval directive, or retain an ambiguity check.
Useful? React with 👍 / 👎.
…es doc_id on the agent surfaces
The doc-targeting block ("The user has specified document: ...") was
appended to the system prompt by agent_instructions() and the three
*_agent_config bundles, and placed as its own system block by the Messages
chat lane. A per-request target in the system prompt breaks the cached
prefix, sticks across turns, and on cloud splices SDK text onto the
server-served instructions. The cloud's managed chat puts the same block at
the head of the conversation, so every surface now does the same.
- chat(): all three lanes prepend the block as the first user message
(the Messages lane moved off its system block).
- New client.document_context(doc_id): the block text for callers who own
the conversation (the framework routes), to lead their first message.
- BREAKING: doc_id removed from agent_instructions(), openai_agent_config(),
anthropic_runner_config(), claude_agent_config(), agent_tools(),
as_openai_tools(), as_anthropic_tools() and as_claude_mcp(). The
tool-layer allowlist was local-only and raised on cloud; it stays
internal to local chat(doc_id=).
- The block is one get_document per id: names are unique per library
(uploads suffix a taken name), so the shadow check and its listing sweep
went. The cloud endpoint carries metadata, so local get_document() gained
the key for parity. Rendering matches the cloud's: one document is an
object, several a list.
Live-verified on a local store and the cloud library across chat() (answer
lane, chat_completions, responses, messages) and the OpenAI Agents,
Anthropic tool_runner and Claude Agent SDK routes.
Claude-Session: https://claude.ai/code/session_01VxguPoTv2BmzS9d3erzTrS
doc_id sat first in the positional list on agent_instructions,
openai_agent_config and claude_agent_config, and second on
anthropic_runner_config and as_claude_mcp. With it removed, a v0.2.15 call
such as openai_agent_config("pi-a") no longer failed: the id landed on
include_management (truthy, so the management tools came along and
targeting silently vanished) or on server_name. A bare * after the
surviving leading positional turns those calls into an immediate
TypeError; every in-repo caller already passes keywords.
Claude-Session: https://claude.ai/code/session_017FumozBm2xbT2SG6WBxjMe
5451e4e put a `*` on the client methods that dropped doc_id, so a stale positional call raises instead of landing on the next parameter. build_claude_mcp, which as_claude_mcp delegates to, dropped doc_ids the same way and let server_name move into its slot: a stale build_claude_mcp(client, False, ['pi-a']) built an in-process server named ['pi-a'], or on cloud returned the http config with the argument dropped. Now keyword-only, covered by the same test. Doc truths: document_context no longer claims a later turn can move on to another document (chat's docstrings say to keep doc_id constant, and the block is prepended at the head of the conversation, so a retarget lands behind the history); submit_document's metadata inventory and CloudAPI.get_document's key list both name the metadata and folderId keys get_document returns. Docstring trims: the "names are unique per library" rationale sentence and the multi-line test docstrings, one of which still described the listing backfill this PR removed. Claude-Session: https://claude.ai/code/session_018FoZJ97cuM9hAjrk3goZ6u
The managed /chat/completions already takes folder_id and renders a folder targeting block ahead of the document block (pageindex-compute api.py:491, :3182); the SDK had no way to send it, and own-model chat had no folder analogue of document_context(). Both land here. folder_context(folder_id) renders that block as the managed chat renders its own: the name, an id/name/description metadata row (the server's FolderContext), and the directive to discover the folder's documents with browse_documents / search_documents(folder_id=..., recursive=true). One list_folders() call picks the folder by id: the endpoint has no LIMIT and lists the same userId + api_surface partition the managed chat validates against, so what the helper finds is what the server accepts, and it carries the same plan gate, so a non-Max key fails here rather than mid-run. A missing folder raises. Cloud-only: local libraries have no folders. chat(folder_id=) places it. The managed lane sends the field and lets the server render and gate it; the three own-model lanes lead the conversation with one user message, folder block then document block, joined as the server joins them. folder_id joins the prompt-cache key for the reason doc_id does: the block is byte-identical for every conversation about a folder. "root" (and "") targets nothing on every lane and in every mode, as the server treats folder_id="root": folder_context returns "" and chat places nothing. folder_id is keyword-only on chat() and trailing on every other signature, so no positional call shifts. Tests: the wire field on the managed lane; block order and root/"" on the shared renderer; folder_id threaded through the chat, responses and messages engines; the local cloud-only refusal. Claude-Session: https://claude.ai/code/session_01NsQjN5HMmmX4n5NVsZWYaw
_conversation_cache_key splices folder_id into the seed only when one is set, so a conversation without a folder keeps the prompt_cache_key it had before folders existed instead of cold-starting on upgrade; the released key is pinned. folder_id gets the bare * on chat_completions, _responses and _messages, keyword-only on every chat surface as on chat(); every caller already passes it by keyword. The three *_agent_config docstrings name the context helpers in the order the lanes place them: folder, then documents. test_doc_targeting_is_one_lookup_per_document now counts get_document calls; it accepted a second lookup per id before. Claude-Session: https://claude.ai/code/session_01T2i8viYf3Cb6wZcjToLEDD
…xact payload The lane's payload assertion is exact, and folder_id is a managed chat request field, so it appears there too — unset on a folder-less call.
d9c8986 to
c5a3c61
Compare
chat(protocol="chat_completions") reached main while this branch was open, and it is a managed chat_completions call site of its own: a folder_id passed there was dropped on the way to the endpoint, scoping nothing with no error. It carries it now, asserted on the wire beside the answer lane's.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 41ebe98276
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if folder_id: | ||
| payload["folder_id"] = folder_id |
There was a problem hiding this comment.
When callers pass both the new folder_id= argument and a legacy or composed extra_body containing folder_id, the later payload.update(extra_body or {}) silently replaces this value, potentially sending the managed chat to a different folder. _refuse_skeleton() already reserves stream and doc_id for exactly this reason, so add folder_id to that argument-key validation rather than allowing the explicit scope to be overridden.
Useful? React with 👍 / 👎.
1 participant
What
The doc-targeting block ("The user has specified document: …") is conversation content, not guidance. It now lives in exactly one place on every surface: the first user message. Nothing is appended to the system prompt any more.
client.document_context(doc_id): returns the block text for callers who own the conversation (the framework routes). Lead your first message with it:prompt.)doc_idremoved from eight methods —agent_instructions(),openai_agent_config(),anthropic_runner_config(),claude_agent_config(),agent_tools(),as_openai_tools(),as_anthropic_tools(),as_claude_mcp(). The prompt-side targeting moves todocument_context(); the tool-layer allowlist was local-only and raised on cloud, so it is no longer a public knob (it stays internal to localchat(doc_id=)).get_documentper id. Names are unique per library (uploads suffix a taken name), so the same-name shadow check and its full-library listing sweep went (10 s → 1 s on a 749-doc cloud library). The cloud metadata endpoint carriesmetadata, so localget_document()gained that key for parity.Folders (#499, rebased in)
The managed
/chat/completionsalready takesfolder_idand renders a folder block ahead of the document block (computeapi.py:491,:3182); the SDK now sends it and mirrors it.client.folder_context(folder_id): the folder analogue ofdocument_context(): the folder's name, an id/name/description metadata row (the server'sFolderContext), and the directive to discover its documents withbrowse_documents/search_documents(folder_id=..., recursive=true). Onelist_folders()call: the endpoint has no LIMIT, lists the sameuserId+api_surfacepartition the managed chat validates against, and carries the same plan gate, so a non-Max key fails here rather than mid-run. Cloud-only: a local client raises.chat(folder_id=)(alsochat_completions()and theresponses/messageslanes): the managed lane sends the field and lets the server render and gate it; the own-model lanes lead the conversation with one user message, folder block then document block, joined as the server joins them.folder_idjoins the prompt-cache key likedoc_id."root"(and"") is the library itself:folder_contextreturns"",chatplaces nothing, on every lane and in every mode, as the server treatsfolder_id="root".folder_idis keyword-only onchat()and trailing on every other signature: no positional call shifts.Why
System prompt = who you are and how you work. A per-request target there breaks the cached prefix, sticks across turns, and on cloud splices SDK text onto the server-served instructions. The cloud's own managed chat (compute
api.py) and the web app both place document context in the user turn; this aligns the SDK with them.Verification
python -m pytest tests: 480 passed. Without-frameworks CI leg simulated (agents / anthropic / claude_agent_sdk blocked): green.chat_completions(),protocol="responses",protocol="messages", OpenAI Agents, Anthropic tool_runner, Claude Agent SDK}: 14/14 answered from the targeted document; every tool call carried the exactdoc_name, no browse detours.chat(q, folder_id=F)lists the folder's documents,folder_id="root"answers normally, own-model over cloud tools callsbrowse_documents(folder_id=F, recursive=true)first and gives the same list.Follow-ups (not in this PR)
sdk/agents.mdx: dropdoc_idfrom theagent_instructions()/ tool-helper examples and tables, documentdocument_context(); land with the release. The folder docs (chat.mdx, documents.mdx) are in VectifyAI/pageindex-docs#53.get_document()now returnsmetadatalocally too. Plusfolder_context()andchat(folder_id=).metadatamixes internal keys (thumbnailKey, …) into user tags; compute's managed chat dumpsnext_steps/user_idinto its own block; compute'sjson.dumpsescapes non-ASCII names while the web app and SDK keep them raw.https://claude.ai/code/session_01VxguPoTv2BmzS9d3erzTrS
https://claude.ai/code/session_01NsQjN5HMmmX4n5NVsZWYaw