[2witstudios]

Problem

A user asked the Global Assistant to generate three images. The model emitted three generate_image tool calls, all three failed, and it told the user "image gen isn't wired into the tool set yet on this environment."

Image generation is fully built and working — the OpenRouter backend, the admin gate, the credit check and the UI renderer are all correct. The only thing broken was tool exposure on the Global Assistant route.

apps/web/src/app/api/ai/global/[id]/messages/route.ts hand-rolled its own core/non-core split using only CORE_TOOL_NAMES (nine page/drive tools). generate_image is not one of them, so it landed in nonCoreTools and was never sent to the model as a callable tool — while its bare name was still injected into the system prompt by buildNonCoreToolNamesPrompt. The model read the name, called it directly, and the AI SDK rejected it as an unknown tool.

web_search on the same route had the identical latent defect. Both are fixed here.

The page-chat route already solved exactly this with an ALWAYS_UPFRONT_TOOLS set passed to applyToolExposureMode. The Global Assistant never got that treatment.

Changes

1. Make the tools callable (8134af3)

• New pure helper splitToolsForExposure(tools, alwaysUpfront) in apps/web/src/lib/ai/tools/tool-exposure.ts — the file that already owns this logic. A tool is upfront when CORE_TOOL_NAMES.has(name) || alwaysUpfront.has(name).
• The global route's two Object.fromEntries(...filter...) blocks collapse to one call to it.
• buildNonCoreToolNamesPrompt now receives the reduced nonCoreTools, so the upfront tools correctly stop being advertised as execute_tool targets.

2. Close the same dead end in the tool_search catalog (defb1f5)

Found while self-reviewing commit 1. The route still passed the full filteredAllTools as the tool_search catalog, so generate_image/web_search stayed discoverable there — but they are no longer in execute_tool's dispatch map.

TOOL_DISCOVERY_PROMPT (system-prompt.ts:32) tells the model: "All other tools ... call execute_tool({tool_name, parameters}) to run them. Use tool_search("select:tool_name") to get parameter schemas first." So a model running tool_search("image") would find generate_image and dispatch it via execute_tool → Unknown tool "generate_image". Same advertised-but-not-callable dead end, one level deeper.

• New pure helper excludeAlwaysUpfront(tools, alwaysUpfront), wired into the route's createToolSearchTool. Core tools stay searchable (they're directly callable, so a lookup is harmless).

3. Share the constant, and fix its rationale (60059cd)

From a four-angle review pass (reuse / simplification / efficiency / altitude) over commits 1–2:

• The rationale comment was factually wrong for this route. It was copied from chat/route.ts and claimed deferral "trips execute_tool's allowlist check". That check is enabledTools != null && !enabledTools.includes(name) — and the Global Assistant route never sets enabledTools in experimental_context, so it is a permanent no-op there. The real failure on this route is prompt advertising without schemas. Wrong reasoning attached to correct code is worse than none: a maintainer who verified the stated reason would correctly conclude the set was unnecessary and delete it, reintroducing the bug.
• The set was duplicated as a literal in two route files, kept in sync only by that comment. It is now exported once from tool-exposure.ts and imported by both api/ai/chat and api/ai/global/[id]/messages. The alwaysUpfront parameter stays, so a route wanting a different set still passes one — /v1/chat/completions keeps its deliberately narrower {web_search}. Behaviour unchanged: both literals were already identical.
• selectToolSearchCatalog → excludeAlwaysUpfront. It has two callers — the tool_search catalog and the deferral candidate set inside applyToolExposureMode — and the old name described only the first, making const deferrable = selectToolSearchCatalog(...) read as a contradiction.
• applyToolExposureMode derives nonCoreTools from splitToolsForExposure instead of re-filtering deferrable, so the deferral rule is defined once. Provably identical: !alwaysUpfront && !CORE ≡ !(CORE || alwaysUpfront), and both preserve input key order, so the object fed to buildNonCoreToolNamesPrompt and createExecuteTool is byte-identical.

