Skip to content

refactor(tui): 删除 & <command> 工作区命令旁路#421

Merged
phantom5099 merged 7 commits into1024XEngineer:mainfrom
phantom5099:main
Apr 23, 2026
Merged

refactor(tui): 删除 & <command> 工作区命令旁路#421
phantom5099 merged 7 commits into1024XEngineer:mainfrom
phantom5099:main

Conversation

@phantom5099
Copy link
Copy Markdown
Collaborator

Summary

本 PR 删除 TUI 中 & <command> 的工作区命令执行能力,不再允许通过聊天输入框直接触发本地 shell 执行。

删除后,& ... 不再被识别为特殊命令入口,而是按普通文本输入处理。NeoCode 内部不再保留这条绕过主链的 TUI shell 旁路,用户如需自由执行命令,应使用外部真实终端。

Background

当前 & <command> 走的是一条独立执行链路:

TUI input
  -> runWorkspaceCommand
  -> RunWorkspaceCommandCmd
  -> DefaultWorkspaceCommandExecutor
  -> exec.CommandContext

这条链路与现有 TUI / Runtime / ToolManager 主链完全分叉,带来几个问题:

  • 直接绕过统一运行体系
  • 额外维护一套 workspace command 专属消息协议和状态文案
  • README、菜单提示、测试里都要为这条旁路单独兜一层
  • 语义不清晰:既不是明确的 terminal passthrough,也不是受控 tool 调用

本次改动选择直接删除该能力,而不是继续保留中间态。

Changes

Removed command entry

  • 删除 & <command> 的输入识别与解析逻辑
  • 删除 Enter 提交时的 workspace command 特殊分支
  • 删除 workspace command 的 activity/status 更新逻辑

Removed execution path

  • 删除 RunWorkspaceCommandCmd
  • 删除 WorkspaceCommandResultMsg
  • 删除 DefaultWorkspaceCommandExecutor
  • 删除 internal/tui/infra/workspace_exec.go

Removed UI and documentation residue

  • 删除 command menu 中的 & <command> 建议项
  • 删除相关状态文案
  • 删除 README 中的 & <command> 使用说明

Removed obsolete tests

  • 删除所有围绕 workspace command 的单元测试
  • 删除对 shell 参数包装、输出清洗、专属结果消息的测试残留
  • 保留其余 TUI 输入、文件引用、图片引用、local slash command 测试不变

Behavior Change

Before

& git status

会被 TUI 当作本地工作区命令执行。

After

& git status

会被当作普通文本输入处理,不再触发本地命令执行。

Validation

已确认:

  • & <command> 相关旧符号、旧文案、旧测试残留已清理
  • go test -run '^$' ./... 通过,说明当前改动下全仓编译通过
  • 全仓搜索未发现 RunWorkspaceCommandCmdWorkspaceCommandResultMsgDefaultWorkspaceCommandExecutor& <command> 等残留引用

说明:

  • go test ./... 目前仍存在仓库既有的 Windows 权限问题,错误集中为:
    • config: write config file: sync target directory: ... Access is denied
  • 该问题与本 PR 删除 & <command> 无直接关系,本 PR 未引入新的测试失败类型

Risk

这是一个显式行为变更:

  • 依赖 & <command> 的用户工作流会失效
  • 但这是有意收敛,不保留兼容入口或 fallback

Notes

本 PR 不尝试:

  • & <command> 迁移到 ToolManager
  • 提供新的 shell passthrough 语法
  • 增加 terminal mode、! 模式或兼容提示

若后续仍需要“用户在 NeoCode 内直接运行命令”的能力,应以独立产品语义重新设计,而不是恢复这条旧旁路。

@codecov
Copy link
Copy Markdown

codecov Bot commented Apr 23, 2026

Codecov Report

❌ Patch coverage is 81.63265% with 9 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
internal/session/storage_helpers.go 71.87% 6 Missing and 3 partials ⚠️

📢 Thoughts on this report? Let us know!

Copy link
Copy Markdown

@fennoai fennoai Bot left a comment

