[theonlychant]

…hpad/memory collision via #688 dynamic loading #800

Changes

[kovtcharov]

@claude please review this PR.

[github-actions]

PR Review — #811 (Tool-registry scaling / dynamic tool loading)

Thanks for tackling #688 — the bundle-gated prompt approach is the right idea, and isolating it behind an opt-in ToolLoader on the base Agent keeps the blast radius small. Registry stays the source of truth, unbundled tools stay visible, subclasses that don't opt in are unaffected. Nice shape. 👍

A handful of things to address before this is merge-ready.

🟡 PR hygiene

• Title is provisional — "trying to address the issue in::Tool-registry scaling: resolve scratc…" should be tightened to a conventional-commits title, e.g. feat(agents): add ToolLoader for bundle-gated tool visibility (#688).
• Body is empty (the three - bullets are the template placeholder). Per CLAUDE.md → "PR Descriptions — Tight and Value-Focused": lead with why (12K→~3-4K token reduction in typical sessions, resolves scratchpad/memory collision) and include a short test plan.
• Branch is main (headRefName: main). Opening a PR from your fork's main into upstream main makes follow-up pushes messy — future reviews would benefit from a feature branch (e.g. feat/tool-loader).

🟡 Silent fallback in _keywords_match — CLAUDE.md violation

src/gaia/agents/base/tool_loader.py:370-377:

try:
    if re.search(pattern, message, re.IGNORECASE):
        return True
except re.error:
    # Treat bad regex as a plain substring match
    if pattern.lower() in message.lower():
        return True

This is exactly the pattern CLAUDE.md → "No Silent Fallbacks — Fail Loudly" prohibits. A broken regex silently degrades to substring matching — a bundle author will never learn they shipped r"file|(" that only partially works. Validate the patterns at register_bundle() time and let re.error propagate with the bundle name in the message:

for pattern in bundle.keywords:
    try:
        re.compile(pattern)
    except re.error as e:
        raise ValueError(
            f"invalid keyword pattern {pattern!r} in bundle {bundle.name!r}: {e}"
        ) from e

🟡 Encapsulation break — poking at _state

src/gaia/agents/chat/agent.py:472:

if hasattr(self, "rag") and self.rag and getattr(self.rag, "indexed_files", None):
    self.tool_loader._state["rag"].activated = True

Reaching into ToolLoader._state from a subclass ties ChatAgent to the loader's internals. Add a public ToolLoader.mark_activated(bundle_name: str) (or pre_activate(bundle_name)) that raises KeyError if the name is unknown — then the ChatAgent code becomes self.tool_loader.mark_activated("rag"), and the private dict can be refactored without touching agents.

🟡 reset_session() isn't wired anywhere

ToolLoader.reset_session() exists (tool_loader.py:356) but nothing calls it. For a long-lived Agent instance handling multiple conversations, session bundles (scratchpad, memory, MCP) stay warm forever — potentially re-introducing the very collision #688 is trying to solve. Either:

• document that subclasses are responsible for calling it at conversation boundaries, or
• hook it into whatever the Agent's "new conversation" signal is (e.g. from process_query when conversation history is empty, or an explicit Agent.reset() method).

A test covering "two consecutive conversations with the same Agent instance" would catch the regression.

🟡 Docs missing

Per CLAUDE.md → "Documentation Requirements": "Every new feature must be documented." ToolLoader is a new public API on Agent — subclass authors need to know it exists and how to define bundles. At minimum:

• Add a section to docs/sdk/core/agent-system.mdx or docs/sdk/core/tools.mdx covering ToolBundle, the three activation policies, and a small example.
• Register it in docs/docs.json navigation if it warrants its own page.

🟢 Tool-in-multiple-bundles is silent (minor)

tool_loader.py:240-241 — _tool_to_bundle[tool_name] = bundle.name overwrites. If someone accidentally puts search_web in both browser and rag, the last-registered bundle wins for activation purposes and the earlier assignment becomes a ghost. Two options: (a) warn/raise on duplicate tool registration, or (b) make _tool_to_bundle a dict[str, list[str]] and use all owning bundles for session-warm tracking. (a) is simpler and fits "fail loudly."

🟢 Typing consistency (nit)

tool_loader.py uses from __future__ import annotations but still imports FrozenSet, List, Optional from typing. Since GAIA targets 3.10+, the modern form (frozenset[str], list[str], str | None) reads cleaner and matches the rest of the repo. Not blocking.

🟢 Prompt-cache invalidation — worth noting

resolve_tools_for_turn rebuilds _system_prompt_cache on every turn (agent.py:491-494) whenever the bundle set shifts. This saves output tokens but invalidates any LLM-side prefix cache for the system prompt on turns that change the active tool set. For keyword bundles this is expected; for session bundles the prompt is stable once activated, so impact should be minimal. Consider logging a bundle_delta at debug level so we can measure cache-hit regressions in eval runs later.

Tests

Good coverage on the loader in isolation (477 lines, 6 test classes, the disambiguation tests for scratchpad-vs-memory are the right shape). To round out:

• Integration test: instantiate a real Agent subclass with a tool_loader, call resolve_tools_for_turn, assert the rebuilt system prompt contains only the expected tool names.
• reset_session integration: two process_query calls, assert session bundles reset between them.
• tool_loader is None path: the hasattr/early-return branch in resolve_tools_for_turn isn't covered.

Summary

Design is sound and the PR is small and focused. Blocking items before merge:

1. Fix the silent-fallback in _keywords_match (CLAUDE.md rule).
2. Clean up PR title/body and move off main branch.
3. Add a public API for pre-activating bundles (no more _state["rag"] poking).
4. Decide on reset_session wiring and document it.
5. Add SDK docs for ToolBundle / ToolLoader.

Happy to re-review once those are in. Tagging @kovtcharov-amd for the architecture / API-shape call on items 3 and 4.

[theonlychant]

the PR changes is to address the issue in issue(feat: Google account connection for AgentUI (OAuth via PKCE) #915

[itomek]

@theonlychant Thanks for the contribution — appreciate the engagement on multiple issues. The PKCE primitives in pkce.py are correct, and the tool_loader design (bundle-gated activation) is the right shape for #688.

The bot review above covered the tool_loader portion in detail; I won't restate those points. A few structural things that need to be resolved before this can land:

This PR bundles three unrelated work streams

Looking at the non-merge commits in this branch:

• 2f79fa97 — Tool-registry scaling (#688)
• 04d18ca5 — Google connections / OAuth scaffold (#915)
• 2f2523bb — Synthetic mbox dataset (#848)

These are three separate issues. They need to be three separate PRs — mixing them means a reviewer who blocks on one stream blocks the others, and the diff is hard to follow.

On the connections/ work for #915 — please back it out

Heads-up that #915 is already in flight on our side and a PR will land in the next day or two with the full implementation (token refresh, loopback server, FastAPI routes, AgentUI Settings page, keychain round-trip, SSE events). Your contribution is genuinely appreciated, but to avoid a merge collision and divergent designs, the cleanest path is to drop the src/gaia/connections/ files and the tests/unit/test_pkce.py test from this PR. Once our PR lands you'll see the architecture and can build the next feature on top of that foundation if you'd like.

The diff is mostly stale-branch noise

The PR shows +6017/-933 across 70 files, but the working branch is your fork's main, which is far behind upstream. Roughly 25 of those 70 file changes are already-merged work showing up as "new" because the merge base is stale. Examples:

• schemas/agent-manifest.schema.json (deleted) and util/gen_manifest_schema.py (deleted) — already removed by #914
• AGENTS.md (+168) — already added by #904
• cpp/benchmarks/bench_utils.h (+7) — already changed by #874
• MANIFEST.in (+16), .github/workflows/publish.yml, .github/workflows/pypi.yml — overlap with #908/#909
• .claude/agents/*.md — already updated in main
• Various docs under docs/sdk/ and docs/spec/ — overlap with #918

A reviewer can't tell genuine new lines from already-merged ones without redoing the rebase mentally, and pushing further commits from your-fork:main will keep compounding this.

Recommended path forward

Close this PR and reopen as two focused PRs from feature branches in your fork:

1. feat/tool-loader for #688 — addressing the bot's blocking items above.
2. feat/email-mbox-fixtures for #848 — the synthetic mbox fixture and only the tests it enables.

Skip connections/ in your queue — we're handling #915.

Before branching, pull upstream amd:main into your fork's main so the stale-diff problem doesn't follow you into the new branches.

Thanks again for the work — the design instincts on tool_loader are good, and once #915 lands you'll have a fresh codebase to build on.

[theonlychant]

Alright yea I don’t know why it pushed it here I’ll make a new PR sorry my apologies

@theonlychant Thanks for the contribution — appreciate the engagement on multiple issues. The PKCE primitives in pkce.py are correct, and the tool_loader design (bundle-gated activation) is the right shape for #688.

The bot review above covered the tool_loader portion in detail; I won't restate those points. A few structural things that need to be resolved before this can land:

This PR bundles three unrelated work streams

Looking at the non-merge commits in this branch:

• 2f79fa97 — Tool-registry scaling (Dynamic tool loading based on conversation context via memory #688)
• 04d18ca5 — Google connections / OAuth scaffold (feat: Google account connection for AgentUI (OAuth via PKCE) #915)
• 2f2523bb — Synthetic mbox dataset (feat(email): synthetic .mbox dataset for email triage agent testing #848)

These are three separate issues. They need to be three separate PRs — mixing them means a reviewer who blocks on one stream blocks the others, and the diff is hard to follow.

On the connections/ work for #915 — please back it out

Heads-up that #915 is already in flight on our side and a PR will land in the next day or two with the full implementation (token refresh, loopback server, FastAPI routes, AgentUI Settings page, keychain round-trip, SSE events). Your contribution is genuinely appreciated, but to avoid a merge collision and divergent designs, the cleanest path is to drop the src/gaia/connections/ files and the tests/unit/test_pkce.py test from this PR. Once our PR lands you'll see the architecture and can build the next feature on top of that foundation if you'd like.

The diff is mostly stale-branch noise

The PR shows +6017/-933 across 70 files, but the working branch is your fork's main, which is far behind upstream. Roughly 25 of those 70 file changes are already-merged work showing up as "new" because the merge base is stale. Examples:

• schemas/agent-manifest.schema.json (deleted) and util/gen_manifest_schema.py (deleted) — already removed by refactor(agents): remove YAML manifest agent support (#912) #914
• AGENTS.md (+168) — already added by docs(agents): add AGENTS.md — multi-agent coordination rules #904
• cpp/benchmarks/bench_utils.h (+7) — already changed by ci(cpp): widen memory_per_step_growth_kb threshold to 75% #874
• MANIFEST.in (+16), .github/workflows/publish.yml, .github/workflows/pypi.yml — overlap with feat(packaging): ship Agent UI dist/ in PyPI wheel #908/test(ui): regression coverage for renderer→backend port wiring #909
• .claude/agents/*.md — already updated in main
• Various docs under docs/sdk/ and docs/spec/ — overlap with docs(agents): clear stale YAML-manifest references after #914 #918

A reviewer can't tell genuine new lines from already-merged ones without redoing the rebase mentally, and pushing further commits from your-fork:main will keep compounding this.

Recommended path forward

Close this PR and reopen as two focused PRs from feature branches in your fork:

1. feat/tool-loader for Dynamic tool loading based on conversation context via memory #688 — addressing the bot's blocking items above.
2. feat/email-mbox-fixtures for feat(email): synthetic .mbox dataset for email triage agent testing #848 — the synthetic mbox fixture and only the tests it enables.

Skip connections/ in your queue — we're handling #915.

Before branching, pull upstream amd:main into your fork's main so the stale-diff problem doesn't follow you into the new branches.

Thanks again for the work — the design instincts on tool_loader are good, and once #915 lands you'll have a fresh codebase to build on.

Yea sorry my apologies I don’t know why it pushed it those changes here I’ll fix that

[itomek]

Yea sorry my apologies I don’t know why it pushed it those changes here I’ll fix that

@theonlychant no worries, and thanks for the contribution. Should I close this PR? Do you already have one that supersedes it?