fix(dogfood): batch 2 — five branches that conflicted with batch 1, plus /v1/explain - #2451
Open
noahgift wants to merge 3 commits into
Open
fix(dogfood): batch 2 — five branches that conflicted with batch 1, plus /v1/explain#2451noahgift wants to merge 3 commits into
noahgift wants to merge 3 commits into
Conversation
…1, plus /v1/explain The five branches deferred from #2449 because they overlapped work already in that batch, now resolved against a main that contains it. Plus aprender#2375's worst finding, the last uncovered cluster of the audit. Rebased by re-applying batch 2's net diff onto the squashed main rather than merging: #2449 landed as a single squash commit, so batch 2's base no longer shares commit identity with main and a merge produces conflicts that are artefacts of the squash, not real overlaps. ## Where taking one side wholesale would have shipped a defect **subprocess.rs** — both branches touched the same two lines. HEAD binds the spawn to `apr_binary()`, the RUNNING executable; the incoming branch still had `Command::new("apr")` resolved through $PATH — the P0 this audit filed and #2424 fixed, where a bare `apr` ran a 26-day-old 0.60.0 while `apr.version` reported 0.63.0. The incoming branch fixes the OTHER half: HEAD discarded a failing command's stdout whenever stderr was non-empty. Kept both. **/ready** — batch 1 lists it in NATIVE_ROUTES and its comment says it is mounted, but the actual `.route("/ready", ..)` lived on the incoming branch. Taking batch 1 wholesale left a route the index advertises and the router 404s. Mutation-confirmed: the 404 body lists `GET /ready` among available routes while refusing it. **/metrics** — the mirror image. The incoming branch registers them unconditionally; batch 1 registers them inside `if config.metrics`, which is what makes `--no-metrics` withhold telemetry rather than hide a banner. Taking the incoming side would have double-registered and undone it. **apr trace --reference** — the two branches fix #2407 in OPPOSITE directions: refuse the argument in the MCP wrapper, or pass it through. Confirming the clap arg exists said "pass it through" and was the wrong check — the flag was declared and its implementation was a stub printing `{"comparison": "reference comparison not yet ..."}` at exit 0. What settles it is that the same branch also fixes the CLI to return `NotImplemented`, so pass-through becomes correct: one source of truth, no wrapper allowlist to drift. **imatrix leakage** — the duplicate-`falsify_id` fix was incomplete on main: `leakage_unreadable` still stamped the IMPROVEMENT gate's `-001` in both its message and its GateReport. The const existed; one path had not been switched. ## /v1/explain (#2375 finding 2) Returned HTTP 200 with SHAP values derived from the feature INDEX (`0.1 - i * 0.02`) and a literal `prediction: 0.95`, with `State` bound as `_state` so the answer was identical whether a model was loaded or not. Kernel SHAP needs a background dataset `ExplainRequest` does not carry, so it now fails and names what is missing — the rule the CLI adopted in #2407. The load-bearing falsifier is black-box: `[1.0, 2.0, 3.0]` and `[-40.0, 0.001, 999.0]` returned byte-identical explanations. ## Tests that were locking defects in - `lint_exit_convention_tests` invoked `ollama_tools_lint::run(p, None, ..)`; `--request-file` is required in non-streaming mode, so that member failed the argument guard (exit 5) BEFORE reaching the parse whose exit-4 convention the test exists to assert. - `malformed_response_errors` asserted `CliError::InvalidFormat`, whose Display hardcodes "Invalid APR format" — sending a user hunting for a corrupt model when a captured JSON response failed to parse. - The MCP stdio falsifiers ran whatever binary sat in the shared target dir; all six failed against `apr 0.63.0 (d16c608)` from a worktree at 11f958f, showing the exact pre-fix symptom. `apr_binary()` now always builds. apr-cli 6978 · aprender-mcp 120 · aprender-serve api 1781+4. Closes #2403, #2417, #2418, #2404, #2407, #2419. Refs #2391, #2377, #2376, #2394, #2375, #2373. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Two artefacts of rebasing batch 2 by net diff, both caught by the integration
chain rather than by review.
**README reverted a fix that had just landed.** Batch 2 branched from batch 1
BEFORE the `1767 -> 1768` contract-count fix was pushed to it, so
`git diff batch1..batch2` contained that line going 1768 -> 1767 — a reversion,
not a change. Applying it to a main that already had 1768 silently undid it, and
`FALSIFY-README-007` failed exactly as it did the first time:
FALSIFY-README-007: README lacks `**1768** provable contracts`
Generalisable: **a net diff taken against a base that has since MOVED encodes a
revert of everything added to that base after the branch point.** The direction
of the hunk (`-1768 / +1767`) is the tell.
**.pv/ is generated.** contracts.idx, contracts.idx.mtime and lint-previous.json
are caches; they rode along in the diff and add nothing but churn. Restored to
main's copies.
readme_contract: 11 passed.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
noahgift
enabled auto-merge
August 11, 2026 16:43
…he fixed one
`ci / lint` failed on two dead-code errors. The first was not merely unused code
— it was a lost fix.
`subprocess.rs` ended the merge with BOTH renderers: `display_cmd` (OsStr, from
the transport branch) and `display_command` (str, from the args branch). Every
call site resolved to `display_cmd`, which does
format!("{} {}", program.to_string_lossy(), args.join(" "))
That is exactly the #2403 defect `quote_arg` was written to remove: the echoed
command reads `--prompt What is 2+2?`, which is a DIFFERENT command from the one
that ran, so a user copying it out of an error message reproduces something else.
`quote_arg` survived, `display_command` (its only caller) did not get used, and
clippy reported the symptom — an orphaned helper — rather than the cause.
`display_cmd` now quotes via `quote_arg`, keeping the OsStr signature every call
site needs (they all pass `apr_binary()`). `display_command` is folded into it
rather than kept as a second implementation of the same thing, and the test that
pinned the quoting now pins it on the surviving function.
Second error: `REFERENCE_UNSUPPORTED` in trace.rs is genuinely dead. It belongs
to the "refuse `reference` in the MCP wrapper" approach that was deliberately NOT
taken — the CLI now returns `NotImplemented` for `apr trace --reference`, so the
wrapper passes the flag through and lets the CLI answer. Removed.
Worth recording: `cargo check` and the test suites were green on this branch;
only `cargo clippy -- -D warnings` caught it, and I had not run clippy on
aprender-mcp before pushing. Dead-code warnings after a conflict resolution are
worth reading as "which of two implementations did I orphan, and was it the
fixed one?"
aprender-mcp: 120 passed. clippy clean on aprender-mcp, apr-cli, aprender-serve.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
github-merge-queue
Bot
removed this pull request from the merge queue due to failed status checks
Aug 11, 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
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.
The five branches deferred from #2449 because they overlapped work in that batch, now resolved against a
mainthat contains it. Plus aprender#2375's worst finding — the last uncovered cluster of the audit.Where taking either side wholesale would have shipped a defect
subprocess.rs— both branches touched the same two lines, and each held a fix the other regressed:apr_binary(), the running executable. The incoming branch still hadCommand::new("apr")resolved through$PATH— the P0 this audit filed and fix(mcp,code): apr 0.63.0 ran apr 0.60.0 — both subprocess backends resolved a bare apr through PATH #2424 fixed, where a bareaprran a 26-day-old 0.60.0 whileapr.versionreported 0.63.0./ready— batch 1 lists it inNATIVE_ROUTESand its comment says it's mounted, but the actual.route("/ready", ..)lived on the incoming branch. Taking batch 1 wholesale left a route the index advertises and the router 404s. Mutation-confirmed — the 404 body listsGET /readyamong available routes while refusing it./metrics— the mirror image. The incoming branch registers them unconditionally; batch 1 registers them insideif config.metrics, which is what makes--no-metricswithhold telemetry rather than hide a banner line.apr trace --reference— the branches fix #2407 in opposite directions. Confirming the clap arg exists said "pass it through" and was the wrong check: the flag was declared and its implementation was a stub printing{"comparison": "reference comparison not yet ..."}at exit 0. What settles it is that the same branch also fixes the CLI to returnNotImplemented— with the CLI failing honestly, pass-through is right (one source of truth, no wrapper allowlist to drift).imatrix leakage — the duplicate-
falsify_idfix was incomplete on main:leakage_unreadablestill stamped the improvement gate's-001. The const existed; one path hadn't been switched./v1/explain (#2375 finding 2)
Returned HTTP 200 with SHAP values derived from the feature index (
0.1 - i * 0.02) and a literalprediction: 0.95, withStatebound as_state— so the answer was identical whether a model was loaded or not. Kernel SHAP needs a background datasetExplainRequestdoesn't carry, so it now fails and names what's missing.The load-bearing falsifier is black-box:
[1.0, 2.0, 3.0]and[-40.0, 0.001, 999.0]returned byte-identical explanations.Tests that were locking defects in
lint_exit_convention_testsinvokedollama_tools_lint::run(p, None, ..);--request-fileis required in non-streaming mode, so that member failed the argument guard (exit 5) before reaching the parse whose exit-4 convention the test exists to assert.malformed_response_errorsassertedCliError::InvalidFormat, whose Display hardcodes "Invalid APR format" — sending a user hunting for a corrupt model when a captured JSON response failed to parse.apr 0.63.0 (d16c608b1)from a worktree at11f958f25, showing the exact pre-fix symptom.apr_binary()now always builds.Verification
TOTAL_FAILS=0The only lib failures are in
aprender-gpu, whichworkspace-testexcludes (ci.yml:231) — and that crate produced a different failing set on each of four runs, which is #2450, not this branch.Rebased by re-applying batch 2's net diff onto the squashed main: #2449 landed as one squash commit, so a merge produces conflicts that are artifacts of the squash. That has one trap, caught by the integration chain — the diff reverted the
1767 → 1768README fix that landed on batch 1 after batch 2 branched off it.Closes #2403, #2417, #2418, #2404, #2407, #2419.
Refs #2391, #2377, #2376, #2394, #2375, #2373.
🤖 Generated with Claude Code