Skip to content

fix(cosh-ng): preserve raw entry - #2743

Open
Zhilinlinlin wants to merge 1 commit into
mainfrom
fix/cosh-ng-2739-raw-entry
Open

fix(cosh-ng): preserve raw entry#2743
Zhilinlinlin wants to merge 1 commit into
mainfrom
fix/cosh-ng-2739-raw-entry

Conversation

@Zhilinlinlin

@Zhilinlinlin Zhilinlinlin commented Aug 21, 2026

Copy link
Copy Markdown
Collaborator

Why

The /usr/bin/cosh transparency classifier treated the explicit raw TUI subcommand as an unknown token and delegated it to bash. Bash then tried to execute raw as a script and exited 127.

What changed

Route raw through its existing legacy invocation normalizer before the wrapper classifier. The raw -c … and raw -- … passthrough forms remain shell-transparent; all other raw shapes start the TUI and pass normalized launch arguments to the bootstrap.

Related issue

closes #2739

User / Agent impact

/usr/bin/cosh raw <adapter> again launches the same TUI path as the direct cosh-shell raw <adapter> entry instead of failing with exit 127.

Risk and compatibility

  • Public CLI, API, configuration, or documented behavior changed
  • Privileged or security-sensitive behavior changed
  • Cross-component contract changed
  • Migration or rollback guidance is needed

Restores the pre-regression raw wrapper contract while retaining the non-interactive passthrough behavior.

Validation

  • cargo test -p cosh-shell runtime::invocation (22 passed)
  • crates/cosh-shell/scripts/check-layout.sh
  • cargo fmt --all -- --check
  • cargo clippy -p cosh-shell --all-targets -- -D warnings
  • git diff --check

Documentation and rollback

No documentation changes. Revert 1d3c374c5 to restore the current wrapper behavior.

@Zhilinlinlin
Zhilinlinlin requested a review from SunnyQjm as a code owner August 21, 2026 07:06
@github-actions github-actions Bot added the component:cosh-ng src/cosh-ng label Aug 21, 2026
@Zhilinlinlin
Zhilinlinlin force-pushed the fix/cosh-ng-2739-raw-entry branch from e298e66 to e854c1e Compare August 21, 2026 07:08

@qoderai qoderai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

本次审查范围内未发现需要修改的问题。


🤖 Generated by QoderView workflow run

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: e298e668fa

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

