v3.10 — Hardening + new tools (test, web_fetch, web_search, research) - #10
Merged
Conversation
Separate allowlist from the general shell tool (TEST_ALLOWED_COMMANDS, default pytest+ruff) and dedicated timeout (TEST_TIMEOUT), so 'run the tests' / 'lint this' stay first-class router intents with their own purpose-built safety boundary rather than depending on whatever SHELL_ALLOWED_COMMANDS happens to allow. 5 tests: allowed runner, reported failure, blocked runner, empty command, and explicit proof that the shell allowlist has no bearing here.
Fetches a URL and returns tag-stripped text. Two protection layers, both important given Forge's own network position (NiPoGi behind WireGuard, alongside other personal services): - SSRF guard (always on, NOT configurable): every IP a hostname resolves to is checked against loopback/private/link-local/ reserved/multicast ranges before any request is made. Redirects are not followed automatically for the same reason (a 200 from an allowed host could otherwise redirect to an internal address). - Optional domain allowlist (WEB_FETCH_ALLOWED_DOMAINS), empty by default -- any public domain is fetchable subject to the guard above. HTML-to-text extraction uses stdlib html.parser only, no new heavy dependency (bs4/lxml). Known residual limitation documented in the module docstring: the IP check and the actual request are two separate DNS lookups, so DNS rebinding between the two is not fully closed -- acceptable for a personal single-user tool. 13 tests using requests-mock (new dev dependency): SSRF cases (loopback/private/link-local/unresolvable), domain allowlist, content-type handling, HTML stripping, redirect non-following, HTTP errors.
Fixes: StarletteDeprecationWarning: Using httpx with starlette.testclient is deprecated; install httpx2 instead. httpx2 is Pydantic's maintained fork; starlette.testclient (via fastapi.testclient) has preferred it since 1.2.0/1.3.1. Dev-only -- no runtime behavior change, plain httpx elsewhere in the app is unaffected. Confirmed the full suite runs with zero warnings under -W error after this change.
Merges what was briefly a separate 'code' agent into the existing
review graph instead of keeping a parallel concept -- it was the
same read+review flow with one extra step, and three near-identical
'code' things (tools/code.py stub, graphs/review.py, agents/code.py)
would have been confusing for both the router and future maintenance.
- graphs/review.py: optional test_path parameter. Without it,
behavior is byte-identical to before (2-step read_file -> llm_review).
With it, a run_tests node runs pytest via the dedicated test tool
and feeds the output to the review as primary evidence.
- api.py: ReviewRequest gains an optional test_path field.
- cli.py: 'forge review <file> [question] [--tests <path>]'.
- tools/review.py (new): dispatchable wrapper, content = JSON
{"file_path","question","test_path"}. This is the actual point of
the change -- Forge's UI is a single conversational page with zero
tabs, so a review must be reachable by just asking for it in chat,
not only via CLI/API. Wired into the router prompt (description +
two worked examples, one with test_path) the same way files/memory
already are.
Verified end to end with mocked LLMs: a plain chat message routes to
review and returns the analysis, no separate call needed.
284 tests (was 275), lint/format clean, zero warnings under -W error.
Real bug hit in production use: a chat-dispatched review of '/hello.go' failed with 'File not found: /hello.go' (resolved against the actual filesystem root -- there was no confinement at all), while the same request without the leading slash routed differently (to 'files') and worked, since files.py is properly confined. graphs.review.run() itself is intentionally left unconfined -- it's also called directly by the CLI (a human-typed path, same trust level as shell access) and by the /review API endpoint (its own tempfile, outside the workspace by design). Confinement now lives at the tools/review.py dispatch boundary instead, where router output on untrusted chat text actually needs it -- same threat model as tools/files.py. A leading '/' is stripped rather than rejected, so '/hello.go' and 'hello.go' resolve to the same workspace-root file -- this is also what actually closes the escape: Path(workspace) / '/etc/passwd' would otherwise silently discard the workspace prefix (pathlib joins replace the left side entirely when the right side is absolute), so stripping first is what guarantees the join always stays relative. 5 new tests (was test_review_tool.py's 3, now 8): confinement on file_path and test_path, the leading-slash case that was actually observed, and traversal rejection for both fields. 287 tests, lint/format clean.
Same bug as the review fix in the previous commit, just surfacing in
files.py instead: '/hello.go' was rejected as 'escaping the
workspace' while 'hello.go' worked, even though both should resolve
to the same file at the workspace root. Root cause is the same
pathlib gotcha (Path(workspace) / '/x' discards the workspace prefix
entirely when the right-hand side is absolute), just previously
manifesting as a correct-but-unhelpful rejection here instead of a
silent unconfined read (review had no confinement at all yet).
Observed live: router chose 'files' for the same read request with
and without a leading slash, and only one of the two actually worked.
Traversal protection (../../etc/passwd) is untouched -- lstrip('/')
only affects a leading slash, resolve()+relative_to() still catches
'..' escapes exactly as before.
1 new regression test (test_files_leading_slash_means_workspace_root).
288 tests, lint/format clean.
Real bug hit in production use: 'relis hello.go et donne moi ton avis'
routed correctly to review, but the answer shown was just 'hello.go'
(8 chars) -- no actual analysis.
Root cause confirmed via logs: the review LLM call returned 61 raw
chars, but _llm_review_node was feeding that through
router.parser.parse_router_output -- the parser built to extract a
{"tool":...,"content":...} decision out of router output. A small
model heavily fine-tuned on the router's JSON habit apparently
answered the review prompt (which explicitly asks for plain text, no
JSON) with a degenerate JSON echo instead of real analysis, e.g.
{"tool":"chat","content":"hello.go"}. The router parser dutifully
'succeeded' at extracting that content as the decision, silently
discarding everything else -- producing a tiny, plausible-looking but
meaningless result with no visible sign anything had gone wrong.
Fix: review now has its own minimal response cleaner
(_clean_review_response) instead of reusing the router's JSON-first
extraction cascade. It only strips <think> blocks and a leaked-prompt
echo; a stray JSON blob, if the model still produces one, is shown
as-is rather than unwrapped -- a visibly wrong response beats a
silently truncated one that happens to look valid.
Side effect, also a real fix: review answers were being silently
capped at 400 chars (router.parser's _MAX_FALLBACK_CHARS, sized for
tool decisions, not analysis prose) any time the model answered in
plain text -- which was every successful case, since the prompt asks
for plain text. Review now has its own 4000-char ceiling.
Also added: review.raw_output is now logged unconditionally
(mirrors orchestrator.py's router.raw_output), since this call
previously had zero raw-output visibility -- the exact garbage the
model produced in the reported bug couldn't be confirmed from logs,
only inferred from the length mismatch.
3 new tests: the exact degenerate-JSON-echo case, think-block
stripping, and the 400-char cap removal. 291 tests, lint/format
clean.
The previous fix (stop reusing router's JSON parser) made the failure
visible instead of silent -- confirmed live: the raw JSON echo
({"tool":"code","content":"hello.go","done":true}) is now shown
as-is, exactly as designed. But the underlying cause was still
unaddressed: the model kept answering in router JSON despite the
prompt already saying 'plain text (no JSON)'.
Every other place in this codebase where a small local model needed
to follow an instruction reliably, a bare instruction wasn't enough
-- it needed a concrete worked example (files:write, files read-then-
write, memory recall rephrasing). The review prompt had zero examples
of its own expected output shape, just an instruction. Same fix
applied here: a GOOD ANSWER example plus two explicitly labeled
NEVER DO THIS shapes (both {"tool":"chat",...} and
{"tool":"code",...}, since the router picked 'code' this time,
'chat' the first time -- the model isn't fixating on one specific
tool name, it's fixating on the JSON shape itself).
Also added both new label markers (GOOD ANSWER, NEVER DO THIS) to
the leaked-prompt detector, in case the model echoes the example
scaffolding verbatim instead of writing its own review.
Tradeoff worth noting: the prompt is now ~3x longer (roughly 400 ->
1180 chars before file content), which costs real prompt-processing
time on the Deck's APU every review call. Judged worth it given the
alternative was a meaningless response every time.
1 new test locking the prompt's GOOD ANSWER / NEVER DO THIS content
in place. 292 tests, lint/format clean.
Not yet proven against the live model -- next real test on hardware
is what actually validates this, the same way the previous two
review bugs were only confirmed/found through real usage.
Retested live after the prompt fix (previous commit): the model kept wrapping its answer in router-style JSON despite the explicit GOOD ANSWER / NEVER DO THIS example -- but this time the wrapped 'content' was a genuine multi-sentence review, not a degenerate echo. The few-shot example alone wasn't enough to break the JSON habit entirely; it just stopped producing garbage inside the wrapper. Now have two real, contrasting data points to calibrate against: - Bug #1: {"tool":"chat","content":"hello.go"} -- 1 word, 8 chars, clearly not a review. Must NOT be trusted/unwrapped. - Bug #2: {"tool":"chat","content":"The code is correct and follows..."} -- 40 words of genuine analysis. Should be unwrapped to clean prose, not shown as a JSON blob to the user. _try_unwrap_router_json(): if the whole cleaned response is a {"content":...} object, unwrap it when content is >= 8 words or >= 40 chars (comfortably separates both real cases seen so far), else leave the raw JSON visible -- same 'visibly wrong beats silently truncated' principle as the previous two review fixes, just narrowed to genuinely degenerate cases instead of every JSON-shaped response. Both paths log a warning either way, since the model wrapping at all is still the underlying issue GBNF grammar would fully close later if this heuristic proves insufficient. 2 new tests (the exact substantive and degenerate shapes observed live). 294 tests, lint/format clean. Still not fully proven against the live model in this exact form -- next real test on hardware is what validates it.
Real ambiguity hit in production use: the exact same phrasing
('Relis hello.go') routed to files one time and review another,
across otherwise-similar conversation turns. Root cause found in the
router prompt itself, not in conversation-history noise (that was
briefly suspected and ruled out -- a router-side sliding history
window was considered and rejected, since it would reintroduce the
FIFO cache-invalidation problem v3.8 specifically fixed):
review's own FIRST worked example was 'Peux-tu relire X ?' with no
request for feedback, mapped to review. files' own example set had
no bare 'relis X' case at all. So the model had literally been shown
that the verb alone means review, while never being shown the
opposite anchor -- not a coin-flip, a real gap in the few-shot set.
Fix:
- review's first example now pairs 'relire' with an explicit
request for feedback ('et me donner ton avis'), not the bare verb.
- files gets a new fourth example: 'Relis hello.go' (bare) -> read.
- Both tool descriptions spell out the distinction explicitly: the
verb 'lire'/'relire' alone -> files:read; paired with a request for
an opinion/analysis -> review.
Same tools, same verb, opposite target -- now anchored by whether
feedback is requested, in both the description and a worked example
on each side, consistent with the pattern already proven throughout
this session (this model needs to see the shape, not just read a
rule about it).
1 new test locking the disambiguation in place. 295 tests, lint/format
clean.
…style Real bug hit testing web_fetch on a Wikipedia page: the output was almost entirely navigation menu and a 179-language picker list -- the actual article content got pushed past the 6000-char output cap before it ever appeared. script/style/noscript/head being skipped wasn't enough; real sites put their non-content chrome in semantic nav/header/footer/aside tags, which were being extracted as regular text. Added those four tags to _SKIP_TAGS. Still stdlib html.parser only, no new dependency. 1 new test reproducing the shape of the real page (nav + header + article + aside + footer), asserting the article content survives and all four chrome tags are excluded. Separately (not a web_fetch bug, noted but not addressed here): the original request was 'actualité sur l'IA' and the fetched URL was Wikipedia's general AI overview article, which was never going to have current news regardless of extraction quality -- a URL-selection issue upstream of this tool, not a fetch/extraction issue. 296 tests, lint/format clean.
Real bug hit in production use: web_fetch was registered as a tool but had zero entry in TOOL_DESCRIPTIONS/_TOOL_EXAMPLES, so the router fell back to the generic 'content is the input this tool expects' wording. Result: the router picked web_fetch but produced malformed content (empty or non-URL text), surfacing as '[error] unsupported scheme: "" (only http/https)'. Added a description explaining the expected shape (a single URL) and a positive worked example, same pattern as every other tool in this file. Also addresses a second, separate real failure from the same session: asked for 'les dernières actualités en bourse', the router picked web_fetch, guessed a URL, and got HTTP 404. web_fetch fetches a URL it's given -- it has no search capability, so any URL for a vague 'latest news' request is a guess with no grounding, and a guess is very likely wrong. Added an explicit warning in the description plus a second, contrastive example: a vague news request should map to 'chat' (answer honestly that browsing/search isn't available), not a fabricated URL. 2 new tests: description+examples present when enabled, both absent when web_fetch isn't in ENABLED_TOOLS. 298 tests, lint/format clean.
Real case hit in production use: fetching boursorama.com/bourse/ truncated at the old 512KB default before reaching the actual content (indices, news) -- confirmed by manually inspecting the page that the real content exists in the raw HTML, just pushed far down past a large navigation megamenu. Root cause is NOT something raising this value fully fixes: the megamenu is wrapped in plain, non-semantic <div>s rather than <nav>/<header>, which tools/web_fetch.py's tag-based extractor can't distinguish from real content (it filters by tag name only, not role/class/heuristics). A proper 'main content' extraction heuristic (à la Readability/trafilatura) would need a real dependency, which this tool has deliberately avoided throughout -- not adding one now. Raised WEB_FETCH_MAX_BYTES default from 512KB to 2MB as a cheap, honest partial mitigation (gives more raw bytes a chance to reach the real content on heavy pages), and documented the actual limitation in config.py so it doesn't get rediscovered as a mystery later: web_fetch is reliable on simpler/semantic sites (docs, articles, wikis), best-effort on heavy non-semantic corporate portals. No test changes needed -- no existing test pinned the old byte-cap value. 298 tests, lint/format clean.
New tool, distinct from web_fetch: queries a search index for ranked results (title/URL/snippet) given a query, rather than fetching a page whose URL is already known. Requires a self-hosted SearXNG instance (SEARXNG_URL, default http://127.0.0.1:8888) -- not a cloud search API, consistent with Forge's self-hosting posture (same reasoning as running its own llama.cpp/embedding servers). SearXNG needs "json" added to its own search.formats in settings.yml (disabled by default upstream to discourage scraping public instances; safe on a private, self-hosted one). Router integration follows the same pattern as every other tool: - TOOL_DESCRIPTIONS + two worked examples (plain query, and the 'actualités en bourse' case that previously had nowhere good to go) - A step_context steering hint after a web_search result, same loop-guard reasoning as the existing memory/files hints -- but with a genuine two-way branch (answer from snippets vs fetch one result for full detail) instead of forcing a single path, since forcing either one would be wrong depending on whether the snippets already answer the question Also corrects web_fetch's own description and example set, which had become stale: it used to teach a flat chat refusal for 'actualités en bourse' (accurate when Forge had no search capability at all) -- now points to web_search first when enabled, only falling back to a chat refusal when web_search isn't. The old refusal example moved to web_search's own example set, where it's now correctly answered. 16 new tests (8 for the tool itself via requests-mock, 8 for the router prompt integration). 311 tests, lint/format clean. Not yet validated against a live SearXNG instance or the real model -- next real test once SearXNG is deployed is what actually confirms the routing and chaining behavior.
Real bug hit in production use: SearXNG worked, the router correctly picked web_search with a sensible query -- but the raw results list (5 titles/URLs/snippets) was shown to the user verbatim instead of a synthesized answer. Root cause: neither web_search worked example set 'done':false, so the orchestrator treated the search itself as a complete, single-step answer and returned its raw output directly. The step_context steering hint added alongside web_search (telling the model to answer from the snippets or fetch a specific result) only ever gets a chance to fire on a SECOND step -- which never happened, since nothing told the router a second step was needed in the first place. Exactly the same shape of bug 'done':false already exists to solve for memory's 'recall' action; web_search's own examples just never actually used it. Both examples now end with ,"done":false, matching the recall pattern. 1 new test asserts every web_search example line in the prompt includes it, so this can't silently regress again. 312 tests, lint/format clean. Not yet re-validated against the live model -- next real 'actualité du jeu vidéo' style test is what confirms the two-step chain actually produces a synthesized answer instead of a raw list.
Real bug hit in production use, immediately after the previous
done:false fix: the router correctly chained into a second step this
time, but repeated web_search with the same query instead of
following the steering hint -- tripping the loop guard
('repeated call to tool=web_search with identical content') instead
of producing an answer.
Root cause: the hint was prose-only ('respond with tool:chat and
answer naturally...'). Every other case in this file where a small
local model needed to reliably follow a multi-step instruction
needed the exact JSON shape shown, not just described -- proven
repeatedly on this same file (memory's recall hint, review's
GOOD ANSWER example, web_search's own done:false fix earlier today).
The web_search hint was the one spot that still only had prose.
Rewrote it as two explicit, numbered JSON templates -- one for
answering from the snippets, one for fetching a specific result --
instead of a single descriptive paragraph, so the model has something
to copy the shape of rather than paraphrase into its own.
Test assertions updated for the new wording; 312 tests, lint/format
clean.
Not yet re-validated against the live model -- next real test is
what actually confirms the loop no longer happens.
Two prompt-engineering attempts at the web_search steering hint (prose-only, then an explicit worked JSON example) both failed to stop the model from repeating an identical web_search call on the second step -- confirmed live both times. Also ruled out a KV-cache correctness bug (the same architecture-level issue root-caused for this model in v3.8) by disabling LLAMA_CPP_CACHE_PROMPT and reproducing the exact same repeated output with fresh, uncached processing. This is a genuine self-correction limit of this model class for 'recognize you already tried X, do something else,' not a fixable prompt-wording or infra problem. Rather than a third guess at prompt phrasing, extended the existing memory-tool loop-guard fallback (orchestrator.py) to also cover web_search: a repeated identical call degrades to the already- successful previous result (the raw search listing) instead of surfacing the internal 'Stopped: the router tried to repeat the same step.' message. Same reasoning as memory's own fallback, same code path, just widened from a single tool to a small tuple -- every other tool still hard-fails on a repeat, since that's still a genuine bug signal there. Honest tradeoff: this doesn't give a synthesized answer when the loop happens, it gives back the raw search results -- strictly better than an error, not a full fix for synthesis reliability with this model. 2 tests: the new web_search fallback (mirrors the existing memory one), and the existing 'other tools still hard-fail' test renamed and kept passing unchanged in behavior. 313 tests, lint/format clean.
… call This is the real fix for the web_search chaining problem, not another prompt tweak. Two different steering-hint designs for a router-decided second step after web_search both failed live (the model repeated the identical search instead of following either option), and disabling prompt caching reproduced the exact same failure, ruling out the KV-cache bug already known for this model (v3.8) -- this is a genuine small-model limit at multi-step self-correction, not a fixable prompt or infra problem. The fix removes the decision from the router's hands entirely: - forge/tools/web_search.py refactored to expose search() -> list[dict] (structured results), with run() now a thin formatter on top for direct chat/router dispatch. No behavior change for existing callers. - forge/graphs/research.py (new): search -> fetch top N result URLs (via web_fetch.run(), a failed individual fetch is skipped, not fatal) -> single LLM call synthesizing one answer from snippets + fetched excerpts. Same deterministic-sequence pattern already proven for graphs/review.py, and reuses its anti-JSON-habit prompt technique (GOOD ANSWER / NEVER DO THIS) plus its own dedicated response cleaner (not the router's JSON-first parser). - forge/tools/research.py (new): dispatchable wrapper, content = plain query text (no JSON needed, single field). Single call from the router's perspective -- never done:false, never a second step. - Router prompt: three-way hierarchy now taught explicitly -- research is the default for an actual answer about something current (moved the 'actualités en bourse' example here, no done:false); web_search is only for when the user wants links/ sources themselves, not an answer; web_fetch defers to research now instead of the stale 'use web_search then decide' advice. - New config: RESEARCH_FETCH_TOP_N (default 3), RESEARCH_FETCH_CHARS_PER_RESULT (default 1500). 24 new tests (11 web_search incl. the new search() structured API, 8 for the research graph's nodes/edges/error paths, 3 for the dispatch wrapper) plus 5 router prompt tests rewritten for the new three-way hierarchy. 328 tests, lint/format clean. Not yet validated against a live SearXNG + model -- next real 'actualité du jeu vidéo' test is what confirms this actually produces a synthesized answer without ever looping.
Real bug hit on research's very first live run: the search+fetch
worked correctly (single call, no loop), but the synthesized answer
-- a genuine, substantive multi-paragraph response about 2025 game
releases -- was shown wrapped in {"tool":"chat","content":"..."}
instead of clean prose.
Root cause: graphs/research.py's _clean_synthesis_response was
written with its own copy of the think-block-stripping and leak-
marker logic from graphs/review.py, but the conditional-unwrap fix
(review's third iteration on this exact problem) was never ported
over -- two independently duplicated implementations drifted the
moment one of them got a fix the other didn't.
Fix: extracted the shared logic into forge/text_cleaning.py
(strip_think_blocks, try_unwrap_router_json) and both graphs/review.py
and graphs/research.py now import it instead of keeping their own
copies. Prompt-specific leak markers stay defined separately in each
file (tied to each prompt's own wording, not shared behavior -- same
reasoning as TOOL_DESCRIPTIONS duplication in router/prompt.py).
7 new tests for the shared module directly, 1 new test locking in
research's substantive-unwrap case (mirrors review's existing test).
336 tests, lint/format clean.
Not yet re-validated against the live model -- next real research
run is what confirms the synthesized answer now displays as clean
prose instead of raw JSON.
Caught while checking whether v3.10 was ready to close: .env.example hadn't been touched all session (test/web_fetch/web_search/research config was completely undocumented there), and README.md's Roadmap table still listed v3.7 as 'current' -- v3.8, v3.9, and this entire v3.10 session were missing from both the Configuration/Tools tables and the Roadmap. - .env.example: new sections for the test tool, review graph, web_fetch, web_search (SearXNG), and research, following the same style/detail level as every existing section (container-networking note repeated for SEARXNG_URL, same as LLAMA_CPP_URL already had). - README.md Configuration table: all new env vars added. - README.md Tools table: review/test/web_fetch/web_search/research rows added; git's row now explains why it stays read-only-only by design (a write has real cost if the router hallucinates). - New prose section explaining why research exists alongside web_search -- the router-chaining reliability problem and why the fix is architectural (a graph) rather than another prompt attempt, same style as the existing grammar-constrained-decoding writeup. - CLI usage example for review --tests, /review's test_path field noted in the API table. - Roadmap table: v3.7 marked done, v3.8/v3.9 added as done, v3.10 added as current with a summary of this session's actual scope. No code changes -- 336 tests still passing, lint/format clean.
…ions Follow-up to the previous doc commit -- missed two spots on the first pass: the file-tree listing under Architecture still only showed graphs/review.py (with a stale description, pre-dating its optional test_path step) and no graphs/research.py or text_cleaning.py at all; and the very first 'Core Concept' tool list at the top of the README was still frozen at the v3.2 tool set (chat/code/files/shell/git), missing memory (v3.7) and everything from this session. Both updated to the full current tool/module set.
Two long-standing open items closed at the user's request. 1. Date awareness: the model has no reliable notion of 'today,' only a stale training-time sense of it -- confirmed live (confused 2025 and 2026 in a research synthesis) until the user manually stated the date in their own prompt. forge/context_info.py centralizes today_line() (a single ISO-date prompt line, deliberately the machine's local naive date rather than UTC -- Forge runs as one instance on the user's own machine, so 'today' means today where that machine is) and it's now injected into all three prompts that generate user-facing text: the router, review's analysis prompt, and research's synthesis prompt (the one where this actually mattered live). 2. Vague file reference resolution (open since v3.9): asking Forge to 'analyse le contenu' or 'améliore-le', referring implicitly to a file mentioned earlier without naming it again, could make the model answer from imagined content instead of ever reading the real file -- a cross-turn reference resolution problem, harder than a single missing worked example since it spans multiple turns of conversation history, not one call. The history block (router/prompt.py's _format_history) already contains the real path in any persisted files/review confirmation (e.g. '[ok] written 9 bytes to notes.py') -- the model just had no instruction to go find and reuse it. Added a standing instruction (not step_context-gated like the read/write hints below it, since this spans turns, not steps within one run) telling the model to resolve a vague reference against the most recent real path already visible in history, and to read the file first if it doesn't already have the current content. Both verified not to disturb the KV-cache-stability contract from v3.8: the file-reference instruction is a pure function of alone (present iff history is non-empty), same as the rest of _format_history, and the date line only changes once per calendar day -- test_history_block_is_stable_regardless_of_step_context still passes unchanged. 7 new tests (context_info module, date line present in all three prompts, vague-reference instruction present/absent by history presence). 343 tests, lint/format clean. Not yet validated against the live model for either fix -- next real test on hardware is what confirms the model actually acts on the grounding/instruction rather than just having it available in the prompt.
CI failure on the PR: 'executable not found: pytest' -- 2 tests failing in GitHub Actions while passing locally. Root cause: the subprocess is deliberately run with a minimal, hardcoded env (PATH=/usr/local/bin:/usr/bin:/bin, no leaked host secrets/tokens -- same pattern as tools/shell.py) and the runner lookup relied on that same restricted PATH. That's fine for coreutils (ls, cat, grep all reliably live in /usr/bin), but pytest/ruff are pip-installed console scripts that can land anywhere depending on how Python was set up -- a venv, --user, or (this case) GitHub Actions' own hostedtoolcache via actions/setup-python. My own sandbox happened to have pytest at /usr/local/bin (a system-wide install), which is exactly why this passed there and nowhere else. Fix: resolve the runner's absolute path via shutil.which() against the real process PATH (which does include wherever pip actually put it), then invoke that resolved path directly. The subprocess still runs with the same minimal, hardcoded env as before -- only the lookup changed, not the sandboxing. 1 new test: symlinks the real pytest into a directory that is nowhere in the hardcoded PATH, points /home/claude/.npm-global/bin:/home/claude/.local/bin:/root/.local/bin:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin at only that directory, and confirms the tool still finds and runs it -- reproduces the exact CI failure shape locally rather than just asserting the fix abstractly. 344 tests, lint/format clean.
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
1 participant
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.
23 commits. Every fix here was found through real usage, not anticipated in advance —
the branch grew by testing each new piece live and following the bugs it actually produced.
New tools
test— dedicated pytest/ruff runner, its own allowlist (TEST_ALLOWED_COMMANDS)independent of the general shell tool's.
web_fetch— fetches a known URL. Non-configurable SSRF guard (blocksprivate/loopback/link-local resolved IPs, no auto-redirects), optional domain
allowlist, stdlib-only HTML→text extraction.
web_search— SearXNG-backed (self-hosted, not a cloud API), returns rankedlinks/snippets only.
research— search → fetch top N results → single synthesized answer, run as onedeterministic call. Exists specifically because asking the router to chain
web_searchinto a second step (answer or fetch) proved unreliable with thismodel — two different steering-hint designs both failed the same way (repeated
the identical search instead of following either option), and disabling prompt
caching reproduced the exact same failure, ruling out a KV-cache bug. The fix is
architectural: remove the decision from the router entirely, same pattern as
review.reviewWhat was briefly a separate "code" agent (read → run tests → LLM review) got merged
into the existing
reviewgraph instead — three near-identical "code" conceptswould have confused both the router and future maintenance.
reviewnow optionallyruns a file's tests first (
test_path) and feeds the output to the analysis asprimary evidence. Dispatchable directly from chat (
tools/review.py), consistentwith the single-conversation, zero-tab UI constraint — no dedicated form was added.
Real bugs found and fixed, in the order they were hit
review(and, once retested,files) treated a leading/as an escape attempt or read from the host filesystem root instead of theworkspace. Fixed in both:
/hello.goandhello.gonow mean the sameworkspace-root file.
reviewvsfiles— traced to the prompt itself, nothistory:
review's own first worked example was "relire" with no request forfeedback, teaching the model that the bare verb meant review. Rewritten with an
explicit contrastive example on each side.
reviewreturning 8 characters instead of an analysis — root cause:reusing the router's JSON-first parser on a prompt that asked for plain text.
A small model conditioned on the router's JSON habit answered with a degenerate
JSON echo, and the parser "successfully" extracted it as if it were real. Fixed
with a dedicated response cleaner (
text_cleaning.py, shared withresearchafter the same bug reappeared there independently) that conditionally unwraps a
JSON-wrapped answer only when the content looks substantive, otherwise shows the
raw text so a bad response is visibly wrong instead of silently truncated.
web_fetchswamped by navigation chrome on a real Wikipedia fetch — fixedby excluding
nav/header/footer/aside, not justscript/style.web_fetchwith zero router description — produced malformed content("unsupported scheme: ''"). Given a description + example like every other tool.
web_searchresult dumped raw instead of continuing to a synthesis step —missing
"done":falseon the worked examples. Fixed, then hit loop-guardrepeats on the next step regardless (prose-only hint, then an explicit JSON
example, both failed) — which is what motivated building
researchinstead ofcontinuing to patch the router-chaining approach. The loop guard also gained a
graceful degrade-to-previous-result fallback for
web_search(mirroring theexisting one for
memory), for the cases whereresearchisn't in play.research's first live run: search+fetch worked (no loop), but thesynthesis was still shown wrapped in router JSON — the unwrap fix from V3.5 hardening #3 had
never been ported over. Shared the logic properly this time.
"today" (confused 2025/2026 in a live research synthesis) — now injected into
all three user-facing prompts; and a vague file reference ("améliore le
contenu", naming nothing) could make the model invent content instead of
reading the real file — the router now knows to resolve that against the most
recent real path already visible in conversation history.
Also
gitstays strictly read-only — a write (commit/push) is deliberately never arouter-reachable decision, only a separate human-confirmed flow.
.env.exampleandREADME.md(Configuration, Tools, Roadmap, architecturefile tree) updated — they'd fallen behind since v3.7.
httpx2added as a dev dependency, clearing aStarletteDeprecationWarning.Testing
344 tests (up from ~230 at the start of this branch),
ruff checkandruff format --checkclean, zero warnings underpytest -W error.