json: Fixed json enum handling

[Silverside]

Added common_json_value handling for enum values.

Overview

• This fixes a bug introduced in b10585 (commit d9f918d) which causes any JSON values created that come from an enum type variable to emit as booleans rather than integers as expected. This was noticed in the GET /v1/models llama-server endpoint as data/meta/vocab_type being returned to clients as a boolean. In my case I was expecting to get "vocab_type":2 but instead I was getting back "vocab_type":true.

Requirements

• I have read and agree with the contributing guidelines
• AI usage disclosure: No

[ZitouniNidhal]

Reviewed the changes — looks good to me.
The fix addresses the enum serialization issue cleanly by handling enum values explicitly through common_json_value, and the added tests provide good coverage for the reported regression. The scope of the change is focused and the implementation is easy to follow.

[pwilkin]

/bot review

[ggml-gh-bot]

Automated code review

Reviewing PR #28518 - a fix in common/json.h for enum values being emitted as booleans, plus a new test file. I read the diff, common/json.h, common/json.cpp, tools/server/server-context.cpp (the reported vocab_type site), and the test infra.

The bug diagnosis is correct: llama_vocab_type is an unscoped enum (include/llama.h:72), so {"vocab_type", meta.model_vocab_type} in get_res_model_info() (tools/server/server-context.cpp:4538) implicitly converted enum -> bool via the common_json_value(bool) ctor, since no ctor matched the enum exactly. The fix adds a properly constrained enum ctor and extends common_json_is_value, which is the right root-cause fix rather than casting at the call site. No blocking issues found.

Will slow the review

(point 1) New test file needs maintainer sign-off. Per AGENTS.md, adding a new file under tests/* requires maintainer approval, and test additions for small fixes are scrutinized for CI compile-time cost. tests/test-json.cpp adds a whole new test target for what is a one-ctor fix. The test is small, and common/json.h is new shared infrastructure so some coverage is defensible - but please confirm with maintainers in the PR that they want a dedicated test-json target before this goes in.

(point 2) Duplicated signed/unsigned branch in common/json.h:85-91. The enum ctor duplicates the body of the integral ctor directly below it. A simpler version delegates to the existing ctor:

template <typename T, typename std::enable_if_t<std::is_enum_v<T>, int> = 0>
common_json_value(T val) : common_json_value((std::underlying_type_t<T>) val) {}

This picks VAL_INT/VAL_UINT via the integral ctor and removes the copy. (Edge case: an enum : bool would land on the bool ctor, which is arguably the right output anyway.)

(point 3) Style inconsistency with the adjacent code in common/json.h. The new ctor uses std::enable_if_t / std::is_signed_v (C++14 helpers) while the three existing SFINAE ctors in the same struct use std::enable_if<...>::type / std::is_signed<T>::value. Either style compiles (the project is C++17), but matching the neighboring lines keeps the header uniform.

Nits

(point 4) tests/test-json.cpp:14 hardcodes t.verbose = true;. The existing convention (see tests/test-peg-parser.cpp) is testing t(std::cout); with verbosity driven by the LLAMA_TEST_VERBOSE env var and an optional filter argument to main. Following that pattern keeps the test suite output uniform.

(point 5) Test coverage is all-zero values (ENUM_0, 0, false). Worth adding one signed enum with a negative enumerator (e.g. mirroring llama_rope_type, which has -1) to pin the VAL_INT vs VAL_UINT selection, and optionally an enum class case since the ctor supports scoped enums too.

Everything else checks out: the two templated ctors have disjoint enable_if conditions so no ambiguity is introduced; common_json_is_value gains enums so the static_assert in common_json(T&&) now accepts them; json.cpp is part of llama-common so the test target links; the CMake target name test-json doesn't collide with test-json-schema-to-grammar; and the {"vocab_type", ...} site now emits an integer as expected.

This review was generated automatically by pi coding agent using zai-org/GLM-5.3. It may contain mistakes. Maintainers make the final call.

[ngxson]

I don't think it's useful to add such test. server tests should already covered it

[Silverside]

I wasn't a big fan of adding tests of this form, but it was the only sane way I could think to handle the regression test requirement of a bug-fix PR. Server tests do not cover this problem, but they are remarkably lacking in coverage of what shape and form is expected in JSON values returned to clients in general. Also it didn't seem like a good idea to have only the server cover tests for a piece of code that lives in common. Should I go back and add a one-off edge case test for this one field that I found emitted wrong from llama-server? Something like:

assert type(res.body["data"][0]["meta"]["vocab_type"]) is int

in test_server_models() of tools/server/tests/unit/test_basic.py

[pwilkin]

/bot review

[ggml-gh-bot]

Automated code review

Follow-up review at commit f97ccfd (previous review covered 9fb8a26).

Changes since the last review: the tests/test-json.cpp target and its CMake entry were removed, and the enum ctor in common/json.h was simplified to delegate to the integral ctor, with the SFINAE style switched to match the adjacent ctors.

All points from the previous review are resolved:

• (point 1) Test file removed as requested - the new tests/* target concern no longer applies.
• (point 2) The enum ctor now delegates to the existing integral ctor instead of duplicating the VAL_INT/VAL_UINT branch:

template <typename T, typename std::enable_if<std::is_enum<T>::value, int>::type = 0>
common_json_value(T val) : common_json_value((typename std::underlying_type<T>::type) val) {}

This is correct. An enum's underlying type is always integral, is_enum and is_integral are disjoint so the two template ctors can never be ambiguous, and the delegating call cannot recurse back into the enum ctor (the underlying type is not an enum). Signed enums (e.g. an enumerator of -1 over int) correctly land on VAL_INT via the integral ctor. The enum : bool edge case now lands on the plain common_json_value(bool) ctor, which is a sensible output.

• (point 3) std::enable_if<...>::type / std::is_enum<T>::value now match the style of the neighboring SFINAE ctors in the same struct. Same for the common_json_is_value addition.
• (points 4 and 5) Moot with the tests removed.

I re-checked the fix against the reported site: {"vocab_type", meta.model_vocab_type} in get_res_model_info() (tools/server/server-context.cpp:4538) now emits an integer. No other json emission site passes a raw enum that relied on the old implicit enum-to-bool conversion, and the common_json_is_value extension keeps the static_assert in common_json(T&&) accepting enum types.

No blocking issues, no new findings. The diff is now a minimal, single-purpose fix and looks good from my side.

This review was generated automatically by pi coding agent using zai-org/GLM-5.3. It may contain mistakes. Maintainers make the final call.

[stonybeach]

Fixes #29091

[pwilkin]

Pending @ngxson 's approval.