fix!: make an undeclared wire name unrepresentable, and delete the ones nothing reads - #428
Merged
Merged
Conversation
Twelve public builder methods went away, because none of them could reach llama.cpp. Backward compatibility is deliberately not preserved: a method that writes a key the receiver discards is worse than no method, since the call site reads as configuration and behaves as a no-op. Five wrote request keys that no pinned llama.cpp has ever read -- withTfsZ (tfs_z), withPenalizeNl (penalize_nl), both withPenaltyPrompt overloads (penalty_prompt) and withUseChatTemplate (use_jinja). The request schema silently discards unknown fields, so these were invisible at runtime and uncatchable by any integration test. Seven wrote CLI flags the server argument parser does not register. #426 had already stopped them emitting -- setGrpAttnN, setGrpAttnW, enableDumpKvCache, setHfRepoV and setHfFileV became no-ops, enableMlock and disableMmap were re-pointed onto --load-mode -- which kept callers loading but left the API claiming capabilities it does not have. setLoadMode(LoadMode) is the whole replacement: LoadMode.MLOCK was --mlock, LoadMode.NONE was --no-mmap. Verified against the history rather than assumed, because it changes what this is: all eleven wire names were inherited from kherud/java-llama.cpp at fork point 49be664 (its own pin was llama.cpp b4916), and SIX of them were already non-functional at that pin -- the four request keys appear zero times in b4916's examples/server + common, and --grp-attn-n/-w carried set_examples({LLAMA_EXAMPLE_MAIN, LLAMA_EXAMPLE_PASSKEY}) there exactly as they carry {COMPLETION, PASSKEY} at b10883. A seventh, --dump-kv-cache, was alive at b4916 and already dead at b9994, this repo's first recorded commit. So the dominant cause is not version drift, it is surface that was never executed against the receiver; the docs/history writeup lands with the guard in a later commit of this series. Two follow-on deletions: ParameterJsonSerializer.buildIntArray had withPenaltyPrompt(int...) as its only caller and is gone with it, and ChatAdvancedTest's three model-backed "must produce output" tests for tfs_z and penalty_prompt are gone -- they passed for years while the server ignored the parameter, which is precisely the false confidence being removed here. Its testUseChatTemplateInGenerate is renamed to testMessagesInGenerate and says in its javadoc that it was always measuring withMessages() alone. README's InferenceParameters snippets were repaired in passing: besides the two dead calls they used a set* naming the class has not had for a long time (it has zero set* methods), so the documented examples did not compile. Verified: mvn test 1740 tests green across the reactor (19 fewer, matching the deletions exactly), PIT 320/320 killed at 100%, SpotBugs 0 bug instances, spotless and javadoc:jar clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AnNYn8W1xuVxVJtyL34GyH
The request body was assembled by string concatenation over the parameter map,
and five public setters take a JSON fragment from the caller and stored it in
that map verbatim: withJsonSchema, withResponseFormat, withStreamOptions,
withMessagesJson and withToolsJson. A fragment carrying a top-level comma
therefore did not only set its own field, it injected SIBLING fields into the
body -- and because the native parser resolves duplicate keys last-wins, the
injected value won over one the application had already set.
Demonstrated against the built classes rather than reasoned about:
InferenceParameters.of("hi").withNPredict(16).withCachePrompt(true)
.withResponseFormat("{\"type\":\"json_object\"}, \"n_predict\": 999999, \"cache_prompt\": false")
{"n_predict": 16, "response_format": {...}, "n_predict": 999999, ...}
and nlohmann parsing that body yields n_predict=999999, cache_prompt=false. Any
host that builds tools/response_format/messages JSON from something it does not
fully control -- an agent tool registry, a client request, a template -- could
have its own limits overridden that way. It is now rejected at the call site
that supplied the fragment, with the offending key named.
The fix is an invariant rather than five patches: every value stored in the map
must be EXACTLY ONE well-formed JSON value, checked centrally in withPut. Note
FAIL_ON_TRAILING_TOKENS is load-bearing -- plain readTree parses the first value
and ignores what follows, so without it the injection payload would have been
silently truncated to its first object instead of rejected, trading one quiet
wrong answer for another.
The invariant immediately caught a latent defect it was not written for:
JsonParameters.withEnum stored getArgValue() unquoted (q8_0, not "q8_0"), which
is not a JSON value at all. It had no production caller -- the CLI side has its
own putEnum, where a bare string is correct because argv is not JSON -- so it is
deleted rather than fixed. This is exactly the class of never-executed surface
the previous commit removed.
Two further changes follow from the same reasoning:
* toJson() is now the wire serializer and LlamaModel calls it at all six
payload sites. It renders through Jackson, and emits keys in sorted order --
JSON objects are unordered so the server does not care, but the old renderer
walked a HashMap and produced a different byte sequence run to run, which is
needlessly hard to diff, log or assert on.
* toString() becomes a redacted debug view and deliberately NOT valid JSON:
"InferenceParameters{keys=[...], values=redacted}". A parameter set carries
the prompt, the message history and the tool definitions, so a log line built
from one leaked the whole payload. Making the redacted form unparseable means
a caller who still passes toString() to the native layer fails at the parser
instead of quietly sending a different body -- a loud incompatibility rather
than a silent one.
JsonParameters joins the PIT gate (targetClasses), since it now carries a
security invariant rather than plumbing. The rest of the parameters package
stays off it on purpose: ~200 one-line builder setters would add cost, not
signal. Getting it to 100% needed one more test than expected -- the excerpt
boundary in the rejection message (the message shows a bounded prefix so a
hostile fragment cannot flood a log through it) is observable only at exactly
the limit, and is now pinned from both sides.
Verified: reactor tests 1748/49/6 green, PIT 334/334 killed at 100% (was
320/320; the 14 new mutations are JsonParameters), SpotBugs 0 bug instances,
spotless and javadoc:jar clean. The injection reproducer now prints
"REJECTED: response_format must be exactly one well-formed JSON value" and a
legitimate fragment still round-trips unchanged.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01AnNYn8W1xuVxVJtyL34GyH
Every name this library puts on a wire is now an enum constant carrying the
contract it must satisfy, and the base classes accept nothing else. Four new
types: args.ModelOption (107 value-taking CLI options), args.CliContract,
parameters.RequestField (58 request keys, package-private on purpose -- the
point is a closed set, not an escape hatch) and parameters.RequestContract.
ModelFlag gains its contract too. 111 ModelParameters call sites and all 58
InferenceParameters keys moved off string literals; the maps stay keyed by the
wire string because that is what toArray()/toJson() emit.
This is the structural half of what the previous two commits fixed by hand. The
history says why it is the right shape: of the eleven dead names deleted in C1,
six were already non-functional at the fork point's own llama.cpp pin, so the
dominant failure was never version drift -- it was surface that could be
declared without anything checking it. A string key makes that state
representable; an enum constant with a declared contract does not.
The contract lives on the constant rather than in a list inside the checking
test, which is what stops the exemptions rotting: --vocab-only is
PROJECT_PSEUDO (stripped from argv before the parse) and the eleven OAI-layer
request keys are OAI_LAYER (consumed before llama.cpp's schema runs), both
stated where the name is declared.
Three properties become checkable that could not be expressed while the names
were literals scattered through ~200 setters, all in the new
WireNameRegistryTest:
* Uniqueness across both CLI registries. Two setters writing the same argv key
silently let the last call win; nothing could see that before.
* Reachability. Every declared constant must actually be emitted by some public
builder method -- driven reflectively over all of them with several argument
shapes. This is the inverse of the C++ contract test: that one asks whether
every emittable name is still accepted by llama.cpp, this one asks whether
every declared name is emittable at all. Writing it immediately found two
gaps in its own driver rather than in the code (--fit is emitted by the
constructor, --samplers only by a varargs setter), which is the useful kind
of first failure.
* The SpotBugs OCP_OVERLY_CONCRETE_PARAMETER suppression list no longer rots.
Only the stale-entry direction is checked, deliberately: a suppression naming
a method that no longer exists does nothing at all, which is exactly how the
setTensorReadLazy -> setLazyMode rename reddened main. The opposite direction
already reds spotbugs:check, and is not derivable here anyway -- OCP fires
only when a method uses nothing beyond the interface, so setPoolingType
(compares a concrete constant) and withMiroStat (calls ordinal()) are
legitimately absent from the list.
Both new guards were falsified before being trusted: an added-but-unemitted
ModelOption and an invented suppression entry each fail with the offender named.
ModelParameters.isUnset(String) is gone, replaced by isUnset(ModelOption) and
isUnset(ModelFlag). It prefixed "--" onto a bare caller-supplied key, so it
would answer a confident "true" forever about a name no builder can emit.
One trap this change contains and closes in the same commit: the CMake extractor
that feeds the C++ contract test read ModelFlag.java + ModelParameters.java, and
the flag literals just left ModelParameters. Measured rather than assumed -- the
extractor would have seen 31 of 138 names. It now reads the two registries, and
they are exactly the two files it should ever need to read. (Its own floor check
would have caught this as a configure failure rather than as silent coverage
loss, but the fix belongs here, not one commit later.)
Verified: reactor tests 1754/49/6 green, ctest 531/531 with the flag contract
back at 138 names, PIT 337/337 killed at 100%, SpotBugs 0 bug instances,
spotless and javadoc:jar clean.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01AnNYn8W1xuVxVJtyL34GyH
The CLI flags had a contract test; the request body and the fine-tuning config
did not. They are the quieter two: llama.cpp's request schema discards a key it
does not know without a word, and train_engine.cpp reads with
`j.value(key, default)`, which falls back to the default. A field that stops
being read produces no error anywhere -- the parameter simply stops having an
effect, and every Java test asserting the string mapping keeps passing. That is
how four request keys survived this library's entire recorded history unread.
Each surface now has a receiver that can be asked what it accepts:
ModelFlag + ModelOption -> common_params_parser_init(..., LLAMA_EXAMPLE_SERVER)
RequestField -> server_schema::make_llama_cmpl_schema(...)
TrainingField -> jllama_train::config_keys()
The first two oracles are upstream's own tables, walked including aliases and
(for the schema) nested subfields under their dotted path. The third had to be
created: train_engine.cpp's parse now goes through a `jllama_train::keys`
constant per field and `config_keys()` returns those same constants, so a
changed spelling moves both at once rather than drifting.
TrainingField is new on the Java side and TrainingParameters writes through it.
Both ends of that contract are ours, which is exactly why it looked like it did
not need a guard -- and why it had none at all: LlamaTrainerIntegrationTest is
gated on a system property no CI job sets, so nothing runnable covered it.
The extractor is now generic (extract-java-cli-flags.cmake ->
extract-java-wire-names.cmake) and emits a contract column beside each name, so
an exemption is read from the registry instead of being repeated inside the test
that checks it. It also got precise: it matches enum constant declarations only,
so the comment-stripping heuristic the old any-string-literal scan needed is
gone, and it now fails the configure when a registry declares a name twice.
Both exemption checks are inverted deliberately. A PROJECT_PSEUDO or OAI_LAYER
name cannot go stale by outliving its Java constant -- it lives on it -- but it
can go stale the other way: upstream may later register a name we exempted, at
which point the exemption hides a real check. Both tests assert that, and that
the exempt set is non-empty, so a generator that dropped the contract column
would not silently exempt everything.
All three new guards were falsified before being trusted, each naming the
offender: an unread request key ("gone_upstream"), a schema key wrongly declared
OAI_LAYER ("temperature is declared OAI_LAYER but the request schema now reads
it"), and a renamed trainer key, which reports both directions -- the key Java
writes that the engine never reads, and the one the engine reads that Java never
writes.
Verified: ctest 536/536 (was 531; +4 request, +1 trainer), reactor tests
1756/49/6 green, PIT 337/337 killed at 100%, SpotBugs 0 bug instances, spotless,
javadoc:jar and clang-format 22.1.8 all clean.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01AnNYn8W1xuVxVJtyL34GyH
A new docs/history entry carries the numbers behind the four preceding commits, so a later reader does not re-derive them: which of the eleven retired names were dead at which pinned llama.cpp version, the fork-point archaeology showing six of them were already non-functional at kherud's own pin (b4916) and therefore never worked in this repository at all, the inherited JsonParameters design with the comment stating the trade it made, and the field-injection reproducer with its output and the last-wins duplicate-key behaviour that made it exploitable. It also records the two things the guards do NOT cover, which is the part a summary usually loses: an OAI_LAYER key is checked only for absence from the schema (that the OAI layer genuinely reads it is documented, not asserted), and the trainer's C++-to-C++ pairing rests on both sides being written against the same `keys` constants rather than on a test. CLAUDE.md gains a "Wire-name registries" section stating the three registry/receiver/contract triples and the rules for touching one — in particular that the exemption checks are inverted on purpose, and why. Its C++ test table, the SpotBugs section (half of the by-name suppression list is a test now; the other half is not derivable and the reason is stated) and the PIT section (JsonParameters is on the gate, the rest of the package deliberately is not) are brought in line. Two claims that were true when written are now narrowed rather than deleted: CLAUDE.md and TODO.md both said the trainer path has no runnable guard. Two slices of it now do — the parameter build and the configuration key set — while the Java-to-JNI-to-native round trip still runs nowhere, which is what the entries now say. TODO.md gains the request-key exposure gap, stated as a countable list rather than a recollection: the Java layer writes 47 of the schema's keys, and the remainder is re-derivable by diffing RequestField.values() against the field table the C++ test already walks. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AnNYn8W1xuVxVJtyL34GyH
… was not
The OAI_LAYER contract exempts a key from the request-schema check because the
OpenAI/task layer consumes it before make_llama_cmpl_schema sees the body. The
test asserted exactly that -- the key is *not* in the schema -- on the inverted
reasoning that an exemption can only rot by upstream later adopting the name.
That reasoning was incomplete. Absence from the schema is satisfied just as well
by a key nothing reads at all, so the exemption was a hole exactly the size of
the problem the registry was built to close. chat_template sat in it: a public
InferenceParameters.withChatTemplate writing a name whose only occurrence in
upstream sources is the /props payload the server *emits*.
Driving the parsers directly cannot close it -- the eleven keys have three
different consumers and two need a live server_context -- so the generator now
sweeps upstream's own sources for a reader *shape* (json_value(x, "k", ...),
.contains("k"), .at("k")) over globbed tools/server/*.cpp + common/*.cpp and
emits the hit count. The shape is load-bearing: chat_template does occur as a
bare literal, so a token grep would have called it live. Measured at b10883,
every other OAI_LAYER key has at least one reader; chat_template has zero.
EveryOaiLayerKeyIsReadSomewhereUpstream fails on a count of zero, naming the key
and the remedy, and fails loud if the swept set or the source corpus is empty.
Verified by falsification: run against the tree that still declared the key, it
printed exactly that.
The dead name is deleted, not deprecated (registry rule 2). One consumer was
affected: the Android app passed its chat-template override per request, where
llama.cpp discarded it -- it now sets it at load time, which is where upstream
reads it. Two tests pinned the dead key and went with it; one of them explained
its own missing behavioural assertion with the wrong cause.
Also adds the CHANGELOG entries this branch was still missing, and repoints two
javadoc references to the extractor's post-rename filename.
ctest 537/537, mvn test 1754/0 failures, SpotBugs 0, PIT 337/337 (100%),
javadoc clean, reactor build green.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01AnNYn8W1xuVxVJtyL34GyH
TODO.md carried `--spec-synth-len` / `--spec-synth-rates` twice: once as a
standalone entry from the b10649 bump arguing they should never be exposed, and
once as a sub-bullet of the b10878 flag-audit entry restating a shortened version
of the same reasoning. Two records of one decision drift apart, and the shorter
one reads like a deferral ("no consumer use case here") rather than the settled
non-goal it is.
The b10649 entry is now the single record and says so; the audit entry points at
it. It also names why an audit keeps re-surfacing them: the sweep answers "is this
name reachable from the typed Java surface", which is a different question from
"should it be" -- so a future re-find is expected and is not a signal to reopen the
decision.
No behaviour, no code, no counts change.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01AnNYn8W1xuVxVJtyL34GyH
bernardladenthin
had a problem deploying
to
startgate
September 10, 2026 20:23 — with 
GitHub Actions
Error
GitHub Actions
Error
bernardladenthin
had a problem deploying
to
maven-central
September 10, 2026 20:23 — with
GitHub Actions
Failure
GitHub Actions
Failure
bernardladenthin
had a problem deploying
to
maven-central
September 10, 2026 20:23 — with
GitHub Actions
Failure
GitHub Actions
Failure
|
bernardladenthin
deleted the
claude/parameter-surface-typed-contract
branch
September 10, 2026 20:34
This was referenced Sep 10, 2026
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
2 participants
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.
Summary
args.ModelOption+args.ModelFlag,parameters.RequestField,parameters.TrainingField) declaring the contract it must satisfy, and the parameter base classes accept nothing else, so an undeclared name cannot reach the wire at all. CMake extracts the declarations at configure time and C++ tests feed them to llama.cpp's own server argument parser, its completion-request schema, and the trainer's key list — on every platform. This is the only place these can be checked: the request schema and the trainer both discard an unknown key silently, so a Java test asserting the string mapping (hasKey("--mlock")) stays green forever while the name is dead.loadModel()throws, the model does not load); a dead request key is discarded without a word, so the parameter simply stops having an effect. Six of the eleven found in the first pass were already non-functional at the kherud fork point (b4916); only--mlock/--no-mmapare recent drift. Replacements where one exists:setLoadMode(LoadMode),ModelParameters.setChatTemplate(String),--jinjaat server start.InferenceParametersbuilt the request by concatenating"key": valuestrings, so a fragment passed towithJsonSchema/withResponseFormat/withStreamOptions/withMessagesJson/withToolsJsoncould close its own object and append arbitrary keys — with duplicate keys resolving last-wins in the native parser, an injectedn_predictorgrammarsilently beat the one the builder wrote. The body is now a real JSON tree and every stored value must parse as exactly one well-formed JSON value at write time.Test plan
ctest537/537,mvn test1754 / 0 failures, SpotBugs 0, PIT 337/337 (100%), javadoc clean, full reactor build greenCHANGELOG.md,CLAUDE.md("Wire-name registries"), and a newdocs/history/parameter-wire-surface.mdrecording the archaeology, the injection reproducer and every measurementWhat the guards caught that a review would not
Each of these was found by running something, not by reading:
--grp-attn-n/-ware present incommon/arg.cppat every pinned tag butset_examples()-scoped away fromLLAMA_EXAMPLE_SERVER, so the server parser rejects them exactly like a deleted flag. A textual sweep of upstream sources is structurally blind to that; only the real option table sees it.chat_template, found by the exemption's own hole. AnOAI_LAYERkey is exempt from the schema check because the OpenAI layer consumes it first — but "absent from the schema" is satisfied just as well by a key nothing reads. The generator now also sweeps upstream for a reader shape (json_value(x, "k", …),.contains("k"),.at("k")); the shape is load-bearing, becausechat_templatedoes occur as a bare literal upstream — in the/propspayload the server emits — so a token grep would have called it live. It scored zero readers. Its one real consumer, the Android app, was applying its chat-template override per request where llama.cpp discarded it; it now sets it at model load.Deliberately breaking
No deprecation window: a method that keeps writing a name nothing reads is a trap with a warning label on it, and this is a major-version window. Verified against the downstream consumer in this workspace (srcmorph): it calls none of the deleted methods and only typed setters, so it stays source-compatible.
Related issues / PRs
Follows #426 (CLI-flag contract guard), which this generalises from one surface to three.
Checklist
CONTRIBUTING.mdandCODE_OF_CONDUCT.md5.2.0-SNAPSHOTline; it is described in full indocs/history/parameter-wire-surface.mdrather than withheld🤖 Generated with Claude Code
https://claude.ai/code/session_01AnNYn8W1xuVxVJtyL34GyH
Generated by Claude Code