Skip to content

feat(TUI):收敛多模态输入链路:runtime 单入口提交 + session 归一化落地,清理 TUI 过渡职责并修复 schema/目录行为#337

Merged
phantom5099 merged 11 commits into
1024XEngineer:mainfrom
phantom5099:main
Apr 17, 2026

Conversation

@phantom5099

Copy link
Copy Markdown
Collaborator

问题背景

当前代码在“图片输入 -> 归一化 -> 发送”链路上存在职责分散和过渡残留:
TUI 承担了过多输入校验与能力判断,runtime 缺少统一提交入口,session 侧没有独立归一化组件,导致边界不够清晰且行为不一致。
同时,provider/tool schema 还存在会触发 400 的兼容性细节问题,providers 目录加载语义也与预期不一致。

本次修改(做了什么 & 为什么)

  1. 引入 runtime 单入口提交 Submit
  • 修改:internal/runtime/runtime.gointernal/runtime/input_prepare.gointernal/tui/services/runtime_service.gointernal/tui/core/app/update.go
  • 内容:新增 Submit(ctx, PrepareInput),由 runtime 统一串联 PrepareUserInput + Run;TUI 改为调用 RunSubmitCmd
  • 原因:避免上层手动编排两段调用,减少跨层耦合,让“输入归一化 + 执行”成为一个稳定入口。
  1. 输入归一化下沉到 session 子层
  • 修改:新增 internal/session/input_preparer.go,并在 internal/app/bootstrap.go 注入 SetUserInputPreparer(...)
  • 内容:在 session 层完成会话解析/创建、工作目录解析、图片落盘、parts 组装、MIME 推断与结构化错误(AssetSaveError)。
  • 原因:把“会话/资产写侧能力”留在 session 领域,runtime 只做编排,避免 runtime 继续膨胀成兼容层。
  1. 增加可观测事件,打通前置副作用可追踪性
  • 修改:internal/runtime/events.gointernal/runtime/input_prepare.go
  • 内容:新增 input_normalizedasset_savedasset_save_failed 及 payload;失败场景统一事件化。
  • 原因:输入归一化和附件落盘不再是黑箱,便于定位与恢复。
  1. 收敛 TUI 职责,删除过渡校验/缓存逻辑
  • 修改:internal/tui/core/app/input_features.gointernal/tui/core/app/app.gointernal/tui/core/app/update.go
  • 内容:移除模型图片能力缓存与发送前阻断;TUI 仅做轻量 token 吸收和附件排队,硬校验交给 runtime/session;会话切换时统一清理运行态残留。
  • 原因:TUI 保持“交互/展示层”定位,避免页面层承担领域规则。
  1. 修复 schema 兼容性问题,避免 provider 400
  • 修改:internal/provider/openaicompat/chatcompletions/request.gointernal/provider/openaicompat/chatcompletions/provider_test.go
  • 内容:对 tool schema 做 OpenAI chat-completions 顶层归一化(确保 object,移除顶层 oneOf/anyOf/allOf/enum/not,且不改原 schema)。
  • 原因:避免请求被模型接口因 schema 顶层约束直接拒绝。
  1. 修复 todo 工具 schema 的数组定义缺失
  • 修改:internal/tools/todo/write.gointernal/tools/todo/write_test.go
  • 内容:为 artifacts 数组补齐 items
  • 原因:修复 “array schema missing items” 报错,保证工具 schema 合法。
  1. 调整自定义 provider 目录处理语义
  • 修改:internal/config/provider_loader.gointernal/config/loader_test.go
  • 内容:loadCustomProviders 改为先 MkdirAll(providersDir)ReadDir;同步修正测试期望。
  • 原因:目录不存在应自动创建,不再依赖“目录必须预先存在”的隐式前提。

行为变化

  • 图片输入不再被 TUI 预先按模型能力阻断,会进入统一归一化链路。
  • 用户消息以 runtime 事件为准写入展示,减少重复渲染风险。
  • provider/tool schema 错误导致的 400 明显减少,错误定位更直接。

测试

  • 新增与更新测试覆盖 runtime/session/tui/config/provider/tool 相关路径:
    • internal/session/input_preparer_test.go
    • internal/runtime/input_prepare_test.go
    • internal/provider/openaicompat/chatcompletions/provider_test.go
    • internal/tools/todo/write_test.go
    • internal/config/loader_test.go
    • 多处 TUI 与 services 测试同步到 Submit 新接口
  • 已执行:go test ./...(通过)

@codecov

codecov Bot commented Apr 17, 2026

Copy link
Copy Markdown

fennoai[bot]

This comment was marked as outdated.

@phantom5099

Copy link
Copy Markdown
Collaborator Author

/code 修复review提到的问题,之后提高测试覆盖率,完成后应该在Fork仓库内开新PR而不是直接提交到目前的PR

@fennoai

fennoai Bot commented Apr 17, 2026

Copy link
Copy Markdown
Contributor

完成情况

已完成你在触发评论里要求的两件事:修复 review 指出的问题,并补充测试覆盖;同时未直接提交到当前 PR 分支 main,而是推送到 fork 新分支并新开了 PR。

