Security: patch dependency CVEs, migrate langchain to 1.x, audit app security, remove Slack-demo cruft

[amirfz]

Overview

Security-hardening pass, in four parts:

• Dependency CVEs — ~140 open Dependabot / pip-audit findings down to 1: chromadb CVE-2026-45829, which has no upstream fix and was audited as not exploitable from this codebase. Adds .github/dependabot.yml so future advisories arrive as PRs.
• Breaking migration — langchain-core / -openai / -text-splitters 0.3.x → 1.x, langchain-community → 0.4.x. Required source changes, not just version bumps.
• Application security — SSRF guard (safe_get) for agent-controlled URL fetches, hardened against redirect chains, DNS rebinding, and credential forwarding.
• Dead code and dependency footprint — removes Slack-era code left behind in the core library, and drops dependencies that were unused or disproportionate to their actual usage.

Version bumped to 0.6.0: this removes public API and changes a major dependency version.

Status: 361 passed, 2 skipped, 0 failed. pip-audit: the single finding above. CI green.

The sections below are the working history, written as the work progressed across several review rounds. Where they differ from this summary — notably final dependency placement (transformers, unstructured, pypdf) and test counts — the summary above is current.

---

Summary

Security-hardening pass covering three layers: dependency CVEs, application-level security review (with fixes), and dead-code/exposure-surface cleanup. Includes a full independent review pass with its own fixes applied (section 5).

1. Dependency CVEs: ~140 open findings → 1

Started from ~140 open Dependabot alerts / pip-audit findings across the dependency tree. Closed all but one:

