fix(sampling): preset fallback (not greedy 0) when generation_config absent — #228 review guard

[rsafier]

Adds the guard requested in the #228 review, on top of current main.

Background

The generation_config.json sampling-defaults feature from #228 was absorbed into main during the #229 merge (sampling_setup.rs / completions.rs now default temp/top_k/top_p from state.default_*). What did not land is the guard the SeedSource/GLM-5.2 review made a merge condition:

Correctness: … If generation_config.json is missing or fails to parse, these likely default to 0/0.0 — meaning temperature=0 and top_k=0 silently force greedy decoding instead of falling back to preset.
Recommendation: Block until (1) state.default_* fallback values are confirmed safe, (2) regression tests cover missing generation_config.json, (3) comment trimmed.

Main's load_sampling_defaults currently avoids literal greedy (it falls back to hard-coded 0.6/20/0.95), but those magic constants duplicate the model's curated MODEL.toml preset and can silently drift from it — so the fallback is safe-by-accident, not by construction, and there was no test locking it in.

The guard

Source the per-field fallback from the same [sampling.non_thinking] preset that already drives the penalties. Single source of truth; the defaults are provably non-degenerate for any well-formed MODEL.toml. Behavior when generation_config.json is present (every shipped Holo/Qwen model) is unchanged — a present config that legitimately requests temperature=0 is still honored; the guard only backfills absent fields.

• runtime.rs: field resolution split into a pure, testable resolve_sampling_defaults(Option<&Value>, &SamplingCategory, top_n_sigma, min_p); load_sampling_defaults now takes the preset and warn!s when the config is absent/unparseable.
• serve.rs: pass &sampling_presets.non_thinking (SamplingPresets is Copy, still used downstream).

Review items closed

1. Fallback values confirmed safe — now the model's curated preset, non-greedy by construction.
2. Regression tests (4): missing config → preset (asserts temp/top_k/top_p all non-zero); present config overrides preset; partial config falls back per-field; present temperature=0 honored.
3. Comment — the resolution logic carries a factual doc comment; no editorializing.

Validation

• cargo fmt -p spark-server --check clean.
• cargo clippy -p spark-server -- -D warnings clean.
• cargo test -p spark-server --bin spark → 569 passed, 0 failed (incl. the 4 new).

Delivers the guard #228 asked for; #228's own branch predates #229 and is closed as absorbed. Signed commit.

[SeedSource]

[BETA] Automated PR review — SeedSource AI pipeline

Advisory only, not a merge gate. AI-generated (internal risk-triage, Atlas serve gate, GLM-5.2 review), attributed per Atlas convention.

Triage

• Risk: low - serve-test-required (triage advisory): True - path-heuristic (authority): True
• Rationale: Fixes sampling default fallback to prevent silent greedy behavior when config is missing.
• Serving-path files: crates/spark-server/src/main_modules/serve.rs, crates/spark-server/src/main_modules/serve_phases/runtime.rs

Serve gate

• Build: 18s - Qwen3.6-35B-A3B-FP8 on GB10 node1
• Top-k KL vs main: 0.0001 (no shift vs main) - decode delta +0.1%
• Coherence probe: coherence=pass ping=10/10 (note: our test_coherence.py is flaky here; KL is the reliable serve signal)

GLM-5.2 code review

Correctness: Logic is sound. Field-by-field fallback via and_then on Option<&Value> correctly handles absent/unparseable configs and partial configs. Explicit temperature=0 is honored. Tests cover the key cases.

Risk: Low. The change replaces hard-coded fallbacks (0.6/20/0.95) with preset values. If SamplingPresets::default().non_thinking differs from the old constants, behavior changes for models lacking generation_config.json — this is the intended fix, but it's a behavioral change worth flagging in release notes. The sampling_presets variable is assumed in scope at the call site (not shown in diff); verify it's initialized before this line.

Quality: Clean extraction into resolve_sampling_defaults improves testability. One concern: top_n_sigma and min_p still fall back to CLI args rather than the preset — intentional per the doc comment, but the "single source of truth" claim is slightly overstated. The warn log fires when gen_cfg.is_none() but not when the config is present yet missing individual fields; consider a debug log per-field for traceability. Tests are adequate and well-scoped.

_{Pipeline: build+serve on GB10, GLM-5.2 via NVIDIA NIM, internal risk-triage classifier. External diff treated as untrusted input. Public repo.}