关键修复

  • 修复 internal/tui/core/app/input_features.go

  • 仅吸收显式 @image:<path> 令牌,避免 @<path> 自动上传的信任边界风险

  • 吸收内联图片令牌时保留原始空白布局,不再用 strings.Fields 改写提示词格式

  • 修复 internal/session/input_preparer.go

  • Prepare 在本次新建会话后发生附件保存失败时,新增回滚(JSONStore 场景)避免孤儿会话残留

  • 修复 internal/config/provider_loader.go

  • loadCustomProviders 改为缺失目录返回空结果,不再在读取路径执行 MkdirAll,避免只读目录写入要求

  • 修复 internal/tui/core/app/update.go

  • 用户消息去重改为按 run id 去重,不再按内容去重,允许连续发送相同文本消息

测试补充

  • 更新 internal/tui/core/app/input_features_test.go
    • 覆盖显式前缀要求、空白布局保留、非存在文件仍走 runtime/session 校验等分支
  • 更新 internal/session/input_preparer_test.go
    • 覆盖“新建会话失败回滚”与“已有会话失败不回滚”
  • 更新 internal/tui/core/app/update_test.go
    • 覆盖用户消息按 run id 去重、同内容跨 run 保留
  • 更新 internal/config/loader_test.go
    • 同步 loadCustomProviders 语义断言

验证

  • 已执行:go test ./...(通过)

提交与 PR

View job run

xgopilot and others added 2 commits April 17, 2026 07:43
…overage

- require explicit @image: token and preserve whitespace when absorbing inline image refs
- dedupe user-message rendering by run id instead of message content
- rollback newly created session when asset save fails
- make custom provider directory load read-only friendly when directory is missing
- add/adjust tests for all updated behaviors