Correction: the web_fetch claim in earlier revisions was wrong

Earlier revisions of this description (and two comments below) claimed web_fetch was a live page-chat bug being deferred. That was incorrect — retracted in b2560f9. Recording it here rather than quietly deleting it, since reviewers may have read it.

The claim came from a review finding I under-verified: I confirmed WEB_SEARCH_TOOLS = {web_search, web_fetch} (core/tool-filtering.ts:128) and inferred the rest. But web_fetch appears nowhere in chat/route.ts. It is never destructured as a runtime override (Step 2 takes only web_search and generate_image) and never re-added after the allowlist (Steps 4/4b), so it flows through filterToolsForAgentAllowlist like any ordinary tool. That yields two outcomes and no third:

• allowlist permits it (or is null) → present, and execute_tool's re-check passes → dispatches fine;
• allowlist omits it → removed entirely at Step 3 → not upfront, not in execute_tool's dispatch map, not in the tool_search catalog → the model never sees it.

There is no present-but-rejected state, so there is no bug to fix.

The actual membership rule for ALWAYS_UPFRONT_TOOLS

The correction is sharper than the mistake. Membership is not "shares a composer toggle" — it is:

a tool belongs iff it bypasses the agent allowlist on the way in (destructured out of the baseline before filterToolsForAgentAllowlist, re-added after it when the toggle is on).

That bypass is precisely what makes execute_tool's re-check reject the tool on the way out: it is present in the tool set but absent from enabledTools, so deferring it produces "not permitted" exactly when the user toggled it on. web_search and generate_image are the only two tools with that property. web_fetch has no bypass, so it has nothing to be rescued from — it is correctly absent, not deferred.

This rule is now stated in the constant's doc comment and pinned by an explicit expect(ALWAYS_UPFRONT_TOOLS.has('web_fetch')).toBe(false), so the set's boundary is asserted rather than merely described.

Scope guarantees

• Reachability only. filterToolsForImageGen and shouldExposeImageGen are untouched. Verified both gates remove the tool from the set entirely when off (tool-filtering.ts:250-259, 344-353), so ALWAYS_UPFRONT_TOOLS can only promote a tool that already survived the gate — it cannot widen who can generate images. image-gen-exposure.test.ts passes unmodified (verified: empty diff).
• No applyToolExposureMode wholesale swap on the global route. That route injects TOOL_DISCOVERY_PROMPT itself under an explicit "Stable order" comment (prompt-cache sensitive); the helper would emit a second copy and reorder it, and it short-circuits when there are no non-core tools, whereas the route always wants the execute_tool scaffolding.
• applyToolExposureMode's coreTools/upfrontOverrides spread left alone. It orders core-then-overrides, whereas splitToolsForExposure preserves input key order — collapsing them would change the tool array order sent to the provider, which is prompt-cache sensitive, so it would not be behaviour-identical.

Testing (TDD)

Both helpers were written RED first and observed failing before implementation:

SyntaxError: Export named 'splitToolsForExposure' not found in module '.../tool-exposure.ts'
SyntaxError: Export named 'selectToolSearchCatalog' not found in module '.../tool-exposure.ts'

splitToolsForExposure — 100% branch coverage (core=true short-circuit, core=false+always=true, core=false+always=false, explicit-empty vs defaulted alwaysUpfront, empty tool set): the four required cases plus "no tool appears in both halves; union equals the input".

excludeAlwaysUpfront — omits always-upfront tools, handles empty/defaulted args, plus an invariant test: every tool left in the catalog is either core (directly callable) or present in nonCoreTools (callable via execute_tool). Nothing may be discoverable yet unreachable.

ALWAYS_UPFRONT_TOOLS — pins the set's contents (so a new toggle can't be added to one route and forgotten in the other) and asserts its members are promoted past the core-only split.

Gates