• langchain-core, langchain-openai, langchain-text-splitters: 0.3.x → 1.x. langchain-community: 0.3.x → >=0.4.2,<0.5 (not 1.x — community hasn't cut a 1.x release; 0.4 is its current line). This is a breaking major-version migration for the 1.x packages, not a version bump — the langchain meta-package is gone (community 0.4 depends on langchain-classic instead). Note: importing langchain-community 0.4.x now prints a "being sunset, no longer actively maintained" deprecation warning upstream — worth flagging on a CVE-focused PR, since we're landing on a package that won't get future security fixes. Sherpa only uses GoogleSerperAPIWrapper and two document loaders from it; dropping the dependency entirely looks feasible and is worth a follow-up (added to Dependency risk-surface reduction: drop dead/heavy deps (importlib, hydra-core, transformers, tokenizers, markdown), narrow unstructured, de-dupe spacy #510). Source changes required for the 1.x packages:
  • llm.predict() (removed in core 1.0) → llm.invoke() in sherpa_ai/utils.py
  • retriever.get_relevant_documents() (removed) → retriever.invoke() in sherpa_ai/tools.py
  • Found and fixed a pre-existing bug surfaced by the migration: run_manager.add_handler(...) in sherpa_ai/models/sherpa_base_chat_model.py / sherpa_base_model.py called a method that doesn't exist on CallbackManagerForLLMRun — it was silently crashing usage/cost tracking on every LLM invocation. Replaced with direct usage_metadata extraction from the response message.
  • Closes CVE-2026-44843 (unsafe deserialization via broad load() allowlists), CVE-2026-34070 (path traversal in legacy load_prompt), CVE-2026-40087, CVE-2026-26013 (SSRF via image token counting), CVE-2026-41481 (SSRF redirect bypass in text splitters), CVE-2026-41488, and others.
• transformers 4.x → 5.14.1. Verified it is imported nowhere in sherpa_ai/ — it's dead weight in the optional group, so the major bump carried zero source risk. Closes 8 CVEs (RCE in model-loading paths, ReDoS in tokenizers/optimizers, deserialization of untrusted data). Flagged in follow-up issue Dependency risk-surface reduction: drop dead/heavy deps (importlib, hydra-core, transformers, tokenizers, markdown), narrow unstructured, de-dupe spacy #510 for possible outright removal.
• pypdf 3.17 → 6.13, unstructured 0.10 → 0.18, markdown 3.4 → 3.8, pytest/pytest-asyncio 8→9/0.25→1.0, black 23→26 — patch/minor-line bumps to CVE-fixed versions, no source changes needed.
• chromadb 1.5.9 — no upstream fix exists yet for CVE-2026-45829 (pre-auth code injection via trust_remote_code on a collection-creation HTTP endpoint). Audited actual usage: trust_remote_code is never set anywhere in this codebase, and sherpa_ai only ever acts as a chromadb client (PersistentClient embedded, or HttpClient against a user-operated server) — it never runs the vulnerable server-side endpoint itself. Confirmed not exploitable via this codebase as written. This is the one open pip-audit finding; documented inline above its pyproject.toml pin so the next auditor doesn't have to re-derive this. Revisit when a fix ships upstream.
• Added .github/dependabot.yml (grouped by dependency family, per-directory: src, docs, both demo apps, plus GitHub Actions) so future advisories become PRs automatically instead of silent alerts.

Verification: pip-audit on the full dependency export now shows 1 finding, 1 package (chromadb, no fix available). Was ~140 findings across ~20 packages at the start.

2. Test suite: tightened, not just green

Several integration tests were passing on assertions loose enough to hide real breakage (e.g. len(content) > 0 instead of checking actual retrieved content). Tightened before/during the dependency bumps so the suite is an actual regression net going forward:

• test_qa_agent_with_vector_shared_memory.py: now asserts exact chunk/metadata round-trip, bidirectional content discrimination (a "comets" query returns the comets doc, not the avocado one, and vice versa), and adds a new test proving the session_id filter actually excludes cross-session matches (previously untested). Also fixed a latent bug where meta_data2 used the wrong session id.
• test_search_tool.py, test_context_search.py: previously passed even when parsing broke, because the "no good result" fallback string satisfied the old assertions. Now assert the mocked Serper title/snippet/link actually flow through.
• test_utils.py: added coverage for chunk_and_summarize/chunk_and_summarize_file (previously zero coverage — the only callers of the now-migrated llm.predict()/llm.invoke() path), plus a full suite of new SSRF-guard tests (section 5).
• New tests/unit_tests/models/test_sherpa_models.py: sherpa_ai/models/ had zero coverage; these tests are what actually caught the two real bugs above (broken usage tracking, missing import) before the langchain bump even landed.

Two tests previously assumed to be "flaky LLM non-determinism" turned out to be deterministic bugs, found and fixed at the source (not by loosening the test):

• test_number_citation_validator.py: spaCy NER segmented "one thousand two hundred thirty-four" inconsistently between the cassette's answer and the source text, so NumberValidation rejected a correct answer. Fixed by adding a lexical (non-NER) word-number extractor in utils.py that's consistent regardless of context. (The now-dead NER-based function was removed in the review-fix round, section 5.)
• test_qa_agent_actions.py::test_qa_agent_citation_validation_multiple_action: SearchArxivTool._run regex-scanned the whole Atom feed for <title>/<summary>, and the feed-level <title> tag misaligned the count vs. per-entry summaries, causing an IndexError on every live run. Fixed by parsing per-<entry> block (now also logs a warning when an entry is skipped for missing fields — section 5).

Both fixes preserve the original protective intent of the tests (grounded numeric answers, correct single-result arXiv search) — nothing was weakened to make them pass.

3. Application-level security audit (code, not dependencies)

Ran a focused audit of sherpa_ai/ itself for injection, path traversal, SSRF, secrets handling, and LLM/prompt-injection-specific risks. Documenting here even though most areas came back clean, so the coverage is on record:

Checked and clear:

• No eval/exec/os.system/os.popen/subprocess(shell=True) anywhere in sherpa_ai.
• No unsafe deserialization — pickle is imported in agents/agent_serializer.py but never actually used (serialization is pure JSON); dead import worth removing separately.
• No yaml.load; prompt/config loading is JSON- or path-based off developer-supplied config, not user input.
• SQL layer (database/user_usage_tracker.py) uses SQLAlchemy ORM (session.query(...).filter_by(...)) throughout — no raw string-built SQL.
• No hardcoded secrets; all keys read from env; the one non-PyPI dependency (en_core_web_sm wheel) is pinned to a specific GitHub release, not a floating ref.
• No exposed web/API server in this repo (no Flask/FastAPI/Bolt app) — auth/access-control review is out of scope here.

Found and fixed — see section 5 for the fix, this section for the original finding:

1. LinkScraperTool/LinkScraperAction (sherpa_ai/utils.py check_url/scrape_with_url, sherpa_ai/tools.py, sherpa_ai/actions/link_scraper.py) — the LLM chooses the URL to fetch as a tool argument, with no check against internal/link-local/metadata addresses (e.g. 169.254.169.254, localhost). Since untrusted external content (Slack messages, scraped pages) can prompt-inject the agent's tool-argument choice, this is a reachable SSRF path, not just theoretical.
2. PromptReconstructor's URL extraction pipeline (sherpa_ai/scrape/prompt_reconstructor.py) had the same unguarded-fetch pattern, triggered directly by any URL in raw Slack message text. Resolved by removal, not by patching — see section 4, this was dead Slack-bot-demo code with zero live callers.

Found and fixed: scrape/file_scraper.py's download_file fetched an externally-supplied URL with a bearer token attached, with no validation at all — an SSRF vector and a token-exfiltration path (the token could be sent to an attacker-controlled or internal host via redirect). Fixed in section 5.

4. Removed leftover Slack-bot-demo code from core

Sherpa used to ship a Slack bot (apps/slackbot/apps/slackapp); it was deleted at some point, but its implementation files were folded into src/sherpa_ai/ during a restructure instead of being removed. This left dead Slack-specific code, config, and docs baked into what's now a plain Python library, with no actual slack-sdk/slack-bolt dependency — just Slack-shaped code with real coupling to a bot that no longer exists:

• sherpa_ai/scrape/prompt_reconstructor.py (PromptReconstructor) — required a Slack Block-Kit message dict as a constructor arg; unused anywhere in core beyond its own test. This also removes one of the SSRF-adjacent findings above.
• sherpa_ai/utils.py: get_link_from_slack_client_conversation — only consumer was the above.
• sherpa_ai/output_parsers/md_to_slack_parse.py (MDToSlackParse) and sherpa_ai/post_processors.py (md_link_to_slack) — markdown→Slack-mrkdwn converters with zero callers outside their own tests.
• sherpa_ai/prompt.py (SlackBotPrompt) — generic chat prompt template, just misnamed from the bot era; imported nowhere.
• sherpa_ai/verbose_loggers/verbose_loggers.py (SlackVerboseLogger) — unused, no test coverage.
• sherpa_ai/config/__init__.py — SLACK_SIGNING_SECRET/SLACK_OAUTH_TOKEN/SLACK_VERIFICATION_TOKEN/SLACK_PORT/SLACK_ENABLED env vars read by nothing.
• docs/How_To/slack_bot/* (4 pages + 17 images), plus references in docs/index.rst and docs/llms.txt — tutorial content for deploying a bot app that no longer exists in this repo.
• .gitignore: stale apps/slackbot/.env entry from the same removed app; also added usage_logs* patterns to stop test-run artifacts from getting staged.

Nothing here was load-bearing — no current agent/tool/action class calls into any of it.

5. Independent review pass, and fixes for what it found

After the above landed, an independent reviewer (given only "this is a security-hardening pass," no other context) audited the diff and found real issues in the SSRF fix itself plus code-quality gaps. All were fixed, not deferred:

SSRF guard hardening (the guard added for finding #1/#3 in section 3 had bypasses):

• assert_safe_url validated DNS once, but requests.get re-resolves DNS and follows redirects by default — an attacker-controlled public host could 302-redirect the request onto 169.254.169.254 (the guard never inspected the redirect target), or exploit a TOCTOU window via DNS rebinding (public IP at validation time, private IP at connection time). Added safe_get(): disables automatic redirects, validates every hop before following it (max 5), and pins the actual connection to the pre-validated IP for http (kept by-hostname for https to preserve TLS SNI/cert validation, with the narrowed residual window documented).
• _is_public_address used a denylist (is_private/is_loopback/...), which missed CGNAT space (100.64.0.0/10) and IPv4-mapped IPv6 loopback (::ffff:127.0.0.1). Now uses ipaddress.is_global as the primary allowlist check, with the denylist retained as defense-in-depth.
• scrape/file_scraper.py's download_file now routes through safe_get with forward_headers_on_redirect=False, so the bearer token is validated against and never forwarded across a redirect target.
• Both scrape_with_url/check_url (utils.py) and file_scraper.py's downloader now go through the same hardened path. New tests cover: redirect-to-metadata rejected, safe redirect followed, IP pinning verified, redirect-loop limit, CGNAT/IPv4-mapped-IPv6 rejection.

Code quality:

• Removed extract_numeric_entities (dead since combined_number_extractor switched to the lexical extract_word_numbers — see section 2); documented why the lexical approach replaced NER.
• SearchArxivTool now logs a warning (with the missing field and entry id) when an arXiv entry is skipped for missing title/summary/id, instead of silently dropping it.
• Renamed _usage_metadata_from_result → usage_metadata_from_result since it was imported across a module boundary despite the underscore signaling private. Investigated whether its take-first behavior on multi-generation (n>1) responses should aggregate instead — deliberately left as take-first and documented why: providers differ on whether per-generation usage_metadata holds that generation's own tokens or the request-wide total, so summing could double-count; n>1 isn't used in Sherpa's current call paths.
• Documented the one residual CVE (chromadb, no fix available) inline in pyproject.toml rather than leaving it as an unexplained gap.

Full suite after this round: 334 passed, 2 skipped, 0 failed.

6. CodeQL false positive on test assertions

test_search_tool.py had 3 assertions of the shape "https://www.google.com" in search_result (checking that a mocked search result actually contains the expected link). CodeQL's py/incomplete-url-substring-sanitization rule flags this AST shape (<url-literal> in <expr>) regardless of intent or container type — it kept firing even after rewriting to check membership in a regex-extracted list of link tokens, and inline # codeql[...] suppression comments weren't honored by this repo's CodeQL setup in either placement tried. Resolved by switching to exact-equality assertions (_extract_links(search_result) == [...]) instead of membership — a strictly stronger check than the original, and outside the rule's match set (which targets containment/startswith/endswith, not ==).

7. Human review pass (oshoma) — all requested changes addressed

• extract_word_numbers false positives (blocking). Tested against ordinary English, the lexical number extractor added in section 2 mis-fired on: a bare "point" (returned 0), a bare "one" (pronoun/article usage — "no one", "at one point" — returned 1), and repeated words like "one one one" (reads as a spoken digit sequence, not addition — returned 2). Since these feed NumberValidation's rejection logic, a correct answer containing any of these words could get wrongly rejected. Added targeted guards for all three cases (standalone-ambiguous-word rejection, "point" must be flanked by number words, no adjacent-repeated-word runs) and new tests covering both the false positives and the legitimate compounds ("twenty one", "two point five", "one hundred") that must still work.
• word2number moved from the optional dependency group into main dependencies (blocking). combined_number_extractor now calls word_to_float on almost any text, so a normal install without the optional group would hit an ImportError on number validation — a pre-existing landmine this PR's number-extraction change made load-bearing.
• Version bumped to 0.6.0 (blocking). This PR removes public API classes/functions (the Slack-era dead code in section 4) and does a langchain major-version migration — both warrant a minor version bump to signal the break, not a patch.
• safe_get IP selection (non-blocking, fixed anyway). The IP-pinning fix in section 5 sorted resolved addresses alphabetically as text and pinned to the first one — effectively a coin flip between IPv4/IPv6 depending on which address happened to sort first, which could pin to an address the network can't actually reach and turn a working site into a spurious failure. Now prefers IPv4 (more universally reachable) and falls back to the next validated address on a connection error, rather than failing outright.
• Host header could leak credentials (non-blocking, fixed anyway). Built from parsed.netloc, which carries user:pass@host userinfo for URLs with embedded credentials — producing a malformed header and putting credentials somewhere they shouldn't be. Now built from hostname[:port] only.
• Usage-tracking test gap (non-blocking, fixed anyway). The existing tests only built messages without usage_metadata, so they exercised the zeroed-out fallback path, never the actual extraction code the earlier langchain-migration fix (section 1) was for. Added a test that populates real usage_metadata and asserts it's passed through verbatim.
• chunk_and_summarize/chunk_and_summarize_file read .content instead of .text (non-blocking, fixed anyway). In langchain-core 1.x, .content can be a list of content blocks rather than a plain string, which would break the " ".join(...) call downstream. Switched to .text, which normalizes this.
• PR description inaccuracy corrected. Section 1 above previously said langchain-community went to 1.x — it's actually >=0.4.2,<0.5 (community hasn't cut a 1.x release), and that version now prints a "being sunset, no longer maintained" warning — worth noting on a CVE-focused PR, added to the Dependency risk-surface reduction: drop dead/heavy deps (importlib, hydra-core, transformers, tokenizers, markdown), narrow unstructured, de-dupe spacy #510 follow-up.

The reviewer also suggested splitting this into several smaller PRs (dependency/langchain/SSRF work vs. Slack cleanup vs. number-extraction vs. a langchain-community removal follow-up). Kept as one PR per discussion — everything here is one coherent security-hardening pass and splitting would mean re-deriving the same review context across multiple PRs.

Full suite after this round: 348 passed, 2 skipped, 0 failed.

Follow-up work (filed, not blocking this PR)

• Dependency risk-surface reduction: drop dead/heavy deps (importlib, hydra-core, transformers, tokenizers, markdown), narrow unstructured, de-dupe spacy #510 — Dependency footprint reduction. Separate analysis (independent of the CVE work above) of which dependencies are disproportionately heavy relative to actual usage in src/sherpa_ai/. Top findings: importlib (main dep) is the PyPI Python-2.7 backport shim — meaningless/dead on Python 3.10+; hydra-core and transformers/tokenizers/markdown (optional) are imported nowhere; unstructured is the heaviest core dependency for what amounts to trivial PDF/markdown loading, replaceable by the already-present pypdf; spacy is declared with conflicting pins in both the main and optional groups; pinecone-client may be a redundant second vector backend on an EOL API given chromadb is already used substantively; langchain-community (section 7) is a good candidate to drop entirely given Sherpa only uses two things from it and it's no longer maintained upstream.
• Dead pickle import in agents/agent_serializer.py (noted in section 3, not fixed here — trivial cleanup, low priority).

Test plan

• pytest -m "not external_api": 348 passed, 2 skipped, 0 failed
• pip-audit on full dependency export: 1 finding (chromadb, no fix available, confirmed non-exploitable here)
• Application security audit performed, findings fixed (not just documented) — section 3 + 5
• Independent (AI) review pass performed on the full diff, findings fixed — section 5
• Human review pass performed, all requested changes addressed — section 7
• Dependency footprint follow-up filed as Dependency risk-surface reduction: drop dead/heavy deps (importlib, hydra-core, transformers, tokenizers, markdown), narrow unstructured, de-dupe spacy #510
• CI run on this PR: all checks green (tests ×2, Analyze actions/python, CodeQL) — see section 6 for the one CodeQL false-positive encountered and how it was resolved

🤖 Generated with Claude Code

https://claude.ai/code/session_01CSV1Mo49ZkeZjpqVLoFHDz

[oshoma]

I ran this through Opus; findings below, with my thoughts merged in. Short version: overall this is great, we should make a few improvements and then merge it. And you might want to split it into a few PRs.

For Lumberify this removes a bunch of vulnerabilities and there are no compatibility issues.

Suggestions follow.

Should change before merge

1. Move the number-extraction change out of this PR. If it stays, it needs stop-word handling and tests. I ran the new extract_word_numbers as written:

'That is a fair point, but we need 42 units.'   -> ['0']
'The point of the analysis is clarity.'         -> ['0']
'At one point the shipment was delayed.'        -> ['1']
'No one knows the exact figure.'                -> ['1']
'Phone one one one for support.'                -> ['2']
'one thousand two hundred thirty-four'          -> ['1234']   # the intended case works

The bare word "point" comes back as the number zero, and "one" in ordinary sentences comes back as one. Those invented numbers feed straight into verify_numbers_against_source, which rejects an answer when a number in it isn't in the source. So an answer containing the word "point" gets rejected unless the source happens to contain a literal 0.

The old approach had its own problems and I'm not arguing for putting it back, but this is the only new logic in the PR with no tests of its own, and it changes when answers get rejected. So we should either evaluate it separately from the security work or improve the implementation.

2. Move word2number from the optional group into the main dependencies. word_to_float raises an error when the package is missing, and combined_number_extractor now calls it on almost any text. So number validation raises an import error for anyone who installs sherpa normally. The test suite installs the optional group, so CI won't catch it. (Pre-existing problem before this PR.)

3. Bump the version to 0.6.0. This PR removes public API classes and functions and changes the langchain major version, so we should signal that it's a major version change.

Maybe split this into a few PRs

• This PR: dependency bumps, the langchain migration, and the SSRF fixes.
• A separate PR: removing the Slack code and the other dead code. It's good cleanup and worth doing, but it breaks the public API. Bundling it means anyone who just wants the vulnerability fixes has to accept breaking changes to get them.
• A separate PR: the number-extraction change.
• A follow-up: dropping langchain-community, added to #510.

Not a hard objection, just an idea.

Worth fixing, not blocking

4. In safe_get, pick an address the machine can actually reach instead of taking the first one alphabetically.

Some background, since this one's subtle. For plain http:// URLs the code looks up the site's name, checks the addresses that come back, and then connects to one of those numbers directly rather than by name. That's the right call — it stops someone from giving a safe answer during the check and a different, internal answer a moment later.

The problem is which address gets picked. Most sites have more than one — an older-style one like 93.184.216.34 and a newer-style one like 2606:2800:220:1:248:1893:25c8:1946. The code sorts them as text and takes the first, which isn't a meaningful order; it just depends on which character the address starts with. 2606:... beats 93... because "2" sorts before "9", but for another site 104.16.0.1 would beat 2606:... because "1" sorts before "2". It's effectively a coin flip.

That matters because plenty of networks can only reach the older-style addresses. Normally the HTTP library handles this quietly — it tries one, and if it can't get through it tries the next. Pinning to a single address throws that fallback away, so if the coin flip picks the unreachable one, the fetch just fails and looks like the site is down.

Either prefer an address of a kind the machine can use, or try them in turn until one connects. Still only ones that passed the check, so nothing is given up on security. Only affects http:// — https:// connects by name, so it's unaffected.

5. Build the Host header from the hostname and port rather than parsed.netloc. netloc can carry a username and password, so for a URL like http://user:pass@host/ the header becomes user:pass@host — malformed, and it puts credentials somewhere they shouldn't be. The current test passes because the test URL has neither.

6. Add a usage-tracking test where usage_metadata is actually populated. Both new tests build a message without it, so they only exercise the fallback path. The fix itself — reading the metadata off the message — isn't covered.

7. Read the summary back with .text instead of .content in chunk_and_summarize. In langchain-core 1.x .content can come back as a list rather than a string, which would break the " ".join(...) below it. Unlikely with ChatOpenAI's defaults, but it's a one-word change.

8. Update the description to say langchain-community >=0.4.2,<0.5. It currently says 1.x. Related and more important: importing that version now prints a warning that langchain-community is being sunset and is no longer maintained. For a PR about reducing vulnerabilities, it's worth noting that we're landing on a package that won't get future fixes. Sherpa only uses GoogleSerperAPIWrapper and two document loaders from it, so dropping it looks cheap — worth adding to #510.

[20001LastOrder]

Hi @amirfz , I think overall the PR looks great. It seems there are two small issues in utils.py needed to be addressed before it can be merged.

1. extract_word_numbers merges comma-separated enumerations into phantom numbers

Place: src/sherpa_ai/utils.py — _NUMBER_WORD_RUN_PATTERN

The run separator [\s,-]+ lets a number-word run cross commas, so an enumeration is captured as a single phrase and handed to word_to_float/w2n.word_to_num, which misparses it. Reproduced against word2number (the exact code path word_to_float wraps):

Input	extract_word_numbers result	Expected
"We tried one, two, three approaches."	["5"]	["2", "3"] (or [])
"Options: five, six or seven days."	["11", "7"]	["5", "6", "7"]
"It cost two point five million dollars."	["2"]	["2500000"]

Since this feeds combined_number_extractor → NumberValidation, a phantom number extracted from an answer that doesn't exist in the source causes a correct answer to be rejected — the same failure class as the bare "one"/"point"` cases already fixed in the section-7 round, via a different route.

Suggested fix: drop , from the separator class so runs never cross a comma:

_NUMBER_WORD_RUN_PATTERN = re.compile(
    rf"\b{_NUMBER_WORD}(?:(?:[\s-]+(?:and[\s-]+)?){_NUMBER_WORD})*\b",
    re.IGNORECASE,
)

With that change, "one, two, three" splits into three separate runs: "one" is dropped by the existing standalone-ambiguous guard, "two" → 2, "three" → 3 — the actual component numbers instead of a phantom 5. All existing tests still pass under this change: the compound-number cases ("one thousand two hundred thirty-four", "twenty one", "two point five") contain no commas, and the rejection cases are unaffected.

Trade-off worth documenting in the comment above the regex: a word-form number written with a comma ("one thousand, two hundred thirty-four") now splits into 1000 and 234 instead of 1234. That style is rare in prose, digit-form "1,234"is already handled byextract_numbers_from_text`, and splitting into two real component numbers is a much safer failure mode for validation than merging unrelated numbers into one phantom value.

2. safe_get forwards credential headers across redirects by default

Place: src/sherpa_ai/utils.py — safe_get signature, forward_headers_on_redirect: bool = True.

Plain requests strips Authorization on cross-host redirects, but since safe_get re-issues each hop manually with allow_redirects=False, that protection never engages; this flag is the only control. With the default True, caller headers (including Authorization) are forwarded to every redirect target, even cross-host or https →http. No current caller is exposed (file_scraper.py passes False; scrape_with_url sends no headers), but the next caller who passes a token and misses the flag reintroduces the exact token-exfiltration path this PR fixed in download_file.

Suggested fix: default the flag to False

[amirfz]

Addressed both issues from the latest review in 6d71094:

1. _NUMBER_WORD_RUN_PATTERN no longer treats , as a run separator, so comma-separated enumerations (e.g. "one, two, three") split into separate numbers instead of merging into a phantom value.
2. forward_headers_on_redirect now defaults to False in safe_get, so caller headers (including Authorization) aren't forwarded across redirects unless explicitly opted in.

Existing tests pass (39 relevant cases in test_utils.py).

[amirfz]

Pulled in issue #510's dependency-risk-reduction work in 37ea2f6: dropped importlib, hydra-core, transformers, tokenizers, markdown; de-duped spacy (kept main, per @oshoma's review); moved en_core_web_sm and pypdf to main deps; replaced unstructured's loaders in load_files with the existing pypdf-based extractor + plain read, then dropped unstructured entirely. 279 tests pass. pinecone-client consolidation is left for a follow-up since it needs a real v2→v3 SDK migration.

[20001LastOrder]

Looks good now, merging...

[20001LastOrder]

@oshoma, I think you also need to approve the PR before it can be merged.

[oshoma]

Pushed two commits (1b0b237, 5681c68) with follow-up review findings. Each was reproduced with a failing test before being fixed; suite goes 351 → 361 passed, CI green.

Number extraction

"two point five million" → 2, not 2500000. This was row 3 of @20001LastOrder's table. The fix in 6d71094 addressed rows 1 and 2, but this case has no comma — word2number parses a decimal followed by a magnitude as just its integer part, dropping both the fraction and the magnitude. The consequence is the failure mode this thread has been chasing throughout: source says 2500000, answer says "two point five million", extractor yields 2, correct answer rejected. Now splits the decimal from its trailing magnitude words and recombines.

A leading or trailing point discarded real numbers. The guard added in 713151c for the bare-point false positive rejected the entire run, so "At some point one hundred people left." returned [] instead of 100. That's a false negative — a hallucinated number slips past validation rather than a correct one being rejected, so it's the less dangerous direction, but it's still a hole. A leading/trailing "point" is the ordinary English noun, so it's now stripped and the rest of the run is still evaluated. The bare-point, bare-one, and repeated-word rejections that guard was added for are unchanged and still tested.

Validation messages (pre-existing, not from this PR)

verify_numbers_against_source and check_if_number_exist both had this shape:

message = "Don't use the numbers"
f"{joined_numbers} to answer the question. Instead, stick to the numbers mentioned in the context."

The second line is a bare expression statement — evaluated and thrown away. So whenever number validation failed, the feedback actually returned to the LLM was "Don't use the numbers", with no numbers and no guidance. check_if_number_exist was worse: it accumulated the offending numbers into message and then overwrote them on the next line.

This predates the PR, but it's the exact subsystem being reworked here and no test covered it. Both now build the full message, with tests.

Runtime model download

extract_entities fell back to spacy.cli.download() when en_core_web_sm was missing (3150b3a). That shells out to pip and installs a wheel from the network on first use — a runtime code-fetch path, which seems like the wrong trade in a security-hardening PR. Replaced with an actionable error pointing at python -m spacy download en_core_web_sm.

Nothing reaches that path unless entity validation is explicitly enabled: BaseAgent.validations defaults to [], and nothing loads src/conf/config.yaml. CI installs the model via the test group. So neither a default install nor CI is affected.

Also

• Removed the dead import pickle in agents/agent_serializer.py, noted as unfixed in the description.
• .gitignore: tests that patch sherpa_ai.config wholesale leave cfg.USAGE_LOG_FILE_PATH as a MagicMock, which the usage logger then uses as a file path. Eight files literally named <MagicMock name='cfg.USAGE_LOG_FILE_PATH' id='...'> appeared in src/ after a single offline run. Ignored for now — the real fix is for those tests to pass a temp path instead of relying on a mocked config attribute.

On transformers

For the record, since it's in the commit rationale: I initially flagged moving it from optional into main dependencies as unnecessary, and I was wrong. BaseAgent.llm is typed Optional[BaseLanguageModel] and qa_agent.py:191 calls self.llm.get_num_tokens directly, so any consumer passing a non-OpenAI model hits the GPT-2 tokenizer fallback in production. Sherpa's own model classes all override get_token_ids, which is what misled me — but that's the wrong frame for a library that accepts a user-supplied LLM. a881cb9 has it right.

Still open

• load_files markdown handling — separate comment to follow.
• pinecone-client v2 EOL and the langchain-community sunset warning — both tracked in Dependency risk-surface reduction: drop dead/heavy deps (importlib, hydra-core, transformers, tokenizers, markdown), narrow unstructured, de-dupe spacy #510.

[oshoma]

@amirfz @20001LastOrder one thing I would like your decision on rather than silently inheriting: dropping unstructured changed what actually lands in the vector store for .md files:
load_files went from UnstructuredMarkdownLoader(f).load() to a plain open().read(). Both commit messages frame it as a mechanical swap to shed the dependency, but the two produce materially different text. Same input file, both loaders:

Before (UnstructuredMarkdownLoader)

'Quarterly Report\n\nThe revenue was $42 million, up from last year.\n\nRegion A grew 12%\n\nRegion B shrank 3%\n\nMargins held steady.\n\nprint("not prose")'

After (plain read)

'# Quarterly Report\n\nThe **revenue** was $42 million, up from [last year](https://example.com/2025).\n\n- Region A grew 12%\n- Region B shrank 3%\n\n> Margins held steady.\n\n```python\nprint("not prose")\n```\n'

I'd rather we pick deliberately than default into it:

Against the new behavior — #, **, -, > and code fences now get embedded and retrieved, and land in the LLM's context. That's noise in the embeddings and wasted tokens on every retrieval.

For the new behavior — the old loader silently dropped every URL. [last year](https://example.com/2025) became just last year. I can't recall, was this intentional? For citation validation and link scraping, throwing away the links in source documents may be worse than carrying some syntax.

The new code also fixes a real bug: loader was initialized once outside the loop and never reset, so a .gitkeep following a .pdf or .md re-loaded the previous file and produced duplicate documents.

As a third option we could also keep the plain read (no dependency, keeps URLs) and strip the syntax we don't want with a small markdown-to-text pass.

Whichever way we go, it should get a test; there's no coverage on load_files output content today, which is why this slipped through in the first place.

Same question applies to .pdf, though less sharply: UnstructuredPDFLoader did layout-aware partitioning, extract_text_from_pdf does raw pypdf extraction. Different text, probably fine, but also untested.

[oshoma]

I pushed a few more commits with tweaks. In general this looks good to go, excepting one last question for @amirfz and @20001LastOrder here: #509 (comment).

[amirfz]

@oshoma @20001LastOrder Pushed two commits addressing the open items from this thread and #510:

ef822ed — markdown loader + drop pinecone-client

• Replaced the plain open().read() for .md files with a new markdown_to_text() helper (src/sherpa_ai/utils.py). It strips headers, bold/italic markers, list bullets, blockquotes, and code fences (embedding noise), but keeps links as "text (url)" instead of silently dropping the URL like the old UnstructuredMarkdownLoader did.
• Dropped pinecone-client per Dependency risk-surface reduction: drop dead/heavy deps (importlib, hydra-core, transformers, tokenizers, markdown), narrow unstructured, de-dupe spacy #510's finding Edit summary and add link to the notebooks #6 (deprecated v2 SDK, redundant with chromadb): removed the ConversationStore class, the pinecone branch of get_vectordb(), PINECONE_* config vars, save_to_pine_cone() in extract_github_readme.py, and the matching env-sample/CI entries. Nothing else in the codebase referenced any of it.

5e47e28 — fixes from a critical review of the above
A review of ef822ed turned up real bugs in the first pass of markdown_to_text():

• The code-fence regex was eating live content, not just the language tag (an inline `ls -la` mid-sentence lost the word "ls"), and fenced-block contents were being run back through the header/bullet/quote regexes afterward (a # install deps shell comment inside a ```bash block got stripped as if it were a markdown header). Fixed by stashing fenced blocks out before the other passes run and restoring them verbatim.
• Single-asterisk emphasis matching was pairing up unrelated asterisks across a line ("a * b and c * d" got mangled) — tightened to require no surrounding whitespace, so multiplication/glob-style usage survives.
• Link URLs with a nested paren (Wikipedia-style .../Foo_(bar)) were getting truncated — widened to allow one level of nesting. Images (![alt](url)) were leaving a stray ! — fixed.
• Added regression tests for each of these cases plus the original oshoma-comment example.

Both commits are rebased on top of the latest security/deps-tightening and CI is green. load_files test coverage (the gap you flagged) now covers markdown stripping end-to-end via tmp_path.

[20001LastOrder]

Hi @oshoma, @amirfz . I think the latest changes addressed #509 (comment). I recommend we merge this PR as it is now, since it is becoming a bit too large at this point.

[amirfz]

happy to proceed. needs your approvals though

[20001LastOrder]

Let's get it merged to kickoff further updates and refactoring!