Repository navigation
Conversation
…validation Chat Completions ingress accepted any `image_url` content block shape, so a flattened or empty block reached the Session and later raised an opaque `TypeError: string indices must be integers` from the multimodal extraction helper, while `url: 1` / `url: ""` were exported as bogus image entries. Validate the canonical block in `check_messages` so every protocol rejects it with a full field path before it enters the Forest.
Chat Completions, Responses and Anthropic Messages must collapse onto one `messages` / `tools` / `chat_template_kwargs` triple, because that triple is the only input of the Session state hash used for prefix matching, resume and training export. Add independent golden fixtures per protocol over 17 semantically equivalent requests (text, assistant tool calls, tool results, image inputs, tools, thinking toggles and a full multiturn conversation), assert the three fields and the resulting state hash agree, and assert illegal inputs are rejected with exact field paths. The 6 `xfail(strict=True)` cases pin the validation-strictness divergences that still exist across the three ingress paths, so fixing them surfaces as an unexpected pass instead of silently changing behaviour.
Nyanpasu 审查看板审查状态： ✅ 已通过 审查版本： cedcde7 新头 cedcde7 仅为合并 main：PR 十个文件与已批准的 63f63ec 逐字节一致，合并未触及 PR 文件及其直接依赖。前轮遗留的 CI 待确认项已闭环——cedcde75 上 8 项检查（pre-commit、Python 3.10–3.12、GPU 单测与 GPU/NPU 集成）全部通过，其中 Python 3.10 全量套件含 tests/agentic 260 项结果。三项审查问题维持已解决，未发现新问题。PR 描述中的 D1–D6 协议验证差异及字段路径问题仍待维护者决策，属已报告事项而非本 PR 缺陷。
审查发现待处理
已解决或已取代
提交范围 · 接收 10 · 建议移出 0 · 待确认 0接收 10 个文件 · 建议移出 0 个文件 · 待确认 0 个文件。移出与待确认部分暂停深审，不代表审查通过。
精简审查与验证依据
生产代码的必要性与替代方案
测试的必要性与替代方案
Powered by Nyanpasu with glm-5.3[1m] xhigh, please check the suggestions carefully.
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
rai-studio-bot
left a comment
rai-studio-bot
left a comment
There was a problem hiding this comment.
已完成对 fb36c51c 的审查，未发现阻塞问题；一项测试盲点及修正建议见行内评论。图片结构校验与现有 canonical／导出合同一致。
函数级依赖隔离验证为 257 passed、6 xfailed。完整 pytest 因本地缺少 PyTorch、Ray、NumPy 而未能收集；当前无 CI 检查记录，未运行需要多节点 GPU 的集成测试。PR 列出的既有协议差异仍需维护者决定统一策略。
| ("mutate", "expected_changed_field"), | ||
| [ | ||
| pytest.param( | ||
| lambda m, t, k: ([{"role": "user", "content": "hi "}] + m[1:], t, k), "messages", id="trailing-space" |
There was a problem hiding this comment.
请让这个用例只增加尾随空格。_MESSAGES[0] 是 system: "S"，当前写法把它整体替换成 user: "hi "；即使哈希错误地忽略尾随空格，role 和正文的变化仍会让断言通过。函数级隔离验证中，在哈希入口对字符串 content 执行 rstrip() 后，新增套件仍为 257 passed、6 xfailed。保留原消息，只追加一个空格，才能捕获该回归。
| lambda m, t, k: ([{"role": "user", "content": "hi "}] + m[1:], t, k), "messages", id="trailing-space" | |
| lambda m, t, k: ([{**m[0], "content": m[0]["content"] + " "}, *m[1:]], t, k), | |
| "messages", | |
| id="trailing-space", |
There was a problem hiding this comment.
已在 0f0180a1 验证修复：用例现在保留原消息，只追加尾随空格。函数级隔离套件为 257 passed、6 xfailed；在哈希入口注入 rstrip() 后，恰好 trailing-space 用例失败，其余为 256 passed、6 xfailed。该问题已解决。
There was a problem hiding this comment.
已确认并修复。原写法把 _MESSAGES[0] 整体替换，role 与正文一并变化，尾随空格并非唯一变量；现改为在原消息上追加一个空格（{**m[0], "content": m[0]["content"] + " "}），使空白字符成为唯一差异。
按建议的函数级隔离方式复核：在 _messages_tools_template_state_hash 入口对字符串 content 注入 rstrip() 后，修改前的用例哈希仍然变化（断言通过，盲区复现），修改后的用例哈希不再变化（断言失败，回归被捕获）；去掉变异体后两者均正常，套件 294 passed / 6 xfailed，tests/agentic 263 collected。
The old mutation swapped the system message for a user one, so role and text changed together and the case stayed green even when the hash ignored trailing whitespace. Verified with an rstrip() regression injected at the hash entry point: undetected before, failing now.
rai-studio-bot
left a comment
rai-studio-bot
left a comment
There was a problem hiding this comment.
已完成 0f0180a1 复查，上轮测试问题已修复，未发现新增问题，同意本次变更。修复验证见原讨论。
函数级隔离验证为 257 passed、6 xfailed，尾随空白裁剪变异已被测试捕获。完整测试仍受本地 PyTorch／Ray／NumPy 依赖缺失限制，当前无 CI 检查记录；需要多节点 GPU 的集成测试未运行。
|
@rai-studio-bot 请再次深度 review，专注于：
可以进一步参考 https://github.com/ShigureLab/nyanpasu/blob/main/AGENTS.md 中的原则进行 review |
rai-studio-bot
left a comment
rai-studio-bot
left a comment
There was a problem hiding this comment.
针对本轮三项重点重新审查 0f0180a1，发现两项非阻塞的测试设计／精简问题，详见行内评论。原先的通过结论补充为本轮意见；未发现需要阻塞的生产逻辑缺陷。
新增 image_url 校验是必要的外部输入边界校验：Chat 可传入错误结构，显式导出也复用它，下游会直接读取 image_url.url。建议保留。工具 arguments 中的非法 JSON 字符串、重复键和非有限数字同样可真实到达，不宜一并删除。
本地函数级隔离验证仍为 257 passed、6 xfailed。CI 的 Python 3.10–3.12 与 GPU 单测已通过；pre-commit 因 clang-format 修改三个协议 JSON fixtures 而失败，请应用对应格式化结果。多模态集成检查仍在运行。
| CHAT, _chat([_user("hi")], tools=[{"type": "function", "function": {"name": "f", "parameters": {}}}]) | ||
| ) | ||
| ) | ||
| responses = canonical_of(normalize(RESPONSES, _responses_input("hi", tools=[{"name": "f", "parameters": {}}]))) |
There was a problem hiding this comment.
D4 把协议格式差异当成了应修复的 canonical 差异。仓库 docs/en/guide/agentic-rollout.md 已区分：Responses function tool 使用 type/name/parameters，Anthropic 允许省略 type；_FUNCTION_TOOL_FIELDS 也明确实现了这一区别。这里却删除 Responses 的 discriminator，再要求它与合法 Chat／Anthropic 请求得到相同结果。这个 strict xfail 因而把“为缺少 type 的 Responses tool 猜测默认类型”变成了期望行为，而非验证合法等价输入。建议移除这条收敛 xfail；如需记录缺字段行为，将其作为 Responses 自身的异常输入测试。合法工具的跨协议一致性已有 golden 覆盖。
There was a problem hiding this comment.
已在 63f63ec9 确认修复：删除了要求缺少 type 的 Responses 工具与合法请求收敛的 xfail，合法工具 golden 保持不变。保留的当前行为断言不再要求改变协议语义，此问题已解决。
| pytest.param({1: "x"}, "JSON object keys must be strings", id="non-string-key"), | ||
| pytest.param({"a": {1, 2}}, "must contain JSON-compatible values, got <class 'set'>", id="python-set"), |
There was a problem hiding this comment.
按本轮“只测试真实可达输入”的要求，建议删去 non-string-key、python-set 两例，以及下方第 195 行的整数键转换断言。三种 HTTP 入口先经过 request.json()，agent 显式导出也先经 json.loads()（runner/ipc.py:281–283）：整数对象键到达这里时已经是字符串，Python set 则无法通过 JSON 序列化。这几条直接调用内部 helper 的测试固化了传输边界之后不可达的 Python 状态。保留非法 JSON 字符串、重复键、溢出数字和转义 surrogate 等用例，它们确实可以出现在工具 arguments 字符串中；不需要为此次测试清理扩大生产代码改动。
There was a problem hiding this comment.
已在 63f63ec9 确认删除整数键、Python set 两例及 template kwargs 整数键转换断言；真实可达的非法 arguments 字符串测试仍保留。函数级隔离套件为 255 passed、5 xfailed，此问题已解决。
- Remove the D4 convergence xfail: a Responses tool without its required type discriminator is malformed input, not a legitimate cross-protocol equivalent; the drop behavior stays pinned in the divergence inventory. - Remove non-string-key / python-set argument cases and the integer-key template-kwarg assertion; all transport entries JSON-decode first, so those Python states cannot reach the helpers. - Apply clang-format to the three protocol JSON fixtures (pre-commit).
rai-studio-bot
left a comment
rai-studio-bot
left a comment
There was a problem hiding this comment.
已完成 63f63ec9 复查，上轮两项测试意见均已修复，未发现新增问题，同意本次变更。验证细节已回复原讨论。
函数级隔离验证为 255 passed、5 xfailed；三个协议 JSON fixtures 仅调整格式，解析后的内容与上版完全一致。当前提交暂无 CI 检查记录，格式检查是否通过仍待新一轮 CI 确认；本地完整测试及多节点 GPU 集成测试未运行。
|
@SigureMo 任务已完成，请求 review |
|
本赛题已经由 #335 合入并锁定，感谢参与～ |
4 participants
What
Adds a canonical-request golden test suite for the three Agentic ingress protocols — Chat Completions, Responses, Anthropic Messages — and tightens one validation gap in the shared canonicalizer.
tests/agentic/(new): independent golden fixtures per protocol + three test modules (257 passed, 6 xfailed).relax/agentic/session/state.py(+8):check_messagesnow validates the canonicalimage_urlcontent block.Two commits:
fix(agentic): reject malformed image_url blocks in canonical message validationandtest(agentic): cover cross-protocol canonical request goldens.Relates to #<任务 No.8>
Why
messages/tools/chat_template_kwargsis the only input of_messages_tools_template_state_hash, i.e. the identity of every Session Forest node — prefix matching, partial-rollout resume and the training export all key off it. If two protocols map one semantic request to two different triples, the same trajectory reaches the Session under two different state hashes.Nothing in the repo asserted that the three ingress paths converge, and nothing asserted that a malformed multimodal block is rejected before it enters the Forest.
Before this change,
{"type": "image_url", "image_url": "https://…"}(object flattened to a string) passed validation and then raisedTypeError: string indices must be integersfrom the export-side_multimodal_inputs_from_messages;url: 1/url: ""were silently exported as{'images': [1]}/{'images': ['']}.How
Convergence goldens —
fixtures/{chat_completions,responses,anthropic_messages}.jsoneach carry the same 17case_ids (text, non-ASCII, system chunk merging, assistant tool call, parallel tool calls, tool result with empty output, reasoning-only, reasoning + text, HTTP-url image, base64 data-url image, image + multiple text blocks, tools with/without description, thinking on/off, full multiturn conversation). The three fixtures deliberately differ in surface form: scrambledfunction.argumentskey order,argumentsas a JSON string vs. a dict,instructionsvs.systemvs. system message,reasoning.effortvs.thinking.type,input_imagevs.image(source)vs.image_url.helpers.pyruns each payload through its normalizer and projects the result onto the canonical triple. Three assertion layers: cross-protocol agreement, agreement with the pinnedfixtures/canonical.json, and identical state hash.REQUIRED_CASE_IDSfails the suite if a scenario required by the task brief is dropped.Illegal input — 84 parametrized cases across the three protocols assert the exact
message,paramandstatus_code == 400, including nested field paths such asmessages[2].tool_calls[0].id must be a non-empty stringandmessages[2].content[0].image_url.url must be a non-empty string. Covers unknown/illegal roles, missing or non-string tool-call ids, empty-string and empty-list content, malformed tool arguments (invalid JSON, JSON scalar, list, duplicate key, non-finite number), incomplete tool results, and 13 illegal image shapes (Chat 5 / Responses 2 / Anthropic 6). Two extra tests assert a rejected image block can never reach multimodal extraction, and a canonical one always survives it.Stable serialization — key-order invariance of the state hash over every canonical field, digest shape, content sensitivity (trailing space, dropped tool, thinking toggled off), uniqueness across the 17 goldens, tool-argument collapse to a single JSON form (including the JS
JSON.stringifyintegral-float rule and non-ASCII kept literal), recursive nestedchat_template_kwargssorting, and function-schema field projection.Validation fix —
check_messagesrejects a canonicalimage_urlblock whose payload is not an object (TypeError) and whoseurlis not a non-empty string (ValueError), so all three protocols now refuse it through the shared收口 with a full field path. Unknown block types are intentionally left as-is.Testing
Non-vacuity was checked by mutating one protocol's fixture in a throwaway copy (Responses
text_only→"What is 2+3?") and confirming all three layers catch exactly that case and nothing else:The fix was checked against
HEADby loadinggit show HEAD:relax/agentic/session/state.pyas a separate module: all four malformed image shapes areACCEPTEDat HEAD, all fourREJECTEDafter the change, and the flattened-string shape crashes the export helper at HEAD.Equivalent-to-hook local checks (same versions as
.pre-commit-config.yaml):Committed blobs are LF (verified via
git show, localcore.autocrlf=trueaffects checkout only).Not run locally, CI is authoritative:
pre-commit run --all-files— the hook repositories could not be cloned in this environment; the equivalent ruff / docformatter / whitespace / final-newline checks above were run individually instead.tests/integration/— requires multi-node GPU hardware; the dev machine is Windows without CUDA.pytest tests/in full additionally fails locally ontorch._inductor.exc.InductorError: InvalidCxxCompiler: cl is not found(no MSVC toolchain) intests/utils/replayandtests/utils/training, unrelated to this change.Type of Change
The remainder of the PR is test-only. No public API, argument parsing, or Controller/Service/Launcher logic is touched.
Known cross-protocol divergences (requesting a decision)
The suite found that the conversion layer converges correctly on all 17 equivalence cases; the remaining asymmetry is in validation strictness at each ingress, not in canonicalization. Fixing these changes runtime behaviour beyond this task's scope, so they are pinned with
@pytest.mark.xfail(strict=True)— a fix surfaces as an unexpected pass rather than going unnoticed.id(it disappears from the canonical form); Responses reportsinput[1].call_id must be a non-empty string, Anthropic reportsmessages[1].content[0].id must be a non-empty stringtool_call_idname, then drops it silently (tools == [])type; Anthropic keeps and projects itdocument): Chat passes it through verbatim, others drop itRelated, for the "accurate field path" acceptance criterion: errors raised by a protocol's own validator use protocol-native paths (
input[…],messages[…].content[…].source…), but errors raised after conversion, insidecheck_messages, report canonical-array paths. A Responses client that sends{"instructions": …, "input": [user, function_call]}getsparam=inputwithmessages[2].tool_calls[0]…— the field name changes and the index shifts with the presence ofinstructions. Chat is unaffected (canonical array == request array). Which path a client should see looks like a deliberate product choice, so it is reported rather than changed here.Screenshots / Logs
n/a — pure CPU tests, no model service or network involved.