Post-review fixes: proxy input hardening, non-blocking Ollama stop, client shutdown, loud arg decode - #86
Merged
Merged
Conversation
Backfills tests for the input validation merged in #71, which landed without coverage for its new 400 responses. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
subprocess.run() blocked the event loop for the duration of "ollama stop". Switch to asyncio.create_subprocess_exec + await so concurrent coroutines stay responsive. Reimplements #66; lets the command's stderr surface rather than suppressing it. Co-authored-by: hobostay <110803307+hobostay@users.noreply.github.com> Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
… bad tool-call args - Add aclose() to the LLMClient protocol and all clients; ProxyServer closes the active client's httpx pool in _async_stop, eliminating the unclosed-pool ResourceWarning. Completes #67, which added the method but wired no caller. - AnthropicClient._convert_messages now raises a clear ValueError naming the tool and offending payload on malformed tool-call argument JSON, instead of an opaque JSONDecodeError. Addresses #69 (kept loud rather than swallowing to an empty dict). Co-authored-by: hobostay <110803307+hobostay@users.noreply.github.com> Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
The facade covers validation, retry nudges, and step enforcement; prerequisites are a granular-API feature (StepEnforcer.check_prerequisites). Makes the existing "caller wires" intent explicit in the Mode 3 section. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
This was referenced May 26, 2026
antoinezambelli
added a commit
that referenced
this pull request
Jun 1, 2026
* Proxy: native-only + transparent OpenAI passthrough
Make the OpenAI-compatible proxy native-tool-call-only and forward the
client's tools/messages verbatim, bypassing the lossy ToolSpec round-trip
that dropped schema detail and leaked empty tool names.
- Remove the proxy's --mode surface; the proxy always drives the backend
client native. LlamafileClient's prompt-injection machinery is retained
for non-proxy WorkflowRunner / direct-client use (it still wins for some
models in full-guardrail workflow evals).
- Add raw_openai_tools to the LLMClient protocol; LlamafileClient's native
path sends it verbatim. Other clients accept-and-ignore (vLLM also gains
the previously-missing passthrough/inbound_anthropic_body kwargs).
- run_inference forwards raw OpenAI messages/tools only on the clean first
attempt (use_raw_messages gate); any mutation falls back to fold+serialize.
- respond tool is now opt-in (--inject-respond-tool, default off).
- No instrumentation (proxy_trace/guardrail_stats deliberately not ported).
- Tests: drop removed mode-guard tests; respond tests opt in explicitly; add
native-passthrough, detachment, respond-default, and first-attempt-gate
coverage. Docs: ADR-012 revision + BACKEND_SETUP proxy note.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* Add prompt-injection as opt-in proxy capability (--backend-capability)
The proxy serves tool-call-capable backends natively (verbatim tool/message
passthrough). This adds prompt-injection back as an explicit opt-in for
non-function-calling backends (llama.cpp / llamafile without a tool template).
- New --backend-capability {native,prompt} (default native), declared once at
construction and frozen — no runtime probing or mid-request mode mutation.
- prompt capability reuses LlamafileClient's existing prompt path (build the
tool prompt, downgrade tool/assistant-tool_call history to text, parse the
JSON tool call back into native tool_calls). No client changes.
- Handler suppresses verbatim raw passthrough when in prompt mode so inference
folds normally and the client injects the tool prompt.
- Rejected for backends that are native-only (vLLM, Ollama, anthropic protocol).
- Docs: BACKEND_SETUP + ADR-012 updated to native-first + prompt opt-in.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* Proxy: log effective backend_timeout at startup
The configurable backend_timeout (#91) was validated, stored, and threaded
into every client request, but never surfaced at launch. Extend the
"Proxy ready" line to report the effective value so the operative timeout
is visible/diagnosable from the startup log.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* vLLM: single source of truth for model identity (#75)
VLLMClient kept two identity fields with distinct roles — model_path (the
verbatim wire "model" field, which vLLM validates against its
--served-model-name) and model (the derived registry-lookup key). The proxy's
external-mode served-name adoption set both by hand (model_path = served;
model = served), duplicating the derivation logic and storing the full served
name where the constructor's rule stores the stem.
Extract the path->key derivation into _derive_model_field and wrap both
assignments in _set_model_identity, then call it from __init__ and from the
proxy. External adoption now upholds the same (model_path, model) invariant as
construction: an HF-repo-id served name reaches the wire verbatim while the
registry key is the derived stem.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* Clients: consistent malformed-tool-call + response-shape handling
Audited malformed-tool-call and unexpected-payload handling across the
OpenAI-shape clients against the reference set by OpenAICompatClient (#89) and
LlamafileClient. Standardize on one principle, applied uniformly:
- Malformed argument JSON (a model mistake) -> TextResponse, routing the raw
output back through the inference loop so the rescue/retry path can recover.
- A broken provider envelope (missing choices/message) or unexpected args type
(a contract violation, not the model's fault) -> BackendError: fail loud and
consistent, never a stray KeyError/IndexError.
Changes:
- vLLM: replace the bare-json.loads _parse_tool_args (which *raised* on
malformed args, unlike llamafile's retry-driving TextResponse) with a
_parse_tool_calls mirroring the reference. Route both send() and send_stream()
through it so streaming and non-streaming agree: a fully accumulated but
unparseable arguments string finalizes as a TextResponse, not an exception.
- llamafile / openai_compat: guard the bare data["choices"][0]["message"]
subscripts -> BackendError on a broken envelope (matching what vLLM already
did for choices). llamafile also hardens function/name access.
- ollama: defensive .get on function/name (both paths); document that Ollama
emits dict args by contract, so no json.loads is needed there.
Tests: vLLM _parse_tool_calls (string/dict/empty/malformed/unexpected/missing-
function/reasoning) + streaming malformed-fragment parity; envelope-guard tests
for llamafile and openai_compat. 1092 unit tests green.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* LlamafileClient: remove runtime auto mode; native-first, frozen capability
Drop mode="auto" and its runtime probe-and-mutate (_resolve_and_send: try
native, fall back to prompt on HTTP error, recording resolved_mode). This was
the last vestige of the mid-request capability mutation the proxy rewrite
excised everywhere else; the proxy already declares its capability up front via
--backend-capability. With auto gone, resolved_mode is always == self.mode, so
the whole tri-state indirection collapses to a direct dispatch on self.mode.
The default is now native. This is both hardening and a deliberate posture
shift: local-model function-calling support has matured into the more reliable
path, so native-first is the right default. Prompt-injection is preserved as an
explicit opt-in (mode="prompt") and is the theoretically correct fallback for
non-FC backends — but it is honestly flagged, in the docstring and docs, that
models tend to struggle to drive the prompt-injected protocol reliably on more
complex, multi-step interactions. Capability is declared-and-frozen: an invalid
mode (including the old "auto") now raises ValueError rather than silently
degrading.
- llamafile.py: validate mode in __init__; default native; delete
_resolve_and_send and the resolved_mode attribute/branches; dispatch send /
send_stream on self.mode; rewrite the class docstring (native-first rationale
+ prompt caveat).
- eval_runner.py: --llamafile-mode choices [native, prompt], default native.
- docs (BACKEND_SETUP, EVAL_GUIDE): native-first wording + the prompt caveat.
- tests: drop the auto-mode suite; assert native default + ValueError on "auto".
Consumers verified unaffected: the proxy (both sites), batch_eval, and the
integration script all pass mode explicitly. 1086 unit tests green.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* fix(eval): honor manual context budget in batch_eval via start_with_budget
batch_eval brought servers up with a bare server.start() (no ctx_override)
and resolved the budget separately via server.resolve_budget(), so
--budget-mode manual --num-ctx N was a no-op for llama-server: the server
booted at the model's full native context (no -c), and resolve_budget(MANUAL)
just read that full value back from /props. (Ollama was unaffected — its
context is per-request via set_num_ctx.)
Route both the initial bring-up and _recover_server through the prod
start_with_budget() path, which threads manual_tokens -> ctx_override -> -c
at launch and returns the resolved budget. _recover_server gains
budget_mode/manual_tokens params so a restarted server reuses the same
budget. Drops the now-redundant standalone resolve_budget() on the happy
path (still used on the recovery branch to read back the resolved value).
This also fixes FORGE_FAST mode, which the old bare-start() path never
supported.
Smoke-tested live (Ministral-3 14B-Reasoning, native, --num-ctx 20000):
server boots with -c, rows record budget_tokens=20224 (server-clamped)
instead of the previous 262144 full-native read-back.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* chore(release): 0.7.3 — native-first proxy
Bump version 0.7.2 -> 0.7.3 and add the CHANGELOG entry covering this
branch plus the commits that landed on main since 0.7.2 (OpenAICompatClient
#89, --backend-timeout #91, and fixes #71/#72/#73/#86/#94).
Headline: native-first proxy. BREAKING — the proxy --mode flag is renamed
to --backend-capability (no alias; --mode was only introduced in 0.7.1).
Native is the default and only auto-selected protocol; prompt-injection is
an explicit opt-in for non-FC llama.cpp/llamafile backends.
USER_GUIDE: --mode -> --backend-capability, with the caveat that prompt mode
tends to degrade on more complex multi-step interactions. BACKEND_SETUP,
EVAL_GUIDE, and ADR-012 were already updated earlier on this branch.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
antoinezambelli
added a commit
that referenced
this pull request
Jun 3, 2026
* Args-validation: route malformed args through tool-error channel
Models occasionally emit a structurally valid tool call with malformed
args content (e.g. arguments="" instead of arguments="{}"). Pydantic
rejected at ToolCall construction, crashing the stage with
ValidationError. Observed at 86% of error rows on Qwen3-Next prompt
mode (77/89), same family on Qwen3.6 (rig-02).
This is conceptually "tool called with bad args" — the call exists,
the inputs are wrong — same as FileNotFoundError at runtime. Should
ride the tool-error channel with max_tool_errors=2 budget, not crash.
- ToolCall / TextResponse: BaseModel → @DataClass. args is no longer
validated at construction; ResponseValidator enforces dict-shape.
Audit: no .model_* API on ToolCall anywhere in forge.
- ResponseValidator: new args-shape branch after unknown-tool check.
Unknown-tool runs first (cheap; no point validating args on a
hallucinated tool name).
- nudges.tool_arg_validation_nudge: schema-derived message naming the
tool, the received args type, and the required JSON-object shape.
- inference: parse-error nudges drain max_tool_errors (record_result)
not max_retries (record_retry). Message prefix
[ToolArgValidationError] vs [UnknownTool].
- Exhaustion message simplified: includes which budget and nudge kind.
Smoke (Ministral-3-14B-Reasoning, 26 scenarios × 25 runs prompt-mode):
score 78.77% (vs v0.7.0 baseline 79.5% at n=50). Delta within
±1.6% noise band — no regression. Patch never tripped on this model;
this is a regression check, full bake on a model that hits the path
to follow.
884 tests pass.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* batch_eval/sampling: 32GB-eval lineup — Qwen3.6-27B, Qwen3.6-35B-A3B-UD, Nemotron-3-Nano
GGUF + server-flag + sampling-default entries for the three rig-02
32GB-tier models added for the v0.7.1 run. Launch config of record
for eval_results_rig-02_v0.7.1.jsonl (31,200 rows).
* clients: route malformed tool-call args through the tool-error channel
Unify all OpenAI-shape clients on one decode_tool_args helper
(clients/base.py): JSON-string args are parsed; malformed or non-dict
payloads ride through on the ToolCall as raw (non-dict) args instead of
collapsing to a TextResponse (openai_compat, vllm, llamafile) or raising
(anthropic streaming). ResponseValidator's args-shape check then routes
them to the tool-error channel + max_tool_errors budget — the same lane
as a runtime tool error — rather than a retry nudge.
Completes the client normalization #86 started (one decoder, all
clients) and keeps fail-loud (never coerced to {}). Also closes an
unguarded json.loads crash in the anthropic streaming finalize.
Behavior change: structural malformed-args now drains max_tool_errors
(2), not max_retries (3), in proxy mode. Wire-invisible; no public
signature changes.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* review fixes: honest ToolCall.args type + de-stale comments + llamafile malformed test
From the helper-vs-inline structural review (verdict: keep the shared
decode_tool_args helper). Two correctness/honesty caveats actioned:
- ToolCall.args annotated dict[str, Any] while the runtime contract now
intentionally allows non-dicts (the docstring already says so). Widen
to Any so the type stops lying.
- Stale comments in openai_compat/vllm streaming finalize still claimed
malformed args yield a retry-driving TextResponse; corrected to the
raw-args → tool-error-channel routing (they invited the exact drift
the helper prevents).
- Add an explicit llamafile malformed-args test (non-stream native path).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* guardrails: make tool-call faults consistent across all three modes + expose proxy max_tool_errors
From the H1/H2 design review. Two consistency gaps closed:
1. The Guardrails middleware facade recorded EVERY validator failure as a
retry (max_retries) and returned action='retry' — diverging from
run_inference/proxy, where malformed args drain the tool-error budget
and tool-call faults ride the tool channel. The facade now:
- routes malformed args (tool_arg_validation) to max_tool_errors,
- returns a new action='tool_error' for tool-call faults (unknown
tool name OR malformed args), with nudge.role='tool' so callers
emit the correction on the tool-result channel.
Channel vs budget are now two explicit kind-sets in nudge.py
(TOOL_CHANNEL_KINDS ⊃ TOOL_ERROR_KINDS); unknown-tool rides the tool
channel but still drains the retry budget, matching run_inference.
_TOOL_ERROR_KINDS moved from inference.py to the shared nudge module.
2. Proxy exposed max_retries but not max_tool_errors, hiding the budget
exactly where malformed-arg recovery now matters. Added --max-tool-errors
(default 2) threaded ProxyServer → HTTPServer → handler → ErrorTracker.
Nobody depends on the middleware facade yet, so the CheckResult.action
addition is free. Channel parity in run_inference unchanged (it emits
role=tool for list-branch corrections regardless of nudge.role).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* steps: guard arg-match prereqs against non-dict args + ADR-016
Closes the last crash vector from the design review (hole 4):
StepTracker.check_prerequisites did args.get(match_arg), which raises on
a non-dict args. ResponseValidator fences this before dispatch in the
runner/proxy, but a granular caller that bypasses check() could reach it
directly. Treat non-dict args as unsatisfied (block, don't crash).
ADR-016 records the malformed-args → tool-error-channel decision in its
honest framing: a native-mode conditioning bet, not an ontology claim;
prompt mode degrades to the prior retry shape; the tool-error budget
coupling is deliberate but revisitable. CHANGELOG held for release time.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* eval data: tag generations + add 32GB tier (v0.7.4)
Inject a per-row `gen` field so one dashboard can fold eval waves run
against different code states. gen is a comparability epoch, not a
release version: v0.6.0 -> gen 1 (carries the Anthropic ablation +
Retired-tier models, neither re-run since), and v0.7.0 plus the new
32GB tier -> gen 2.
Rename the 32GB wave to eval_results_v0.7.4.jsonl (its landing release)
and keep it as a separate file beside v0.7.0 — same gen, distinct wave,
so each wave keeps its own landing commit for reproducibility.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* eval dashboard: eval-generation badges + retired toggle
report.py now accepts multiple result files and keeps the newest gen per
config (dedup_latest_gen), so the board folds all generations into one
view. Lagging rows (gen < newest) get a superscript badge backed by a
commit/date legend; Retired-tier models are carried forward but hidden
by default (--include-retired, or a sidebar checkbox in the HTML). Adds
MODEL_FAMILIES entries for the 6 32GB models so they render clean family
names and cross-backend keys instead of raw GGUF stems.
React dashboard: Show-retired checkbox (dimmed rows + a "retired" pill),
superscript gen badges with provenance tooltips from the data blob.
Regenerated docs/results/ from the three gen-tagged datasets.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* release: 0.7.4 — version bump, CHANGELOG, registry 32GB tier
Bump 0.7.3 -> 0.7.4 and add the 0.7.4 CHANGELOG entry (malformed args ->
tool-error channel; 32GB eval tier + dashboard eval-generations). Move
the 6 32GB models (Mistral-Small-3.2, Qwen3.5/3.6 27-35B, Nemotron-3
Nano) from Unpublished to Current now that they're in the published eval,
and reword the tier definitions for the dashboard's eval generations.
Also scrub a stale bring-up note from the Qwen3.5-122B footnote (it leaked
operator smoke-probe process into a public doc) and exclude the built
dashboard dist/ from the hatchling sdist sweep.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* changelog: de-escalate dataclass entry from BREAKING to Changed
The ToolCall/TextResponse pydantic->dataclass move only breaks callers
who serialized these via the pydantic .model_* API or relied on
construction-time validation; attribute reads and keyword construction
are unchanged. Reserving BREAKING for forced-migration changes (cf.
0.7.3 --mode rename) keeps the badge meaningful.
---------
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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.
Follow-ups from reviewing the AI-generated PR batch (#65–#73). Reimplements the keepers cleanly and backfills missing tests.
ollama stopno longer blocks the event loop (async subprocess; stderr surfaces instead of being suppressed).aclose()on all clients + theLLMClientprotocol, wired into proxy_async_stopso the httpx pool is closed on shutdown. The original added the method but wired no caller.ValueError(naming the tool + offending payload) on malformed tool-call argument JSON, instead of an opaqueJSONDecodeError. Kept loud rather than swallowing to an empty dict.Content-Length, non-object body) the merge landed without.Guardrailsfacade does not enforce tool prerequisites; that's a granular-API feature (StepEnforcer.check_prerequisites).No version bump / CHANGELOG entry — changelog tracks formal releases only; this rides into the next one. 1022 unit tests pass.
Supersedes #66 (still open — close on merge). Ideas from #67/#69 credited via co-author trailers. Thanks @hobostay.