Choose a reason for hiding this comment

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

Completed 4-pass review (code quality, performance, security, documentation accuracy).

I found one noteworthy regression risk around default config directory resolution when HOME is set to a relative path.

Comment thread internal/config/loader.go
func defaultBaseDir() string {
home, err := os.UserHomeDir()
if err != nil {
home := strings.TrimSpace(os.Getenv("HOME"))
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

defaultBaseDir now prefers raw HOME over os.UserHomeDir(). If HOME is set but relative (for example . in some shells/CI wrappers), the default path becomes relative (./.neocode) and silently drifts from the documented home-directory behavior. Consider accepting HOME only when absolute (or normalize and validate), otherwise fall back to os.UserHomeDir().

func defaultBaseDir() string {
home, err := os.UserHomeDir()
if err != nil || strings.TrimSpace(home) == "" {
home := strings.TrimSpace(os.Getenv("HOME"))
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Same default-base-dir regression risk as runtime config loader: using raw HOME without absolute-path validation can redirect migration target to a relative directory unexpectedly. Keeping script behavior aligned with loader is good, but both should validate HOME (absolute + non-empty) before trusting it.

@phantom5099
Copy link
Copy Markdown
Collaborator Author

@xgopilot 批判性review整个PR,分析所有潜在风险点,找出所有漏洞,确保每个地方的修改不是退步的,方案上的设计也不能碎片化,更不能妥协式兼容或兜底旧代码,不许输出正向表扬

@fennoai
Copy link
Copy Markdown

fennoai Bot commented Apr 23, 2026

Findings

  1. High: HOME 被直接信任,可能把默认配置目录重定向到相对路径或非预期位置,造成配置读写漂移与潜在安全面扩大。

    • internal/config/loader.go:176 现在优先用 os.Getenv("HOME"),未校验是否为绝对路径。HOME="." 时默认路径会落到 ./.neocode
    • scripts/migrate_context_budget/main.go:52 同步引入同类行为,迁移脚本可能在错误目录执行并修改错误目标。
  2. Medium: PR 主题为删除 TUI & 旁路,但混入了 config/session/script 语义变更,导致变更集碎片化、回滚颗粒度变差。

    • 涉及 internal/config/*internal/session/*scripts/migrate_context_budget/main.go。这些改动不属于“删除 TUI 旁路”最小闭环,且会扩大发布风险面。
  3. Medium: 缺少“& 作为普通文本发送”的回归测试,行为变更未被测试锁定。

    • internal/tui/core/app/update_test.gointernal/tui/core/app/command_menu_test.go 删除了 workspace command 相关用例,但没有新增断言:输入 & git status 必须走普通消息发送链路(而非本地执行)。未来重构时可能无感回退。
  4. Medium: 新增分支覆盖不足,且关键路径是安全/路径解析逻辑。

    • internal/session/storage_helpers.go 新增 canFallbackToAbsolutePathOnPermission 与 permission fallback 分支;
    • internal/config/atomic_write.go 新增 Windows 分支(runtime.GOOS == "windows")直接跳过目录 fsync;
    • 当前补测不足(Codecov 已提示多文件低 patch coverage),这些分支一旦回归会直接影响配置持久化与路径约束安全边界。

Open Questions

  1. 是否接受本 PR 同时携带 config/session/script 行为变更?如果目标仅是移除 TUI & 旁路,建议拆 PR,避免发布时无法独立回滚。
  2. HOME 的期望语义是否是“仅当绝对路径时才采信”?若是,需要在 loader 与脚本两处统一校验规则并补测试。

View job run

@phantom5099
Copy link
Copy Markdown
Collaborator Author

Findings

  1. High: HOME 被直接信任,可能把默认配置目录重定向到相对路径或非预期位置,造成配置读写漂移与潜在安全面扩大。

    • internal/config/loader.go:176 现在优先用 os.Getenv("HOME"),未校验是否为绝对路径。HOME="." 时默认路径会落到 ./.neocode
    • scripts/migrate_context_budget/main.go:52 同步引入同类行为,迁移脚本可能在错误目录执行并修改错误目标。
  2. Medium: PR 主题为删除 TUI & 旁路,但混入了 config/session/script 语义变更,导致变更集碎片化、回滚颗粒度变差。

    • 涉及 internal/config/*internal/session/*scripts/migrate_context_budget/main.go。这些改动不属于“删除 TUI 旁路”最小闭环,且会扩大发布风险面。
  3. Medium: 缺少“& 作为普通文本发送”的回归测试,行为变更未被测试锁定。

    • internal/tui/core/app/update_test.gointernal/tui/core/app/command_menu_test.go 删除了 workspace command 相关用例,但没有新增断言:输入 & git status 必须走普通消息发送链路(而非本地执行)。未来重构时可能无感回退。
  4. Medium: 新增分支覆盖不足,且关键路径是安全/路径解析逻辑。

    • internal/session/storage_helpers.go 新增 canFallbackToAbsolutePathOnPermission 与 permission fallback 分支;
    • internal/config/atomic_write.go 新增 Windows 分支(runtime.GOOS == "windows")直接跳过目录 fsync;
    • 当前补测不足(Codecov 已提示多文件低 patch coverage),这些分支一旦回归会直接影响配置持久化与路径约束安全边界。

Open Questions

  1. 是否接受本 PR 同时携带 config/session/script 行为变更?如果目标仅是移除 TUI & 旁路,建议拆 PR,避免发布时无法独立回滚。
  2. HOME 的期望语义是否是“仅当绝对路径时才采信”?若是,需要在 loader 与脚本两处统一校验规则并补测试。

View job run  查看作业运行

@xgopilot 修复

@fennoai
Copy link
Copy Markdown

fennoai Bot commented Apr 23, 2026

已按本线程 4 条 findings 完成修复并推送到当前 PR 分支 phantom5099:main(commit: defab26)。

修复内容

  • 修复 HOME 直接信任问题:
    • internal/config/loader.godefaultBaseDir() 仅采信绝对路径 HOME,否则回退 os.UserHomeDir(),若仍非绝对路径则回退 dirName
    • scripts/migrate_context_budget/main.go:同样仅采信绝对路径 HOME,否则回退到安全默认。
  • 新增回归测试,锁定 & 行为:
    • internal/tui/core/app/update_test.go 新增 TestUpdateSendAmpersandInputAsPlainText,断言 & git status 走普通发送链路并原样进入 runtime prepare input。
  • 补齐分支覆盖:
    • internal/config/atomic_write.go 引入可测的 atomicGOOS,并新增 internal/config/atomic_write_test.go 覆盖 Windows noop 分支与非 Windows 分支。
    • internal/session/helpers_test.go 新增 parent permission fallback 的允许/拒绝分支测试。
  • HOME 相关测试补充:
    • internal/config/loader_test.go
    • scripts/migrate_context_budget/main_test.go

验证结果

  • 定向测试通过:
    • go test ./internal/config -run 'Test(DefaultBaseDir|FsyncDirectory|Loader)'
    • go test ./internal/session -run 'TestResolvePathForContainment|TestStorageHelpers'
    • go test ./internal/tui/core/app -run 'TestUpdateSendAmpersandInputAsPlainText|TestUpdateSendWithImageAttachmentsUsesPreparePipeline'
    • go test ./scripts/migrate_context_budget -run 'TestDefaultBaseDir'
  • 全量测试通过:go test ./...

未做项

  • “是否拆分 PR 以消除变更集碎片化” 属于提交流程决策,本次未改写历史;当前仅补齐风险修复与回归测试。

View job run

- enforce absolute HOME for default config base dir resolution
- add ampersand plain-text send regression test in TUI
- add branch coverage for session permission fallback and atomic fsync

Generated with [codeagent](https://github.com/qbox/codeagent)
Co-authored-by: phantom5099 <245659304+phantom5099@users.noreply.github.com>
@phantom5099 phantom5099 merged commit 8a1c949 into 1024XEngineer:main Apr 23, 2026
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.

2 participants