test(SAMPLE-REASONING): pin the two qwen3 branches nothing caught, and correct a 2-core that was never possible (#605, #606) - #636
Merged
Conversation
…d correct a 2-core that was never possible (#605, #606) FOLLOWING_AGENTS_PROTOCOL Three follow-ups accepted when the recipe-sweep PRs merged, all evidence repair rather than behaviour: no src/ or include/ file changes. 1. #630 LOW-1 -- qwen3.cpp:55's reasoning-reopen guard was load-bearing and unpinned. Deleting it left the focused suite FULLY GREEN (7 cases / 7 passed, 157 assertions / 0 failed, Status: SUCCESS!), because all four existing tool-call assertions put <think> BEFORE the <tool_call>, where `ps > p` can never fire. Added the one input that reaches it. Mutation re-run with the assertion in place: 8/7 passed / 1 failed, 164/163 passed / 1 failed, Status: FAILURE!, and the single failure is `CHECK( p->is_reasoning_end("<tool_call>x<think>y") == false )` -- values `true == false`. Head is the correct side: upstream walks the token ids backwards and tests the reasoning-start id at every index BEFORE the tool-call id (qwen3.py:263-268), so it meets <think> first and returns False. 2. #630 LOW-2 -- the seed_oss refactor (now `class SeedOssParser(Qwen3Parser)` exactly as upstream) changed extract_reasoning on the thinking=false arm, and nothing pinned it. Judged worth a test: the change is invisible on the production path -- OpenAIServingChat::MakeParserEngine (serving_chat.cpp:580) takes the header default thinking=true, under which a bare engine and Qwen3Parser are byte-identical -- so the thinking-OFF arm is the ONLY observer there is, and an unpinned invariant that no production path exercises is exactly the one a later refactor silently drops. Driven through get_parser_engine("seed_oss") so it pins the routing too. Mutation: route seed_oss back to the pre-#630 bare ParserEngine and the new case reds alone (8/7 passed / 1 failed, 164/163 / 1 failed, Status: FAILURE!) on `CHECK( content == Opt(output) )`. Recorded as coverage BEYOND upstream: `test_qwen3_reasoning_parser.py` has NO qwen3 is_reasoning_end case at all (the only one in the suite is the base family's, test_base_thinking_reasoning_parser.py:111) and its THINKING_DISABLED_CASES run with tool parsing suppressed, so both overrides sit on branches upstream's own fixtures cannot reach. Written from scratch against the upstream SOURCE, disclosed in the test header and in the spec's Tests-to-port section with the mutation that proves each case. Filed there rather than porting-inventory §9: §9 is the register for IMPLEMENTATION deviations forced by the no-Python constraint, and the precedent for beyond-upstream TEST coverage is the row spec (kv-persistence-lmcache.md:537, kv-events.md:196). 3. #624 LOW-1 -- serve-recipe-args.md said "2-core CI runner" twice for a run that a 2-core box cannot produce: the case returns early at test_cpu_threadpool.cpp:501 when `cores < 4`, and `fits = cores / 2` (:512) reporting 2 threads with `over = cores + 1` (:513) reporting 5 pins it at exactly 4 -- which is what ubuntu-latest gives a public repo. Corrected both lines (:278 prose, :284 table) and added the derivation so the number carries its own proof next time. #631 was already retitled 4-core upstream of this and is untouched; this only makes the spec agree with it. The three test anchors were re-verified in the tree, not trusted from the finding (#632). Verification, all observed in this worktree at 983b4aa + this diff: focused RED (guard mutated, new assertion present) exit 1, Status: FAILURE! focused GREEN (tree restored byte-for-byte) exit 0, 8/8, 164/164 full serial ctest exit 0, 408/408, 0 failed scripts/agent-preflight.sh --staged exit 0, all gates green python3 scripts/check-windows-portability.py exit 0 Both mutated sources restored byte-for-byte and re-verified by md5 and an empty `git diff -- src/`. test_cpu_threadpool is green here (20-core box); its CI red is #631 and unrelated. Following-Agents-Protocol: true AI-Assisted: true Assisted-by: AGENT:claude-opus-5 [Claude Code]
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
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.
Closes the three LOW findings accepted as follow-up when the recipe-sweep PRs
merged today. Issues: #605
(findings 1 and 2, the
SAMPLE-REASONINGW3 brick reviewed in #630) and#606 (finding 3, the
serve-recipe-argsspec reviewed in #624). Nosrc/,include/or behaviourchange — this is a test and a record.
1. #630 LOW-1 — a load-bearing branch with no test
src/vllm/parser/qwen3.cpp:55's reasoning-reopen guard (if (ps != npos && ps > p) return false;)was correct but unpinned. Reproduced on unmodified
mainfirst: deleting it leftthe focused suite fully green — 7 cases / 7 passed, 157 assertions / 0 failed,
Status: SUCCESS!, exit 0. All four existing tool-call assertions put<think>BEFORE the
<tool_call>, sops > pnever fires.Added the one input that reaches it. With the assertion present and the guard
still deleted:
One failure, and it is the new assertion. (The doctest output above is verbatim
from the mutation run, so its line numbers are the ones the file had at that
moment; in the committed file the assertion is
test_qwen3.cpp:288and theseed_oss case below runs from
:303, failing at:317.) Head is the correct side: upstream'sbackward token walk tests the reasoning-start id at every index before the
tool-call id (
vllm/parser/qwen3.py:263-268), so it meets<think>first andreturns
False.2. #630 LOW-2 — the seed_oss thinking-off arm
Judged worth pinning rather than skipping. The seed_oss refactor is a fidelity
fix (
class SeedOssParser(Qwen3Parser),seed_oss.py:24), and the reviewerestablished it is invisible in production:
OpenAIServingChat::MakeParserEngine(
serving_chat.cpp:580) takes the header defaultthinking=true, under which abare engine and
Qwen3Parserare byte-identical. That is precisely the argumentfor the test — the thinking-off arm is the only observer the invariant has, so
an unpinned inheritance that no production path exercises is exactly the kind a
later refactor drops silently. Driven through
get_parser_engine("seed_oss")soit pins the routing as well as the behaviour.
Mutation — route seed_oss back to the pre-#630 bare
ParserEngine:Coverage beyond upstream, recorded
test_qwen3_reasoning_parser.pyhas no qwen3is_reasoning_endcase at all(the only one in the suite is the base family's,
test_base_thinking_reasoning_parser.py:111), and itsTHINKING_DISABLED_CASESrun with tool parsing suppressed. Both
Qwen3Parseroverrides therefore sit onbranches upstream's own fixtures cannot reach. The three affected TEST_CASEs are
written from scratch against the upstream source and disclosed in the test header
plus a new subsection of
.agents/specs/reasoning-parsers.md§ Tests to port,which tabulates each case with the mutation that proves it.
Filed in the row spec rather than
porting-inventory.md§9: §9 is the registerfor implementation deviations forced by the no-Python constraint, and the
precedent for beyond-upstream test coverage is the row spec
(
kv-persistence-lmcache.md:537,kv-events.md:196). Flagging the deviationfrom the finding's wording explicitly rather than silently.
3. #624 LOW-1 — a wrong fact on main
.agents/specs/serve-recipe-args.md:278and:284said "2-core CI runner" for arun a 2-core box cannot produce. Anchors re-verified in the tree, not trusted
from the finding (that is #632):
tests/vt/test_cpu_threadpool.cpp:501—if (cores < 4) { … return; }, so abox that produced a ratio has at least 4.
:512-513—fits = cores / 2,over = cores + 1; the recorded run reports2 and 5 threads, pinning
cores == 4.Both lines corrected, and the derivation added so the number now carries its own
proof. #631 is untouched — it was already retitled 4-core upstream of this;
this only makes the spec agree with it.
Gates
All observed in this worktree at
983b4aa4c+ this diff:Status: FAILURE!, 1 failure, the new assertionStatus: FAILURE!, 1 failure, the new casectest --test-dir buildscripts/agent-preflight.sh --stagedpython3 scripts/check-windows-portability.pypython3 scripts/check-agent-record.pyBoth mutated sources were restored byte-for-byte and re-verified by md5 and an
empty
git diff -- src/. Every build's exit code was checked before its test run,and each mutation was rebuilt rather than re-running a stale binary — the first
guard-deletion attempt in fact failed
-Werroron an unusedps, which adiscarded build log would have hidden.
test_cpu_threadpoolis green here (20-core box); its CI red is the pre-existing#631, and
windows-msvc-*red is the pre-existing #584. Neither is from this diff.🤖 Generated with Claude Code