Gate	Result
bun run typecheck	✅ 16/16 tasks
bun run lint	✅ 14/14 tasks
bun test .../tool-exposure.test.ts	✅ 23 pass
bun test .../image-gen-exposure.test.ts (unmodified)	✅ 5 pass
bun run test:unit — @pagespace/lib	✅ 407 files / 9209 tests
bun run test:unit — web	✅ 1041 files / 15713 tests, 1 pre-existing failure below

Reproducing locally: test:unit needs DATABASE_URL pointed at the migrated test Postgres (as scripts/test-with-db.sh does); without it ~24 DB-backed tests fail on relation "users" does not exist. Run typecheck and lint sequentially — both trigger web:build, and racing them leaves .next/types partially written, producing spurious TS6053 errors.

Pre-existing / flaky failures (not caused by this PR)

1. apps/web/src/lib/messages/__tests__/grouping.test.ts > isFirstInGroup > breaks the group when messages cross midnight even within the 5-minute window — AssertionError: expected false to be true. Verified pre-existing: the same test fails identically on master in a separate checkout, and this diff changes no files it touches. Timezone-dependent, environmental.
2. packages/lib/src/security/__tests__/distributed-rate-limit.integration.test.ts > weighted sliding window — failed once under full-suite parallel load, then passed 7/7 twice in isolation. Timing-sensitive flake; this diff touches zero packages/lib files.
3. bun run test:security locally reports 6/50 suites failing (Session Service, Device Auth Utilities, Permissions, Login Route, Signup Route, Mobile Login) — all auth/session suites, none touching AI tool exposure. Running the identical command on master in a separate checkout fails 9/50, a strict superset of the same six. So this branch introduces zero new security-suite failures, and CI's Security Test Suite check passes on the PR.

Files changed

• apps/web/src/lib/ai/tools/tool-exposure.ts
• apps/web/src/app/api/ai/global/[id]/messages/route.ts
• apps/web/src/app/api/ai/chat/route.ts (import shared constant; literal removed)
• apps/web/src/lib/ai/tools/__tests__/tool-exposure.test.ts

🤖 Generated with Claude Code

https://claude.ai/code/session_018ukUtoKKTgQDH35docNcE6

[coderabbitai]

📝 Walkthrough

Walkthrough

The change adds reusable helpers for always-upfront tool exposure, applies them to the global messages route for web_search and generate_image, and adds tests for partitioning, catalog filtering, overlap handling, and dispatch reachability.

Changes

Tool exposure override

Layer / File(s)	Summary
Exposure partitioning and catalog helpers apps/web/src/lib/ai/tools/tool-exposure.ts, apps/web/src/lib/ai/tools/__tests__/tool-exposure.test.ts	Adds helpers for splitting upfront versus deferred tools and filtering always-upfront tools from tool_search; tests cover fallback behavior, overlap, empty sets, and reachability.
Global route exposure wiring apps/web/src/app/api/ai/global/[id]/messages/route.ts	Configures web_search and generate_image as always-upfront tools and uses the helpers for runtime exposure and tool_search catalog construction.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Sequence Diagram(s)

sequenceDiagram
  participant GlobalMessagesRoute
  participant ExposureHelpers
  participant Model
  participant tool_search
  participant execute_tool
  GlobalMessagesRoute->>ExposureHelpers: Split filtered tools with always-upfront overrides
  ExposureHelpers-->>GlobalMessagesRoute: Return coreTools and nonCoreTools
  GlobalMessagesRoute->>Model: Expose direct and deferred tools
  Model->>tool_search: Search deferred tool catalog
  tool_search-->>Model: Return searchable tools
  Model->>execute_tool: Dispatch selected deferred tool

Loading

Possibly related PRs