stdout_tty: bool,
stderr_tty: bool,
) -> Invocation {
if args.first().and_then(|arg| arg.to_str()) == Some("raw") {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Register or split the over-threshold invocation file

This branch and its inline tests grow runtime/invocation.rs from 682 to 751 lines without adding it to the large-file inventory or providing a split plan/waiver. Running the required crates/cosh-shell/scripts/check-layout.sh reports this file as an unregistered large production file and exits with one violation group, so the mandated layout gate now fails; split the module or register the required owner plan/waiver.

AGENTS.md reference: src/cosh-ng/AGENTS.md:L53-L59

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Resolved in 1d3c374: moved invocation tests to invocation_tests.rs, so check-layout.sh now passes.

@SunnyQjm

Copy link
Copy Markdown
Collaborator

PR number: #2743
head_sha: e854c1e
reviewed_at: 2026-08-21T07:12:08Z

评审结论

Findings

  • [P3] src/cosh-ng/crates/cosh-shell/src/main.rs:120&args[2..] 用魔数下标表达"classifier 消费了 raw 一个 token"这一知识,与 runtime/invocation.rsraw 分支的判定逻辑形成两处隐式耦合。更稳妥的做法是把 launch args 归一化下沉到 invocation owner(如 TuiEntry 携带已剥离的子命令参数),main.rs 只消费结果。非阻断,可作后续整理。

未发现 blocking package/module/public API 组织问题。

结构核查(对照 code organization 规则)

  • Owner 归属:变更落在 runtime/invocation.rs(runtime owner 的 startup/invocation 分类职责)和 main.rs(argv bootstrap),符合 standard.md §2/§4 的入口层定位;改动仅限 crates/cosh-shell/,无跨 crate 混杂。
  • Root src:未新增 root implementation 文件,main.rs 增量为 argv 切片逻辑,属 bootstrap 范畴。
  • Public API:未触碰 lib.rs,未新增 pub mod/pub useTuiEntry.rawpub(crate),不进入 public API inventory,无分类计数漂移。
  • Self-crate path:未新增 cosh_shell::...
  • 依赖方向:runtime → main 入口消费,未引入 shell_host/adapter/hooks/ui 的 forbidden direction。
  • 测试位置:新增用例挂在 runtime/invocation.rs 既有 inline mod tests 中,延续现状;测试组织是否需外迁以 shell-test-organization 为准,不在本规则判定范围。

剩余风险

  • 行数:runtime/invocation.rs 本次约 +19 行 production、+43 行测试,该文件不在 large-file-inventory 登记中;main.rs 增量极小。是否越过 700/500 阈值以 CI check-layout.sh 实测为准(此处为 diff 增量估算)。
  • 行为面:raw 在非全 TTY 场景(如 stdin 非终端)现在也一律进入 TUI,不再降级为 ExecShell。PR 描述称这是恢复 direct entry 的既有 contract,属预期行为,但脚本化调用方若依赖旧降级路径需知悉。
  • normalize_raw_invocation 递归重分类假定归一化结果不再以 raw 开头;现有 escape-hatch 测试(raw -craw --)覆盖了该前提,无新增风险。

Validation

  • 已通过(PR 自报):cargo test -p cosh-shell runtime::invocation::tests(22 passed)、cargo fmt --checkcargo clippy -D warningsgit diff --check
  • 未跑(本评审不执行工具):check-layout.shinventory-public-api.sh、全量 cargo test -p cosh-shell,建议以 CI 实测收口。

@Zhilinlinlin
Zhilinlinlin force-pushed the fix/cosh-ng-2739-raw-entry branch from e854c1e to 5279cd3 Compare August 21, 2026 07:18
Route explicit raw TUI requests through the wrapper while retaining the legacy
non-interactive passthrough forms.

Fixes: 76b5a76 ("feat(cosh-ng): [shell] invert cosh entry to exec-bash passthrough")

Signed-off-by: Zhilinlinlin <aiyiqi@linux.alibaba.com>
@Zhilinlinlin
Zhilinlinlin force-pushed the fix/cosh-ng-2739-raw-entry branch from 5279cd3 to 1d3c374 Compare August 21, 2026 07:22
@Zhilinlinlin

Copy link
Copy Markdown
Collaborator Author

Addressed the valid P3 in 1d3c374c:

  • TuiEntry now carries normalized launch_args;
  • main.rs consumes that result directly, so it no longer relies on a raw-specific argv slice.

The test split also keeps runtime/invocation.rs below the source-layout threshold.

@KaiLongZhou
KaiLongZhou self-requested a review August 21, 2026 07:45
@KaiLongZhou

Copy link
Copy Markdown
Collaborator

Code Review: fix(cosh-ng): preserve raw entry

总体结论:未发现阻塞问题 ✅

PR 正确修复了 /usr/bin/cosh raw <adapter> 被错误分类为 unknown token 导致 exit 127 的回归。修复方案精准、测试充分、代码组织合理。


修复正确性分析

根因classify_invocation() 的 allowlist scan 不认识 raw 这个 positional token,直接 fall through 到 ExecShell,bash 尝试执行 raw 命令 → exit 127。

修复方案:在 classifier 顶部拦截 raw 子命令:

  1. 先尝试 normalize_raw_invocation() 处理 legacy passthrough(raw -c … / raw -- …
  2. 若 normalize 返回 None(即显式 TUI 请求),直接构造 TuiEntry 并携带 launch_args

验证关键路径

调用形式 预期路径 实际行为
cosh raw cosh-core TUI raw 拦截 → normalize 返回 None → TuiEntry { launch_args: ["cosh-core"] }
cosh raw TUI (默认 adapter) TuiEntry { launch_args: [] }configured_raw_invocation fallback 到 config default
cosh raw -c "echo ok" ExecShell passthrough normalize 返回 ["-c", "echo ok"] → 递归 classify → ExecShell
cosh raw -- echo ok ExecShell passthrough normalize 返回 ["--", "echo ok"] → 递归 classify → ExecShell
cosh raw --shell zsh cosh-core TUI (zsh override) normalize 返回 None → TuiEntry { launch_args: ["--shell", "zsh", "cosh-core"] }
cosh --shell zsh raw cosh-core ExecShell (raw 是 unknown) --shell zsh consumed → raw 触发 fall-through → ExecShell

main.rs 的消费端改动也正确:从 entry.launch_args 获取已剥离 raw token 的参数,直接传给 configured_raw_invocation(),与 cosh-shell raw 直接入口的 &args[2..] 语义一致。


详细检查

1. 架构与代码组织 ✅

  • 修改限于 invocation.rsmain.rs、新增 invocation_tests.rs,均在 runtime/ 下,符合 owner 约定
  • 未新增 root src/*.rs 文件
  • invocation.rs 690 行,低于 700 行阈值
  • TuiEntry 新增 launch_args 字段是合理的结构扩展,没有破坏现有抽象

2. 功能正确性 ✅

  • raw 拦截位于 classifier 函数顶部,在 allowlist scan 之前,确保 raw 不会被误判为 unknown token
  • 递归调用 classify_invocation 是安全的:normalize 后的 args 不会以 raw 开头(raw token 已被剥离),不会无限递归
  • TuiEntry.launch_args 在两个构造路径(raw 拦截 和 正常 TUI fall-through)都正确填充
  • cosh-shell raw 直接入口行为一致:TTY 检查均不做(显式 raw 子命令无条件启动 TUI)

3. 安全性 ✅

  • 无新的安全门变更
  • raw 拦截不涉及命令解析或自动批准逻辑
  • 非 UTF-8 参数仍安全地走 ExecShell 路径(arg.to_str() 返回 None → Some("raw") 不匹配)

4. 测试覆盖 ✅

  • 新增 invocation_tests.rs 独立测试模块,覆盖:
    • raw_subcommand_enters_tui_and_is_marked_for_launch_normalization:多种 raw 参数组合 + 非 TTY 场景
    • raw_non_interactive_escape_hatches_keep_their_legacy_passthrough-c-- passthrough
  • invocation.rs 内联测试迁移了 agent_namespace_builds_gateway_plancosh_entry_matches_only_the_cosh_basename 到新文件,保持测试完整性
  • 现有测试的 TuiEntry 构造全部更新为包含 launch_args 字段
  • 测试分层正确:纯逻辑 unit test 放在 src/ 内的 #[cfg(test)] 模块

5. PR 描述可复现性 ✅


观察(非阻塞)

  1. invocation.rs 接近 700 行阈值:当前 690 行,随着后续 classifier 扩展可能突破。可考虑将 normalize_raw_invocation 及其测试拆分到独立模块,但不阻塞本次合并。

  2. raw 路径不做 TTY 检查:这是有意为之,与 cosh-shell raw 直接入口保持一致(显式子命令无条件启动 TUI)。建议在 classify_invocation 的 doc comment 中补充说明这一设计决策。


LGTM. 修复精准、测试充分、无阻塞问题。

@KaiLongZhou

Copy link
Copy Markdown
Collaborator

Review Check: LGTM ✅

code-reviewer 的 review 分析准确、完整,逐行验证了 6 条行为路径全部正确。经独立代码审核确认:

  • 修复正确性raw 子命令在 classifier 顶部拦截,递归调用安全(normalize 剥离 raw token),TuiEntry.launch_args 双路径填充正确
  • 安全性:非 UTF-8 参数安全跳过 raw 拦截
  • 测试覆盖:新增 2 个测试函数覆盖 TUI 入口 + passthrough escape hatches,现有测试全部更新
  • CI:cosh-ng 相关检查全部通过(Test cosh-ng ✅, fast checks ✅, release build ✅)

未发现遗漏的 P0/P1 问题,可以合并。

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Nightly][cosh-ng] bug: 调用透明合同倒置后 /usr/bin/cosh 不再识别 raw 子命令,cosh raw cosh-core 崩退 exit 127 (followup #2597)

3 participants