Repository navigation
refactor(search)!: use abnegate-search for chat web search - #113
Merged
Merged
Conversation
std::env::set_var and remove_var are unsound while another thread reads the environment, and cargo test runs tests on threads. zone_core::vpn::Hold, config's tests and the local embedding provider's test each guarded their changes with a lock of their own, so a test could rewrite variables while another in the same binary read them, and the SAFETY comments claimed an exclusion none of them had. zone_core::variables::Variables is now the one guard: it takes a single process-wide lock, records each variable before its first change and restores it before the lock is released. Hold keeps its constructors but holds a Variables, and hands it out so a test can change more variables under the same lock. Variables::install sets a value a whole test binary shares under the same lock. Config's, vpn's, the run_command environment test, the local embedding cache test and the chat agent and handover suites change variables only through it. Safe set and remove over unsafe calls must not reach production, where worker threads read the environment without the lock, so variables and Hold compile only for zone_core's own tests and its new test-support feature, which zone_context and zone_server enable as dev-dependencies. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
zone_search was extracted to abnegate/crates as abnegate-search, so the local copy goes and zone_server uses the published crate. The crate stays neutral about where SearXNG runs and starts switched off; zone keeps its own policy in services::search: the Gluetun query URL, search on unless SEARCH_ENABLE_WEB_SEARCH turns it off, and off while public web waits for the VPN (zone_core::vpn::allows_public, the same gate the old crate re-implemented). The seven VPN-gating tests move with it, changing the environment only under the Hold they already take. The crate reads the timeout from SEARCH_TIMEOUT_SECONDS. There is no fallback to SEARCH_TIMEOUT_SECS, and the config test pins that the old name no longer sets the timeout. The search observer now reports a typed Outcome, recorded under its stable label, which adds the disabled, decode_error and too_large statuses to zone_searxng_requests_total. Every result title, URL and snippet now passes through the crate's sanitizer, which redacts credential-shaped text such as a presigned URL's token before the model reads it; a web tool test pins that. The capability text names the search tool only generically, so the chat test now asserts that sentence and that an agent model is offered web_search. BREAKING CHANGE: SEARCH_TIMEOUT_SECS is no longer read; set SEARCH_TIMEOUT_SECONDS instead. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The manager container sees only the variables compose lists, so the search timeout could never be set in Docker: neither the old SEARCH_TIMEOUT_SECS nor the renamed SEARCH_TIMEOUT_SECONDS reached it. Listing it beside SEARCH_RESULT_COUNT, with the same 15-second default the server uses, makes the setting take effect there for the first time. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The renamed timeout had no entry in .env.example or CONFIGURATION.md, so an operator had no way to find it. Both now list it next to SEARCH_RESULT_COUNT, along with the 1-60 and 1-20 ranges the client holds the two settings to. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Clippy 1.98 flags Routing as a large enum: the Chat variant's Route makes every Routing at least 856 bytes, 216 more than the next largest variant, and clippy -D warnings fails zone_server on main. Boxing the route keeps the enum near the media variants' size; it is built once per turn, so the allocation costs nothing that matters. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Contributor
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. 🧰 Additional context used📚 Code guidelines (1)No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
⛔ Files ignored due to path filters (1)
📒 Files selected for processing (38)
💤 Files with no reviewable changes (7)
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 SummarySummary by CodeRabbit
WalkthroughZone replaces its local web-search crate with ChangesWeb search integration
Test environment guards
Priority: ➖ Normal Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant ConfigFromEnv
participant SearchService
participant AbnegateSearch
participant VpnAccess
participant WebAgent
ConfigFromEnv->>SearchService: load web-search settings
SearchService->>AbnegateSearch: read shared environment configuration
SearchService->>VpnAccess: check public access
SearchService->>ConfigFromEnv: return Zone search configuration
ConfigFromEnv->>WebAgent: provide search configuration
Merge Risk: ⚪ Minimal · up to The search migration and test-guard changes have no established merge-blocking issue. Normal checks remain appropriate before merging. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to The inspected application controls preserve VPN and offline restrictions. However, the shared client's parsing and request-limit guarantees could not be fully verified. The timeout-setting rename also requires configuration changes during upgrade and rollback. Retained concerns Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
Resilience and Maintainability Implications
Hardening Proposals
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 75.51% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 98 functions across 22 files. (9 skipped: 9 unsupported.)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
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. Comment |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Breaks your
.envSEARCH_TIMEOUT_SECS→SEARCH_TIMEOUT_SECONDS. The old name is no longer read and there is no fallback. Rename it in.envif you set it. Docker deployments were not affected before this PR, because compose never passed the old name to the manager. Compose now passesSEARCH_TIMEOUT_SECONDS, so the setting takes effect there for the first time.Summary
Moves zone's chat web search onto
abnegate-search("0.1.3", a caret whose floor is the tested version), the crate extracted fromrunner/zone_searchand published from abnegate/crates, and deletes the local copy (about 1,400 lines).zone_search::{SearxngClient, SearchContext, SearchHit, TimeRange, format_search_context, sanitize_query, needs_web_search}abnegate_search::*client::module path.SearchHitandWebSearchConfigare#[non_exhaustive], so zone builds them withSearchHit::new(..),WebSearchConfig::new(url)andwith_enabled(..).zone_search::WebSearchConfig::from_env(Gluetun URL, on by default, VPN gate)zone_server::services::search::{from_environment, defaults, DEFAULT_QUERY_URL}http://gluetun:8080/...query URL, search on unlessSEARCH_ENABLE_WEB_SEARCHturns it off, and search off while public web waits for the VPN. The VPN check now callszone_core::vpn::allows_public()instead of the copy the old crate kept.zone_search::observe_searches(fn(&'static str, ..))abnegate_search::observe_searches(fn(Outcome, ..))metrics::record_searxngtakes the typedOutcomeand recordsoutcome.as_str()as thestatuslabel.runner/Cargo.tomldropszone_searchfrommembers/default-membersand addsabnegate-search = "0.1.3"beside the otherabnegate-*carets.zone_server/Cargo.tomlswaps the path dependency for{ workspace = true }.Cargo.lockwas regenerated by cargo.The env rename also updates
.env.example,docker-compose.yml,docker-compose.dev.yml(now passingSEARCH_TIMEOUT_SECONDS=${SEARCH_TIMEOUT_SECONDS:-15}besideSEARCH_RESULT_COUNT) anddocs/CONFIGURATION.md.helm/**sets noSEARCH_*variables, so nothing changed there. TheREADME.mdtree and thezone_searchpaths indocs/SYSTEM-PROMPT-RESEARCH.mdnow point at the crate.The wrapper's tests change
SEARCH_*variables, andconfig::testschangesSEARCH_TIMEOUT_SECONDS, in the same test binary. Each used to guard its changes with a lock of its own (zone_core::vpn::Holdhad one,config::testsanother, the local embedding provider's test a third), so under threadedcargo testthey could race, and the// SAFETYcomments claimed an exclusion none of them had. The first commit addszone_core::variables::Variables: one process-wide lock, each variable saved before its first change and restored before the lock is released.vpn::Holdnow holds aVariables(same constructors, plusvariables()to change more variables under the same lock), andVariables::installsets a value a whole test binary shares (the chat agent suite'sZONE_CHAT_AGENT_CWD, the handover suite'sPATH) under the same lock.config::tests,vpn::tests, therun_commandenvironment test,services::search::tests, the local embedding cache test and those two suites change the environment only through it;git grep -n 'set_var\|remove_var' runnernow matches onlyzone_core/src/variables.rs. The helpersconfig::testsand the wrapper's tests each kept are gone.Variablesoffers safeset/removeover the unsafe calls, and production reads the environment on worker threads without its lock, so it must not reach a server or CLI build.variablesandHoldcompile only undercfg(any(test, feature = "test-support")).zone_core's newtest-supportfeature is enabled only byzone_context's andzone_server's dev-dependencies, and resolver 2 keeps it out of normal builds. Nothing outside tests usedHold. The module documentation states what the lock does not cover: a read that bypassesstd::env, such as libc'sgetenvinsidegetaddrinfo.Commits:
test(core): serialise every environment change behind one lockrefactor(search)!: use abnegate-search for chat web searchfix(compose): pass SEARCH_TIMEOUT_SECONDS to the manager. Compose never passed the timeout to the manager, under either name.docs(search): document SEARCH_TIMEOUT_SECONDSrefactor(chat): box the route a chat turn carries(see below)Behaviour differences
Each item is covered by a zone test or by the crate's own suite.
Configuration (tests in
services::search::testsandconfig::tests)SEARCH_*set, search is on (still subject to the VPN gate) and querieshttp://gluetun:8080/search?q=<query>&format=json. The crate's own default is off andhttp://127.0.0.1:8080/..., and the wrapper overrides both. Test configs useservices::search::defaults()(off, Gluetun URL), asWebSearchConfig::default()did before.defaults()builds the config withWebSearchConfig::newand does not pass it through the crate'sbounded()clamp; that is harmless, because its result count (5) and timeout (15s) are already in range, andSearxngClient::newclamps whatever it is given.SEARCH_TIMEOUT_SECS→SEARCH_TIMEOUT_SECONDS(see top).environment_matrix_validates_secrets_and_loads_server_settingspins both halves: the old name leaves the timeout at 15s, and the new name sets it.Duration(config.web_search.timeout), nottimeout_secs: u64. The client still holds it to 1–60s and the result count to 1–20, whatever built the config.WebSearchConfig'sDebugprints[REDACTED]for the query URL, which can carry a credential.Metrics (
metrics::tests::searxng_outcomes_keep_their_status_labels)zone_searxng_requests_totalandzone_searxng_duration_secondskeep the labelsok,empty_query,http_errorandstatus_error. Three statuses are new:disabled: a search attempted while switched off. It used to go unrecorded.decode_error: a body that is not SearXNG JSON. It used to count ashttp_error.too_large: a body over 1 MiB, which the old client read without a limit.chat-qualitydashboard groups bystatus, so the new labels show up as new series and nothing breaks.Client (crate suite)
status_error.zone-server/web-searchtoabnegate-search/<version>.Error::Disabled("Search is switched off") instead ofOffline("Public web access is offline until the VPN is enabled."). Zone checksenabledbefore it builds a client and maps any error to its own message, so the model and the user see no change. The VPN refusals inagent/web.rsstill come fromzone_core::vpn.Prompt text (crate suite;
chat_ws_testsupdated)The search capability now opens "The server can search the public web via SearXNG", not "Zone can search…". It refers to "a search tool" and "a page-fetching tool" generically, where it used to name
web_searchandfetch_url. Both tools still register under the same names with the same descriptions.chat_ws_testsnow asserts the new opening and the generic "When a search tool is among the callable tools" sentence, and checks that the agent model is offeredweb_search(on the wire or in the deferred tool list), since the capability text no longer names it.format_search_contextsanitises each result's title, URL and snippet:<and>become‹and›;A result can no longer close
<web_search_results>or forge a line that looks like another result. The surrounding wording is unchanged.Every title, URL and snippet also goes through
abnegate_secret::sanitize(cratesrc/context.rs,untrusted). It strips terminal escape sequences and invisible or bidi characters, and replaces credential-shaped substrings (token and signature query values, JWTs, URL userinfo, long high-entropy runs; hex digests are exempt) with[REDACTED]. A presigned result URL therefore reaches the model as…?token=[REDACTED], and afetch_urlon that text fails, where the old client passed the credential through. Citations are unaffected:identifyregisters each hit by its rawhit.url, so the identifier still names the address SearXNG returned. Pinned byagent::web::tests::a_credential_in_a_result_url_reaches_the_model_redacted.The
needs_web_searchheuristic, query sanitising, URL templating and thetime_rangeparameter (TimeRange::PARAM→TimeRange::PARAMETER, same wire name) are unchanged.Verification
Run in
runner/after touching every.rsfile, with output showing the crates built from this worktree. Unless noted, with--no-default-features --features zone_context/test-utils:cargo fmt --all --check: PASScargo clippy -p zone_core -p zone_context -p zone_server --all-targets -- -D warnings: PASScargo clippy -p zone_core -p zone_context -p zone_server --all-targets --all-features -- -D warnings(CI's form, scoped to the crates this PR touches): PASScargo check -p zone_core -p zone_server: PASS. Run with-v, it showszone_corebuilt with only--cfg 'feature="sqlx"', sovariablesandHoldare absent.cargo tree -e normal,build,features -i zone_coreshows notest-support, while-e all,featuresshows it enabled through the dev-dependency.cargo check --workspace --exclude zone_desktop --all-targets: PASScargo test(not nextest), so the environment tests share one process:cargo test -p zone_core --lib -- vpn:: variables:: shelling_tools_give_the_child_only_the_context_environment: PASS, 12/12.cargo test -p zone_context --lib --features local-embeddings -- adapters::web:: cache_dir_env_override: PASS, 25/25 (andcache_dir_env_overridealone, 1/1).cargo test -p zone_server --lib -- config:: services::search:: agent::web::: PASS, 71/71. The builtzone_servertest binary was then run 25 more times withconfig:: services::search:: agent::web:: vpn --test-threads=16: 25/25 PASS.zone_core::variables::tests::every_change_is_undone_to_the_value_before_the_first,zone_core::vpn::tests::a_hold_takes_the_lock_every_variables_guard_takes(fails ifHoldtakes any other lock),agent::web::tests::a_credential_in_a_result_url_reaches_the_model_redacted.zone-verify.sh … scoped -- -p zone_core -p zone_context -p zone_server --lib --test chat_search_tests --test chat_ws_tests --test knowledge_url_tests --test message_embedding_tests --test sync_tests --test chat_agent_tests --test chat_handover_testsagainst a throwaway Postgres: PASS, 4399/4399 (40 skipped)python3 scripts/test-compose-up.py(fakedocker/compose onPATH, starts no containers): PASS, 15 tests.docker compose --env-file .env.example -f docker-compose.yml [-f docker-compose.dev.yml] config(parses config only): PASS. The manager getsSEARCH_TIMEOUT_SECONDS: "15".git grep -n 'zone_search\|SEARCH_TIMEOUT_SECS\b'matches only the regression test inconfig.rsthat proves the old name is ignored.This branch also carries
refactor(chat): box the route a chat turn carries, cherry-picked unchanged frommigrate/sanitize(71de4d0); its patch is byte-identical.maincurrently failsclippy -D warningsonws::chat::Routing(large_enum_variant, clippy 1.98), and carrying the same commit lets this PR and the sanitize PR merge in either order. For the same reason,clippy -D warningsfails on each commit before it, as it does onmain.Not verified:
readyverify runs later);clippy --all-featuresover the whole workspace (only the three touched crates);fetch_urlof a redacted presigned URL against a real bucket.Follow-ups
WebSearchConfig::load_with(reader)), so zone's wrapper (services/search.rs,ENABLED_VARIABLEandQUERY_URL_VARIABLE) need not repeatSEARCH_ENABLE_WEB_SEARCHandSEARCH_SEARXNG_QUERY_URL.🤖 Generated with Claude Code