• 2witstudios/PageSpace#1306: Changes global-route tool_search injection and advertised tools.
• 2witstudios/PageSpace#1310: Adjusts global-route core/non-core exposure and dispatchable catalogs.
• 2witstudios/PageSpace#1462: Introduces related per-agent always-upfront exposure and searchable catalog wiring.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and accurately summarizes the main change: making generate_image and web_search callable on the Global Assistant.

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch pu/global-tool-exposure

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[2witstudios]

@coderabbitai review

[coderabbitai]

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[2witstudios]

@coderabbitai review

[coderabbitai]

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[2witstudios]

Review-pass summary (for human reviewers)

Automated review returned no findings, so I ran a four-angle critique pass (reuse / simplification / efficiency / altitude) over my own diff and acted on it. Three things are worth a reviewer's attention:

1. I had written a factually wrong comment — now fixed. Commit 1 copied the rationale from chat/route.ts, claiming deferral "trips execute_tool's allowlist check." That check is enabledTools != null && !enabledTools.includes(name), and this route never sets enabledTools in experimental_context — the check is a permanent no-op here. The real failure mode on the Global Assistant is that a deferred tool is advertised by name in the system prompt while its schema never reaches the model. The comment now states each route's actual failure mode. Wrong reasoning attached to correct code is a trap: a maintainer verifying the stated reason would conclude the set is unnecessary and delete it.

2. ALWAYS_UPFRONT_TOOLS is now shared, not duplicated. It was a literal in two route files kept in sync by prose. It's exported once from tool-exposure.ts; the alwaysUpfront parameter stays, so /v1/chat/completions keeps its deliberately narrower {web_search}. Both literals were already identical, so behaviour is unchanged.

3. A real adjacent bug I found and deliberately did NOT fix — please sanity-check this call.

The web-search toggle gates two tools — WEB_SEARCH_TOOLS = new Set(['web_search', 'web_fetch']) (core/tool-filtering.ts:128) — but web_fetch is in no always-upfront set.

• On page chat this is a live instance of this same bug: that route passes the agent's saved enabledTools to execute_tool (chat/route.ts:1580), so an agent toggling web search on whose allowlist omits web_fetch gets it rejected at dispatch.
• On the Global Assistant it's harmless — no allowlist is set, so web_fetch still dispatches fine.

I left it out because promoting it changes page-chat behaviour and upfront context on a route this PR otherwise only refactors, and it deserves its own tests. It's recorded in the shared constant's doc comment so it can't be lost. Happy to fold it in here if you'd rather.

Also flagged but not fixed: the admin global-prompt viewer (complete-request-builder.ts:144) applies only filterToolsForReadOnly, so it lists these tools unconditionally regardless of gate/toggle state. That inaccuracy predates this PR and fixing it needs a product decision about which toggle state a preview should assume.

Validation: typecheck 16/16, lint 14/14, @pagespace/lib 9209 tests, web 15713 tests. Two non-PR failures are documented in the description (one pre-existing TZ test verified failing identically on master, one timing flake that passes 7/7 in isolation).

[2witstudios]

Additional findings from adversarial verification (all pre-existing, none fixed here)

I had the riskiest claim in this PR — that commit 3's change to applyToolExposureMode is byte-identical, given it affects page chat and /v1/chat/completions — adversarially refuted rather than assumed. It survived (details now in the PR description). Two unrelated pre-existing nits turned up while tracing it, logged here so they aren't lost:

1. Unguarded prototype-chain lookups on tool maps. allowedTools[tool_name] (tools/execute-tool.ts:23) and tools[n] (tools/tool-search-tool.ts:28) have no Object.hasOwn guard, so tool_name: "constructor" resolves to a prototype member. Both dead-end harmlessly on the !t.execute check today, so it's hardening rather than a live bug.

2. clientToolSet keys are caller-controlled with no charset validation. In /v1/chat/completions, clientToolSet[def.function.name] = tool({...}) is guarded only by typeof name !== 'string' || !name (openai-api/validate-inference-request.ts:247-248) — no regex, no length cap. An integer-like name like "0" becomes a real key and JS hoists it to the front of iteration order, which would violate the "key order follows the input's key order" contract documented on the new helpers. It does not reach applyToolExposureMode (client tools merge after the exposure call), so it's not a defect in this PR — but it's the one input class that could break the ordering guarantee downstream.

