Repository navigation
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configuration
You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
| } | ||
| match *self.bytes.get(self.at)? { | ||
| b'\'' | b'"' => self.string(), | ||
| b'[' => self.sequence(b'[', b']'), |
There was a problem hiding this comment.
🔴 Important: value → sequence/dict → value recurses with no depth limit, and this reader only runs on text serde_json has already turned down. serde_json stops at depth 128, so very deep nesting is exactly what gets past reads_back_as_an_argument(trimmed) and lands here. A parameter value of tens of thousands of [ (or {'a': repeated), balanced or not, recurses once per bracket. That overflows the stack, which aborts the whole process. It isn't a panic that can be caught.
The text is model output, and a user can steer it, so one request can take the router down. It isn't limited to Seed-OSS either. inferred serves every tagged table: an undeclared or non-string parameter in Qwen3-Coder or Qwen 3.5, and DSML's string="false" values via json(.., None) in dsml.rs:448.
A depth counter in Literal would fix it. Return None past a limit (128 matches serde_json, and the later reads_back_as_an_argument(&json) check refuses anything deeper anyway):
struct Literal<'a> {
// ...
depth: usize,
}
fn sequence(&mut self, open: u8, close: u8) -> Option<()> {
self.depth += 1;
if self.depth > 128 { return None; }
// ...
self.depth -= 1;
Some(())
}Do the same in dict. Add a test with something like "[".repeat(100_000) so it stays covered.
| while self | ||
| .bytes | ||
| .get(self.at) | ||
| .is_some_and(|b| b.is_ascii_alphanumeric() || *b == b'.' || *b == b'_') |
There was a problem hiding this comment.
🟡 Nit: the scan accepts alphanumerics, . and _ but not a sign after the exponent. Python's repr writes very small and very large floats with one: repr(1e-7) == '1e-07', repr(1e20) == '1e+20'. So {'tol': 1e-07} scans 1e, which serde rejects, and the whole dict falls back to being a string. That's the bug this rule exists to fix, just for an object holding such a float. A top-level 1e-07 is fine because the JSON check takes it first. Allowing +/- right after e/E would fix it, plus a test case like "{'x': 1e-07, 'y': 1e+20}".
| value.push(char::from_u32(code)?); | ||
| self.at += digits; | ||
| } | ||
| other => value.push(other), |
There was a problem hiding this comment.
🟡 Nit: Python keeps the backslash on an escape it doesn't recognise: '\d+' is the three characters \d+, and 'C:\path' keeps its \. This arm pushes only the character, so a model writing {'pattern': '\d+'} gets {"pattern": "d+"}, and the regex is silently corrupted. \a, \b, \f and \v likewise become letters here instead of BEL, BS, FF and VT. repr never writes any of these, but the text is the model's. Matching Python means pushing '\\' then other for anything outside \\ \' \" a b f n r t v x u U and newline (plus octal, if you want \0 to match Python's \012 too).
8dd35d0 to
d9aa08c
Compare
b8bf967 to
4858370
Compare
| .transition("calls", "call_close", "content") | ||
| .transition("calls", "call_open", "calls") | ||
| .calls(CallSyntax::Tagged) | ||
| .opens_turn("<|im_start|>assistant") |
There was a problem hiding this comment.
🔴 Important: <|im_start|>assistant is the ChatML opener, and Seed-OSS doesn't use ChatML. Its template opens every turn with <seed:bos>{role}\n and closes it with <seed:eos>, so the generation prompt ends <seed:bos>assistant\n. prompt.rfind("<|im_start|>assistant") never matches, so Engine::seed falls back to replaying the whole prompt. That brings back, for this table, the bug b7006ae fixed.
Concretely: a user message or tool result that quotes <seed:think> (a question about the model, or a fetched page or doc that mentions it) takes content + think_open = reasoning during the replay. Nothing closes it, so the engine starts the output in reasoning. The model's whole answer then reaches the client as reasoning_content, and its <seed:tool_call> blocks are never read as calls, because reasoning has no row for call_open.
The fixture replay doesn't catch this because it feeds only prompt_tail(fixture), never a full prompt with an opener. The 0-differences preview therefore says nothing about this row.
| .opens_turn("<|im_start|>assistant") | |
| .opens_turn("<seed:bos>assistant") |
A test along the lines of Alex's probe prompts in qwen3.rs would cover it: a prompt with <seed:bos>user\nwhat is <seed:think>?<seed:eos><seed:bos>assistant\n, then an output that should come out as content.
4858370 to
4a76a47
Compare
…ands for
Seed-OSS's template writes every argument with {{ value }}, so an object
or a list arrives as Python's repr, {'size': 'large'} or ['n1', 'n2'],
which inference read as a string. Inference now reads a Python literal
(None, True, False, numbers, strings in either quote with Python's
escapes, lists, tuples and dicts, trailing commas allowed) and writes the
JSON it stands for with json.dumps's separators, so the arguments read as
the reference writes them. Prose with an apostrophe, a bare word or an
unbalanced bracket is not a literal and stays a string; JSON still keeps
the model's own bytes, since it is read first. Two expectations change
with the rule: 1_000 under integer is 1000, as Python reads it, and
{'a': 1} is an object.
Signed-off-by: Simo Lin <25425177+slin1237@users.noreply.github.com>
Co-authored-by: Chang Su <8605658+CatherineSue@users.noreply.github.com>
The Qwen tagged rows under Seed-OSS's own spellings: reasoning between <seed:think> and </seed:think>, each call between <seed:tool_call> and </seed:tool_call> as <function=NAME> and <parameter=KEY> tags, typed by the request's tools. A plain <tool_call> is text here. Signed-off-by: Simo Lin <25425177+slin1237@users.noreply.github.com> Co-authored-by: Chang Su <8605658+CatherineSue@users.noreply.github.com>
WebWorld 32B and 14B and K-EXAONE through the Qwen3 table; Hermes 4, Granite 4.1 and Jamba 2 through Qwen2.5's; Step-3.5-Flash, Nanbeige 4.2, MiMo V2.5 and Nemotron 3 Nano through the Qwen tagged syntax; Seed-OSS through its table. Each with the prompt shape its fixtures show: WebWorld and MiMo write their own <think>, K-EXAONE, Nanbeige and Nemotron open it in the prompt, Step-3.5-Flash always opens it, the rest have no thought, Seed-OSS writes its own <seed:think>. Over bellwether's scale-run fixtures of 2026-10-06 every one replays with 0 differences. The known-difference list tolerates a table that lists nothing for a template without a thought. Signed-off-by: Simo Lin <25425177+slin1237@users.noreply.github.com> Co-authored-by: Chang Su <8605658+CatherineSue@users.noreply.github.com>
4a76a47 to
0e87975
Compare
hello-alexmcc
left a comment
There was a problem hiding this comment.
Rule-17 review of #2841 at 0e87975. This PR is stacked on #2840's feat/symphony-dsml at 4012d72, with three commits: d0df257, 26b7ff9 and 0e87975.
Verdict: request changes.
- The new Seed-OSS table names a turn opener that Seed-OSS's template never writes.
- The table is not a contract subject (rule 17).
Gates. Run at 0e87975 with Rust 1.98.0, a target dir of its own and -j 4. Each exited 0:
cargo +nightly fmt -p smg-symphony --checkcargo clippy -p smg-symphony --all-targets --all-features -- -D warningscargo test -p smg-symphony: 197 lib, 3 fixture and 7 contract testsRUSTDOCFLAGS="-D warnings" cargo doc -p smg-symphony --no-deps
Parity. every_recorded_qwen_model_parses_like_its_reference over the eleven new slugs in smg-project/bellwether's 2026-10-06 preview sets.
With SYMPHONY_CORPUS_ALLOWANCES=1, it exits 0 in 318 s with 0 differences:
| Slug | Table, prompt shape | Cases | Bitwise | Separators only | Listed | Allowed |
|---|---|---|---|---|---|---|
| webworld-32b | Qwen3, model writes the thought | 2436 | 7 | 2427 | 2 | 0 |
| webworld-14b | Qwen3, model writes the thought | 2436 | 7 | 2427 | 2 | 0 |
| k-exaone-236b-a23b | Qwen3, prompt opens the thought | 2436 | 7 | 2427 | 2 | 0 |
| hermes-4-14b | Qwen2.5 | 2435 | 2428 | 2 | 1 | 4, reasoning not written |
| granite-4.1-3b | Qwen2.5 | 2435 | 2428 | 2 | 1 | 4, reasoning not written |
| ai21-jamba2-3b | Qwen2.5 | 2435 | 2428 | 2 | 1 | 4, reasoning not written |
| step-3.5-flash | tagged, always opens | 2436 | 0 | 2319 | 2 | 115, declared-type conflict |
| nanbeige4.2-3b | tagged, opens | 2436 | 7 | 2312 | 2 | 115, declared-type conflict |
| mimo-v2.5 | tagged, model writes | 2436 | 2319 | 0 | 2 | 115, declared-type conflict |
| nvidia-nemotron-3-nano-30b-a3b-bf16 | tagged, opens | 15 | 7 | 6 | 2 | 0 |
| seed-oss-36b-instruct | Seed-OSS, model writes | 2435 | 2315 | 5 | 0 | 115, declared-type conflict |
Without the switch, the run fails on exactly those 472 corpus cases and on nothing else. Nemotron 3 Nano has 15 preview cases, so its evidence is thin.
Blocker 1. Seed-OSS's table names ChatML's turn opener, and Seed-OSS never writes it (formats/seed_oss.rs, .opens_turn("<|im_start|>assistant")).
- What the template writes. The cached
ByteDance-Seed/Seed-OSS-36B-Instructtemplate at497f1dcaends the generation prompt with{{ bos_token + "assistant\n" }}. Rendered offline, that is…<seed:eos><seed:bos>assistant\n.<|im_start|>occurs nowhere in the template. - What follows.
seednever finds the opener and replays the whole prompt, so #2839's fix does not reach this table. - Probe at 0e87975.
- Prompt
<seed:bos>user\nWhat does <seed:think> do?<seed:eos><seed:bos>assistant\n, outputIt opens a thought.. - As shipped, the client gets reasoning
It opens a thought.and no content. - With
.opens_turn("<seed:bos>assistant"), it gets content. The clean prompt gives content either way.
- Prompt
- Suggested fix.
<seed:bos>assistantas the table's opener, with that prompt as a test written failing first.
Blocker 2. seed_oss() is not a contract subject (rule 17; tests/contract.rs). The subjects are qwen3, qwen3 tagged, qwen2.5 and deepseek v4.1. So the five property tests never run on the new table. A Seed-OSS corpus should cover:
- the thought;
- one and several calls;
- a value written as a Python literal;
- a cut stream;
- Qwen's
<tool_call>as text.
Should-fix 3. Two of the eleven families reuse a Qwen table but open their turn differently (tests/bellwether_parse_fixtures.rs, MODELS). I rendered each family's generation prompt offline from its cached tokenizer at the manifest's pin. Eight of them write <|im_start|>assistant. Two do not:
k-exaone-236b-a23bends with<|endofturn|>\n<|assistant|>\n<think>\n. It goes through the Qwen3 table, so the replay runs over the whole prompt, and #2839's blocker 1 comes back here.- Probe: prompt
<|user|>\nWhy did you print <tool_call> there?<|endofturn|>\n<|assistant|>\n<think>\n, outputThe user asks about the marker.\n</think>\n\nIt opens a tool call.. - The client gets the whole thought,
</think>included, as content. - With
.opens_turn("<|assistant|>"), it gets reasoningThe user asks about the marker.\nand content\n\nIt opens a tool call.. The clean prompt is right either way.
- Probe: prompt
granite-4.1-3bends with<|start_of_role|>assistant<|end_of_role|>. It has no thought, so for now the whole-prompt replay costs nothing there.
The opener belongs to the model, not to the call syntax. A family that shares a table with Qwen needs its own opener, set in MODELS here and wherever a table is later picked for a model, with a stray-marker test for K-EXAONE. The fixtures cannot show this, because the parity test feeds only the tail.
Should-fix 4. A Python literal with a signed exponent is not read (tagged/value.rs, Literal::number).
- Why.
numberreads ASCII alphanumerics,.and_, so it stops at the sign of an exponent.1.5e-07is cut to1.5e, which fails as JSON, and so the whole literal stays a string. - When Python writes one.
reprwrites a signed exponent for every float below 1e-4 or from 1e16 up:repr(0.00001)is1e-05, andrepr(1e16)is1e+16. - Probes at 0e87975. These are Seed-OSS calls with no tools declared:
| Value written | Argument read | |
|---|---|---|
{'x': 1e-05} |
the string "{'x': 1e-05}" |
wrong |
{'x': 1.5e-07, 'n': 'a'} |
a string | wrong |
{'x': 0.0001} |
{"x": 0.0001} |
right |
bare 1e+16 and -2.5e-05 |
numbers | right, but only because they are JSON |
- Suggested fix. Read
[eE][+-]?digits, and addrepr's own spellings to the test table:1e-05,1.5e-07,1e+16and-0.0.
Nit 5. Literal::string reads \a, \b, \f and \v as the letters themselves, and an octal \NNN as \0 followed by digits. repr writes \x07-style escapes instead, so none of this shows today.
Rule 17.
| Item | Evidence | Result |
|---|---|---|
| Fixtures or verdict report, ran green | The gates, and the eleven slugs above. This is preview evidence: the 2026-10-06 sets are not on bellwether main. | Green, as preview |
| The five property tests for a new format | seed_oss() is not a contract subject. |
Missing (blocker 2) |
| No protected path changed | 5 files, all under crates/symphony/. |
OK |
| Sign-off, no AI trailer, Chang Su | d0df257, 26b7ff9 and 0e87975 each end with Signed-off-by: Simo Lin and then Chang Su's Co-authored-by. None has an AI trailer. |
OK |
| STATE.md updated | Symphony's STATE.md lists the stack's heads, #2841 at 0e87975 among them. | OK |
Rule 18. No change to Input, Event, Format's shape, the authority order or a waiver. The literal rule changes value inference for every tagged table, so two expectations change: 1_000 is now 1000, and {'a': 1} is now an object. Worth telling Simo, but rule 7 does not list it.
Description
Stacked on #2840 (
feat/symphony-dsml); the base is that branch, so the diff is this step alone: three commits. Eleven more of the families bellwether has recorded, read through the tables that exist, plus one table and one rule the fixtures asked for.Problem
bellwether's scale run holds parse sets for 36 Qwen checkpoints and DeepSeek V4.1, which #2839 and #2840 replay, and for 24 other families. Eleven of those write one of the syntaxes the crate already reads:
<tool_call>with a JSON object (WebWorld, K-EXAONE, Hermes 4, Granite 4.1, Jamba 2),<tool_call>with<function=/<parameter=tags (Step-3.5-Flash, Nanbeige 4.2, MiMo V2.5, Nemotron 3 Nano), and the tagged syntax under Seed-OSS's own<seed:think>/<seed:tool_call>markers. Each spells its thought differently, and Seed-OSS's template writes every value with{{ value }}, so an object arrives as Python's repr,{'size': 'large'}, which inference turned into a string.Solution
<think>; K-EXAONE, Nanbeige and Nemotron open it in the prompt and honour thinking off; Step-3.5-Flash always opens it; Hermes 4, Granite, Jamba 2 have no thought; Seed-OSS writes its own<seed:think>.seed_oss(declared), the Qwen tagged rows under Seed-OSS's spellings; a plain<tool_call>is text there, and both probe cases match the reference without a listing.tagged::value): text that is not JSON but is a Python literal (None,True,{'size': 'large'},['n1', 'n2'], a tuple, with Python's string escapes and trailing commas) is the JSON it stands for, written withjson.dumps's separators so the arguments read as the reference writes them. Prose with an apostrophe, a bare word or an unbalanced bracket is not a literal and stays a string; JSON still keeps the model's own bytes. Two old expectations changed with the rule:1_000underintegeris 1000 now, as Python reads it, and{'a': 1}is an object.Changes
crates/symphony/src/tagged/value.rs:python_literaland its reader (strings, numbers, lists, tuples, dicts), used byinferred; module doc; two tests of the reader and two expectations updated.crates/symphony/src/formats/seed_oss.rs(new, one test),formats/mod.rs,lib.rs.crates/symphony/tests/bellwether_parse_fixtures.rs: eleven rows,Family::SeedOss, the known-difference list tolerating an empty list for a template without a thought.Test Plan
cargo +nightly fmt -p smg-symphony --check,cargo clippy -p smg-symphony --all-targets --all-features -- -D warnings,cargo test -p smg-symphony(197 lib tests, 3 fixture tests skipping withoutBELLWETHER_FIXTURES, 7 contract tests),RUSTDOCFLAGS="-D warnings" cargo doc -p smg-symphony --no-deps, codespell with the repository's list: all exit 0 at 0e87975.Preview against bellwether's scale-run fixtures of 2026-10-06 (local, not CI), the eleven slugs through
every_recorded_qwen_model_parses_like_its_reference: 0 differences, every replay conserving its bytes and agreeing with every other chunking.<think><think><seed:think>"Allowed" is the corpus class bellwether Extract multimodal module into standalone llm-multimodal crate #56 now refuses at record time (a reference argument whose type the tagged text cannot carry) and, for the templates without a thought, the reasoning they never write (bellwether refactor: Extract MCP module into standalone workspace crate #52). Before the Python-literal rule, Seed-OSS had 217 differences, every one an object or list written as Python's repr.
The 37 slugs of feat(symphony): formats are data, and one engine runs them #2839 and feat(symphony): DeepSeek's DSML through the table, the second tagged assembler #2840 are unchanged by the rule: their templates write JSON, which inference reads first.
Checklist
cargo +nightly fmtpassescargo clippy --all-targets --all-features -- -D warningspasses