Close the 1.2.0 audit findings: flashAttn fail-fast, knob sweep, plugin IT, release-asset guards - #194
Merged
Merged
Conversation
Reproduced against the shipped fat jar, not inferred: enabling flashAttn makes every model load fail with error: unknown value for --flash-attn: '--reasoning-format' LlamaException: Failed to parse model parameters Upstream's -fa takes a mandatory [on|off|auto], but the binding models it as a bare flag -- enableFlashAttn() calls setFlag, which stores null, and the argv renderer emits the key with no token after it. llama.cpp then consumes the NEXT argv token as the value, so the error names a flag the user never set and nothing loads. There is no degraded mode. srcmorph cannot spell it correctly through the binding's public API: setFlag unconditionally stores null and putScalar is protected. So until a binding release exposes a value-taking setter, refusing the knob with a message naming the cause beats letting the user decode a parse error about --reasoning-format. The guard sits in the plan phase, next to the model-path and GGUF checks, so a multi-model run fails before its first group generates rather than after. A second guard in the provider covers callers that build a config directly without going through an engine. Also corrects the contextSize warning this cycle added. It offered RoPE scaling as the escape hatch; srcmorph exposes no knob for it. What llama.cpp actually does is cap the slot to the model's trained context, so a file the plan calls "fits" can still die mid-run -- which is what the warning now says, with lowering contextSize as the only real fix. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AnNYn8W1xuVxVJtyL34GyH
Three markers, because the thing that makes this dangerous is that it will not announce itself: bumping llama.version alone leaves both guards throwing, the knob still looking broken, and every gate still green. So it has to be written down where someone will actually trip over it. - LlamaCppJniAiGenerationProvider.model(), at the throw: what to replace it with, and the four things to delete along with it. - EngineSupport.validateFlashAttnIsNotRequested: a pointer back, so the two guards cannot be removed one at a time. - TODO.md: the full mechanism, the upstream fix that has to land first, and the reminder to set the knob against a real model once it does -- so far only the refusal has ever been exercised. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AnNYn8W1xuVxVJtyL34GyH
…ng one Two gaps that share a cause -- both are invisible while everything is loaded from the classpath, which is how this reactor builds and tests. module-info omitted `requires net.ladenthin.llama`. The old rationale in its javadoc was that the isolated module-info-compile execution does not need the module name -- true of compiling that file, and irrelevant to running the result. On the module path every llama.cpp-backed path, including the plan-time GgufModelInspector, failed for want of a readable module. Deliberately not transitive: the binding's types appear only in package-private members, never in an exported signature, so no consumer needs it transitively -- with a note saying what would change that. The model-backed tests' capability check skipped unconditionally. That is right on a developer machine with an unsupported OS/arch and wrong in CI, where a vanished native library or an unchecked-out model would quietly restore a green build with zero real inference -- the exact shape that let a dead provider ship through two releases. CI now sets requireNativeLlama, which turns those skips into failures naming what is missing. Verified in both directions rather than assumed: with the model hidden, the plain run reports Skipped: 1 and BUILD SUCCESS, the property run reports BUILD FAILURE and names the file. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AnNYn8W1xuVxVJtyL34GyH
… and runbook Almost none of these were "not a ship blocker" because they were wrong about being wrong -- three adversarial verifiers kept confirming the facts and refuting only the severity. With the bar moved to "fix everything before the tag", they come back. Release notes stated things the tree contradicts: jackson pinned at 2.22.2 while the entry says 2.22.1; a reactor figure of "30 + 17 tests" matching no module (the core alone runs 601); and a paragraph calling a builder "the obvious next step" inside the very release that ships it, two sections above the entry announcing it. Two public constructors quietly gained a sixth parameter (cachedPromptTokens) and a mojo @parameter was removed -- Maven fails a consumer POM that still declares one, and the CLI fails at parse time, so both are now written down as breaking. Three sampling javadocs justified their values with upstream behaviour that does not exist. llama.cpp's defaults are top_p 0.95, min_p 0.05 and n_batch 2048 (common.h) -- so 0.9 does not "match the library default", 0.0 does not "preserve existing behaviour" (it overrides min-p), and 0 is not "decide for me". The values are deliberate srcmorph choices and stay; only the reasoning is corrected. DEFAULT_FLASH_ATTN was wrong on both halves: false emits nothing, and what that leaves in force is llama.cpp's auto, not off. README claimed snapshots publish on every push to main; they publish only from a dispatch with publish_to_central. RELEASE.md verified three artifacts of four -- the parent POM is the easy one to forget and the one whose absence breaks resolution of the other three -- and its pointer to the workspace runbook was one directory level too shallow to resolve. The PIT comment justified an exclusion by citing a sibling comment that no longer exists, whose claim (mojos are not gated) the plugin module now contradicts by gating all five. 603 tests, 0 failures, 0 skipped. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AnNYn8W1xuVxVJtyL34GyH
Keep a Changelog dates the release, not the edit, and the tag is now expected to be cut on the 1st. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AnNYn8W1xuVxVJtyL34GyH
…ed URLs Three release-path defects, one of which I first fixed the wrong way round. Nothing asserted that every attached jar carries its detached .asc: the collection steps skip a missing signature with `[ -e "$f" ] || continue`, so a failed signing step yields an attach that looks complete. My first attempt verified before the upload and refused to attach -- which breaks the reason both attach jobs run on publish failure at all: when Central is unreachable the GitHub assets are the only way to reach the artifacts, and withholding them over a missing signature is strictly worse than shipping one unsigned. Correct order is report, upload, assert: annotations first (never non-zero), upload unconditionally, job fails afterwards. Verified structurally -- the job `if:` is untouched, neither the report nor the upload step carries a condition. url and scm were inherited with each module's artifactId appended, so every published module POM pointed at .../srcmorph/srcmorph. The parent's child.*.inherit.append.path flags fix scm (confirmed in the effective POM) but Maven 3.9.11 silently drops the one on the project URL, so that value is now declared explicitly per module, where the effective POM can prove it: all three resolve to the repository root. The SBOM submission action sits in the report job, which the release path needs, and says nothing about artifact correctness -- continue-on-error so a third-party outage cannot block a publish. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AnNYn8W1xuVxVJtyL34GyH
The same two steps now exist in java-llama.cpp, BitcoinAddressFinder and streambuffer, byte-identical. Name the policy file they are documented in and align the release job's copy with the snapshot job's wording so a diff across the four repos stays empty.
…model A knob is not exercised by being set -- it is exercised by the native binding accepting the value the provider derives from it, and those two came apart twice. dryPenaltyLastN was forwarded at its own -1 default, which the binding rejects outright, so every generation threw before producing a token; flashAttn maps onto a bare-flag setter that cannot express the on|off|auto value llama.cpp has required since b10273. Neither is visible to a mapping unit test (the mapping did exactly what it was written to do) and neither is visible to the rest of the suite, which runs on the mock provider and never loads the native library. So: one case per knob, that knob at a non-default value, a real generation, assert output came back. Per-knob rather than all-at-once because an all-at-once config names no culprit and one rejected value masks every knob behind it. 36 cases in ~35 s -- the committed model is small and the output budget is eight tokens; the assertion is "the binding accepted this", not "the 135M model said something sensible". everyKnob_isSweptOrExplicitlyExcluded() reflects over LlamaCppJniConfig and fails when a knob is neither swept nor listed with a reason, in both directions, plus a count check so an empty list cannot satisfy it. That is the part a hand-maintained list cannot promise: a knob added later reds this class until somebody decides how it is covered. Three knobs are excluded with reasons: modelPath (identifies the run), devices (backend device names exist only on the machine that enumerates them), and flashAttn (currently refused, pinned by its own test instead). Verified by falsification, not by reasoning: a deliberately invalid value reds exactly the case that carries it and names the knob in the failure.
The publish jobs build seventeen fat jars -- one per net.ladenthin:llama native classifier plus the default -- sign them and attach them. Exactly one of the seventeen was ever checked: smoke-fatjar launches the default jar. The sixteen GPU jars could not be covered the same way and were shipped on faith. They still cannot be launched meaningfully: a GitHub-hosted runner has no CUDA/ROCm/SYCL/OpenVINO device, and the only command that would load the native library is a real generation -- `Plan` with the mock provider, the one thing that does run there, never touches it. A launch that asserts nothing about the classifier is worse than no launch, because it reads as covered. What can be asserted without a device is that each jar is the artifact it claims to be: exactly one jar per requested classifier and no unexpected one, at least one native library in each, the native for the OS/arch the name promises, a default jar spanning more than one OS, and -- the load-bearing check -- a native set that differs from the default jar's. If -Dllama.classifier= ever stops being wired through (a renamed property, a pom refactor), Maven resolves the default artifact and the loop ships sixteen copies of the CPU build under GPU names; nothing else in this pipeline would notice. A classifier shape nobody has mapped fails the script rather than passing unchecked. Verified locally against a real opencl-android-aarch64 jar plus the default, and by falsification on all five failure paths: identical-to-default, wrong OS/arch for the name, unknown classifier shape, an unexpected classifier jar present, and a requested classifier missing.
Adds the three CHANGELOG entries for what landed (knob sweep, classifier fat-jar verification, unsigned-asset guard), refreshes the core module's test count to the measured 639 (639 / 39 / 32 across the reactor, 0 skipped, BUILD SUCCESS), and describes the sweep in CLAUDE.md so the next person knows a new knob has to be swept or excluded. TODO.md gains the two items deliberately left out of 1.2.0: a maven-invoker-plugin IT (no CI job runs the plugin through a real Maven lifecycle today, so plexus XML binding, goal-prefix resolution and the srcmorph.* property names are untested), and the standing limit that the sixteen GPU classifier jars are verified structurally but never launched -- closing that needs hardware, not another script. The flashAttn entry now also names the sweep case that has to be un-excluded when llama 5.2.0 lands.
srcmorph-maven-plugin had no test that involved Maven. PluginArchitectureTest, MojoPhaseSkipTest, MojoConfigurationMappingTest and PIT at 62/62 all call Java methods directly, which means everything only Maven does was unverified: plexus binding of the <configuration> XML onto the @parameter fields, goal-prefix resolution, the srcmorph.* property strings, lifecycle-phase binding, per-execution configuration overriding the plugin-level block, and the descriptor maven-plugin-plugin generates. The CLI's Jackson binding is a different code path, so ExamplesConfigBindingTest does not cover it either. A renamed @parameter property would have shipped green. The fixture in .github/plugin-it/ consumes the plugin from the local repository through a real lifecycle, with the default mock provider -- no GGUF, no GPU, no network. Its <configuration> mirrors the worked example in the module README, so it also fails when the documented XML stops being the XML that works. Five checks: the full lifecycle with all three goals bound to phases and configured entirely from the pom (including the nested <condition><extensions> routing tree); the generated descriptor read out of the packaged jar (prefix, four goal names, the property expressions the other checks drive); the goal prefix plus -Dsrcmorph.planOnly=true, asserting the plan printed, the .java rule matched by id, and nothing written; -Dsrcmorph.aiVersion=9.9.9 reaching the written header; and -Dsrcmorph.skip=true writing nothing. Two things worth recording, both found by running rather than by reasoning. An explicit value in the fixture's <configuration> beats a -D property, so the property check drives a parameter the pom leaves unset -- asserting an override of <outputDirectory> would have been testing a misunderstanding of Maven. And Maven's maven-metadata-local.xml accumulates goal prefixes, so on a machine that once installed this plugin under a different prefix the short form keeps resolving; that is why the descriptor is asserted directly rather than trusting the invocation alone. The job needs only startgate (it compiles what it needs and consumes no artifact), installs -pl srcmorph-maven-plugin -am -- provably parent + core + plugin, never srcmorph-cli and its ~80 MB assembly -- and gates both publish jobs, like smoke-fatjar. Falsified: renaming srcmorph.planOnly reds the test with a message naming it.
.claude/worktrees/<run-id>/ is a full git worktree of this repository, created inside it while a task runs in an isolated copy and removed when that run ends. While it exists, a plain `git add -A` would stage a second copy of the entire tree. Only worktrees/ is ignored; anything else under .claude/ stays visible.
Four adversaries attacked the integration test added in the previous commit,
one of them by mutating the plugin and re-running. Three of the surfaces the
test named were not actually covered, and one fixture file was working against
it. All findings were reproduced here before fixing, and every fix falsified
after.
settings.xml is deleted. Its stated reason for existing was false: `mvn
srcmorph:generate` resolves without a <pluginGroups> entry, because Maven
matches the prefix against the descriptor of the plugin the fixture declares --
which is exactly the check that was wanted. Adding the pluginGroup opened a
second resolution path through the group's repository metadata, which maps the
prefix by artifactId no matter what the descriptor says. Measured: with a
mutated <goalPrefix> the run succeeded with the file and failed without it, and
not as a stale-local-repo artifact -- Central serves that metadata, so a
pristine runner was masked too. Verified after removal: a mutated prefix now
fails the goal-prefix run for real, so it is a genuine second guard beside the
descriptor assertion instead of decoration.
Three configured elements were inert, each for its own reason. <subtrees> was
set to the engine's own fallback value, so a bound and an unbound element looked
identical; <excludes> matched a file the fixture did not contain; the three
execution-level <configuration> blocks changed nothing the assertions looked at.
Deleting any of them left the test green. The fixture now carries a second
source root reachable only through the second <subtree>, a package-info.java the
exclude pattern actually removes, and an <aiVersion> set on one execution only --
each pinned, each verified to red the test when the element is dropped.
The descriptor check pinned 3 of 19 property strings, leaving the headline
failure mode open for the other 16. It is now a two-way diff of the whole set:
a rename fails, and a new property fails until somebody lists it deliberately.
Verified by renaming srcmorph.force, which no run drives.
Also: the calibrate goal is exercised (it was the only goal whose execute() the
test never ran, and it costs a sub-second run); the four skip properties are
checked including an off-diagonal case, since a copy-pasted property among the
three per-phase flags passes every diagonal; both readonly ${project.*}
injections are asserted; "nothing was written" now also checks the mojos' own
default output directory, so it cannot pass while output landed elsewhere; the
resolved plugin is compared against the one just built, so the descriptor
assertions cannot describe an artifact nothing executes; the precondition check
moved above the first assertion; the reactor version comes from help:evaluate
like everywhere else in the workflow; the job records mvn -v and uploads the
generated tree alongside the logs; and the orphaned fat-jar comment block sits
above its own job again.
One finding is left for a human: "Plugin integration test" has to be added to
main's required status checks, or a PR renaming a @parameter property can still
merge with this job red. Releases are already blocked either way.
bernardladenthin
had a problem deploying
to
maven-central
September 1, 2026 06:56 — with 
GitHub Actions
Failure
GitHub Actions
Failure
bernardladenthin
had a problem deploying
to
startgate
September 1, 2026 06:56 — with
GitHub Actions
Error
GitHub Actions
Error
bernardladenthin
had a problem deploying
to
maven-central
September 1, 2026 06:56 — with
GitHub Actions
Failure
GitHub Actions
Failure
Code Review: PR #194 SummaryExcellent work on closing the 1.2.0 audit findings. The implementation demonstrates systematic rigor: Strengths:
Testing: 34 knobs swept, plugin IT with full lifecycle verification, classifier jars verified structurally, module descriptor checked bidirectionally. Assessment: No blockers. Commit messages exemplary. This closes real gaps that masked a non-functional provider through two releases. |
This was referenced Sep 1, 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
flashAttncannot be expressed by the current binding (-fatakes a mandatoryon|off|auto,enableFlashAttn()storesnull, and the renderer then eats the next argv token), so it is refused at plan time with a message naming cause and way out instead of dying on an unrelated flag. The reason nobody noticed is that a knob is only exercised when the binding accepts the value derived from it — soLlamaCppJniKnobSweepTestnow drives all 34 sweepable knobs at non-default values through a real generation, one case each, with a reflective completeness check that fails when a knob is neither swept nor excluded with a reason.srcmorph-maven-pluginis now run as a plugin in CI. Every existing check calls Java methods directly, so plexus XML binding, goal-prefix resolution, thesrcmorph.*property strings, lifecycle binding and the generated descriptor were all unverified — a renamed@Parameterproperty would have shipped green. Newplugin-itjob + fixture project, six checks,mockprovider (no GGUF/GPU/network).-Dllama.classifier=would otherwise ship 16 CPU builds under GPU names), and an unsigned asset now reds the release after the upload, never instead of it.Test plan
mvn test: 639 / 39 / 32, 0 failures, 0 skippedbash .github/plugin-it.sh 1.2.0— 6/6 checks passmvn -P release package -DskipTests -Dgpg.skip=true— BUILD SUCCESS, javadoc + sources jars for all three modules (the release-time gatemvn testdoes not cover)publish.ymltriggers onpushonly formain/tags, so this PR is what first validates the branchCHANGELOG.md1.2.0 (30 entries),CLAUDE.md,TODO.mdEvery assertion added here was falsified, not assumed. An adversarial pass over the plugin IT then rewrote three of them:
srcmorph.planOnlysrcmorph.force(driven by no run)<goalPrefix>No plugin found for prefix 'srcmorph'<subtree>src/extra/java<excludes>package-info.java<aiVersion>Related issues / PRs
Refs the 1.2.0 audit cycle. The
flashAttnwiring depends on a value-taking setter innet.ladenthin:llama5.2.0 — tracked inTODO.md, deliberately not a blocker here.Checklist
CONTRIBUTING.mdandCODE_OF_CONDUCT.mdNotes for the reviewer
Two Maven behaviours are load-bearing here and are documented in the code, because both cost a wrong assertion first:
<configuration>value beats a-Dproperty. The fixture sets<outputDirectory>, so a property check on it asserts nothing; the property check drives a parameter the pom leaves unset instead.settings.xmlwith a<pluginGroups>entry masks a changed goal prefix completely.mvn srcmorph:generateresolves without one (Maven matches the prefix against the declared plugin's descriptor); adding one opens a second path through repository metadata that maps by artifactId. Measured: with a mutated prefix the run succeeded with the file and failed without it — and not as a local-repo artifact, since Central serves that metadata. The file was deleted.One thing this PR cannot do for you: add
Plugin integration testtomain's required status checks. Without it a PR renaming a@Parameterproperty can still merge with the job red (releases are blocked either way).Generated by Claude Code