Neither is introduced by this PR and both are outside its mission, so I've left them for separate changes rather than widening the diff further — same call I made on web_fetch above.

Local security suite note: bun run test:security reports 6/50 suites failing on this branch (all auth/session). The same command on master fails 9/50 — a strict superset of the same six. Zero new failures from this PR, and CI's Security Test Suite check is green.

[2witstudios]

Followed up on my own web_fetch question above rather than leaving it as a punt — the answer is more interesting than "out of scope."

By the shared constant's own definition web_fetch should be in the set. The reason not to add it is that it would be the wrong trade for the shared set:

• Page chat: genuinely broken (allowlist rejects it at dispatch).
• Global Assistant: not broken — no allowlist is set, so it still dispatches through execute_tool. Adding it there only spends context on the one surface deliberately engineered to keep upfront schemas small (feat(ai): execute_tool dispatcher — drop 29 non-core tool schemas from model context #1310 dropped 29 schemas for ~5-12k tokens/turn).

So promoting it in the shared default would charge the Global Assistant for a page-chat-only fix. The likely-correct fix is page-chat-specific — and this design already allows it with no further change, since alwaysUpfront stays a per-call parameter: that route can pass new Set([...ALWAYS_UPFRONT_TOOLS, 'web_fetch']) on its own.

That reasoning is now in the constant's doc comment (3c44af7, comment-only). This also answers the fair question of whether sharing the constant was the right altitude given the two routes may want different sets: the shared value is only the default, and the parameter is still per-route, so it doesn't block the divergence.

[2witstudios]

@coderabbitai review

[coderabbitai]

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[2witstudios]

@coderabbitai full review

[coderabbitai]

✅ Action performed

Full review finished.

---

Your included review limit is currently reached under our Fair Usage Limits Policy. This review may still proceed through usage-based billing if eligible. Your next included review will be available in 24 minutes.

[2witstudios]

Correction — retracting my web_fetch claim above

Two comments above I said web_fetch was a live bug on page chat, and asked you to sanity-check my decision to defer it. The claim was wrong. Retracted in b2560f9; flagging it explicitly because I asked you to weigh in on it.

I took it from a review finding, verified the easy half (WEB_SEARCH_TOOLS = {web_search, web_fetch}, true), and never checked the half that mattered: whether the toggle override re-adds web_fetch. It doesn't — web_fetch appears nowhere in chat/route.ts. It's never destructured as an override (Step 2 takes only web_search/generate_image) and never re-added after the allowlist, so it goes through filterToolsForAgentAllowlist normally:

• allowlist permits it → present, execute_tool's re-check passes → works;
• allowlist omits it → dropped entirely at Step 3 → never advertised, never dispatchable.

No present-but-rejected state, so no bug. No action needed from you on this — sorry for the misdirected ask.

The upside is that the correction gives the set a precise boundary it didn't have before. Membership isn't "shares a composer toggle" — it's "bypasses the agent allowlist on the way in": destructured before filterToolsForAgentAllowlist, re-added after it. That bypass is exactly what makes execute_tool's re-check reject the tool on the way out, which is the bug this PR fixes. web_search and generate_image are the only two tools with that property.

That rule now lives in the constant's doc comment and is pinned by expect(ALWAYS_UPFRONT_TOOLS.has('web_fetch')).toBe(false), so the boundary is asserted, not just asserted-in-prose.

The irony isn't lost on me: commit 60059cd in this same PR fixed a comment that stated a plausible-but-false rationale, on the grounds that wrong reasoning attached to correct code is worse than none. I then did the same thing one commit later. The code was always correct — only my explanation of it was wrong.