Skip to content

fixing::branch-issueFeat/tool loader#922

Closed
theonlychant wants to merge 9 commits intoamd:mainfrom
theonlychant:feat/tool-loader
Closed

fixing::branch-issueFeat/tool loader#922
theonlychant wants to merge 9 commits intoamd:mainfrom
theonlychant:feat/tool-loader

Conversation

@theonlychant
Copy link
Copy Markdown
Contributor

Changes

@theonlychant
Copy link
Copy Markdown
Contributor Author

@kovtcharov-amd and @kovtcharov I made the PR less messy here sorry for the confusion

Copy link
Copy Markdown
Collaborator

@itomek itomek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@theonlychant — thanks for the work here, and for the cleanup pass — appreciated. Want to help you land this, so a bit of guidance on shape:

Right now this PR bundles three independent efforts and the description doesn't link any issue or describe what's being implemented, which makes it hard to review against acceptance criteria. All three efforts have (or overlap with) tracked issues:

  • Tool loader#688 (has concrete acceptance criteria, including a token-reduction target).
  • Synthetic mbox fixtures#848.
  • gaia/connections/ OAuth → overlaps directly with #915, which is active design work for Google OAuth via PKCE.

Suggested path forward

  1. Scope this PR down to the mbox fixtures only (closes #848). It's purely additive test data, has its own tests, and carries no runtime risk — easiest first win.
  2. Drop src/gaia/connections/ from this PR. The OAuth/PKCE design is already being worked in #915, so this subtree would need to be re-aligned with that work anyway. Easier to remove now than reconcile later.
  3. Open a follow-up PR for the tool-loader linked to #688. Before that PR, please add a comment on #688 with your proposed bundle design (keyword regex set, session vs. always policies) so we can agree on the shape. #688 also asks for a token-reduction measurement — worth including in the follow-up PR.

Before re-requesting review, please

  • Update the PR title to something like feat(email): synthetic .mbox dataset for email triage tests (conventional commits style, under ~70 chars).
  • Link the issue in the PR description with Closes #848 so it auto-links and auto-closes on merge.
  • Rewrite the PR body to explain what is being implemented and why GAIA needs it. A short Summary + Test plan is the standard shape — see #905 (feature with a clear "Why" section) and #817 (small scoped change, still explains the why) for examples.

Quick technical note for the tool-loader follow-up (not blocking this PR)

ToolLoader.reset_session() is defined but never called from the chat agent, so session bundles stay activated for the process lifetime. _keywords_match silently degrades a bad regex to a substring match — please validate at register_bundle time and raise instead (per the "fail loudly" rule in CLAUDE.md). Filesystem keyword regexes (\.[a-z]{1,5}\b, [/\\]) over-match conversational text like e.g., and/or, 2/3.

Happy to re-review once the PR is scoped to #848. Thanks again for sticking with it.

@theonlychant
Copy link
Copy Markdown
Contributor Author

theonlychant commented Apr 30, 2026

@theonlychant — thanks for the work here, and for the cleanup pass — appreciated. Want to help you land this, so a bit of guidance on shape:

Right now this PR bundles three independent efforts and the description doesn't link any issue or describe what's being implemented, which makes it hard to review against acceptance criteria. All three efforts have (or overlap with) tracked issues:

Suggested path forward

  1. Scope this PR down to the mbox fixtures only (closes feat(email): synthetic .mbox dataset for email triage agent testing #848). It's purely additive test data, has its own tests, and carries no runtime risk — easiest first win.
  2. Drop src/gaia/connections/ from this PR. The OAuth/PKCE design is already being worked in feat: Google account connection for AgentUI (OAuth via PKCE) #915, so this subtree would need to be re-aligned with that work anyway. Easier to remove now than reconcile later.
  3. Open a follow-up PR for the tool-loader linked to Dynamic tool loading based on conversation context via memory #688. Before that PR, please add a comment on Dynamic tool loading based on conversation context via memory #688 with your proposed bundle design (keyword regex set, session vs. always policies) so we can agree on the shape. Dynamic tool loading based on conversation context via memory #688 also asks for a token-reduction measurement — worth including in the follow-up PR.

Before re-requesting review, please

Quick technical note for the tool-loader follow-up (not blocking this PR)

ToolLoader.reset_session() is defined but never called from the chat agent, so session bundles stay activated for the process lifetime. _keywords_match silently degrades a bad regex to a substring match — please validate at register_bundle time and raise instead (per the "fail loudly" rule in CLAUDE.md). Filesystem keyword regexes (\.[a-z]{1,5}\b, [/\\]) over-match conversational text like e.g., and/or, 2/3.

Happy to re-review once the PR is scoped to #848. Thanks again for sticking with it.

Ok I resolved it in #928

@theonlychant theonlychant deleted the feat/tool-loader branch May 4, 2026 23:10
@theonlychant theonlychant restored the feat/tool-loader branch May 5, 2026 01:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

agents tests Test changes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants