Skip to content

fix(cosh-ng): accept quoted dev null as sink - #2710

Open
jfeng18 wants to merge 1 commit into
mainfrom
fix/cosh-ng/quoted-null-sink
Open

fix(cosh-ng): accept quoted dev null as sink#2710
jfeng18 wants to merge 1 commit into
mainfrom
fix/cosh-ng/quoted-null-sink

Conversation

@jfeng18

@jfeng18 jfeng18 commented Aug 20, 2026

Copy link
Copy Markdown
Collaborator

Why

A quoted redirect target such as 2>"/dev/null" was classified as a filesystem write (redirection-write) instead of a null sink, so the approval card showed a misleading reason even though the unquoted form was already classified correctly (#1667).

What changed

  • Fully quoted SAFE_OUTPUT_SINKS targets are now recognized as null redirections.
  • Every other quoted form stays fail-closed (unchanged).
  • apply_null_redirection_policy still resolves to AskUser — noted in a code comment. Layer 2 (auto-approving null redirections) is intentionally out of scope and remains a separate discussion.

Related issue

Refs #1752 (not closing): this PR only fixes the Layer 1 misclassification; the issue's full auto-approve request is not resolved here.

User / Agent impact

The approval-card reason/impact for quoted null redirections now reads as a null redirection instead of redirection-write, aligning with the unquoted behavior from #1667. The execution decision (AskUser) is unchanged.

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

Execution decision is identical before and after (AskUser); only the approval-card reason/impact changes. Low risk.

Validation

  • ECS discriminative two-direction test: misclassification reproduces on pre-fix code, correct classification on fixed code.
  • ECS cargo test: 1342 passed (baseline 1340 + 2 new).
  • cargo fmt and clippy clean with zero warnings.

Documentation and rollback

Single commit — revert it directly.

@jfeng18
jfeng18 requested a review from SunnyQjm as a code owner August 20, 2026 08:10
@github-actions github-actions Bot added the component:cosh-ng src/cosh-ng label Aug 20, 2026

@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

@SunnyQjm

SunnyQjm commented Aug 20, 2026

Copy link
Copy Markdown
Collaborator

PR number: #2710
head_sha: 9bf29dc
reviewed_at: 2026-08-20T08:32:56Z

Findings

  • [P1] CI 实测与 PR 验证声明冲突:Test cosh-ng(08:19:25Z)与 Test cosh-ng fast checks(08:20:04Z)在 head_sha 9bf29dc1 上均为 FAILURE,而 PR body 声称 ECS cargo test 1342 passed。静态评审无法判断失败原因(可能是慢 PTY 集成测试抖动,也可能是本 patch 引入的回归),但 Build cosh-ng release 成功、失败恰好落在被改 crate 的测试门上,合并前必须由作者给出失败日志解释或重跑转绿。
  • [P2] command_risk_parser.rs 本次新增约 74 行(按 diff 增量估算)。该文件未登记于 large-file-inventory,输入中无其基线行数,无法确认叠加后是否越过 700 行阈值。按规则行数判定以 CI gate 实测为准,此处仅为估算提示:若实测越 700,需补 owner note 或拆分计划。

结构评审结论

未发现 blocking package/module/public API 组织问题。逐项核对:

  • Owner 归属:三个文件全部落在 tools/(command risk 分类/解析/测试),符合 standard.md §2 中 tools/ 承担 command risk 的职责定义,无跨 owner 混杂。
  • Public APISAFE_OUTPUT_SINKS 仍为 private const,parse_command 维持 pub(super)apply_null_redirection_policy 仅加注释;lib.rs 未触碰,inventory 分类计数不受影响。
  • Root src / 入口文件:未新增 root implementation 文件,未触碰 lib.rs/main.rs
  • Forbidden dependency:未引入新 import,tools 未新增对 runtime/agent/ui 等方向的依赖。
  • Self-crate path:未新增 cosh_shell::...
  • 测试位置:新增用例放入既有 tools/command_risk_tests.rs 决策矩阵,与 [cosh-ng] feat(cosh-ng): distinguish null redirection from filesystem writes #1667 边界测试职责一致。
  • 范围:仅 cosh-shell crate 内变更,无跨 crate 混入。

安全语义复核(非 blocking)

本 PR 收窄 fail-closed 规则(整词带引号的 /dev/null 改判 null sink),属安全敏感分类器变更。复核认为方向保守可控:开引号须紧邻重定向目标位置、内容逐字节等于 allowlist 条目、闭引号后必须为词边界(刻意排除 {});未闭合引号、转义、后缀拼接('/dev/null'x)、相邻引号续接、$/通配符、空引号、引号内空白全部落回 RedirectionWrite。正反两向测试齐备(含 &>'/dev/null'、argv 不泄漏断言、AskUser 不变的 V-M10),执行决策未变。map_or + #[allow(clippy::unnecessary_map_or)] 为 MSRV 1.74 兼容写法,已有注释说明。

剩余风险

  • 内层内容扫描与 word_ends 各维护一份分隔符集合({} 差异为刻意保守且有注释),后续修改需两处同步。
  • PR body 声称 1342 passed(基线 1340 + 新增 2),与 diff 中两个新 #[test] 相符,但与上述 CI FAILURE 矛盾,以 CI gate 为准。

未跑验证

按评审约束未执行任何工具与构建命令;cargo testcheck-layout.shinventory-public-api.sh、fmt/clippy 均未本地复验,全部以 CI gate 实测为准——当前 Test cosh-ng 两门为 FAILURE,是合并前必须清零的首要事项。

@jfeng18
jfeng18 force-pushed the fix/cosh-ng/quoted-null-sink branch from 9bf29dc to 7c77cb7 Compare August 21, 2026 03:55
@jfeng18

jfeng18 commented Aug 21, 2026

Copy link
Copy Markdown
Collaborator Author

Both findings addressed in 7c77cb7 (single commit, force-pushed):

P1 (CI vs body) — two distinct failures, both explained:

  • Test cosh-ng fast checks: the audit gate failed on the test-inventory overlap ratchet — the two new tests had landed in tools/command_risk_tests.rs, which is compiled into both the lib and bin test targets (main.rs re-declares lib modules), pushing the lib/bin overlap to 696 > 694 ceiling. They were moved to tools/command_risk_quoted_tests.rs, declared only from lib.rs via the existing wrap_tests pattern (the parser internals are pub(super), so a tests/ integration file cannot reach the assessment API either), restoring overlap = 694 = ceiling — the audit gate now passes locally.
  • Test cosh-ng: provider_lifecycle::cosh_core_sync_drains_child_output_while_writing_large_prompt timed out after its 3s budget (71/72 pass on that job; the same code is green on main; zero interaction with this diff — it does not touch provider lifecycle). Will re-run the failed job off-peak to confirm the environmental attribution.

The quoted-sink behavior itself is unchanged; full ECS re-verification on the amended commit is green (fmt/clippy clean, --lib 1344 passed / 0 failed = baseline 1342 + the 2 relocated tests).

P2 (parser line-count registration) — Deferred with data: command_risk_parser.rs is 517 lines after this change (+99) and command_risk_build.rs is 691 (+5), both under the 700 threshold. Registration belongs to the large-file-inventory maintenance flow and will follow if and when an actual crossing occurs.

@SunnyQjm

Copy link
Copy Markdown
Collaborator

PR number: #2710
head_sha: 7c77cb7
reviewed_at: 2026-08-21T06:20:14Z

Findings

  • [P3][advisory] src/cosh-ng/crates/cosh-shell/src/tools/command_risk_build.rs:510 本次 +5 行(纯注释),作者声明当前 691 行,距 700 行 owner-note 阈值仅剩约 9 行余量。行数判定以 CI gate 实测为准(本轮 fast checks 已 SUCCESS,未越线),此处仅提示后续增长需提前登记或拆分。
  • 无 blocking findings。

结构评审结论

未发现 blocking package/module/public API 组织问题。逐项核对:

  • Owner 归属:全部改动落在 tools/(command risk 解析/构建/测试),符合 standard.md §2 中 tools/ 承担 command classification/risk 的职责定义,无跨 owner 混杂。
  • Public APIlib.rs 仅新增 #[cfg(test)] #[path] mod command_risk_quoted_tests;(私有、test-only),不是 pub mod/pub use,inventory 分类计数不受影响;SAFE_OUTPUT_SINKS 仍为 private const,parse_command 维持 pub(super),未为测试新增 production pub(作者说明因 pub(super) 不可达,测试经 assessment API 断言可观察效果,此边界成立)。
  • Root src / 入口文件:新测试文件位于 tools/ 下,未新增 root implementation 文件。
  • Forbidden dependency / self-crate path:未引入新 import,未新增 cosh_shell::...
  • 范围:仅 cosh-shell crate 内变更,无跨 crate 混入,单一 commit。

上轮 findings 复核

  • 上轮 P1(CI FAILURE 与 body 声明冲突):当前 head 7c77cb727Test cosh-ng(04:06:02Z)与 Test cosh-ng fast checks(04:04:49Z)均为 SUCCESS;作者对 overlap ratchet(696>694 回落至 694)与 provider_lifecycle 超时的解释合理,本 diff 不触碰 provider lifecycle。关闭。
  • 上轮 P2(parser 行数登记):作者给出实测 parser 517 行(+99)、build 691 行(+5),均低于 700 阈值,且 layout audit 已随 fast checks 转绿。关闭,保留上方 P3 余量提示。

安全语义复核(非 blocking)

收窄 fail-closed 规则的方向保守:开引号须紧邻重定向目标位置、内容逐字节等于 allowlist 条目、闭引号后必须为词边界(刻意排除 {} 且有注释);未闭合引号、转义、后缀拼接('/dev/null'x)、$/通配符、空引号、引号内空白均落回 RedirectionWrite。正反两向测试齐备(含 &>'/dev/null'、AskUser 不变断言),执行决策未变。#[allow(clippy::unnecessary_map_or)] 为 MSRV 1.74 兼容写法,已有注释。

剩余风险

  • 内层内容扫描与 word_ends 的分隔符集合各维护一份({} 差异为刻意保守),后续修改需两处同步。
  • 测试文件通过 lib.rs #[path] 声明而非 tools/ owner 目录挂载,作者声明为既有 wrap_tests pattern 且服务于 overlap ratchet;测试位置最终口径以测试 SDD 与 CI 的 test-inventory gate 为准。

未跑验证

按评审约束未执行任何工具;cargo testcheck-layout.shinventory-public-api.sh、fmt/clippy 均未本地复验,全部以 CI gate 实测为准——当前 head 上相关检查已全绿。

A quoted redirect target like 2>"/dev/null" was classified as a
filesystem write instead of a null sink, so the approval card
showed redirection-write. Recognize fully quoted SAFE_OUTPUT_SINKS
targets as null redirections; every other quoted form stays
fail-closed. The auto-approve policy itself is unchanged and
remains a separate discussion.

Refs #1752

Assisted-by: Qoder
Signed-off-by: Jiangtian Feng <jiangtianf97@163.com>
@jfeng18
jfeng18 force-pushed the fix/cosh-ng/quoted-null-sink branch from 7c77cb7 to 26035a8 Compare August 21, 2026 09:26
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.

2 participants