Skip to content

fix(*): give web search its config section so maxResults takes effect - #314

Open
Handsome-wzw wants to merge 1 commit into
feat/tool_capability_inventory_ghfrom
fix/web_search_takes_its_config_section
Open

fix(*): give web search its config section so maxResults takes effect#314
Handsome-wzw wants to merge 1 commit into
feat/tool_capability_inventory_ghfrom
fix/web_search_takes_its_config_section

Conversation

@Handsome-wzw

Copy link
Copy Markdown

Summary

Stacked on #312 -- set the base branch to feat/tool_capability_inventory_gh,
not main. Against main the diff would carry that PR's commits as well. It
closes the naming nit raised in review there, so it is a follow-up rather than
an independent change.

tools.web.search.maxResults has been declared in the schema and read by
nobody. Both registration sites built the tool from the key alone:

WebSearchTool(api_key=self.brave_api_key, proxy=self.web_proxy)

so it fell back to its own default of 5, and a deployer who set the field got no
effect and no warning that the setting was inert.

One cause, two symptoms

Web search was the one tool handed a single field instead of its config section.
Every neighbour takes a section -- media_config, exec_config,
sandbox_config, context_config -- and all three CLI entry points already
held config.tools.web.search and reached into it to pull one value out:

brave_api_key=config.tools.web.search.api_key or None,

Passing the section is what wires the field. It also retires the parameter name
in the same move, which is the second symptom: brave_api_key has named a
Serper key ever since the Brave backend went away. That was flagged in review on
#312 as pre-existing and misleading, correctly, and it is not a separate rename
-- passing one field out of a section is exactly why the parameter ended up
named after a vendor rather than after what it configures.

WebSearchTool keeps its own signature. It is still constructible from a bare
key, which its unit tests and the error-path test rely on; only the callers that
have a config section now pass one.

The sub-agent surface

Same change there, and a test of its own. This matters more than it looks: a
first mutation pass caught the main loop dropping max_results and said nothing
when the sub-agent dropped it, because nothing was watching that call site. Two
registration sites with a fix applied to one is the shape this area keeps
taking, and it is the same gap the gate in #312 had to close by hand.

Type

  • Fix
  • Feature
  • Docs
  • CI / tooling
  • Refactor
  • Other

The parameter rename is a refactor riding along, not a second topic: it is the
same edit that wires the field.

Verification

uv run --all-extras pytest -q                        6188 passed, 35 skipped, 1 failed
uv run pytest tests/test_agent_loop_web_tools.py \
              tests/test_tool_capabilities.py \
              tests/test_cli_doctor_commands.py      69 passed
uv run --extra dev ruff check raven tests scripts    All checks passed
uv run --extra dev ruff format --check               817 files already formatted
npx commitlint --from main --to HEAD                 exit 0
scripts/check_commit_messages.py main..HEAD          exit 0

The one failure is test_read_file_image.py::test_an_attachment_that_cannot_be_ read_costs_a_note_not_the_turn, which fails the same way on main with these
commits absent: it makes a file unreadable with chmod 000, which does not
block root, so it fails for anyone running the suite as root and passes in CI.
#313 is the fix. This branch touches neither that file nor the code under it.

Five mutations, each caught:

main loop drops max_results again        1 failed
sub-agent drops max_results again        1 failed
main loop ignores the section's key      4 failed
sub-agent ignores the section's key      2 failed
the web_search gate is removed           3 failed

The second line is the one worth reading. Before its test existed that mutation
passed clean -- the wiring was fixed at both call sites and asserted at one.

  • Relevant tests pass locally
  • Relevant lint / type checks pass locally
  • User-facing docs or screenshots are updated when needed

Risk

One behaviour change, and it is the point: tools.web.search.maxResults now
reaches the tool. An install that set it was getting 5 results and now gets what
it asked for. An install that never set it is unaffected -- the schema default
is 5, which is also the value the tool fell back to. The per-call count
argument still wins over both, unchanged.

brave_api_key is gone from AgentLoop and SubagentManager. Both are
constructed inside this repo only -- the three CLI entry points and the tests --
and all were updated; no deprecation shim, since nothing external names it.

Rollback is a revert. No configuration is read differently beyond the field that
was already meant to be read.

  • Security impact considered
  • Backward compatibility considered
  • Rollback path is clear for risky changes

Related Issues

N/A

`tools.web.search.maxResults` has been declared in the schema and read by
nobody. Both registration sites built the tool from the key alone, so it fell
back to its own default of 5 and a deployer who set the field got no effect and
no warning.

The cause is that web search was the one tool handed a single field instead of
its config section. Every neighbour takes a section -- media_config,
exec_config, sandbox_config, context_config -- and the three CLI entry points
already held `config.tools.web.search` and reached into it to pull one value
out. Passing the section instead is what wires the field, and it retires the
`brave_api_key` parameter name in the same move: it has named a Serper key ever
since the Brave backend went away, which a reviewer flagged on the capability
PR as pre-existing and misleading.

`WebSearchTool` keeps its own signature. It is still constructible from a bare
key, which is what its unit tests and the error-path test rely on; only the
callers that have a config section now pass one.

The sub-agent surface gets the same treatment, and a test of its own. A first
mutation pass caught the main loop dropping `max_results` and said nothing when
the sub-agent did, which is the shape this area keeps taking: two registration
sites, a fix applied to one. Five mutations now, all caught.

Co-authored-by: Claude (claude-opus-5) <noreply@anthropic.com>
Sign up for free to 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