Generated with [codeagent](https://github.com/qbox/codeagent)
Co-authored-by: phantom5099 <245659304+phantom5099@users.noreply.github.com>
fix(tui,session,config): 修复 review 问题并补测试覆盖
@phantom5099

Copy link
Copy Markdown
Collaborator Author

@xgopilot 先分析当前PR中可能存在哪些风险点,然后给我一个让AI能够进行review的提示词

@phantom5099

Copy link
Copy Markdown
Collaborator Author

@xgopilot 请你作为严格的 Go 代码审查者,审查 PR #337(对比基线 origin/main...HEAD),仅输出“问题与风险”,不要输出表扬或泛泛总结。

审查目标:

  • 主链路闭环是否保持可用:用户输入 -> runtime 推理 -> tools -> 结果回灌 -> UI 展示
  • 分层边界是否被破坏:TUI / Runtime / Session / Provider / Tools
  • 多模态输入链路(Submit + InputPreparer + TUI 事件处理)是否有行为回归
  • provider schema 归一化是否引入兼容/语义风险
  • 会话与附件持久化是否有一致性或安全风险

请重点检查这些文件:

  • internal/runtime/input_prepare.go
  • internal/runtime/runtime.go
  • internal/session/input_preparer.go
  • internal/tui/core/app/update.go
  • internal/tui/core/app/input_features.go
  • internal/provider/openaicompat/chatcompletions/request.go

必须覆盖的风险维度:

  1. 正确性:空输入、相对路径、带空格路径、重复消息、跨会话切换、失败回滚
  2. 安全性:本地文件路径边界、MIME 信任、错误信息泄漏
  3. 一致性:事件 payload 与真实状态是否一致(input_normalized/asset_saved/asset_save_failed)
  4. 兼容性:schema 归一化后是否改变工具参数语义
  5. 可测试性:新增逻辑是否缺关键边界测试

输出格式要求:

  • 按严重级别排序:Critical / High / Medium / Low
  • 每条包含:文件:行号 + 问题描述 + 触发条件 + 可能影响 + 建议修复
  • 如果某项无法确认,请明确写“需要补充验证”的具体验证步骤
  • 若未发现 Critical/High,也要列出 Medium/Low 和残余测试缺口

@phantom5099

Copy link
Copy Markdown
Collaborator Author

High

  • internal/session/input_preparer.go:153internal/session/input_preparer.go:181internal/session/input_preparer.go:185
    问题描述:附件路径未做工作目录边界约束,且 declared MIME 直接信任,扩展名命中 image/* 也直接通过。
    触发条件:传入绝对路径(如 /etc/passwd)或伪造 MimeType=image/png / 伪造图片扩展名。
    可能影响:本地任意文件可被当作“图片附件”落盘并进入后续请求链路,存在数据外带与错误归因延后风险。
    建议修复:在 session 层强制 ensurePathWithinBase(workdir, target)declared MIME 仅作候选,必须与文件头探测结果共同校验且最终要求 image/*
  • internal/provider/openaicompat/chatcompletions/request.go:82
    问题描述:schema 归一化强制顶层 type=object,并删除 oneOf/anyOf/allOf/enum/not,会改变工具参数语义。
    触发条件:工具 schema 依赖顶层组合约束或非 object 顶层约束表达合法输入域。
    可能影响:模型可生成原本应被 schema 拒绝的参数形态,导致 tool 参数约束失真(“减少 400”以牺牲语义一致性为代价)。
    建议修复:仅做 provider 必需的最小转换并保留语义(例如将约束迁移到 properties 子层或引入可配置的 strict/compat 策略);至少在日志/事件中标记“schema 被降级”。

Medium

  • internal/session/input_preparer.go:154
    问题描述:相对路径通过 filepath.Abs 按进程 cwd 解析,而非按 session/workdir 解析。
    触发条件:非 TUI 调用 runtime.Submit 传相对图片路径,且进程启动目录与会话工作目录不一致。
    可能影响:同一输入在不同启动目录解析到不同文件,导致行为不一致和排障困难。
    建议修复:统一按 PreparedInput.Workdir(或 requested/default workdir)做相对路径基准解析。
  • internal/tui/core/app/input_features.go:236internal/tui/core/app/input_features.go:272
    问题描述:内联 token 以空白分词,@image:"a b.png" 会在分词阶段被截断,带空格路径无法正确吸收。
    触发条件:用户输入带引号或未引号的空格路径。
    可能影响:附件吸收失败或文本被部分剥离,形成多模态输入回归。
    建议修复:实现带引号/转义的 token 扫描器,或改为正则/状态机解析 @image:<path>(支持空格与引号)。
  • internal/session/input_preparer.go:255
    问题描述:失败回滚仅对 *JSONStore 生效(类型断言),对其他 Store 实现不回滚。
    触发条件:后续接入非 JSON 存储时,Prepare 新建会话后附件失败。
    可能影响:重新出现孤儿会话,持久化一致性退化。
    建议修复:把“回滚新建会话”下沉为 Store 能力接口(如 DeleteSession(ctx,id)),由所有实现统一支持。
  • internal/runtime/input_prepare.go:81internal/runtime/input_prepare.go:86
    问题描述:asset_save_failed 事件使用 input.SessionID,新建会话后失败场景会丢失真实会话上下文(常为空)。
    触发条件:SessionID 为空触发新建会话,随后附件保存失败。
    可能影响:事件追踪与状态恢复难度上升,观测面上看不到实际受影响会话。
    建议修复:在 AssetSaveError 增加 SessionID 字段并由 session preparer 回填;runtime 失败事件优先使用该字段。

Low

  • internal/tui/core/app/update.go:738internal/tui/core/app/update.go:965
    问题描述:会话刷新会清空运行态,但运行中事件分发未按 RunID/SessionID 做隔离过滤,存在跨会话污染的潜在窗口。
    触发条件:需要补充验证。
    需要补充验证步骤:

    1. 运行中触发会话切换(或任何调用 refreshMessages 的路径)。
    2. 观察旧 run 后续 tool/assistant/error 事件是否仍写入当前会话 UI。
    3. 验证 pendingPermissionToolStatesExecutionError 是否被旧 run 事件覆盖。
      可能影响:UI 展示与真实会话状态偏离。
      建议修复:在 handleRuntimeEvent 前增加 RunID/SessionID 关联校验,并对“非当前 run 事件”做丢弃或旁路记录。
  • 可测试性缺口:

    • internal/session/input_preparer_test.go:缺“相对路径按 workdir 解析”测试、缺“declared MIME 与文件头冲突”测试。
    • internal/tui/core/app/input_features_test.go:缺“@image: 带空格/引号路径”测试。
    • internal/provider/openaicompat/chatcompletions/provider_test.go:缺“归一化后 required/组合约束语义不漂移”测试。

View job run

@xgopilot 第一点提到附件路径未做工作目录边界约束,那如果后面想上传其他文件夹的文件该怎么办

@phantom5099

Copy link
Copy Markdown
Collaborator Author

结论

你这个场景不应该靠“取消边界检查”解决,而是改成:默认仅允许 workdir,跨目录必须显式授权

可落地策略

  1. 默认策略(安全基线)
  • targetsession/workdir 内:直接允许。
  • 不在 workdir:默认拒绝。
  1. 显式放行策略(满足上传其他目录)
  • 增加配置白名单,例如:attachment_allowed_paths(绝对路径列表)。
  • 仅当 target 命中白名单目录时允许跨目录上传。
  • 建议同时发事件(如 asset_outside_workdir_allowed)便于审计。
  1. 一次性授权策略(可选)
  • TUI/Runtime 增加一次性确认(类似 permission flow):
    “该文件位于 workdir 外,是否本次允许?”
  • 用户确认后仅对当前 run 生效,不持久化全局放开。

关键实现点(避免绕过)

  • 路径判断前统一做 Abs + EvalSymlinks + Clean,防止软链接逃逸。
  • 校验逻辑:within(workdir) || within(any(allowed_paths))
  • 这样既保留安全边界,也支持你说的“上传其他文件夹文件”。

本次执行说明

  • 已完成:基于当前 PR 语义给出边界设计答复。
  • 未执行:未改代码、未提交变更(本次是问题答复)。

View job run

@xgopilot 如果我想让@文件时只能选工作区内文件,使用+添加文件(你就当作是和@不一样的方式)就能任意添加其他路径上的文件,那么如果我目前直接改成非工作区路径就不允许上传,会影响以后任意添加其他路径上的文件功能的完成吗

@phantom5099

Copy link
Copy Markdown
Collaborator Author

High

  • internal/session/input_preparer.go:153internal/session/input_preparer.go:181internal/session/input_preparer.go:185
    问题描述:附件路径未做工作目录边界约束,且 declared MIME 直接信任,扩展名命中 image/* 也直接通过。
    触发条件:传入绝对路径(如 /etc/passwd)或伪造 MimeType=image/png / 伪造图片扩展名。
    可能影响:本地任意文件可被当作“图片附件”落盘并进入后续请求链路,存在数据外带与错误归因延后风险。
    建议修复:在 session 层强制 ensurePathWithinBase(workdir, target)declared MIME 仅作候选,必须与文件头探测结果共同校验且最终要求 image/*
  • internal/provider/openaicompat/chatcompletions/request.go:82
    问题描述:schema 归一化强制顶层 type=object,并删除 oneOf/anyOf/allOf/enum/not,会改变工具参数语义。
    触发条件:工具 schema 依赖顶层组合约束或非 object 顶层约束表达合法输入域。
    可能影响:模型可生成原本应被 schema 拒绝的参数形态,导致 tool 参数约束失真(“减少 400”以牺牲语义一致性为代价)。
    建议修复:仅做 provider 必需的最小转换并保留语义(例如将约束迁移到 properties 子层或引入可配置的 strict/compat 策略);至少在日志/事件中标记“schema 被降级”。

Medium

  • internal/session/input_preparer.go:154
    问题描述:相对路径通过 filepath.Abs 按进程 cwd 解析,而非按 session/workdir 解析。
    触发条件:非 TUI 调用 runtime.Submit 传相对图片路径,且进程启动目录与会话工作目录不一致。
    可能影响:同一输入在不同启动目录解析到不同文件,导致行为不一致和排障困难。
    建议修复:统一按 PreparedInput.Workdir(或 requested/default workdir)做相对路径基准解析。
  • internal/tui/core/app/input_features.go:236internal/tui/core/app/input_features.go:272
    问题描述:内联 token 以空白分词,@image:"a b.png" 会在分词阶段被截断,带空格路径无法正确吸收。
    触发条件:用户输入带引号或未引号的空格路径。
    可能影响:附件吸收失败或文本被部分剥离,形成多模态输入回归。
    建议修复:实现带引号/转义的 token 扫描器,或改为正则/状态机解析 @image:<path>(支持空格与引号)。
  • internal/session/input_preparer.go:255
    问题描述:失败回滚仅对 *JSONStore 生效(类型断言),对其他 Store 实现不回滚。
    触发条件:后续接入非 JSON 存储时,Prepare 新建会话后附件失败。
    可能影响:重新出现孤儿会话,持久化一致性退化。
    建议修复:把“回滚新建会话”下沉为 Store 能力接口(如 DeleteSession(ctx,id)),由所有实现统一支持。
  • internal/runtime/input_prepare.go:81internal/runtime/input_prepare.go:86
    问题描述:asset_save_failed 事件使用 input.SessionID,新建会话后失败场景会丢失真实会话上下文(常为空)。
    触发条件:SessionID 为空触发新建会话,随后附件保存失败。
    可能影响:事件追踪与状态恢复难度上升,观测面上看不到实际受影响会话。
    建议修复:在 AssetSaveError 增加 SessionID 字段并由 session preparer 回填;runtime 失败事件优先使用该字段。

Low

  • internal/tui/core/app/update.go:738internal/tui/core/app/update.go:965
    问题描述:会话刷新会清空运行态,但运行中事件分发未按 RunID/SessionID 做隔离过滤,存在跨会话污染的潜在窗口。
    触发条件:需要补充验证。
    需要补充验证步骤:

    1. 运行中触发会话切换(或任何调用 refreshMessages 的路径)。
    2. 观察旧 run 后续 tool/assistant/error 事件是否仍写入当前会话 UI。
    3. 验证 pendingPermissionToolStatesExecutionError 是否被旧 run 事件覆盖。
      可能影响:UI 展示与真实会话状态偏离。
      建议修复:在 handleRuntimeEvent 前增加 RunID/SessionID 关联校验,并对“非当前 run 事件”做丢弃或旁路记录。
  • 可测试性缺口:

    • internal/session/input_preparer_test.go:缺“相对路径按 workdir 解析”测试、缺“declared MIME 与文件头冲突”测试。
    • internal/tui/core/app/input_features_test.go:缺“@image: 带空格/引号路径”测试。
    • internal/provider/openaicompat/chatcompletions/provider_test.go:缺“归一化后 required/组合约束语义不漂移”测试。

View job run

@xgopilot 修复这些问题,完成后再提高测试覆盖率,然后提PR到Fork仓库(而非直接提交到当前PR)

@fennoai

fennoai Bot commented Apr 17, 2026

Copy link
Copy Markdown
Contributor

完成情况

已按触发评论要求完成修复、补测并推送到 fork 分支,且已在 fork 仓库创建新 PR:

关键修复

  • internal/session/input_preparer.go

  • 附件路径统一按 session.Workdir 解析;强制 ensurePathWithinBase(workdir, path),阻止越界读取。

  • MIME 校验改为“文件头探测为准 + declared MIME 一致性校验”,不再直接信任 declared/扩展名。

  • AssetSaveError 新增 SessionID,失败事件可回填真实会话。

  • 回滚从 *JSONStore 类型断言改为 Store.DeleteSession 能力接口。

  • internal/session/store.go

  • Store 新增 DeleteSession(ctx,id)JSONStore 实现统一删除能力。

  • internal/runtime/input_prepare.go

  • asset_save_failed 事件优先使用 AssetSaveError.SessionID,修复新建会话失败场景的会话上下文丢失。

  • internal/provider/openaicompat/chatcompletions/request.go

  • schema 归一化不再删除顶层 oneOf/anyOf/allOf/enum/not,避免语义漂移。

  • 仅对非 object 顶层做最小兼容降级,并标记 x-neocode-schema-downgraded=true

  • internal/tui/core/app/input_features.go

  • @image: 解析改为扫描器,支持引号路径(含空格),并保留原文本空白布局。

  • internal/tui/core/app/update.go

  • 新增事件过滤:按当前 SessionID/RunID 丢弃不匹配 runtime 事件,降低跨会话污染窗口。

测试与验证

  • 新增/更新测试覆盖:

  • internal/session/input_preparer_test.go

  • internal/runtime/input_prepare_test.go

  • internal/provider/openaicompat/chatcompletions/provider_test.go

  • internal/tui/core/app/input_features_test.go

  • internal/tui/core/app/update_runtime_events_test.go

  • 以及因 Store 接口扩展同步更新的 runtime store stub tests

  • 已执行并通过:

  • go test ./...

  • go build ./...

未完成项

View job run

xgopilot and others added 2 commits April 17, 2026 08:35
…olation

Generated with [codeagent](https://github.com/qbox/codeagent)
Co-authored-by: phantom5099 <245659304+phantom5099@users.noreply.github.com>
fix: harden multimodal input safety and event isolation
@phantom5099

Copy link
Copy Markdown
Collaborator Author

/code 提高测试覆盖率

xgopilot and others added 2 commits April 17, 2026 08:54
Generated with [codeagent](https://github.com/qbox/codeagent)
Co-authored-by: phantom5099 <245659304+phantom5099@users.noreply.github.com>
@phantom5099

Copy link
Copy Markdown
Collaborator Author

/review

@fennoai fennoai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Found 1 issue that is likely to cause stale session assets on failed multi-image input preparation.

mimeType := strings.TrimSpace(image.MimeType)

meta, err := p.saveImageAsset(ctx, session.ID, session.Workdir, path, mimeType)
if err != nil {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When a later image fails to save, previously saved images in the same Prepare call are not cleaned up for existing sessions. rollbackCreatedSession only removes newly created sessions, so this leaves orphan asset files/metadata that are never referenced by any message. Consider making the save loop atomic for existing sessions as well (e.g., track saved asset IDs and delete on failure, or validate all inputs before persisting).

@fennoai fennoai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Completed review of the PR diff. One compatibility risk is worth addressing before relying on this behavior across providers.

typeName, _ := normalized["type"].(string)
if strings.TrimSpace(strings.ToLower(typeName)) != "object" {
normalized["type"] = "object"
normalized["x-neocode-schema-downgraded"] = true

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Setting x-neocode-schema-downgraded inside the tool schema can itself trigger request rejection on strict schema validators (some providers only allow known JSON-Schema keywords in function parameters). Consider tracking this downgrade flag outside the outbound schema payload (e.g., internal log/metric) to avoid introducing a non-standard key into parameters.

@fennoai fennoai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Focused review found two noteworthy issues in the new input-normalization/schema-compat paths.

Comment thread internal/tui/core/app/update.go Outdated
if !a.shouldHandleRuntimeEvent(event) {
return false
}
if a.state.ActiveSessionID == "" {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ActiveSessionID is now initialized from any first runtime event. This can bind UI state to an invalid session ID in failure paths. Example: draft run with image input -> Prepare creates a new session -> image save fails -> session is rolled back/deleted -> asset_save_failed still carries that transient session ID -> this line sets ActiveSessionID to a deleted session. Subsequent UI flows operate on a non-existent session. Consider only adopting session ID from stable events (e.g. user_message/run-context after successful run start) or verifying existence before assignment.

typeName, _ := normalized["type"].(string)
if strings.TrimSpace(strings.ToLower(typeName)) != "object" {
normalized["type"] = "object"
normalized["x-neocode-schema-downgraded"] = true

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This injects x-neocode-schema-downgraded into the schema sent to upstream providers. That marker is internal-only metadata but is now part of the external tool schema payload, which can trip strict schema validators and reintroduce 400s in compatibility-sensitive providers. Suggest keeping downgrade telemetry out-of-band (logs/metrics) and not mutating outgoing schema with custom keys.

@fennoai fennoai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

发现 1 个需要关注的问题(见行内评论)。其余改动在本地回归测试 go test ./... 通过,未发现额外阻断项。

return session, false, nil
}

session.Workdir = resolved

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

loadOrCreateSession 在已有会话上会先持久化 session.Workdir,后续若图片落盘失败(Prepare 返回 AssetSaveError)则不会回滚该变更。这样一次失败提交会产生“会话工作目录已变更但消息未提交”的部分提交状态。建议把 workdir 更新延后到附件全部保存成功之后,或在失败路径恢复原 workdir。

@fennoai fennoai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consolidated review after deduplicating findings across analyzers. The comments below are the issues I consider actionable.

mimeType := strings.TrimSpace(image.MimeType)

meta, err := p.saveImageAsset(ctx, session.ID, session.Workdir, path, mimeType)
if err != nil {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When Prepare processes multiple images for an existing session, a failure on a later image returns early but does not clean up already-saved assets from earlier iterations. This can leave orphaned asset files/metadata that are never referenced by any message. Consider tracking newly saved asset IDs and deleting them on error for non-new sessions too.

return session, false, nil
}

session.Workdir = resolved

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

loadOrCreateSession persists session.Workdir before image normalization/saving completes. If Prepare later fails (e.g., invalid image path/MIME), input submission fails but session workdir still changes, creating a partial commit. Consider deferring this save until the full prepare succeeds, or restoring the previous workdir on failure.

Comment thread internal/tui/core/app/update.go Outdated
if !a.shouldHandleRuntimeEvent(event) {
return false
}
if a.state.ActiveSessionID == "" {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ActiveSessionID is set from the first accepted runtime event when currently empty. In failure paths where a temporary newly-created session is rolled back (e.g., asset save failure), UI can bind to a session ID that no longer exists. Consider only binding ActiveSessionID from events that guarantee persisted session state (or validate existence before assignment).

typeName, _ := normalized["type"].(string)
if strings.TrimSpace(strings.ToLower(typeName)) != "object" {
normalized["type"] = "object"
normalized["x-neocode-schema-downgraded"] = true

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Injecting x-neocode-schema-downgraded into tool parameters modifies the schema sent to upstream provider. Some strict validators reject unknown/custom keywords at schema root, so this may reintroduce request rejection risks. Safer option: keep compatibility markers in logs/telemetry instead of request payload schema.

@phantom5099

Copy link
Copy Markdown
Collaborator Author

/simplify

@fennoai

fennoai Bot commented Apr 17, 2026

Copy link
Copy Markdown
Contributor

处理结果

已检查评论日志中的未解决 review 意见,并完成值得修复的问题落地。

已修复项

  • internal/session/input_preparer.go

  • existing session 场景下,附件保存循环失败时会清理本次已保存资产,避免残留孤儿附件。

  • existing session 的 workdir 变更改为 Prepare 成功后再持久化,避免失败时部分提交。

  • internal/session/store.go

  • 新增 DeleteAsset(ctx, sessionID, assetID),用于归一化失败回收附件文件与元数据。

  • internal/provider/openaicompat/chatcompletions/request.go

  • 移除外发 tool schema 中的 x-neocode-schema-downgraded 自定义键,避免严格 schema 校验场景下的 400 风险。

  • internal/tui/core/app/update.go

  • 不再从任意 runtime 首事件初始化 ActiveSessionID

  • 仅在稳定事件(user_message / run_context)中更新 ActiveSessionID,避免失败路径绑定到临时已回滚会话。

测试更新

  • internal/session/input_preparer_test.go

  • 新增 existing session 失败清理附件测试。

  • 新增 existing session 在 Prepare 失败时不持久化 workdir 变更测试。

  • internal/session/asset_store_test.go

  • 新增 DeleteAsset 行为测试。

  • internal/provider/openaicompat/chatcompletions/provider_test.go

  • 更新断言,验证降级 schema 不再携带自定义 marker。

  • internal/tui/core/app/update_runtime_events_test.go

  • 更新事件路由测试,覆盖“非稳定事件不绑定 session”与“稳定事件绑定 session”。

验证

  • 已执行:gofmt -w(相关改动文件)
  • 已执行:go test ./...(通过)

code-simplifier 执行情况

按要求调用了 code-simplifier 子代理 2 次,但均因上游 502 Bad Gateway 失败;随后已在主线程完成等价的本地简化审视并保留最小改动。

提交与推送

  • Commit: 2b90cf9
  • Branch: fork-pr-337-1776410957
  • 已推送到 fork 远端同名分支。

View job run

Generated with [codeagent](https://github.com/qbox/codeagent)
Co-authored-by: phantom5099 <245659304+phantom5099@users.noreply.github.com>
@pionxe

pionxe commented Apr 17, 2026

Copy link
Copy Markdown
Collaborator

@xgopilot 仔细审查代码变更。你的核心任务是寻找内存状态污染、并发阻塞、Context 泄漏以及未处理的文件 I/O 异常。不要在简单的拼写或常规语法上浪费精力,直击架构重构带来的副作用。

Review Checkpoints (重点审查清单)

  1. Schema 引用污染 (Deep Copy Trap) - 🌟 极高危:

    • 定位 internal/provider/openaicompat/chatcompletions/request.go
    • 检查在“移除顶层 oneOf/anyOf”等操作时,是否对原 schema 进行了深拷贝 (Deep Copy)?如果代码仅仅是浅拷贝了顶层 Map,或者直接修改了传入的对象,请立刻作为 Blocker 报错,因为这会污染全局工具注册表。
  2. 阻塞与 Context 取消 (Concurrency & Context):

    • 检查 internal/runtime/runtime.go 中的 Submit 函数。内部涉及到 input_preparer 的图片落盘 I/O,这个过程是否响应了 ctx.Done()
    • 检查 internal/tui/core/app/update.go 中的 RunSubmitCmd。这个调用必须是异步非阻塞的(例如包裹在 Goroutine 或 tea.Cmd 中),绝不能阻塞 TUI 的主事件循环(Update 函数)。
  3. 优雅降级与错误透传 (Error Handling & User Experience):

    • 既然 TUI 移除了对模型图片能力的预先阻断,请检查 Runtime 或 Provider 层:如果向不支持视觉的模型发送了图片,是否返回了带有明确语义的 error(而不是底层的索引越界或 nil pointer)?TUI 是否能正常捕获并渲染这个失败事件?
    • 检查 internal/config/provider_loader.go 中的 MkdirAll 操作。如果目录创建失败(如权限不足),函数是否返回了错误并被上层优雅处理(例如 fallback 到无自定义 provider),还是会导致整个应用 panic/fatal 崩溃?
  4. 事件总线安全性 (Event Observability):

    • PR 新增了 input_normalized, asset_saved 等事件。检查这些事件发送逻辑是否会阻塞?如果 TUI 消费端处理不过来,Runtime 侧不能因为发射事件而被卡死。

Output Format

请以 Markdown 格式输出你的审查报告。结构如下:

  1. 🛑 致命隐患 (Blockers): 指出 Schema 浅拷贝污染、TUI 主线程阻塞、启动时 Panic 等严重缺陷的具体代码行,并给出修复建议。
  2. ⚠️ 体验与健壮性问题 (Suggestions): 指出错误提示不友好、Context 未正确传播到 I/O 层等健壮性瑕疵。
  3. ✅ 架构重构亮点 (Passed): 简要肯定 PR 中解耦彻底、单一职责落实得好的代码段。

如果你在某一项中未发现问题,请明确回复“针对 [第 X 项],代码已安全处理”。

@fennoai

fennoai Bot commented Apr 17, 2026

Copy link
Copy Markdown
Contributor

🛑 致命隐患 (Blockers)

  1. Submit 前置事件发射存在无限阻塞窗口(会卡死提交线程)
  • 位置:internal/tui/services/runtime_service.go:66internal/runtime/input_prepare.go:54internal/runtime/input_prepare.go:63internal/runtime/input_prepare.go:89internal/runtime/event_emitter.go:65
  • 问题:RunSubmitCmdcontext.Background()runtime.SubmitSubmit -> PrepareUserInput 在进入 Run 前就会发 input_normalized/asset_saved/asset_save_faileddeliverEvent 在通道满时会阻塞到 ctx.Done();但此时 ctx 不可取消,且还没进入 RunCancelActiveRun 尚不可用),存在“通道满即永久阻塞”的死锁面。
  • 修复建议:
    • RunSubmitCmd 传入可取消 ctx(至少绑定 TUI 生命周期或超时)。
    • 对 Prepare 阶段事件改为限时发送(context.WithTimeout)或 best-effort 丢弃策略。
    • 或将“可取消 run token”注册前移到 Prepare 前,确保用户取消可打断阻塞发送。

⚠️ 体验与健壮性问题 (Suggestions)

  1. 图片落盘链路对 ctx.Done() 响应不充分(长 I/O 期间取消不及时)
  • 位置:internal/session/input_preparer.go:169internal/session/input_preparer.go:217internal/session/store.go:309
  • 问题:Prepare 虽在入口检查了 ctx.Err(),但后续 os.Open、文件头读取、io.Copy 写入附件过程中没有周期性取消检查,取消信号可能要等到 I/O 返回后才生效。
  • 建议:在 saveImageAsset/SaveAsset 的关键 I/O 前后补 ctx.Err() 检查;对 io.Copy 使用带 ctx 的 reader 包装以支持中断。
  1. 自定义 provider 目录读取失败会直接阻断启动,缺少降级路径
  • 位置:internal/config/provider_loader.go:65internal/config/provider_loader.go:70internal/config/loader.go:123
  • 问题:loadCustomProviders 读取目录失败直接返回错误,上层 Loader.Load 直接失败。虽然不会 panic,但会导致应用启动失败,未实现“忽略自定义 provider 并继续启动”的优雅降级。
  • 建议:对权限/读取异常做可配置降级(记录 warning,回退为空自定义 provider)。

✅ 架构重构亮点 (Passed)

  • 针对 第 1 项(Schema 引用污染),代码已安全处理:normalizeToolSchemaForOpenAI 先执行顶层复制(cloneSchemaTopLevel),并且当前仅修改顶层键,不会回写污染调用方原 schema(internal/provider/openaicompat/chatcompletions/request.go:83:107)。
  • 针对 第 2 项中的 TUI 主线程阻塞,代码已安全处理:RunSubmitCmd 返回 tea.Cmd 异步执行,不在 Update 主循环内直接阻塞调用 runtime(internal/tui/services/runtime_service.go:64)。
  • 针对 第 3 项中的视觉不支持模型错误透传,代码已安全处理:provider 4xx 会被 ParseError 解析成语义化错误并上抛,TUI runFinishedMsg 分支可渲染失败状态,未见 nil pointer/index 这类崩溃路径(internal/provider/openaicompat/chatcompletions/provider.go:70internal/provider/openaicompat/chatcompletions/request.go:297internal/tui/core/app/update.go:93)。
  • 针对 第 4 项(事件可观测),新增事件类型与 TUI handler 已接通;但如上 Blocker 所述,发送侧仍存在“通道满 + 不可取消 ctx”阻塞风险。

View job run

@phantom5099

Copy link
Copy Markdown
Collaborator Author

🛑 致命隐患 (Blockers)

  1. Submit 前置事件发射存在无限阻塞窗口(会卡死提交线程)
  • 位置:internal/tui/services/runtime_service.go:66internal/runtime/input_prepare.go:54internal/runtime/input_prepare.go:63internal/runtime/input_prepare.go:89internal/runtime/event_emitter.go:65

  • 问题:RunSubmitCmdcontext.Background()runtime.SubmitSubmit -> PrepareUserInput 在进入 Run 前就会发 input_normalized/asset_saved/asset_save_faileddeliverEvent 在通道满时会阻塞到 ctx.Done();但此时 ctx 不可取消,且还没进入 RunCancelActiveRun 尚不可用),存在“通道满即永久阻塞”的死锁面。

  • 修复建议:

    • RunSubmitCmd 传入可取消 ctx(至少绑定 TUI 生命周期或超时)。
    • 对 Prepare 阶段事件改为限时发送(context.WithTimeout)或 best-effort 丢弃策略。
    • 或将“可取消 run token”注册前移到 Prepare 前,确保用户取消可打断阻塞发送。

⚠️ 体验与健壮性问题 (Suggestions)

  1. 图片落盘链路对 ctx.Done() 响应不充分(长 I/O 期间取消不及时)
  • 位置:internal/session/input_preparer.go:169internal/session/input_preparer.go:217internal/session/store.go:309
  • 问题:Prepare 虽在入口检查了 ctx.Err(),但后续 os.Open、文件头读取、io.Copy 写入附件过程中没有周期性取消检查,取消信号可能要等到 I/O 返回后才生效。
  • 建议:在 saveImageAsset/SaveAsset 的关键 I/O 前后补 ctx.Err() 检查;对 io.Copy 使用带 ctx 的 reader 包装以支持中断。
  1. 自定义 provider 目录读取失败会直接阻断启动,缺少降级路径
  • 位置:internal/config/provider_loader.go:65internal/config/provider_loader.go:70internal/config/loader.go:123
  • 问题:loadCustomProviders 读取目录失败直接返回错误,上层 Loader.Load 直接失败。虽然不会 panic,但会导致应用启动失败,未实现“忽略自定义 provider 并继续启动”的优雅降级。
  • 建议:对权限/读取异常做可配置降级(记录 warning,回退为空自定义 provider)。

✅ 架构重构亮点 (Passed)

  • 针对 第 1 项(Schema 引用污染),代码已安全处理:normalizeToolSchemaForOpenAI 先执行顶层复制(cloneSchemaTopLevel),并且当前仅修改顶层键,不会回写污染调用方原 schema(internal/provider/openaicompat/chatcompletions/request.go:83:107)。
  • 针对 第 2 项中的 TUI 主线程阻塞,代码已安全处理:RunSubmitCmd 返回 tea.Cmd 异步执行,不在 Update 主循环内直接阻塞调用 runtime(internal/tui/services/runtime_service.go:64)。
  • 针对 第 3 项中的视觉不支持模型错误透传,代码已安全处理:provider 4xx 会被 ParseError 解析成语义化错误并上抛,TUI runFinishedMsg 分支可渲染失败状态,未见 nil pointer/index 这类崩溃路径(internal/provider/openaicompat/chatcompletions/provider.go:70internal/provider/openaicompat/chatcompletions/request.go:297internal/tui/core/app/update.go:93)。
  • 针对 第 4 项(事件可观测),新增事件类型与 TUI handler 已接通;但如上 Blocker 所述,发送侧仍存在“通道满 + 不可取消 ctx”阻塞风险。

View job run

@xgopilot 修复这些问题

@fennoai

fennoai Bot commented Apr 17, 2026

Copy link
Copy Markdown
Contributor

已完成

已按触发评论要求修复 3 个问题并推送到当前工作分支:fork-pr-337-1776410957

  1. 修复 Submit 准备阶段事件阻塞窗口
  • 文件:internal/runtime/input_prepare.go
  • 改动:新增 emitPrepareEvent,在 Prepare 阶段事件发送使用限时上下文(200ms)+ best-effort,通道满时不再无限阻塞提交链路。
  • 影响事件:input_normalizedasset_savedasset_save_failederror
  1. 补齐图片落盘链路取消响应
  • 文件:internal/session/input_preparer.gointernal/session/store.go
  • 改动:
    • saveImageAsset、MIME 探测链路关键 I/O 前后补 ctx.Err() 检查。
    • resolveImageMimeType / detectImageMimeTypeFromFile 传递 ctx,取消时尽快返回。
    • JSONStore.SaveAsset 引入 contextReaderio.Copy 过程中按读取周期感知取消,并在复制后/落盘后增加取消检查与清理。
  1. 自定义 provider 目录读取失败降级
  • 文件:internal/config/provider_loader.go
  • 改动:loadCustomProviders 在 providers 目录读取失败(非不存在)时降级为空列表,不再阻断启动。

测试

已补充并更新测试:

  • internal/runtime/input_prepare_test.go
    • 新增:事件队列满时 PrepareUserInput 不阻塞。
  • internal/session/asset_store_test.go
    • 新增:SaveAsset 复制过程中上下文取消返回 context.Canceled
  • internal/config/loader_test.go
    • 更新:providers 目录读取失败场景改为降级断言。

已执行:go test ./...(通过)。

推送与 PR

未做事项

  • 未扩展触发评论之外的改造(如 schema 语义保真策略重设计、TUI 新交互能力设计);本次仅实现你在触发评论中要求修复的项。

View job run

xgopilot and others added 3 commits April 17, 2026 11:56
… cancel-safe asset flow

Generated with [codeagent](https://github.com/qbox/codeagent)
Co-authored-by: phantom5099 <245659304+phantom5099@users.noreply.github.com>
fix(runtime/session/config): resolve submit-event blocking and cancellation gaps
@phantom5099 phantom5099 merged commit 4d49d5e into 1024XEngineer:main Apr 17, 2026
1 of 2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants