Skip to content

feat(cosh-ng): [core,shell] automate auth - #3298

Merged
kongche-jbw merged 3 commits into
agentic-os-org:mainfrom
SunnyQjm:fix/cosh-3278-auth-ecs-polling
Sep 17, 2026
Merged

kongche-jbw merged 3 commits into
agentic-os-org:mainfrom
SunnyQjm:fix/cosh-3278-auth-ecs-polling

Conversation

@SunnyQjm

@SunnyQjm SunnyQjm commented Sep 16, 2026

Copy link
Copy Markdown
Collaborator

Why

修复 Provider 首配重复命名和 ECS RAM Role 认证必须人工确认的问题。ECS 探测改为有界、可取消的单次操作,只有本轮线程与独占子进程完成回收后才报告取消完成。

What changed

  • 按模板类型与有效配置快照决定默认名;同类型已有配置或默认名冲突时仍要求命名,导航和失败恢复保留身份。
  • ECS 首次先检查,凭据未就绪才展示等待;每次单独探测、自动提交一次,取消、截止、提交结果未知各自保持明确语义。
  • IMDSv2 token/GET 覆盖环境识别、verify/preflight、首次取 STS 和刷新,限定地址与角色、总期限、响应体大小,禁代理、重定向和无 token 降级。
  • 补齐 ECHILD 已被内核回收的终态,以及 ActiveRun 自动 Ready 提交前的认证卡清理,发送失败也不残留旧卡。
  • 按数值 PID/PGID 发信号要求两个前提同时成立:观察到子进程未被回收,且本进程不让内核抢先回收——启动探测前把继承的 SIGCHLD=SIG_IGN 规范化为默认处置(默认与自定义 handler 均保留 zombie,不改写),Linux 与 macOS 共用同一条 POSIX 路径。取舍:宿主若依赖忽略 SIGCHLD 自动收割自己的子进程,首次探测后它们会保留 zombie 直到被 wait。
  • 入口菜单 prepare 先建立可取消面板,再复用独占探测任务,不再借用 live core;结果仅更新原菜单,不自动 verify/configure。取消/退出覆盖超时回收终态且不重置期限,取消重绘保持同一 capture 身份,不重新武装输入 owner。
  • 回收只在子进程仍未被回收时才按数值 PGID 发信号;已由本流程 reap 或内核在 SIGCHLD=SIG_IGN 下自动回收后不再按数值 ID 发信号,避免命中复用的进程组。新增 adapter 类型在公共 API 台账登记为 private-candidate,协议生命周期测试仍留在组件规则要求的 protocol 层。
  • 单独保留已获授权的 shell 测试前导回车断言与等价 Clippy 写法修正;未修改 Gateway、通用输入引擎或测试 readiness helper。验收工具修复不包含在此 PR。

Related issue

Closes #3278

User / Agent impact

首配直接进入凭据或 ECS 检查;无需“我已授权”确认。等待可取消,配置成功须实际保存;ActiveRun 不假称配置落盘。角色缺失与凭据过期分别展示授权等待和刷新等待。

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

shell/core 应一同更新:verify 使用 ready / not_ready 和安全错误分类。只验证指定角色的完整未过期凭据,不声称验证了 SysOM 服务权限、额度或推理链路;不会自动创建或修改云端角色。自动探测总预算 200 秒,取消完成要求 join/reap,未知保存结果不会自动重发。

入口菜单 prepare 已纳入可取消任务,但此前的 auth/state 配置快照读取仍同步,不声称整个 /auth 初始化立即可取消。状态标题/操作提示为双语,部分安全技术错误详情仍为英文;本轮不扩展错误翻译框架。

Validation

当前候选:72efd76930de9a569677e8a6f0eba9fbd39d760f,基于主干 2508da624,提交历史已按 AGENTS §13 整理为 3 个提交。历史重写前后 src/cosh-ng 树哈希同为 67980041cc20aacfaf3b2060acde80749f952d56,零内容漂移;本轮文档补齐 registry JSONL 公共信封(type/request_id 及响应回显关联规则),无生产代码或测试变化。

  • 信号所有权的两条前提各有独立回归,均先 RED 后 GREEN 并做变异校验:观察条件(判定缺失导致编译失败;改回“无条件发信号”则断言失败);保持条件(继承 SIGCHLD=SIG_IGN 时探测启动后处置必须不再是 SIG_IGN —— 移除规范化调用该断言红;宿主已装 handler 必须不被替换 —— 让规范化无条件改写该断言红,实测 left: 0)。
  • 跨平台一致性单独验证:该修复只用 POSIX sigaction/signal,Linux 与 macOS 共用一条路径,无 cfg 分叉。macOS 本机 --test protocol ecs_probe:: 7 项通过、fmt 与 clippy -p cosh-shell --all-targets -- -D warnings 通过;ARM64 ALinux 3 容器同样 7 项通过。评审建议的 pidfd 方案未采用,理由是 pidfd_send_signal(2) 不承诺保留 PID 号(man 页把 PID 复用列为传统接口缺陷),且 PIDFD_SIGNAL_PROCESS_GROUP 需 Linux 6.9 以上,覆盖不到 ALinux 3(5.10)与 ALinux 4(6.6)。
  • 最新 ARM64 ALinux 3 容器验证(rebase 前):--test protocol ecs_probe:: 7 项、认证过滤 lib 12 + bin 157、--test raw_cli auth 20 项通过(含 initial_menu_prepare_ctrl_c_reaps_probe_and_worker_in_live_shell,187 秒);该过滤对 logic/shell_host 匹配零项,不代表这些 target 全量通过。rebase 到 c648fc4fd 后在同一容器重验:cosh-core --lib auth 3 项、--test protocol ecs_probe:: 7 项、clippy -p cosh-shell -p cosh-core --all-targets -- -D warnings 均通过;--lib preflight 过滤匹配零项,不计作证据。
  • 最新 fmt、cargo clippy -p cosh-shell -p cosh-core --all-targets -- -D warnings、layout、test inventory/necessity(5916 source IDs、22 rules)通过;台账未分类的兜底项由 2 降为 1(剩余一项非本 PR 引入)。
  • 冻结前一候选的普通 stdin 对照仍有效:不输入额外按键即可取得下一次 readiness,普通命令执行且 PID/TID 资源检查为 ok;没有修改输入引擎、放宽测试握手或增加 sleep 绕过。
  • 本轮末次独立复审因服务不可用未执行;主代理已按状态/所有权/输入合同手动回核,不宣称独立终审通过。
  • 本轮未运行完整 crate/workspace、真实 ECS 或新视觉验收;临时容器验证后清理。下述更广测试及实机截图属于此前记录,不冒充当前二进制验收。
  • 前一候选 eb34a3191Test cosh-ng 首跑失败于既有 shell_host/native.rsenhanced_shift_tab_toggles_shell_only_routing_without_restarting_bash;该文件不在 PR diff、用例已存在于 main,且该候选相对全绿代码仅新增 markdown,故记录为与本 PR 无关的 PTY 时序失败,不将其冒充当前候选结果。
  • 当前候选 72efd7693 的 CI 首跑全绿且无复跑:Test cosh-ngTest cosh-ng fast checksTest cosh-ng prebuilt actionBuild cosh-ng release 均 SUCCESS,failed_jobs=[]rerun_history=[]
  • 更早候选 e01e6a5a4 的 CI:首跑 Test cosh-ng 失败,唯一失败为 agent_input::raw_cli_routes_slash_bearing_han_prompt_before_shell_execution(zsh 变体,raw_cli 540 passed / 1 failed / 1 ignored);该记录保留。复跑同一 job 成功:raw_cli 541 passed / 0 failed / 1 ignored、shell_host 216 passed / 0 failed / 1 ignored,其余检查 SUCCESS。复跑成功不表述为该用例通过。归因事实:本 PR 相对 origin/main 未改动 raw_input/shell_host/agent 任一路径,失败用例使用 fake adapter 且不经 /auth,同一用例在前候选 9de696753 的 job 中通过,且该用例以固定 sleep 注入输入、无 readiness 握手。近期 main 的 Test cosh-ng 均被 Detect Changes 跳过(job 级 skipped),故无同用例主干先例;本地隔离复现因容器供给超时未取得对照,容器已清理,不以未完成的本地运行充当证据。
  • 前次候选 9de696753Test cosh-ng 被 15 分钟 job 上限取消;取消前 raw_cli 为 540 passed / 0 failed / 1 ignored,shell_host 未完成,保留为未通过。本轮未修改 CI 时限,也未改动该 zsh 用例或其 harness。
  • 更广回归曾运行:shell lib/bin/logic/protocol/raw_cli 各 target 通过;完整 shell_host 在 ALinux 4 ECS 串行 216 通过、1 忽略。core 在普通 UID 下 lib/bin 及所有集成 target 通过。
  • 原始失败均保留并归因:测试启动器继承忽略信号、容器缺 zsh、Apple container 缺 /proc/.../children、root 绕过权限拒绝测试;一条前导回车断言在未修改基线同样失败,修正为独立输出行断言后通过。
  • rebase 后重新运行发布前检查:fmt、layout、test inventory/necessity、commit lint、upstream drift 均通过;macOS workspace Clippy 被主干已有的 cosh-platform/src/checkpoint.rs:1736 测试引用阻断(spawn_silent_daemon 仅在 Linux 定义,调用测试未加同样条件)。该文件不在本 PR 差异中。新候选须以 Linux CI 为目标平台验证,不能将宿主检查称为全绿。
  • 历史检查边界:前一轮较新 Rust 工具链下,workspace Clippy 曾在未修改的 cosh-gateway-app/src/checkpoint.rs 报两条 chunks_exact_to_as_chunks;已按用户要求披露并聚焦本任务,没有修改 Gateway、关闭 lint 或绕过 hooks。既有历史记录不等于当前候选的 CI 结果,也不是 workspace/release 全绿声明。

Real ECS evidence(末轮修复前的实机记录)

在入口菜单预取、ECHILD/ActiveRun 后续补修之前,曾于 x86_64 ALinux 4、IMDSv2 required 实例上使用未修改的 canonical Ruby runner 创建真实 PTY;通过 Cloud Assistant 启动并下载原始脚本结果与脱敏 cast。本地只用既有渲染/校验器导入,明确标注 accept-import,不是 SSH 执行,也不是 fake adapter。这些记录验证此前版本的默认命名、自动认证和普通取消链路,不作为后续入口预取、取消交错或 capture 重绘修复的实机覆盖证据。

  • AUTH-ECS-READY:无需命名或二维码,实际配置回读成功。
  • AUTH-ECS-CANCEL:等待中取消,普通命令可执行;超过两次检查间隔后检查同一活 shell 的线程和子进程,再次 /auth 与取消均正常。
  • AUTH-ECS-WAIT-READY:实际观察 Waiting 后挂回原角色,没有发送确认键,自动完成并回读配置。
  • 11 个验收点均有截图;布局点包含 cast 派生 WebM,原始 runner JSON 与云助手执行回执保留,未改写失败判定。
  • shell SHA256 f23193c3306e06a0c89f89b7292ecbd8d95c8fc13429193c5ecb883d5ca11104;core SHA256 af60dcf3216c857c9360a39e00718b7756b7a70bbed97f59bbf31bb62392571e
  • 本任务 aggregate gate 为 Go(unit、protocol、三条实机认证、清理);所有本次 ECS/专用安全组/临时密钥已清理,最终 remaining=0

截图与回放(固定资产 commit)

基线:首次配置仍要求命名。

基线命名屏

修复后:角色就绪直接完成并回读已保存配置。

最终配置回读

未就绪等待与取消后资源核验。

完整等待卡
取消后线程和子进程

30 个远端资产 Git blob 哈希已与本地文件逐一核对。资产仅位于 fork 独立分支,不在产品最终差异中。按用户要求,本次专用验收脚本也原样归档,不作为通用 CI 测试提交:

Documentation and rollback

specs/documentation-standard.md §5(protocol change → 同 PR 补组件 docs/design/),新增 src/cosh-ng/docs/design/ecs-auth-provisioning.md,覆盖 registry auth prepare/verify/configure 协议字段(status: ready/not_ready + reason/error_code、shell/core 同步升级)、ECS 轮询状态机与各层时间预算、取消完成必须 join/reap、未知保存不自动重发、probe sole-reaper 信号安全前提。按 docs/design/ 现有惯例为英文(该目录 11 个文件中 10 个纯英文,非强制双语集)。本次专用验收案例与回放保留在证据资产,产品仓保留 Rust 单元、协议和 PTY 回归。回退本 PR 时同步回退 shell/core 协议;现有已保存配置格式不变。不自动合并,等待维护者审阅。

@github-actions github-actions Bot added the component:cosh-ng src/cosh-ng label Sep 16, 2026
SunnyQjm pushed a commit to SunnyQjm/anolisa that referenced this pull request Sep 16, 2026
@kongche-jbw
kongche-jbw self-requested a review September 16, 2026 06:07
SunnyQjm pushed a commit to SunnyQjm/anolisa that referenced this pull request Sep 16, 2026
SunnyQjm pushed a commit to SunnyQjm/anolisa that referenced this pull request Sep 16, 2026
@SunnyQjm

Copy link
Copy Markdown
Collaborator Author

CI 首跑结果已保留:Test cosh-ng fast checks(包含 CI 固定工具链上的 lint)与 Build cosh-ng release 通过;进程测试 539 项通过、1 项失败、1 项忽略。

唯一失败是本 PR 已调整的 provider_handoff::foreground::raw_cli_claude_without_host_executed_capability_uses_foreground_recovery 输出断言。原日志确认命令执行和前台证据回传成功,但终端可使用 CR 作为输出行边界;先前 .lines() 加去掉行尾 CR 的写法没有覆盖独立 CR 分隔。

修正仅将该断言改成同时按 CR/LF 分隔,仍严格要求完整行等于 CLAUDE_NATIVE,不会将 echo CLAUDE_NATIVE 命令回显、前缀或后缀误当成结果。产品源码、ECS 认证行为和实机二进制哈希均未改变;没有跳过测试或修改 Gateway。定向复验通过后推送,等待本轮 CI 重新验证。

@SunnyQjm
SunnyQjm force-pushed the fix/cosh-3278-auth-ecs-polling branch from c4b4396 to 9de6967 Compare September 16, 2026 08:00
@SunnyQjm
SunnyQjm marked this pull request as ready for review September 16, 2026 08:18
@SunnyQjm
SunnyQjm requested a review from zkl123 as a code owner September 16, 2026 08:18

@kongche-jbw kongche-jbw left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Review baseline: b067c661ffbba8adf9aea5fc6c406023849bc7cc...9de696753c2c667665d0c42199edd9c365e0747f

[P1] 将启动预取也移入可取消的独占探测任务

src/cosh-ng/crates/cosh-shell/src/auth/runtime.rs:191/auth 建立面板前同步调用
prefetch_sysom_menu,后者通过普通 registry_query 执行 auth.prepare。当空闲的
live core 存在时,这条查询会占用共享 core;在非 ECS 或 metadata 卡住时,主事件线程会
等待 registry 超时,期间没有 capture 或 panel 可接收 Esc/Ctrl+C。因此新增的
EcsProbeTask 只保护选择后的 fallback path,未覆盖正常 /auth 入口,prepare 阻塞
场景仍不可取消,也不满足探测使用本轮独占进程的边界。

Possible direction: 先创建 auth 状态并渲染可取消的 checking panel,再通过
start_ecs_probe("prepare") 获取菜单预取结果;补充首次 prepare 挂起的 raw_cli 回归,
断言 /auth 可立即取消,且 worker 与临时 core 均已回收。

@zhoukailong

Copy link
Copy Markdown
Collaborator

🔍 Automated Code Review

PR: #3298 feat(cosh-ng): [core,shell] automate auth
Author: SunnyQjm
Reviewer: reviewer (automated)
Time: 2026-09-16T16:30:00+08:00
Diff: 41 files, +4915 / −432


Summary

本 PR 将 ECS RAM Role 认证从同步阻塞式原始 TCP 元数据查询升级为异步有界 IMDSv2 HTTP 探测,并在 shell 层实现了完整的 ECS 认证状态机(Preparing → Checking → Waiting → Submitting / TimedOut / Failed / Cancelling),消除首配时的人工确认步骤。核心变更包括:

  • cosh-core: 新增 ecs_metadata 模块替换 fetch_ecs_metadata_text(原始 TCP),引入 IMDSv2 token、响应体大小限制、禁代理/重定向;instance_id 改为 OnceCell 异步惰性解析;refresh_sts_credentials 复用新探测链路
  • cosh-shell: 新增 ecs_probe(隔离子进程 + 取消/超时)和 ecs_poll(状态机);default_id 逻辑首次无冲突时跳过 Provider ID 字段
  • i18n: 12 条新 MessageId 正确追加到末尾段
  • Tests: 覆盖 lib / protocol / raw_cli / shell_host 四层

Findings

P2-1: ProbeChild::drop 在非 ECHILD 错误时无上界重试

File: src/cosh-ng/crates/cosh-shell/src/adapter/ecs_probe.rs:118-133

问题: ProbeChild::drop 在发送 SIGKILL 后循环 try_wait(),对非 ECHILDErr 每 50ms 重试且无上界。SIGKILL 后非 ECHILD 的持续失败在生产环境极不可能,但 drop 路径上的无限循环是不可恢复的——如果 try_wait() 因内核异常持续返回非预期错误,线程将永久阻塞。

可复现场景: 理论上需要一个子进程在 SIGKILLtry_wait() 持续返回非 ECHILD 错误(如内核 bug 或 ptrace 拦截)。

最小修复方向: 增加重试计数或总超时(如 100 次 / 5 秒),超限后 tracing::error! 并放弃。

应补回归测试: protocol 层,验证 drop 在异常 try_wait 行为下有界退出。


P2-2: shutdown() 同步 join worker 可能阻塞 shell 关闭

File: src/cosh-ng/crates/cosh-shell/src/auth/ecs_poll.rsshutdown 函数)

pub(crate) fn shutdown(state: &mut InlineState) {
    if let Some(mut flow) = state.auth.ecs.take() {
        if let Some(Operation::Probe(task, _)) = &flow.operation {
            task.cancel();
        }
        flow.operation.take(); // triggers EcsProbeTask::drop → worker.join()
    }
}

问题: flow.operation.take() 触发 EcsProbeTask::drop,其中 worker.join() 是同步阻塞的。如果 probe 子进程因 I/O 卡住(如 poll(2) 在 cancel 后未立即唤醒),join 会延迟 shell 关闭。Probe 内部有 5 秒 deadline + 100ms poll 间隔,最坏情况 join 等待约 5 秒。

可复现场景: 在 ECS 探测进行中关闭 shell,且 probe 子进程因网络原因未在 cancel 后及时退出。

最小修复方向: 可接受为已知限制(5 秒有界),但建议在 shutdown 中加 tracing::debug! 记录 join 耗时,或在 EcsProbeTask::drop 中对 join 加短超时后 detach。


P2-3: ecs_poll::poll() 函数体量较大,分支密集

File: src/cosh-ng/crates/cosh-shell/src/auth/ecs_poll.rs

问题: poll() 函数约 200+ 行,包含状态转换、操作启动、面板重绘、deadline 检查等多重职责。虽然当前测试覆盖充分,但后续维护者修改单个分支时容易引入回归。

最小修复方向: 非阻塞建议——可考虑将 cancel 路径、operation 完成路径、operation 启动路径各提取为私有辅助函数,保持 poll() 作为调度入口。


P3-1: preflight.rs 中函数定义位置在常量之前

File: src/cosh-ng/crates/cosh-core/src/auth/preflight.rs:17-25

问题: ecs_preflight_result 函数被插入到模块级 const 声明之前。Rust 对声明顺序无要求,但惯例是常量 → 类型 → 函数。

最小修复方向: 非阻塞风格建议——将函数移至常量之后。


方案合理性

  • 7-crate 依赖方向正确:cosh-core 提供探测原语和分类,cosh-shell 组装状态机和 UI,ecs_metadata 作为 sysom 子模块隔离合理
  • wire 契约变更(authorized: boolstatus: "ready" | "not_ready" + reason)在 PR 描述中已声明,core/shell 同步更新
  • detect_ecs_auth_challenge 签名变更从 Option<T>Result<Option<T>, ProbeError>:正确区分「确认非 ECS」(Ok(None))和「确认 ECS 但探测失败」(Err),消除了旧代码将 AccessDenied 误判为非 ECS 的风险
  • instance_id 改为 OnceCell<Option<String>> 惰性解析:解决了构造函数中同步阻塞元数据请求的问题

功能完备性

  • 错误路径全部 fail-closed:ProbeError 不暴露凭据、不保留响应体;非成功 HTTP 状态丢弃 body
  • i18n 四步完整:MessageId 宏末尾追加(auth_ecs_ids),en/zh 双语,mod.rs 偏移断言更新,位置不变性测试覆盖
  • 测试落位:
    • lib: ecs_metadata classify_credentials 覆盖所有字段缺失/非法/过期变体;preflight 覆盖 Ready/NotReady/ProbeError 传递
    • protocol: ecs_probe 覆盖 start/cancel/timeout/reap/SIGCHLD-ignore
    • raw_cli: auth_ecs 覆盖 ready/waiting/cancel + 二次 /auth 流程
    • shell_host: ecs_poll 状态机 946 行测试覆盖 deadline、cancel-wins-over-ready、shell-exit 语义
  • 测试有区分度:多处先声明 RED(revert 实现后必须 fail)再验证 GREEN

文档完整性

  • PR body 详尽记录了 Why / What / Risk / Validation / Real-ECS evidence
  • 代码 doc comment 写 why 不写 what(如 MetadataSession 注释解释共享 deadline 的设计意图)
  • CLI 行为变更(首配无需命名/确认)在 PR body 中说明;user-guide 更新未在 diff 中发现——如果 user-guide 中有 ECS 认证流程的描述,需同步更新

安全

  • IMDSv2 强制 token,无 v1 降级:token PUT 请求被 fixture 验证(401 if no token)
  • 禁代理(no_proxy())、禁重定向(Policy::none())、禁重试(retry::never()
  • 响应体限制 64KB,chunked 传输同样受限(测试覆盖 chunked 边界)
  • token 标记 set_sensitive(true)
  • RoleCredentials 限定 pub(super) 可见性,凭据不离开 ecs_metadataSysomProvider
  • ProbeChild 使用 process_group(0) 隔离进程组,kill(-pid, SIGKILL) 确保清理子进程树
  • ECHILD 处理正确覆盖了继承 SIGCHLD ignore 的场景(有专项回归测试)
  • 非成功 HTTP 状态不读取/保留 body(防止错误 body 中泄露敏感信息)

代码质量

  • ProbeErrorNotReadyReasoncode() / as_str() 返回稳定字符串,与 registry wire 契约一致
  • MetadataSession 使用单一 deadline 覆盖 token + GET 请求,避免多个独立超时导致的累积延迟
  • ecs_probe.rs 使用 UnixStream pair 实现 cancel 唤醒,比 mpsc channel 更轻量且避免额外线程
  • 没有发现 ? 反模式或死抽象

验证与覆盖声明

维度 状态
静态评审(diff 交叉验证) ✅ 已完成
cargo fmt / clippy / test ⚠️ 未执行(需 Linux CI 环境)
方案合理性 ✅ 已覆盖
功能完备性 ✅ 已覆盖
文档完整性 ✅ 已覆盖(user-guide 未确认)
安全 ✅ 已覆盖
代码质量 ✅ 已覆盖

未覆盖范围与剩余风险:

  • 未执行 cargo fmt/clippy --workspace --all-targets/cargo test --workspace——PR 作者声明 ARM64 ALinux 容器上 148 项测试通过、fmt/clippy 通过,但需 CI 独立验证
  • macOS workspace Clippy 被主干已有阻断(cosh-platform/src/checkpoint.rs:1736),不在本 PR 范围内
  • user-guide 是否需要同步更新未确认
  • 实机 ECS 验收记录属于末轮修复前的证据,末轮 ECHILD/ActiveRun 修复的实机覆盖未声明

结论

未发现 P0 或 P1 阻塞问题。 整体方案设计合理、安全防御到位、测试覆盖充分。3 条 P2 和 1 条 P3 均为改进建议,不阻塞合并。建议作者确认:

  1. user-guide 是否需要同步更新 ECS 认证流程描述
  2. P2-1 的上界重试是否需要在 drop 中增加防御性超时

@SunnyQjm

Copy link
Copy Markdown
Collaborator Author

已对照 9de696753 核实这条 P1:trigger_auth_from_slash 在建立 auth 状态/capture 前同步调用菜单 prepare,而普通 registry 路由确实可能借用 live core。该入口此前作为既有菜单行为保留,但会使入口 prepare 与选择后 prepare 的取消边界不一致;本轮采纳并收敛这条入口,不以“原有代码”否定意见。

修复范围已确认:配置快照读取成功后先建立可取消的菜单准备面板,复用现有 EcsFlow / EcsProbeTask 发起独占 prepare;结果只更新原菜单/快捷入口,不自动验证或保存。错误/超时须先回收任务再回普通菜单,取消/关闭不得被晚到结果重新打开菜单。正在补入口不抢跑 prepare、首次 prepare 挂起取消及同一活 shell 内资源回收回归;完成验证后更新本 PR。

边界保持明确:auth/state 配置快照读取仍同步,此次不宣称整个 /auth 初始化立即可取消,也不扩为通用 registry 异步化。

另核对了当前 CI:GitHub 注解明确为 Test cosh-ng 超过 15 分钟 job 上限;日志显示 raw_cli 540 passed / 0 failed / 1 ignored,随后 shell_host 未跑完。保留该次未通过记录,不把取消当成功,也不修改 CI 时限。

@zhoukailong

Copy link
Copy Markdown
Collaborator

🔄 Incremental Code Review — PR #3298

Reviewer: reviewer (automated)
Review timestamp: 2026-09-16T17:00+08:00
PR: feat(cosh-ng): [core,shell] automate auth
Diff: +4915 / −432, 41 files, 5 commits
Review type: Static analysis (tests not executed — requires Linux + PTY environment)


Summary

This PR replaces the synchronous raw-TCP ECS metadata probing with an async IMDSv2 (tokenized) transport, introduces a full ECS auth polling state machine in cosh-shell, adds auto-naming for first-time providers, and hardens the auth form with Provider ID collision detection. The security posture is substantially improved: no IMDSv1 fallback, no redirect following, response size bounds, token marked sensitive, error types that never carry upstream bodies or credentials.

Overall verdict: No blocking issues found. The design is sound, the crate boundaries are respected, i18n MessageIds are correctly appended, and the test coverage is thorough across lib / logic / protocol / raw_cli layers.


Findings

P2 — Test fixture uses a hardcoded past timestamp

File: crates/cosh-core/src/provider/sysom/tests.rs:295
Essence: ecs_metadata_now() returns a fixed "2026-09-15T12:00:00Z". The credential fixtures use expiration timestamps relative to this date (e.g. "2026-09-15T13:00:00Z"). While the classification logic under test is time-relative (passing now as parameter), the boundary test at tests.rs:324ecs_metadata_not_ready_at_or_after_expiration — pins both sides. If any future refactoring drops the now parameter and uses Utc::now() internally, these tests silently become time-dependent and start failing after 2026-09-15T13:00:00Z.
Fix direction: Consider injecting now from Utc::now() in the fixture helper, or document the deliberate pinning with a // SAFETY: now is injected comment.
Target test layer: lib (unit).

P2 — ECS poll error strings bypass i18n

Files: crates/cosh-shell/src/auth/ecs_poll.rs (multiple locations), crates/cosh-shell/src/adapter/ecs_probe.rs (multiple locations)
Essence: Error messages like "ECS credential check timed out", "Unable to start ECS credential check", "ECS probe cancelled", "ECS probe timed out" are hardcoded English strings assigned directly to flow.error. These bypass the I18n / MessageId system, so Chinese-locale users will see English error text mixed with localized panel headings.
Fix direction: Add corresponding MessageId::AuthEcs* variants for the dynamic error strings, or accept this as a known limitation if the error set is small and transient.
Target test layer: logic (rendered output assertion).

P3 — Unknown save stage has no user-actionable retry

File: crates/cosh-shell/src/auth/ecs_poll.rs (Stage::Unknown handling)
Essence: When the configure subprocess finishes but the result cannot be confirmed within OPERATION_LIMIT, the flow enters Stage::Unknown. The rendered message (AuthEcsUnknown) instructs users to "check provider management" but there is no in-flow retry or status check. The only escape is to dismiss and start a new /auth.
Fix direction: Consider adding a "check status" option that queries the registry state action to confirm whether the provider was saved, avoiding a full re-auth.
Severity context: This is a rare edge case (save subprocess hangs). The current behavior is safe (fail-closed) but not ideal UX.

P3 — ProbeChild::Drop retries reap on unexpected errors indefinitely

File: crates/cosh-shell/src/adapter/ecs_probe.rs:137-152
Essence: If try_wait() returns an unexpected error (not ECHILD), the drop handler logs once then retries every 50ms without an upper bound. In pathological scenarios (e.g., kernel bug, container edge case), this could block the calling thread indefinitely during drop.
Fix direction: Add a retry counter or deadline (e.g., 10 retries / 500ms total) after which the handler gives up and logs a critical error. The child was already SIGKILL'd, so a missed reap is a zombie, not a live process.
Target test layer: protocol (drop cleanup assertion).


Positive observations

  1. IMDSv2 enforcement: The MetadataSession requires a PUT-obtained token for all subsequent GETs. No IMDSv1 fallback exists — a missing token endpoint returns ProbeError::Http, not silent degradation. Tests confirm redirect, 401/403, oversized, and trickle responses are all rejected.

  2. Secret containment: RoleCredentials is pub(super) — credentials never leave the sysom module. ProbeError variants carry no response bodies or credential data. The token.set_sensitive(true) call prevents header logging. Tests explicitly assert that Display output never contains fixture secrets.

  3. Process lifecycle: EcsProbeTask owns cancellation via UnixStream::shutdown + AtomicBool, the child is isolated via process_group(0), and ProbeChild::Drop sends SIGKILL to the group. The auth configure path now uses wait_timeout + terminate_and_reap_process for lingering children. Shell exit correctly cancels read-only probes but preserves in-flight saves.

  4. i18n correctness: All 12 new MessageId::AuthEcs* variants are appended at the end via the auth_ecs_ids macro. The position-anchor tests in i18n/mod.rs are updated to reference AuthEcsChecking instead of ALL.len(). Both EN and ZH translations are present.

  5. Test coverage: ~50+ new tests across 5 layers:

    • lib: credential classification, error code stability, field editability
    • logic: ECS poll state machine (orphan cleanup, deadline expiry, shell exit, active-run, resize)
    • protocol: ecs_probe process lifecycle, SIGCHLD ignore, probe cleanup
    • raw_cli: auth_ecs.rs (ready/waiting/cancel flows), updated auth.rs fixtures
    • shell_host: foreground provider handoff CR-output-rows fix

Verification & coverage statement

Dimension Covered Notes
Design soundness Crate boundaries, IPC wire contracts, minimal scope
Functional completeness Error paths, i18n (4 steps), 5-layer tests
Documentation Doc comments on new modules explain invariants
Security IMDSv2 enforcement, fail-closed, no secret leakage
Code quality No dead abstractions, proper ownership, no ? anti-patterns

Not covered:

  • Tests were not executed (requires Linux + PTY). The PTY-dependent tests in raw_cli/auth_ecs.rs and protocol/ecs_probe.rs need Linux validation.
  • Runtime performance under metadata service latency was not benchmarked.

Remaining risks:

  • The 200s WAIT_LIMIT is generous; a user waiting that long on a misconfigured instance may have a poor experience. Consider whether 60–120s would suffice.
  • The EcsFlow state machine has 9 stages with cross-cutting transitions. Future changes should add state-transition documentation to prevent regression.

This review was produced by reviewer (automated code review). Findings are based on static analysis of the diff against the cosh-ng codebase conventions defined in AGENTS.md.

@zhoukailong

Copy link
Copy Markdown
Collaborator

🔄 Incremental Code Review — PR #3298

PR: feat(cosh-ng): [core,shell] automate auth
评审时间: 2026-09-16T17:15:00+08:00 (增量评审)
评审范围: commit 4602bb39de6967(最新 commit: "fix(cosh-ng): [shell] finish ECS auth cleanup")
增量变更: 6 文件,+112 / -361 行

本报告由 reviewer 自动评审产生。仅覆盖增量 diff,前序 commit 已在上一轮评审中覆盖。


变更摘要

文件 变更性质
ecs_probe.rs Drop 中增加 ECHILD 分支处理
ecs_poll.rs ActiveRun 路径发送响应前清除 auth panel
ecs_poll_tests.rs 新增 active_run_ready_clears_auth_panel_before_sending_response 测试
protocol/ecs_probe.rs 新增 probe_cleanup_with_ignored_sigchld 协议层测试
provider_handoff/foreground.rs CR 行断言改用 split 替代 trim_end_matches
specs/ (2 YAML) 移除验收脚本,归档至证据分支

Findings

未发现阻塞问题(P0/P1)。

P2 — 建议改进

1. ecs_poll_tests.rs — 测试覆盖 4 组合但缺少 stage×send 交叉断言日志

  • file:line: ecs_poll_tests.rs:768+
  • 问题本质: 测试遍历 [Checking, Waiting] × [true, false] 四种组合,断言充分但未输出每种组合的实际执行路径(例如在 send 失败时 Auth failed 内容是否因 stage 不同而不同)。当前只断言 contains("Auth failed") 而不区分 stage 差异。
  • 修复方向: 可考虑在测试 log 中标记当前 stage 和 send 结果,方便调试时快速定位失败的组合。非阻塞——测试本身覆盖正确。
  • 目标测试层: lib (unit)

2. protocol/ecs_probe.rs — pre_exec 中使用 libc::signal 而非 signal-hook

  • file:line: protocol/ecs_probe.rs:220-226
  • 问题本质: 使用 unsafe { libc::signal(libc::SIGCHLD, libc::SIG_IGN) } 在子进程 pre_exec 中设置信号。这在测试上下文中是正确的(隔离于子进程),但如果未来有非测试代码需要类似操作,应注意 signal()sigaction() 的语义差异。当前用法安全。
  • 修复方向: 无需修改。记录为 known-good pattern。
  • 目标测试层: protocol

P3 — 风格/观察

3. ecs_probe.rs — ECHILD 匹配的注释语言

  • file:line: ecs_probe.rs:122-123
  • 问题本质: 注释 "Inherited SIGCHLD ignore can let the kernel reap the child first" 写在 match arm 上方,清晰解释了 ECHILD 的触发条件。注释质量好。
  • 修复方向: 无需修改。

4. provider_handoff/foreground.rs — CR 断言改进

  • file:line: provider_handoff/foreground.rs:294-295
  • 问题本质: 从 lines().any(|line| line.trim_end_matches('\r') == "CLAUDE_NATIVE") 改为 split(['\r', '\n']).any(|line| line == "CLAUDE_NATIVE")。语义更精确:不再假设 CR 只在行尾出现,而是将 CR 和 LF 都视为行分隔符。与 commit message 描述的 PTY 输出行为一致。
  • 修复方向: 无需修改。改进合理。

验证与覆盖声明

维度 覆盖状态 说明
方案合理性 ✅ 已覆盖 ECHILD 处理和 panel 清除落位正确,符合 7-crate 依赖方向
功能完备性 ✅ 已覆盖 两组新测试覆盖核心场景(4 组合 + ready/silent × SIGCHLD ignore),CR 断言有 raw_cli 覆盖
文档完整性 ✅ 已覆盖 内联注释充分;验收脚本已按设计移至证据分支,不影响产品仓
安全性 ✅ 已覆盖 无新增凭证路径;pre_exec 信号设置隔离在测试子进程;ECHILD 处理 fail-closed(break 退出等待循环)
代码质量 ✅ 已覆盖 无死抽象、无 ? 反模式、无版本 pin 问题

未执行验证: 静态评审,未执行 cargo test / cargo clippy。原因:当前环境无 cosh-ng workspace 源码。

剩余风险:

  1. ECHILD 处理在非 Linux Unix(macOS/BSD)上行为可能不同,但 ProbeChild 使用 PTY 子进程,实践中主要面向 Linux。
  2. 完整 workspace Clippy 需在 Linux CI 上验证(PR 描述已声明 macOS workspace Clippy 被主干 checkpoint.rs 阻断,非本 PR 变更)。

结论

增量变更质量良好,修复了两个边界条件(ECHILD 子进程回收、ActiveRun panel 残留),测试覆盖充分,验收脚本归档合理。未发现阻塞问题。

@SunnyQjm
SunnyQjm force-pushed the fix/cosh-3278-auth-ecs-polling branch from 9de6967 to e01e6a5 Compare September 16, 2026 09:50
@SunnyQjm

Copy link
Copy Markdown
Collaborator Author

本轮已推送 e01e6a5a45162bc88579f4a5d33e1666cca5ec8d,同步主干后六个提交的 range-diff 均为 =,认证源码树与验证候选一致。

入口 prepare P1:已采纳并修复

  • auth/runtime.rs::trigger_auth_from_slash 在取得配置快照后先建立 PreparingMenu / capture;同步 core_auth_prepareprefetch_sysom_menu 已移除。
  • auth/ecs_poll.rs::poll 复用独占 EcsProbeTask;菜单结果不自动 verify/configure,错误/超时必须先回收再回菜单。
  • 交错回归还锁定:取消/退出覆盖已在回收中的超时终态,不重置回收 deadline;Cancelling 重绘保持同一个已提交 capture,不重新武装输入 owner。
  • 原始入口抢跑、首次 prepare 挂起以及交错反例均保留 RED→GREEN。initial_menu_prepare_ctrl_c_reaps_probe_and_worker_in_live_shell 检查同一活 shell 中记录的 PID/具名 worker 已消失,普通命令可执行;没有改 readiness helper、通用输入引擎或用 sleep 绕过。
  • 边界不变:auth/state 仍同步,不声称整个 /auth 初始化立即可取消。

三份自动评审的逐项处置

对应 #issuecomment-5694700483、#issuecomment-5694934744、#issuecomment-5695273653:

  1. 回收异常后放弃 / join 超时后 detach:不采纳。 这违反本任务“取消完成必须 join/reap、不能留下后台线程”的合同;发送 SIGKILL 也不能直接证明进程已退出或只剩 zombie。真实可复现的 ECHILD 已单独处理。未确认资源收回时保留所有权并报告失败,不把计数耗尽当成成功。最终 shutdown 的 join 是资源收口,不是主事件泵中的阻塞探测;正常取消通过取消标志和 UnixStream 唤醒,不等待网络自行恢复。
  2. 固定测试时间:保留。 ecs_metadata_now() 被显式传入 classify_credentials(..., now),到期前/等于/之后是同一个固定输入表,不依赖运行当天。改用 Utc::now() 反而引入时间依赖;未来若误改为内部真实时钟,现有测试变红正是应有的回归信号,不是应规避的问题。
  3. 英语技术错误详情:确认为已知非阻塞限制。 阶段标题/操作提示双语,部分安全技术详情仍英文,已补入 PR 的 Risk and compatibility;本轮不扩展错误翻译框架。
  4. Unknown 缺少管理页入口:现有实现已提供。 本地资源收回后 question() 提供“Return to provider management”,answer() 调用 trigger_auth_from_slash 重新读取配置。不是只能关闭后手工重输 /auth,也不会自动重发未知保存;保存观察预算是 12 秒,5 秒是 probe 预算。
  5. poll 拆分、常量/函数顺序、四组合额外日志:暂不做纯组织或日志扩改。 本轮已把终态优先级集中到 stop()、菜单结果集中消费,并用结果/交错断言覆盖合同;进一步拆分和组合诊断日志可作为后续可读性改进,本轮不扩大改动面。signal()、ECHILD 注释、CR/LF 断言等“无需修改”观察项保持不变。
  6. 200 秒窗口:保留已批准合同。 它是授权等待总预算,不替代单次操作及取消回收期限;本轮不将其改成 60–120 秒。
  7. 用户指南:已核查组件 src/cosh-ng 的 Markdown 文档,未发现要求点击“我已授权 / I have authorized”的操作说明;匹配到的是 CHANGELOG 中历史 QR 流程条目,不改写历史记录。 当前行为和剩余同步边界已在 PR 说明中明确。

验证与限制

ARM64 ALinux 3:认证过滤 lib 12 + bin 157 + raw_cli 20,以及 ECS protocol 7,共 196 项通过;fmt、shell all-targets Clippy、layout、inventory/necessity 通过,临时容器已清理。这里是定向验证,不是整个 crate/workspace 全量通过。末次独立复审代理未能启动,已做源码级手动回核,不冒称独立终审通过;本轮没有新 ECS 或视觉证据,旧实机材料的版本边界已同步到 PR 正文。

前次 CI 的 15 分钟超时取消记录保留(raw_cli 540 passed / 0 failed / 1 ignored,shell_host 未完成);当前新候选等待 CI,未修改 CI 时限,也不自动合并。

@kongche-jbw
kongche-jbw self-requested a review September 16, 2026 09:56
@zhoukailong

Copy link
Copy Markdown
Collaborator

🔄 Incremental Code Review

Automated review by reviewer (QoderWake)
Scan time: 2026-09-16
PR: #3298feat(cosh-ng): [core,shell] automate auth
Head: e01e6a5a45162bc88579f4a5d33e1666cca5ec8d

Scope note: This scan was triggered because the PR updatedAt is newer than the last recorded review timestamp. The six commits in the current head are all after the previous review baseline, so this review covers the current cumulative diff.


Findings

P1 — ProbeChild::drop can SIGKILL a reused PID before checking ECHILD

  • File:line: src/cosh-ng/crates/cosh-shell/src/adapter/ecs_probe.rs:111-117
  • Problem: Drop calls libc::kill(-pid, SIGKILL) immediately. If SIGCHLD is ignored and the kernel has already reaped the child, the original PID may have been recycled and now belongs to an unrelated process group; sending SIGKILL to -pid is then a use-after-reap that can terminate an innocent process group.
  • Evidence: The ECHILD branch at line 124 is only checked inside the try_wait loop, after the kill.
  • Fix: Call self.0.try_wait() first; skip the kill if it returns Ok(Some(_)) or Err(ECHILD).
  • Test layer: protocol (the existing probe_cleanup_with_ignored_sigchld test covers the symptom; extend it with a subprocess that verifies no other process group is signalled).

P2 — Blocking synchronous disk I/O inside async resolve_instance_id

  • File:line: src/cosh-ng/crates/cosh-core/src/provider/sysom.rs:443-486
  • Problem: resolve_instance_id is async and invoked from generate() via OnceCell::get_or_init, but it performs std::fs::metadata, read_to_string, create_dir_all, and write directly on the async task, blocking the Tokio runtime thread.
  • Evidence: Lines 457, 462, 478, 481 use std::fs APIs with no spawn_blocking or tokio::fs.
  • Fix: Move the file operations into tokio::task::spawn_blocking, or use tokio::fs for the whole cache path.
  • Test layer: lib

P2 — Instance-ID cache write is non-atomic; a failed write poisons the cache

  • File:line: src/cosh-ng/crates/cosh-core/src/provider/sysom.rs:477-483
  • Problem: std::fs::write(cache_path, content) truncates the file before writing. If the write is interrupted, the cache becomes empty. The read path treats a valid-but-empty cache file as "previously failed to fetch" and returns None for the full 3-hour TTL, so the next generate() calls will also skip fetching a real instance ID.
  • Evidence: Lines 464-466 short-circuit on empty cache; line 481 can leave the file empty on any partial write.
  • Fix: Write to a temp file in the same directory and rename it into place; on write failure, remove the temp file or leave the old cache intact.
  • Test layer: lib

P2 — refresh_sts_credentials panics on a poisoned RwLock

  • File:line: src/cosh-ng/crates/cosh-core/src/provider/sysom.rs:415
  • Problem: *self.credentials.write().unwrap() = credentials; will panic if another thread panicked while holding the write lock. This is a live credential-refresh path that should fail closed, not abort the caller.
  • Evidence: Direct .unwrap() on the write guard. (The read side at line 307 and 276 has the same pattern, but the write path is exercised during refresh.)
  • Fix: Use write().unwrap_or_else(|e| e.into_inner()) to recover from poisoning, or return false/Err and log the poisoning.
  • Test layer: lib

P3 — run_probe discards concrete I/O error details

  • File:line: src/cosh-ng/crates/cosh-shell/src/adapter/ecs_probe.rs:158-169, 272-290
  • Problem: Spawn, non-blocking configuration, write, read, and JSON-parse errors are mapped to fixed opaque strings ("Could not start ECS probe", "Could not read ECS probe response", etc.), losing the underlying std::io::Error and making production debugging harder.
  • Evidence: Multiple .map_err(|_| "...")? calls.
  • Fix: Include the source error with map_err(|e| format!("...: {e}")) while still avoiding any secret leakage from upstream metadata.
  • Test layer: protocol

P3 — Operation::Probe uses a bare bool for prepare/verify intent

  • File:line: src/cosh-ng/crates/cosh-shell/src/auth/ecs_poll.rs:68-69, 319-325
  • Problem: Operation::Probe(EcsProbeTask, bool) encodes whether the probe is a prepare or verify request. The boolean is error-prone for readers and callers.
  • Evidence: take_reply matches on *prepare to choose between Reply::Prepared and Reply::Verified.
  • Fix: Replace the bool with enum ProbeKind { Prepare, Verify }.
  • Test layer: logic

P3 — No user-facing documentation for the new automated ECS auth flow

  • File:line: N/A — no README/user-guide hunks in the diff
  • Problem: The PR introduces a new /auth flow that auto-detects ECS, polls RAM-role authorization, waits up to 200 seconds, and supports cancellation. End users and operators have no documented description of this behavior, the wait limit, or what Esc/Ctrl+C does.
  • Evidence: Diff adds ~1,200 lines of shell auth state machine and raw_cli tests but zero documentation file changes.
  • Fix: Add a section to the cosh-shell user guide (and Chinese translation if one exists) describing ECS RAM-role auto-detection, the 200-second wait limit, cancellation semantics, and the "save result unknown" state.
  • Test layer: raw_cli / shell_host (behavioral coverage exists; docs should match)

What looks good

  • IMDSv2 only, no downgrade: MetadataSession::start always acquires a token first and fails closed on any non-success token response; there is no IMDSv1 fallback path.
  • Secret containment: read_response returns an empty body for non-success statuses; ProbeError and registry responses carry only fixed classification strings.
  • Redirects/proxies disabled: The metadata reqwest client sets .no_proxy() and .redirect(Policy::none()), and tests reject redirects.
  • Fixed role path: ECS_RAM_ROLE_NAME is interpolated into a literal path; no user-controlled segment reaches the URL.
  • Shared 3-second deadline: MetadataSession::deadline bounds token acquisition plus role fetch via timeout_at.
  • Cancellation ownership: EcsProbeTask::Drop cancels and joins; ProbeChild::Drop sends SIGKILL and waits; protocol tests assert reaping.
  • ActiveRun panel cleanup: clear_active_auth_panel is called before send_auth_response, and the failure case is covered by active_run_ready_clears_auth_panel_before_sending_response.
  • MessageId append-only: auth_ecs_ids adds new IDs at the end of the auth segment, and i18n/mod.rs tests assert relative ordering against AuthEcsChecking.
  • Default provider naming: provider_name_is_taken, field_is_editable, step_back, and retry_field correctly skip or recover around the injected provider_id field.
  • Test coverage: New regressions span lib, protocol, and raw_cli layers, including the ECHILD/ignored-SIGCHLD boundary and the menu-probe cancellation race.

Validation and coverage statement

  • Static review only. No local cargo fmt, cargo clippy, or cargo test was executed for this PR because the review workspace does not contain a checkout of alibaba/anolisa.
  • The PR description reports passing ARM64 ALinux 3 focused tests (148 cases), cargo fmt --all -- --check, and cargo clippy -p cosh-shell --all-targets -- -D warnings for the shell crate. These claims were not independently verified in this run.
  • The pre-existing Gateway workspace Clippy failure mentioned in the PR description (cosh-gateway-app/src/checkpoint.rs) is outside the scope of this review and is correctly not addressed here.
  • Remaining risk: The PID-reuse finding and the async-blocking-disk-IO finding are runtime correctness issues that are unlikely to be caught by the current test suite in normal CI; they need targeted regressions.

Summary

This PR makes a well-scoped, security-conscious change to automate ECS RAM-role authentication. The metadata-handling code is significantly safer than the previous raw-TCP implementation. The main blockers before merge are the ProbeChild::drop PID-reuse race and the async-blocking-disk-IO issue in instance-ID resolution; the cache-write atomicity and RwLock poisoning issues should also be addressed. Documentation for the new user-visible flow is missing and should be added.

Recommended next step: Fix the P1/P2 findings, add the targeted regressions, and update the cosh-shell user guide; then re-request review.

@zhoukailong

Copy link
Copy Markdown
Collaborator

🔄 Incremental Code Review

由 reviewer 自动评审产生 | 扫描时间:2026-09-16 18:10 CST
评审范围:PR #3298 自上次评审以来的新增 commit diff(因状态文件未记录 commit SHA,本次按完整 diff 进行静态评审)。

总体结论

本次变更聚焦 cosh-ng/auth 自动化与 ECS RAM Role 认证流程,核心改动包括:

  • 将 ECS 元数据探测统一为 IMDSv2、有界超时、禁止代理/重定向/IMDSv1 降级的安全实现;
  • 引入可取消的独占探测任务 EcsProbeTask,明确区分 prepare/verify/configure 的生命周期与取消语义;
  • 按模板类型与已有配置决定默认 provider_id,减少首配命名步骤;
  • 将 registry prepare/verify 的响应从 authorized: bool 升级为 status/reason/error_code 结构。

代码在安全边界(token 标记 sensitive、错误响应不携带原始 body、凭据不外传)和所有权/取消语义上做了大量回归测试,整体方案合理。


Findings

P1 — Test cosh-ng CI 未通过

  • 位置src/cosh-ng/crates/cosh-shell/tests/raw_cli/agent_input.rs:46
  • 现象Test cosh-ng job 在 raw_cli::agent_input::raw_cli_routes_slash_bearing_han_prompt_before_shell_execution 失败(540 passed / 1 failed / 1 ignored)。日志显示 zsh 将含汉字的 prompt 当作命令执行并返回 no such file or directory
  • 本质:该测试文件不在本次 PR diff 中,失败看起来与 zsh 容器/环境相关,但当前候选的 required check 未绿。
  • 修复方向:在合并前重新触发或修复该 CI job;若确认是环境预置问题,请在 PR 描述或评论中注明并提交豁免/修复。
  • 应补测试:N/A(已有测试)。

P2 — 公共行为变更缺少文档同步

  • 位置:本次 diff 未触及 src/cosh-ng/README.mdREADME_zh.mddocs/design/shell-integration* 等文档。
  • 现象:PR 变更了用户可见的 /auth 流程(首配默认命名、ECS 自动检测与等待、取消/超时语义、registry prepare/verify 响应 schema),且 PR body 已勾选 "Public CLI, API, configuration, or documented behavior changed",但未见对应组件 README / user-guide 双语更新。
  • 本质:文档完整性不足,下游用户/集成方无法从文档获知新行为与错误码。
  • 修复方向
    1. src/cosh-ng/README.md / README_zh.md/auth 或 provider 配置章节补充:默认命名规则、ECS 自动认证流程、取消/超时行为、prepare/verify 新响应字段。
    2. 如存在独立的 user-guide,同步更新中英双语版本。
  • 应补测试:N/A。

P3 — instance_id 负向结果一旦缓存不再随 TTL 刷新

  • 位置src/cosh-ng/crates/cosh-core/src/provider/sysom.rs:196self.instance_id.get_or_init(resolve_instance_id).await)及 :265-308resolve_instance_id_cached
  • 现象resolve_instance_id 通过 tokio::sync::OnceCell 缓存结果。若首次 generate 时元数据服务瞬态不可用,OnceCell 会永久记住 None,后续即使本地 cache 文件 TTL 过期也不会重试。
  • 本质:负向结果与 TTL 机制脱节,provider 生命周期内实例 ID 可能持续缺失。
  • 修复方向:考虑在 resolve_instance_id_cached 返回前不将负向结果写入 OnceCell,或让 OnceCell 缓存一个带时间戳的值并在 TTL 过期时重新获取。
  • 应补测试cosh-core lib 层新增测试:首次获取失败 -> 时间推进超过 TTL -> 再次调用应重新探测。

P3 — WAIT_LIMIT 总预算未覆盖 Preparing 阶段

  • 位置src/cosh-ng/crates/cosh-shell/src/auth/ecs_poll.rs:2669EcsFlow::new / poll
  • 现象WAIT_LIMIT(200s)仅在 challenge 存在时才会设置到 flow.deadlineStage::Preparing 期间 deadlineNone,整体等待仅受每次 5s 的 OPERATION_LIMIT 限制。
  • 本质:虽然单条探测有界,但整体自动探测的“200 秒总预算”在首次确认 ECS 身份前并不生效,可能与 PR 描述有细微出入。
  • 修复方向:若 200s 总预算 intended 覆盖整个流程,请在 EcsFlow::new 中为 Preparing 也设置 deadline;否则在代码注释或文档中明确说明预算起算点。
  • 应补测试ecs_poll_tests 中增加 Preparing 阶段总预算耗尽场景。

正面观察

  • 安全ecs_metadata.rs 使用 IMDSv2 token、.no_proxy()、禁用重定向与重试、限制响应体 64 KiB、token 标记 set_sensitive(true),且 ProbeError 不携带上游响应内容。
  • 取消/所有权EcsProbeTask 通过独立线程、UnixStream wake、process group SIGKILL 与 ECHILD 处理,确保取消后子进程与 worker 都被回收;回归测试覆盖 50 次取消循环和 ignored SIGCHLD 场景。
  • i18n:新增的 auth_ecs_ids 通过 segment 宏追加到末尾,并更新了 i18n/mod.rs 的相对位置断言,符合“只能在末尾追加 MessageId”的约束。
  • 错误分类:将 CredentialStatus::NotReady 细分为 RoleMissing / CredentialsExpired,并在 UI 上区分“等待授权”与“等待刷新”。

验证与覆盖声明

  • 已执行验证
    • 静态评审 PR diff(7033 行,41 个文件)。
    • 拉取 GitHub CI 状态:Test cosh-ng 失败(agent_input::raw_cli_routes_slash_bearing_han_prompt_before_shell_execution);Test cosh-ng fast checksBuild cosh-ng release、commit lint、PR checks 通过。
  • 未执行验证(受环境限制):
    • 未在本地运行 cargo fmt/clippy/test(无仓库克隆)。
    • 未运行真实 ECS / IMDSv2 环境验证。
    • 未执行 shell_host / workspace 全量测试。
  • 剩余风险:CI 失败根因待确认;文档缺失可能影响用户预期;instance_id 负向缓存与 Preparing 总预算边界属于低概率但真实的语义风险。

下一步建议

  1. 优先确认并解决 Test cosh-ng CI 失败(重跑或修复环境)。
  2. 补充 src/cosh-ng/README.md / README_zh.md 及 user-guide 中关于 /auth 自动化流程与 registry 响应 schema 的说明。
  3. 视排期处理两个 P3(负向缓存、Preparing 总预算),至少补注释说明。

@kongche-jbw kongche-jbw left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Review baseline: 26aced8550e551abf092a9e6ae4fafa5ad634ba0...e01e6a5a45162bc88579f4a5d33e1666cca5ec8d

[P1] 避免在确认子进程所有权前按 PGID 发送信号

src/cosh-ng/crates/cosh-shell/src/adapter/ecs_probe.rs:113
在继承 SIGCHLD=SIG_IGN 时,probe 退出后会被内核自动回收;此时 PID/PGID 可能已复用,
但 drop 仍先执行 kill(-pid, SIGKILL),直到之后的 try_wait() 才看到 ECHILD
在 PID 压力下,取消或正常完成一次短探测可能杀死无关进程组。现有 ignored-SIGCHLD
测试只断言原 PID 消失,没有验证复用后的进程组不被信号命中。

Possible direction: 在发信号前用不会复用的生命周期句柄确认并保持所有权(Linux 可用
pidfd,或由专用 helper 持有并回收进程组),且一旦观察到 ECHILD 就禁止再按数值 ID
发信号。补充继承 SIGCHLD=SIG_IGN 并强制 PID/PGID 复用的 protocol 回归,断言
sentinel 进程组存活。

@SunnyQjm

SunnyQjm commented Sep 16, 2026

Copy link
Copy Markdown
Collaborator Author

PR number: #3298
head_sha: e01e6a5
reviewed_at: 2026-09-16T15:48:21Z

Findings

  • [P1] 为 integration test 新增未登记的 production public APIsrc/cosh-ng/crates/cosh-shell/src/adapter/ecs_probe.rs:23src/cosh-ng/crates/cosh-shell/src/adapter/ecs_probe.rs:63src/cosh-ng/crates/cosh-shell/src/adapter/public.rs:9
    EcsProbeTaskCoshCoreAdapter::start_ecs_probe 被公开并经 adapter facade 导出;补丁中唯一 crate 外调用方是 tests/protocol/ecs_probe.rs:7,且输入中的 public API 台账未登记该 surface。这违反“不得为了测试新增 production pub”。应改为 pub(crate) 并将生命周期测试迁至 crate-local component test;若确有外部调用方,则需登记 owner、caller、稳定性及 inventory 计数。

Validation

未执行工具或测试。剩余风险是跨 cosh-core/cosh-shell 配套变更的独立 commit 边界无法从补丁确认。

@SunnyQjm

SunnyQjm commented Sep 16, 2026

Copy link
Copy Markdown
Collaborator Author

PR number: #3298
head_sha: e01e6a5
verified_at: 2026-09-16T15:55:56Z
tier: T1

1. 验证范围

  • HEAD:e01e6a5a45162bc88579f4a5d33e1666cca5ec8d,匹配作业。
  • Backend:container / arm64。
  • 环境 RUN_ID:20260916T155514Z-f4c27114
  • 清理 RUN_ID:20260916T155521Z-b6c3f487,状态 cleaned
  • doctorcontended=true,但不是本次阻塞原因。
  • 未执行原生编译或测试命令。

2. 结果分类

Blocked

容器启动失败:

default kernel not configured for architecture arm64

宿主无 Docker/Podman 后端,且规则禁止将 PR 编译和测试降级到 host,因此没有首跑或复跑结果。

3. Findings 验证

  • [P1] 为 integration test 新增未登记的 production public API:静态证实
    • EcsProbeTask 声明为 pubsrc/cosh-ng/crates/cosh-shell/src/adapter/ecs_probe.rs:23
    • start_ecs_probe 声明为 pubsrc/cosh-ng/crates/cosh-shell/src/adapter/ecs_probe.rs:63
    • 类型经 facade 导出:src/cosh-ng/crates/cosh-shell/src/adapter/public.rs:9
    • 唯一 crate 外 Rust 使用位于 tests/protocol/ecs_probe.rs:7;其余使用均在 crate 内。
    • inventory 的显式分类表未登记 adapter::EcsProbeTask,仅会落入通用 owner-public-surface
    • 因容器不可用,未能动态验证收窄为 pub(crate) 后的编译结果。

4. 补充测试

无。T1 作业未修改源码。

5. 证据索引

  • cosh-lab-runs/20260916T155514Z-f4c27114/result.json
  • cosh-lab-runs/20260916T155514Z-f4c27114/commands.log
  • cosh-lab-runs/20260916T155521Z-b6c3f487/result.json
  • 截图:无。

6. 未运行 scope

  • crates/cosh-shell/scripts/inventory-public-api.sh
  • crates/cosh-shell/scripts/check-layout.sh
  • cargo check -p cosh-shell --all-targets
  • cargo test -p cosh-shell --test protocol ecs_probe::

7. 结论

Blocked — container 后端缺少 arm64 默认内核,无法执行 focused scope;静态评审的 P1 结论无需修正。

@zhoukailong

Copy link
Copy Markdown
Collaborator

🔄 Incremental Code Review

Automated review by reviewer (QoderWake)
Scan time: 2026-09-16 20:12 CST (UTC 2026-09-16T12:12:22Z)
PR: #3298feat(cosh-ng): [core,shell] automate auth
Head: e01e6a5a45162bc88579f4a5d33e1666cca5ec8d
Incremental scope: diff since last automated review at 2026-09-16 18:10 CST

总体结论

本次扫描检测到 PR #3298updatedAt 已更新(2026-09-16T10:15:45Z2026-09-16T11:43:24Z),但 自上次自动化评审以来没有新增 commit;所有 6 个 commit 的 committedDate 均早于上次评审时间。updatedAt 变化来源于作者 @SunnyQjm2026-09-16T11:43:24Z 发布的 verification 评论(codex-auto-review:verification)。

因此本次增量评审不引入新的代码 findings,仅对已有 findings 的状态进行跟踪更新。


Findings 状态更新

P1 — 为 integration test 新增未登记的 production public API(已证实,仍待修复)

  • 位置src/cosh-ng/crates/cosh-shell/src/adapter/ecs_probe.rs:23src/cosh-ng/crates/cosh-shell/src/adapter/ecs_probe.rs:63src/cosh-ng/crates/cosh-shell/src/adapter/public.rs:9
  • 作者验证结论verification 评论确认 EcsProbeTaskstart_ecs_probe 为公开 API,且唯一 crate 外调用方为 tests/protocol/ecs_probe.rs;inventory 已将其标记为 owner-public-surface,classify_before_acceptance,尚未获得明确稳定性分类。
  • 当前状态:问题存在且已被证实,但源码未被修改修复。该 P1 仍然开放。
  • 修复方向:将 EcsProbeTask / start_ecs_probe 改为 pub(crate),把协议/生命周期测试移至 crate-local component test;若确需公开,则在 public API 台账中登记 owner、稳定性分类并更新 inventory 计数。

此前正式评审(2026-09-16 18:10 CST)中仍未关闭的 Findings

以下 findings 在本次扫描中未观察到对应代码修复或状态更新,仍保持有效:

  1. P1 — Test cosh-ng CI 未通过

    • src/cosh-ng/crates/cosh-shell/tests/raw_cli/agent_input.rs:46
    • raw_cli::agent_input::raw_cli_routes_slash_bearing_han_prompt_before_shell_executionTest cosh-ng job 中失败(540 passed / 1 failed / 1 ignored)。合并前需重跑或修复该 required check。
  2. P2 — 公共行为变更缺少文档同步

    • PR 变更了用户可见的 /auth 流程、ECS 自动检测与等待、取消/超时语义、registry prepare/verify 响应 schema,但 src/cosh-ng/README.md / README_zh.md 及 user-guide 未见对应更新。
  3. P3 — instance_id 负向结果一旦缓存不再随 TTL 刷新

    • src/cosh-ng/crates/cosh-core/src/provider/sysom.rs:196 / :265-308
    • tokio::sync::OnceCell 会永久记住 None,负向结果与 TTL 机制脱节。
  4. P3 — WAIT_LIMIT 总预算未覆盖 Preparing 阶段

    • src/cosh-ng/crates/cosh-shell/src/auth/ecs_poll.rs:2669 附近
    • Stage::Preparing 期间 deadlineNone,200 秒总预算仅在 challenge 存在后生效。

验证与覆盖声明

  • 已执行验证
    • 比对 PR commit 列表与上次评审时间戳,确认无新增 commit。
    • 读取并分析作者最新 verification 评论,确认其对 P1 public API finding 的验证结论。
  • 未执行验证
    • 未对空增量 diff 执行新的 cargo fmt/clippy/test
    • 未重新运行真实 ECS / IMDSv2 环境验证。
  • 剩余风险
    • 已证实的 P1 public API 问题尚未修复。
    • 此前正式评审中的 CI 失败、文档缺失、缓存语义与总预算边界问题仍开放。

下一步建议

  1. 修复 P1 public API 问题:收窄 EcsProbeTask / start_ecs_probe 可见性,或完成台账登记。
  2. 继续推进 P1 CI 失败 的根因确认与修复。
  3. 补充 P2 文档(README / user-guide 中英双语)。
  4. 对两个 P3 至少补充注释说明 intended 语义,建议优先修复负向缓存问题。

@SunnyQjm

Copy link
Copy Markdown
Collaborator Author

CI 首跑失败与复跑记录(候选 e01e6a5a45162bc88579f4a5d33e1666cca5ec8d

首跑 Test cosh-ng 失败,唯一失败用例是 agent_input::raw_cli_routes_slash_bearing_han_prompt_before_shell_execution(zsh 变体);同一 target 其余为 raw_cli 540 passed / 1 failed / 1 ignored。失败输出显示中文提示在拦截生效前被 zsh 直接执行,并伴随 eecho after-path-prompteexit 这类按键重复迹象。原始日志已保留。

归因核对

  • 本 PR 相对 origin/main 未改动 raw_inputshell_hostagent 任一路径(git diff --name-only origin/main...HEAD -- … 为空),而失败用例正落在这些交互路径上。
  • 该用例使用 fake adapter,不进入 /auth;本轮改动集中在 auth 相位、菜单 prepare 与 ECS 任务生命周期。
  • 同一用例在前一候选 9de696753Test cosh-ng 中通过(raw_cli 540 passed / 0 failed)。
  • 该用例以固定 sleep(300ms/50ms/500ms)注入输入,没有 readiness 握手,属时序敏感设计;失败形态与并行负载下 zsh 行编辑尚未就绪一致。

复跑

复跑同一 job 成功:agent_input::raw_cli_routes_slash_bearing_han_prompt_before_shell_execution ... ok,raw_cli 541 passed / 0 failed / 1 ignored,shell_host 216 passed / 0 failed / 1 ignored;当前候选其余已执行检查均为 SUCCESS。

结论边界

首跑失败按原样保留,复跑成功不表述为该用例通过,也不宣称已排除。按仓库分类口径,suspected-flaky 需要主干同用例先例,而近期 main 的 Test cosh-ng 均被 Detect Changes 跳过(已核 job 级 skipped),先例不可得;本地隔离容器复现尝试因供给超时(exit 124)未取得对照数据,容器已清理为 cleaned,不用未完成的本地运行充当证据。

本轮未修改该 zsh 用例或其 harness(超出本任务授权范围),未修改 CI 时限、通用输入引擎或 Gateway。如需推进该用例的时序稳定性,建议另开 issue 单独跟踪固定 sleep 与 readiness 握手的缺口。

@zhoukailong

Copy link
Copy Markdown
Collaborator

🔄 Incremental Code Review

Automated review by reviewer (QoderWake)
Scan time: 2026-09-16
PR: #3298feat(cosh-ng): [core,shell] automate auth
Head: e01e6a5a45162bc88579f4a5d33e1666cca5ec8d

Scope note

本次扫描触发原因是 PR 的 updatedAt2026-09-16T12:51:54Z)晚于上次记录评审时间戳(2026-09-16T12:12:49Z)。但经核对,自上次评审以来没有新增 commit,当前 head 仍为 e01e6a5a45162bc88579f4a5d33e1666cca5ec8d2026-09-16T09:48:12Z)。本次 updatedAt 变化来源于 PR 评论更新(CI 结果记录),而非代码变更。

因此,本次无新增 diff 需要评审。前序自动化评审结论(P0–P3 findings)仍然有效。


验证与覆盖声明

维度 状态 说明
增量代码评审 无需评审 无新增 commit / 无代码变更
新增 diff 行数 0 本次更新仅涉及评论
前序评审结论 仍然有效 见 #issuecomment-5697216007 及更早 review
五维评审(设计/功能/文档/安全/质量) 不适用 无新增代码可供评审

结论

PR 本次更新为非代码变更,不产生新的 finding。建议维护者继续等待 CI 全量结果与人工终审后再决定是否合并。


This review was produced by reviewer (automated code review).

@SunnyQjm
SunnyQjm force-pushed the fix/cosh-3278-auth-ecs-polling branch from e01e6a5 to e1ddfea Compare September 16, 2026 16:16
@SunnyQjm

Copy link
Copy Markdown
Collaborator Author

已推送 e1ddfeaf490bc8056828742d5cd74a0b5453360c(基于 3a39ea6de,七个提交 range-diff 全等价)。逐条处置如下。

已修复

[P1] 在确认所有权前按 PGID 发信号(#pullrequestreview-5221350395)

核实成立。回收入口原先无条件 kill(-pid, SIGKILL),而在继承 SIGCHLD=SIG_IGN 时探测退出后由内核回收,PID 及同名 PGID 可被复用。这与上一轮 ECHILD 是同一域的第二处,按熔断口径改为固定该域不变量而非再补分支:只有当子进程被证明仍未被回收时,才允许按数值 PID/PGID 发信号;一旦观察到已由本流程 reap 或被内核自动回收,立即停止按数值 ID 发信号并结束回收;其他 wait 错误同样不发信号,但保留所有权继续观察,不谎报已清理。

  • 实现:crates/cosh-shell/src/adapter/ecs_probe.rstry_wait() 取证,再决定是否发一次信号。
  • 回归:新增按 try_wait() 四格(Ok(None) / Ok(Some) / Err(ECHILD) / 其他 Err)的判定断言,先 RED 后 GREEN;并做变异校验——把判定改回“无条件发信号”会让该断言失败。既有继承 SIGCHLD=SIG_IGN 的 protocol 回归保持通过(7 项)。
  • 剩余边界如实声明:Ok(None) 与发信号之间仍有微秒级窗口,若子进程正好此刻退出且被立即回收并复用 PID,信号仍可能落到复用者。彻底消除需 pidfd 或由 helper 持有进程组,代价是平台条件编译与新的 unsafe 系统调用路径;本轮先消除可确定复现的路径,是否升级请评审裁决。SIGCHLD 默认处置下子进程保留 zombie,PID/PGID 在被 reap 前不会复用,该窗口不存在。
  • 未做:未实现强制 PID/PGID 复用的 protocol 回归。该断言需 root 写 ns_last_pid 才能确定性构造,在 CI 非 root 环境只会跳过;因此改用可直接断言的判定矩阵,并且不为此在生产代码新增测试注入接口。

[P1] 为 integration test 新增未登记的 production public API(#issuecomment-5696738555、#issuecomment-5696871913)

登记缺失成立:adapter::EcsProbeTask 此前落在台账兜底分类 owner-public-surface,classify_before_acceptance。已在 crates/cosh-shell/scripts/inventory-public-api.sh 显式登记为 private-candidate(与 adapter::CoshCoreAdapter 等同组,动作为 make_crate_private_after_test_or_runtime_migration),台账兜底项由 2 降为 1,剩余一项 shell_host::ShellIntegration 非本 PR 引入,未顺手改动。

不采纳“收窄为 pub(crate) 并把生命周期测试迁到 crate-local component test”:src/cosh-ng/AGENTS.md 明确要求 adapter/control protocol 测试进入 protocol layer,而该 layer 是 crate 外的 integration target,只能访问公开面。两条规则冲突时按组件测试分层执行,并用台账登记稳定性分类,这也是该 finding 给出的第二条路径。若维护者希望改为 crate-private,需要同时调整组件测试分层规则,属另一次改动。

不改代码,附依据

  1. P1 CI 失败:已处理并公开记录(#issuecomment-5697727495)。首跑失败保留;复跑同 job 成功(raw_cli 541 passed / 0 failed / 1 ignored、shell_host 216 passed / 0 failed / 1 ignored)。本 PR 相对主干未改动 raw_input/shell_host/agent,该用例走 fake adapter、不经 /auth,且以固定 sleep 注入输入、无 readiness 握手。复跑成功不表述为该用例通过。
  2. P2 文档未同步:已逐项核对 README.md / README_zh.mddocs/user-guide/{en,zh}。现有描述只有“用 /auth 选择或更新 provider 认证”这类概括句,以及 auth_source = "ecs_ram_role" 配置说明;本 PR 未新增或修改 CLI 命令、flag、配置项,auth_source 语义不变,因此没有任何句子因本 PR 变得不准确,不存在需要修正的过期描述。改变的是 /auth 内部交互步骤(更少人工步骤),属新增体验说明,按 specs/documentation-standard.md 不在强制同步范围。若希望补一段 ECS 首配体验说明,建议单独文档 PR 完成中英双语,避免与认证修复混在同一提交链。
  3. P3 instance_id 负向结果不随 TTL 刷新:现象部分成立,但非本 PR 引入。基线为 instance_id: Option<String> 且在构造器同步解析一次,同样在实例生命周期内不重试;本 PR 改为 OnceCell<Option<String>> 只是把解析时机移出构造器以消除阻塞网络调用,一次性语义未变。磁盘缓存(含负结果的空文件)仍受 3 小时 TTL 约束。已补一行注释固定该语义,避免后续误读为按 TTL 刷新。
  4. P3 WAIT_LIMIT 未覆盖 Preparing:行号 ecs_poll.rs:2669 不存在(文件不足该长度),但所指行为确实如此,且是设计意图:WAIT_LIMIT 是“等待用户在控制台完成角色授权”的总预算,因此从确认 ECS 挑战起算;Preparing 只做一次 prepare,由 5 秒单次操作预算约束,结果为 manual / challenge / 终态失败,不存在无界等待。已补一行注释说明该边界。

验证与未运行范围

ARM64 ALinux 3 容器:探测判定 1 项、protocol ecs_probe:: 7 项、认证过滤 lib 12 + bin 157 + raw_cli 20 通过;fmt、clippy -p cosh-shell -p cosh-core --all-targets、layout、test inventory/necessity 通过。本轮未运行完整 crate/workspace、真实 ECS 或新视觉验收;末次独立自动复审因服务不可用未执行,已改为源码级手动回核,不宣称独立终审通过。新候选等待 CI 重新验证,未自动合并。

@SunnyQjm

SunnyQjm commented Sep 16, 2026

Copy link
Copy Markdown
Collaborator Author

PR number: #3298
head_sha: e1ddfea
reviewed_at: 2026-09-17T01:58:00Z

Findings

  • [P1] 检查与发信号之间仍存在 PGID 复用竞态src/cosh-ng/crates/cosh-shell/src/adapter/ecs_probe.rs:126-130
    try_wait() == Ok(None)kill(-pid, SIGKILL) 不是原子操作。继承 SIGCHLD=SIG_IGN 时,子进程可在两者之间退出并被立即回收,数值 PID/PGID 随后可能被无关进程组复用,造成误杀。静态返回值矩阵和现有 ignored-SIGCHLD 测试均未覆盖该交错窗口。应改用不会依赖可复用数值标识的生命周期/信号机制,并补强制复用后的 sentinel 回归。

  • [P1] 为 integration test 扩大 production public APIsrc/cosh-ng/crates/cosh-shell/src/adapter/ecs_probe.rs:23-63src/cosh-ng/crates/cosh-shell/src/adapter/public.rs:8-9src/cosh-ng/crates/cosh-shell/tests/protocol/ecs_probe.rs:7
    EcsProbeTask、其方法及 CoshCoreAdapter::start_ecs_probe 经 root adapter facade 公开,但补丁中唯一 crate 外调用方是 protocol integration test。登记为 private-candidate 不满足“不得为了测试新增 production pub”的规则。应收窄为 crate-private并将生命周期测试迁至 crate-local component test;若确属外部契约,需要提供真实外部 caller 和稳定性依据。

验证

未执行工具或测试。现有 focused CI 不能证伪上述竞态;完整 crate/workspace 与确定性 PID/PGID 复用验证未运行。

@SunnyQjm

SunnyQjm commented Sep 16, 2026

Copy link
Copy Markdown
Collaborator Author

PR number: #3298
head_sha: e1ddfea
verified_at: 2026-09-17T02:09:03Z
tier: T1

1. 验证范围

  • HEAD:匹配 e1ddfeaf490bc8056828742d5cd74a0b5453360c
  • Backend:Apple container
  • RUN_ID:20260917T015955Z-d8b9d814
  • doctor:contended=true
  • 已运行:
    • cargo test --locked --package cosh-shell --test protocol ecs_probe:: -- --test-threads=1
    • crates/cosh-shell/scripts/inventory-public-api.sh
    • crates/cosh-shell/scripts/check-layout.sh
    • cargo check -p cosh-shell --all-targets

test planorigin/main 分叉和 .cosh-lab 文件干扰而过度扩张,因此沿用作业指定 scope。

2. 结果分类

  • 通过:protocol ECS probe,7 passed、0 failed。
  • 通过:public API inventory。
    • 首跑及复跑均因容器缺少 rg 退出 2。
    • 在容器安装 ripgrep 后通过;属于确定性环境前置问题,不是 flaky。
  • 通过:layout audit。
  • 通过cargo check --all-targets
  • suspected-flaky:无。

3. Findings 验证

  1. 证实:PGID 复用竞态仍存在。
    ecs_probe.rs:126-130try_wait() == Ok(None) 后才调用 kill(-pid, SIGKILL),两者不具原子性。子进程可在间隙退出并被自动回收,数值 PID/PGID 随后可能被复用。现有测试只验证状态矩阵和忽略 SIGCHLD 的一般清理流程,没有强制覆盖该交错窗口。

  2. 证实:为 integration test 扩大了 production public API。
    EcsProbeTaskadapter/mod.rs:47adapter/public.rs:9lib.rs:2 对 crate 外公开;start_ecs_probe 也是 public。搜索结果显示 production 使用方位于 crate 内部 auth/ecs_poll.rs:615,crate 外调用仅见 tests/protocol/ecs_probe.rs:7。inventory 同时将其标记为 private-candidate

4. 补充测试

无;T1 未新增测试源码。

5. 证据索引

无截图。命令证据保存在 .cosh-lab/runs/

6. 未运行 scope

  • 确定性低 pid_max PID/PGID 强制复用复现
  • cosh-shell 全量 crate tests
  • cosh-core tests
  • workspace tests、clippy、fmt

7. 结论

No-Go:focused scopes 虽为绿色,但两项 P1 finding 均被证实;静态评审结论不需要修正。

@zhoukailong

Copy link
Copy Markdown
Collaborator

🔄 Incremental Code Review

This review was produced automatically by reviewer (QoderWake).
Review time: 2026-09-17T00:39:03+08:00

评审范围: PR #3298 自上次评审后的当前提交集,42 files,+5414/-465 lines。
整体结论: COMMENT — 未发现 P0/P1 阻塞问题。


Findings

P2 - 文档完整性

  • [src/cosh-ng/crates/cosh-core/src/registry/auth.rs:6-50, 178-206] 认证 registry 的跨组件协议改动未同步设计文档。
    • 问题本质:prepare 现在输出 mode、实例标识与 ECS 授权数据;verify 从单个 authorized 布尔值扩展为 ready / not_ready(带 reason)及分类错误。该 core/shell 契约决定取消、重试和保存行为,但本 PR 未变更 src/cosh-ng/docs/design/
    • 可复现场景:后续消费者依据原 authorized 响应实现或维护 registry 协议时,无法从规范获知新增状态、错误分类及其语义。
    • 最小修复方向:补充或更新相应的 cosh-ng 设计文档,明确 auth.prepare / auth.verify 的请求、响应、状态转换、超时和错误语义;若该协议明确仅为私有实现,应在设计文档中界定其私有边界。
    • 应补回归测试:在 protocol 层保留并扩展 wire-shape/生命周期断言,使文档化的状态与 core 输出一致。

验证与覆盖声明

  • 已执行验证:静态审阅完整 PR diff;读取根目录与 src/cosh-ng/AGENTS.md、独立评审与文档规范;GitHub CI 中 Test cosh-ngTest cosh-ng fast checksBuild cosh-ng release 均为 SUCCESS。
  • 已覆盖角度:方案、功能、文档、安全、代码质量;重点核对 IMDSv2 固定端点、禁代理/重定向、响应大小与时限、凭据脱敏、进程组回收、取消/保存竞态和 i18n 枚举追加。
  • 未覆盖范围与剩余风险:本地 PR 快照检出受远端连接问题阻断,未能独立运行 cargo 测试;未在真实 ECS 与 macOS 环境执行端到端验证。CI 成功不替代上述环境覆盖。

剩余风险集中于真实 ECS 元数据服务与 PTY/信号时序的环境差异。

@zhoukailong

Copy link
Copy Markdown
Collaborator

This automated review was generated by reviewer for the latest push to PR #3298.

🔄 Incremental Code Review — PR #3298

Title: feat(cosh-ng): [core,shell] automate auth
Review time: 2026-09-17
Scope: 7 new commits since last review (lastReviewedUpdatedAt: 2026-09-17T02:09:06Z; current PR updatedAt: 2026-09-17T03:20:53Z). Diff: +5,485 / -465 lines across 42 files in src/cosh-ng/.


Findings

P1 — Process-global SIGCHLD disposition is mutated without serialization or restoration

  • Location: src/cosh-ng/crates/cosh-shell/src/adapter/ecs_probe.rs, keep_children_waitable() (~line 122)
  • Issue: Every start_ecs_probe() reads and, if inherited SIGCHLD is SIG_IGN, rewrites the process-global disposition to SIG_DFL via libc::signal(). The original disposition is never restored. Hosts or embedding processes that rely on SIGCHLD=SIG_IGN for kernel auto-reap will accumulate zombies for their own children after the first probe.
  • Evidence: Code comment at the function acknowledges the trade-off: "a host relying on an ignored SIGCHLD to auto-reap its own children will retain zombies after the first probe."
  • Risk: Compatibility regression for integrations (test harnesses, launchers, containers) that spawn background children and depend on implicit reaping.
  • Fix direction: Either (a) serialize keep_children_waitable() with a global lock and restore the original disposition after the probe workforce terminates, or (b) add a documented runtime contract that callers must tolerate SIGCHLD=SIG_DFL, backed by an explicit integration test that asserts the host-child zombie behavior. If pidfd remains unsupported, the current POSIX-only path is defensible but the one-way mutation needs clearer guardrails.
  • Regression layer: protocol / shell_host integration tests should exercise the host-child zombie scenario.

P2 — Documentation not updated for public CLI/auth behavior change

  • Location: component README and user-guide (no changes in diff)
  • Issue: The PR removes the manual "I have authorized this ECS instance" confirmation, introduces automatic ECS credential waiting, and changes the /auth menu flow. AGENTS.md requires CLI/configuration changes to update the component README and user-guide bilingually.
  • Fix direction: Add a user-guide section describing the new automatic ECS RAM Role flow, cancel/timeout behavior, and what "Save result is unknown" means. Update the cosh-shell/cosh-core README with the new wire response shapes (status: ready/not_ready with reason, error_code).

P2 — TOCTOU window between try_wait() and kill(-pid, SIGKILL)

  • Location: src/cosh-ng/crates/cosh-shell/src/adapter/ecs_probe.rs, ProbeChild::drop() (~line 152)
  • Issue: drop checks self.0.try_wait(), then signals kill(-pid, SIGKILL) only if the child is still unreaped. If another thread in the same process reaps the child between the check and the kill, the numeric PID/PGID may already be reused by an unrelated process group, causing the signal to hit the wrong group.
  • Evidence: The surrounding keep_children_waitable() and owns_signalable_group() mitigate the kernel-SIG_IGN-reap case but not a concurrent waitpid() from another thread.
  • Fix direction: Hold a global probe mutex around the try_wait()/kill() critical section so no other thread in the process can reap the child while the group signal is pending; or document and assert that no other code path waits on probe PIDs.
  • Regression layer: protocol test that races a second waiter against ProbeChild::drop.

P3 — 250 ms configure-child exit budget may be aggressive

  • Location: src/cosh-ng/crates/cosh-shell/src/adapter/cosh_core_registry.rs, registry_query_classified() (~line 208)
  • Issue: After receiving a configure response, the adapter waits only 250 ms for the child to exit before calling terminate_and_reap_process(). If the core child performs non-trivial cleanup (fsync config, close handles), it can be killed mid-cleanup.
  • Fix direction: Consider a slightly longer grace period (e.g., 1 s) with a tunable, or reap cooperatively by closing stdin/stdout first and then waiting.

P3 — reqwest::Client is rebuilt for every metadata probe

  • Location: src/cosh-ng/crates/cosh-core/src/provider/sysom/ecs_metadata.rs, MetadataSession::start() (~line 234)
  • Issue: Each probe_ecs_ram_role() / detect_ecs_auth_challenge() constructs a fresh client, connection pool, and TLS context. With a 2-second poll interval over 200 seconds, this creates ~100 short-lived clients.
  • Fix direction: Build one bounded client per provider or per probe worker and reuse it across refresh cycles.

P3 — Zone/instance-id validation rejects valid future identifiers

  • Location: src/cosh-ng/crates/cosh-core/src/provider/sysom/ecs_metadata.rs, MetadataSession::text() (~line 276)
  • Issue: Identity text is rejected if it contains any character outside [A-Za-z0-9-]. Current Alibaba Cloud zone IDs match this set, but the contract is tighter than the metadata service itself and could break if a future zone ID contains _ or another allowed character.
  • Fix direction: Validate only that the value is non-empty, does not contain whitespace/control characters, and is not an obvious HTML error page; leave stricter validation to the consumer.

P3 — Saved provider type normalization changes displayed identity

  • Location: src/cosh-ng/crates/cosh-shell/src/auth/provider_management.rs, ExistingProvider::from(CoreSavedProvider) (~line 97)
  • Issue: Saved providers with provider_type "openai" or "generic" are silently normalized to "openai_compat". This changes what users see in provider management and could mask the original configured type.
  • Fix direction: Document the normalization in the user-guide and add a regression test verifying that editing a saved "openai" provider still round-trips to the correct template fields.

What looks good

  • Security: IMDSv2-only, no proxy, no redirects, no retries, no IMDSv1 fallback, token marked set_sensitive(true), error responses never parsed/retained, and safe error classification without leaking credentials.
  • Tests: Extensive coverage across lib (ecs_metadata unit tests), logic (ecs_poll_tests.rs), protocol (tests/protocol/ecs_probe.rs), and raw_cli (tests/raw_cli/auth_ecs.rs) layers; includes cancel/timeout/ECHILD/SIG_IGN regressions.
  • i18n: New MessageId segment is appended at the end of the enum (AuthEcsRefreshing is ALL.len() - 1), with bilingual strings.
  • Public API inventory: EcsProbeTask is registered as private-candidate in inventory-public-api.sh.
  • State machine: EcsFlow correctly distinguishes waiting-for-role vs waiting-for-refresh, treats configure results as non-cancellable, and resolves unknown save outcomes authoritatively.

Verification and coverage statement

  • Static review only. No local compilation or test execution was performed because the host toolchain is Rust 1.75.0, while src/cosh-ng/rust-toolchain.toml requires Rust 1.88+.
  • Cross-validation: Findings were checked against the PR diff and src/cosh-ng/AGENTS.md (fetched from main). No local codebase checkout was used.
  • Covered dimensions: design soundness, functional paths, security boundaries, i18n ordering, documentation completeness, code quality.
  • Not covered: Full workspace cargo clippy --all-targets, cargo test --workspace, and real ECS PTY verification could not be run locally.
  • Residual risk: The global SIGCHLD mutation and the try_wait/kill race are design-level risks that the current tests do not fully exercise under concurrent host waiters; a Linux/macOS integration run with an external child spawner is needed for confidence.

Next steps

  1. Confirm whether the SIGCHLD trade-off is acceptable for the intended host environments, or add serialization + restoration.
  2. Add README/user-guide updates for the new auth flow.
  3. Re-run cargo clippy -p cosh-shell -p cosh-core --all-targets -- -D warnings and the protocol test target on a Rust 1.88+ host.

@kongche-jbw kongche-jbw left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Review baseline: c648fc4fd83f...f794aad21ff8

[P1] 不要把任意 SIGCHLD handler 当作可回收性保证

src/cosh-ng/crates/cosh-shell/src/adapter/ecs_probe.rs:131 会保留所有非
SIG_IGN 处置,但常见的 SIGCHLD handler 会在回调中调用
waitpid(..., WNOHANG) 主动回收子进程。若它在第 157 行的
try_wait() == Ok(None) 与第 161 行的组信号之间回收 probe,PID/PGID 仍可被
复用,kill(-pid, SIGKILL) 仍可能终止无关进程组。新增测试只安装空 handler,
没有覆盖这种实际的 reaper。

Possible direction: 仅在能结构性保证组 leader 不会提前被回收时按数值 PGID
发信号;否则拒绝启动,或由不会提前退出的 supervisor 持有组生命周期。请补一个
handler 实际调用 waitpid(WNOHANG) 的 protocol 回归,并在检查后退出/复用交错下
断言 sentinel 进程组存活。

@SunnyQjm

SunnyQjm commented Sep 17, 2026

Copy link
Copy Markdown
Collaborator Author

PR number: #3298
head_sha: f794aad
reviewed_at: 2026-09-17T04:02:57Z

Findings

  • [P1] 自定义 SIGCHLD reaper 仍可破坏 PGID 所有权src/cosh-ng/crates/cosh-shell/src/adapter/ecs_probe.rs:117-161
    keep_children_waitable() 仅改写 SIG_IGN,却保留任意自定义 handler。若 handler 调用 waitpid(..., WNOHANG),它可在 try_wait() == Ok(None)kill(-pid, SIGKILL) 之间回收 probe;数值 PGID 随后可能复用并导致无关进程组被误杀。现有 protocol 测试只安装空 handler,未覆盖真实 reaper。应在无法保证独占回收权时拒绝按数值 PGID 发信号,或采用不受进程全局 waiter 干扰的生命周期设计,并补 reaping-handler 交错回归。

  • [P1] 为 integration test 扩大 production public APIsrc/cosh-ng/crates/cosh-shell/src/adapter/ecs_probe.rs:21-64src/cosh-ng/crates/cosh-shell/src/adapter/public.rs:6-10src/cosh-ng/crates/cosh-shell/tests/protocol/ecs_probe.rs:7
    EcsProbeTask、其方法及 start_ecs_probe 经 root facade 对 crate 外公开,而补丁中唯一外部调用方是 protocol integration test,违反“不得为了测试新增 production pub”。将其加入 inventory 脚本的 private-candidate 不能替代真实外部 caller;同时分类计数也未同步至 public-api-inventory.mdcurrent-audit.md。应收窄为 crate-private并迁移测试,或提供稳定外部契约依据并完整同步台账。

验证

未执行工具或测试。当前 CI 绿色不能覆盖 SIGCHLD reaper/PID 复用交错;完整 crate/workspace 回归亦未独立验证。

@zhoukailong

Copy link
Copy Markdown
Collaborator

🔄 Incremental Code Review

由 reviewer 自动评审产生
扫描时间:2026-09-17T12:10:00+08:00(自动化任务 anolisa-pr-review-scanner

范围说明

  • 状态文件中记录的上次评审 updatedAt2026-09-17T03:29:07Z
  • 当前 PR updatedAt2026-09-17T03:40:03Z
  • 当前 head SHA:f794aad21ff86dc52fd50be89a97ddb3d55cbaa3
  • 自上次评审以来新增 commit:0 个(commit 列表中最新的 f794aad21 提交时间为 2026-09-17T03:16:00Z,早于上次评审时间)

因此,本次增量评审未发现新增代码 diff。PR 的 updatedAt 变化可能来自评论、描述编辑或其他元数据更新,未引入新的待审代码。

Findings(按 P0–P3 分级)

无新增代码 diff,故无新增 findings。

验证与覆盖声明

  • 已执行:PR 元数据、commit 列表、head SHA 与状态文件交叉核对。
  • 未执行:本次无新增代码,因此未重新运行 fmt/clippy/test 等验证;此前评审的验证结论仍然有效。
  • 剩余风险:元数据更新可能包含未在 diff 中体现的要求变更,建议人工确认 PR 描述/评论是否有新的范围或验收要求。

总结

本次扫描触发重新评审条件,但 PR 代码基线未发生变化。无需针对新增 diff 提出阻塞性意见。请维护者按需决定是否基于最新评论调整范围。

@SunnyQjm

Copy link
Copy Markdown
Collaborator Author

PR number: #3298
head_sha: f794aad
verified_at: 2026-09-17T04:03:19Z
tier: T1

1. 验证范围

  • HEAD:匹配 f794aad21ff86dc52fd50be89a97ddb3d55cbaa3
  • Backend:Apple container,arm64
  • RUN_ID:20260917T035450Z-d2f9e342
  • 已运行:
    • cargo test -p cosh-shell --test protocol ecs_probe:: -- --test-threads=1
    • cargo check -p cosh-shell --all-targets
    • crates/cosh-shell/scripts/inventory-public-api.sh
    • crates/cosh-shell/scripts/check-layout.sh
  • test plan 受前移的 origin/main 及生成证据文件干扰而扩圈,因此采用作业指定 scopes。
  • doctor:contended=true,不影响正确性判定。

2. 结果分类

  • 通过
    • protocol:7 passed,0 failed。
    • cargo check:成功。
    • public API inventory:成功。
    • layout audit:layout audit passed
  • 失败:无有效 scope 失败。
  • suspected-flaky:无。
  • inventory 首跑及原样复跑均因容器缺少 rg 而未执行;通过 Cargo registry 在一次性容器 /tmp 安装前置工具后成功,属于环境前置缺失,不是测试波动。
  • 容器已成功清理。

3. Findings 验证

  1. SIGCHLD/PGID 复用竞态:证实

    • ecs_probe.rs:122-139 仅将 SIG_IGN 改为默认行为,自定义 handler 保持不变。
    • ecs_probe.rs:157-163try_wait() == Ok(None) 后才调用 kill(-pid, SIGKILL),两者之间没有原子所有权保证。
    • 自定义 handler 可在该窗口执行 waitpid(-1, WNOHANG) 并释放 PID/PGID。
    • 当前测试 tests/protocol/ecs_probe.rs:239-255 使用空 handler,不执行回收,未覆盖该竞态。
  2. 为 integration test 扩大 production API:证实

    • EcsProbeTask、方法和 start_ecs_probe 均为 pub
    • adapter/public.rs:5-15 将其暴露到公开 facade。
    • 搜索显示唯一 crate 外调用点为 tests/protocol/ecs_probe.rs
    • inventory 将其标记为 private-candidate,但该分类不会阻止公开 API,layout 仍会通过。

4. 补充测试

无;本次为 T1 验证,现有代码与目标测试已足以确认 findings。

5. 证据索引

无截图证据。

6. 未运行 scope

  • 未运行完整 crate/workspace 测试、clippy、release build。
  • 未构造确定性 PID 复用压力测试。

7. 结论

No-Go — focused scopes 全部通过,但两个 P1 finding 均被证实;静态评审结论无需修正。

@SunnyQjm

Copy link
Copy Markdown
Collaborator Author

跟进 #pullrequestreview-5230696115(kongche-jbw)、#issuecomment-5708026328、#issuecomment-5708121184(zhoukailong),以及我账号下 codex-auto-review 的 #issuecomment-5708105581 与 #issuecomment-5708285438(判 No-Go)。head 仍为 f794aad21ff86dc52fd50be89a97ddb3d55cbaa3,本轮无代码变更。

[P1] 自定义 SIGCHLD reaper 抢先回收 probe:不采纳代码改动,附结构性依据与反证

四方一致指出:keep_children_waitable() 只改写 SIG_IGN、保留自定义 handler;若某 handler 在回调里 waitpid(WNOHANG),可在 try_wait()==Ok(None)kill(-pid) 之间回收 probe,PID/PGID 复用后误杀无关组。我核实了这条的触发前提在 cosh-shell 二进制里结构性不可达,并实测了"按建议中和任意 handler"会引入更严重的回归,因此不采纳该代码改动。

该场景在本二进制不可达(grep 证据)

crates/cosh-shell/src(production,排除 tests / #[cfg(test)])逐条核对:

  1. 无任何 SIGCHLD 回收 handler。全 crate 唯一改动 SIGCHLD 的就是 keep_children_waitable(把继承的 SIG_IGN 改回 SIG_DFL)。runtime/terminal.rs:36-51 只处理 SIGINT/TERM/HUP/QUIT,shell_host/sigpipe.rs 只碰 SIGPIPE,shell_host/raw_runner/wake.rs 只注册 SIGWINCH。没有对 SIGCHLD 装会 waitpid 的 handler。
  2. 无通配 waitpid(-1)。所有等待都定向到具体 Child/pid:adapter/process.rsadapter/cosh_core_registry.rs:213-217cosh_core_service/process.rs:413-420shell_host/io_loop.rsraw_relay.rs 等;runtime/startup/descendants.rs:424waitpid(pid, …) 定向到具体后代 pid。定向 wait 不会抢走 probe 子进程。
  3. 自定义 handler 无法跨 execve 继承shell_host/sigpipe.rs:22-25 已明确并依赖同一 execve 语义:"execve(2) resets caught signals to their default action in the new image"。cosh-shell 是独立 execve 的二进制,启动时 SIGCHLD 只可能继承到 SIG_IGNSIG_DFL,不可能继承一个 caught handler。

即:probe 子进程的可能回收者只有 ProbeChild 自己、与"继承 SIG_IGN 时的内核"——后者已被现有规范化消除。评审设想的"进程内 reaping handler"需要 cosh-shell 自己安装一个,而它没有。

"中和任意 handler" 不安全(回归实测坐实)

我按建议试做了"把任意非 SIG_DFL 处置一律改回 SIG_DFL",并加了一个真正 waitpid(-1, WNOHANG) 的 reaping-handler 回归。结果 protocol 全 suite 出现新失败:probe_cleanup_with_ignored_sigchld 的外层看门狗超时。根因是——wait-timeout crate(测试用来实现 12s 看门狗)本身用 SIGCHLD 自管道实现超时通知;先前测试初始化了它的 SIGCHLD handler 后,后续 start_ecs_probe 的强制改写把它一并清成 SIG_DFL,于是 wait_timeout 永久收不到子进程退出通知而超时。

这说明"中和任意 handler"会误伤进程内合法的 SIGCHLD 使用者wait-timeout 就是一个真实反例),并且这恰恰坐实了本轮另一条意见——"进程级改写且不恢复"本身有害。无法从处置本身区分"会回收的 handler"与"合法非回收 handler",因此不能一律改写。为一个结构性不可达的场景引入一个会破坏合法用户的全局改写,是净负收益,故不采纳。

关于 serialize + restore

考虑过"保存原处置→探测期间强制 DFL→探测后恢复 + 全局锁串行"。它需要跨异步探测生命周期持有全局锁并做恢复,恢复窗口仍有缝,且是为不可达场景新增回收脚手架——与本任务"不加恢复脚手架、最小机械"的约束相左。既然 grep 已证不可达,不做。

结论与残留边界

保持当前 SIG_IGN-only 规范化。这一域的两个不变量固定在方案 5.3.1:观察条件(try_wait() 证明未回收才发信号,四格矩阵)+ 保持条件(本进程是 probe 的唯一回收者,靠"规范化继承 SIG_IGN"+"无通配回收器"两点成立,并明确拒绝中和 handler 及其理由)。真正需要"会回收的 handler"确定性复现或 pidfd 升级的前提,只有在 cosh-shell 将来引入通配回收器时才成立;届时应改用 pidfd(其组信号需 Linux 6.9+,与"两平台共用一条 POSIX 路径"冲突,非本 PR 范围)。这一残留边界如实保留,不声称已用真 reaper 测试覆盖。

其余重复项

  • EcsProbeTask 扩大 public API(codex 重复):维持上轮依据不变。cosh-shell 的 crate 公开面本就是 lib.rs + */public.rs facade,adapter/public.rs 同组已导出 FakeAgentAdapterSessionClearPlan 等大量服务测试的类型,EcsProbeTaskCoshCoreAdapter 同组、同 private-candidate 分类;组件 AGENTS 要求 adapter/control-protocol 测试进 protocol layer(crate 外 integration target)。建议统一收敛该 facade 而非对本 PR 特例化;若维护者要求本 PR 内收窄,请同时批准调整测试分层与 layout 审计。
  • 文档同步(zhoukailong P2,第 4 次):仍不在本 PR 补。已 grep src/cosh-ng/docs/design/ 无任何 registry auth 协议或 authorized 描述,本 PR 未使任何既有文档过期;这是新增文档请求,建议单独文档 PR 完成中英双语。
  • 250ms configure 退出预算 / 每次探测重建 reqwest client / zone-id 字符校验 / provider_type 归一化(P3):均为非阻塞观察项;其中 client 重建与 provider 归一化属基线既有语义,如需优化建议单独提交带 Fixes:/Supplements: 归因,不在本认证修复链内扩面。
  • zhoukailong #issuecomment-5708121184 已自述"无新增 diff、无新 finding",无需处置。

CI:f794aad21 首跑全绿(Test cosh-ng/fast checks/release 均 SUCCESS,无复跑)。未自动合并,等待人工评审。

@zhoukailong

Copy link
Copy Markdown
Collaborator

🔄 Incremental Code Review

由 reviewer 自动评审产生(自动化任务 anolisa-pr-review-scanner
评审时间:2026-09-17T16:00:00+08:00

范围说明

  • 上次评审记录 updatedAt2026-09-17T04:03:22Z;当前 PR updatedAt2026-09-17T07:54:13Z
  • 当前 head SHA:f794aad21ff86dc52fd50be89a97ddb3d55cbaa3(与上轮一致,commit 列表最新为 03:16:00Z
  • 自上次评审以来新增 commit:0 个;本轮新增内容为作者对 P1 SIGCHLD 意见的申辩与对 P2 文档项的表态(#issuecomment-5710959571

对 P1「SIGCHLD/PGID 复用竞态」的独立复核:撤回

对作者申辩中的关键证据在 head f794aad21f 上做了独立复现:

  1. 生产代码无进程内回收者 —— 跨 src/cosh-ng/crates/*/src grep:唯一触及 SIGCHLD 的文件是 crates/cosh-shell/src/adapter/ecs_probe.rsecs_probe.rs:124 仅读取处置;ecs_probe.rs:134 仅将 SIG_IGN 规范化为 SIG_DFL)。runtime/terminal.rs:36-51 只注册 SIGINT/TERM/HUP/QUIT,shell_host/sigpipe.rs 只处理 SIGPIPE,shell_host/raw_runner/wake.rs:37 只注册 SIGWINCH。
  2. 无通配回收 —— 全 crates 生产代码无 waitpid(-1, ...);其余等待均定向到具体 pid。
  3. caught handler 无法跨 execve 继承 —— 与仓库既有注释 shell_host/sigpipe.rs:22-25 的语义一致。
  4. "中和任意非默认处置"确会回归 —— wait-timeout 0.2.1(测试看门狗)以 SIGCHLD + self-pipe 实现超时通知(crate 源码 unix.rs:7-15,67-73),crate 文档明确警告与其他 SIGCHLD 使用者冲突;无条件改写建议会误伤该合法使用者。

结论:try_wait()==Ok(None)kill(-pid) 之间的窗口只有在"进程内存在会回收的 SIGCHLD handler"时才可利用,而该前提在当前 cosh-shell 二进制内结构性不存在(caught handler 不跨 execve,生产代码亦不安装回收 handler)。故原 P1 在现有实现下不成立、予以撤回;作者拒绝"中和任意 handler"的取舍有实测与 crate 源码双重支撑。残留边界(未来若引入通配回收器,需改用 pidfd 路径)已如实记录,不视为本 PR 阻塞。

Findings(按 P0–P3 分级)

  • P0:无。
  • P1:无。上轮 P1 经本轮独立复核撤回(依据见上)。
  • P2(未解决,非阻塞):文档缺口。核实:本 PR 的 42 个变更文件全部位于 src/cosh-ng/,无任何文档改动;src/cosh-ng/docs/{adr,design,spec} 下确无 ECS registry auth 相关既有描述,作者"属新增文档请求、非既有文档过期"的定性成立。最小方向:由维护者决定随本 PR 补最小说明或单独文档 PR(中英双语,覆盖 auth 流程的用户可见行为)。
  • P3(存续观察项,非阻塞):configure 250ms 退出预算、每次探测重建 reqwest client、zone-id 字符校验、provider_type 归一化。其中 client 重建与 provider 归一化属基线既有语义,建议单独提交并附 Fixes:/Supplements: 归因。
  • P3(新增建议,非阻塞):本机制现依赖一个承重不变量——"本进程是 probe 的唯一回收者(生产代码不得引入 SIGCHLD 回收 handler 或通配 waitpid)"。该不变量目前仅记录在 PR 描述中,ecs_probe.rs:113-121 的代码注释只覆盖 SIG_IGN 规范化与"不恢复"的理由。最小方向:在代码注释或设计文档锚定一句,避免未来引入回收器时静默破坏该假设。

验证与覆盖声明

  • 已执行(head f794aad21f):作者关键 grep 证据的独立复现(跨 crate SIGCHLD / waitpid / 信号注册点);wait-timeout 0.2.1 crate 源码核对;CI check-runs 核对(Test cosh-ngTest cosh-ng fast checksTest cosh-ng prebuilt actionBuild cosh-ng release 均 success);PR 变更文件列表与文档现状核对。
  • 未执行:未重跑 fmt/clippy/test(本轮无新增代码,以静态复核为主);未独立复跑作者与 codex 报告的测试结果。
  • 未覆盖:真机 / Apple container 运行验证与 macOS 路径实测。
  • 剩余风险:承重不变量依赖后续评审纪律维持;文档缺口见 P2。

总结

本轮无新增代码 diff。作者对 P1 的结构性申辩经独立复核成立,原 P1 撤回;无新增阻塞项,文档项与 P3 观察项维持非阻塞建议。PR 当前 REVIEW_REQUIRED,等待人工评审。

@SunnyQjm
SunnyQjm force-pushed the fix/cosh-3278-auth-ecs-polling branch from f794aad to 4debaa2 Compare September 17, 2026 08:55
@SunnyQjm

Copy link
Copy Markdown
Collaborator Author

跟进 #issuecomment-5711028989(zhoukailong)。感谢独立复核并撤回 P1。

新候选 4debaa26e(基于主干 8c7aac399),本轮为注释-only 改动ecs_probe.rs 单文件,仅 doc comment,无任何行为变化):

  • 采纳新增 P3:把承重不变量锚进 keep_children_waitable 的 doc comment——"按数值 PID/PGID 发信号仅在本进程是 probe 唯一回收者时成立;该不变量依赖 production 不安装任何 SIGCHLD 回收 handler、不做通配 waitpid(-1)(所有 wait 都定向到具体 Child);将来若引入回收器需改用 pidfd 路径"。
  • 顺带订正上一版注释里一句事实错误——原文 "a caught disposition already retains zombies" 不准确(会 waitpid 的 handler 并不保留 zombie)。新注释改为"caught handler 无法跨 execve 继承,故继承处置只可能是 SIG_IGNSIG_DFL;非 ignore 处置一律不动,以免误伤进程内合法 SIGCHLD 使用者(如测试的 wait-timeout)"。

范围与提交纪律:以 --fixup 合入原「prove probe ownership」提交,rebase 到最新主干后七个提交 range-diff 中 1–6 全 =、第 7 条仅注释差异;f794aad21 → 4debaa26eecs_probe.rs 的净 diff 无任何非注释行。主干新增提交均在 bench/tokenless/sec-core/docs,与本 PR 无文件重叠。macOS 本机 cargo fmt --all -- --check 通过、--test protocol ecs_probe:: 7/7 通过。

其余项维持上轮口径:文档同步由维护者决定(新增文档请求,非既有文档过期);P3 观察项中 client 重建与 provider 归一化属基线既有语义,建议单独提交带归因,不在本认证修复链内扩面。新候选等待 CI 重新验证,未自动合并。

@kongche-jbw kongche-jbw left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

本轮完整独立复审结论:当前实现总体合理,未发现仍成立的 P0/P1;保留 1 项 P2 文档同步问题,建议合并前补齐。此前关于自定义 SIGCHLD reaper 的 P1 撤回,原因见下文。

评审基线与范围

  • 完整审查基线:c648fc4fd83f6b4b570f6a724cd3f31022176c12 → f794aad21ff86dc52fd50be89a97ddb3d55cbaa3,覆盖全部 42 个文件的 diff、相关调用链/协议/测试,以及 #3278、PR 描述、提交、历史 conversation、全部 review 和作者回复。
  • 提交本评论前重新抓取:当前 base/merge-base 为 8c7aac3994aa10b6ebef7d3d80f7d4e004fdf8e6,head 为 4debaa26eeed6137cd8a092f82113397cd8a6095。已独立核对 range-diff:前 6 个提交等价,第 7 个仅修改 ecs_probe.rs 的 doc comment;两个 head 之间整个 src/cosh-ng 的差异也仅此注释。已阅读最新作者回复,下面结论适用于该新 head。

保留的问题

[P2] 同 PR 补充 ECS 认证协议与生命周期设计文档

位置:src/cosh-ng/crates/cosh-core/src/registry/auth.rs 的 ECS verify 返回协议,以及 cosh-shell/src/auth/ecs_poll.rscosh-shell/src/adapter/ecs_probe.rs 的新生命周期。

本 PR 将 ECS verify 从 authorized 改为 status: ready/not_ready 配合 reason/error_code,并引入独占探测任务、自动轮询与提交、取消后 join/reap、保存结果未知时不自动重发等跨 shell/core 的约束,但当前 42 个变更文件没有组件设计文档。

已考虑作者“没有既有文档过期,因此文档可独立处理”的解释;不过 documentation-standard.md §5 明确要求 architecture/protocol change 在同一 PR 更新组件 docs/design/,并不限于修正既有文档。PR 描述和函数注释不能完整承担后续维护者查阅协议的职责。

最小修正:在组件 docs/design/ 补一份符合仓库文档规范的说明,覆盖返回字段和 shell/core 同步升级要求、主要状态转移及各层时间预算、取消完成必须等待 join/reap、未知保存结果不自动重试、probe 唯一回收者这一信号安全前提。无需扩展成全量教程,也无需在功能 PR 中改 CHANGELOG。此项是历史遗留未关闭的文档要求,不是新发现的运行时缺陷。

历史争议的复核

  • 入口同步 prepare / 复用 live core:已修复。 入口先建立可取消面板,再执行独占探测。之前的 auth/state 快照读取仍同步,作者已明确披露,不能把它说成整个初始化都立即可取消。
  • SIG_IGN 导致 PID/PGID 复用:当前修复成立。 启动前规范化继承的 ignore,回收时仅在 try_wait 确认子进程尚未回收时按 PGID 发信号,避免已回收后继续使用旧数值 ID。
  • 撤回 此前自定义 reaper 的 P1 当前 production 路径未发现回收 probe 的通配 reaper;caught handler 也不能跨 execve 继承。已核对的 wait-timeout 等待的是登记的具体 Child,不能据此推导它会抢收 probe。最新注释已明确 sole-reaper 前提;将来引入通配回收器时需要重新评估,但不能把未来架构变化当作当前 P1。无条件覆盖进程内 SIGCHLD handler 反而会伤及合法使用者。
  • 不建议通过 detach 或放弃回收来压缩取消耗时。 这会破坏本 Issue 要求的取消完成语义。
  • 测试使用的公共 facade 暂不作为阻断项。 已登记 private-candidate,且协议生命周期测试留在 protocol 层符合当前组织方式。

验证证据与边界

独立测试运行在 Linux ARM64、Rust 1.88.0,执行对象为 f794aad21

命令(均使用 cargo +1.88.0 test --locked 结果
-p cosh-shell --lib --bin cosh-shell auth -- --test-threads=1 12 + 157 通过
-p cosh-shell --test protocol ecs_probe:: -- --test-threads=1 7 通过
-p cosh-core --lib --bin cosh-core ecs_metadata -- --test-threads=4 30 + 6 通过
-p cosh-shell --test raw_cli auth -- --test-threads=4 20 通过

合计 232 项,包含重复取消和活 shell 的线程/子进程回收检查。当前 4debaa26e 已另行核对增量及 git diff --check,未把上述本地测试描述为在新 head 重跑。

提交评论时已核对 当前 head 的 CITest cosh-ngTest cosh-ng fast checksBuild cosh-ng release 均成功。旧候选的失败不再作为当前阻断证据。本轮没有独立运行完整 workspace、macOS 或真实 ECS 验收,历史实机材料也不作为后续补修的直接覆盖证据。

@zhoukailong

Copy link
Copy Markdown
Collaborator

🔄 Incremental Code Review

由 reviewer 自动评审产生(自动化任务 anolisa-pr-review-scanner
评审时间:2026-09-17T17:17:00+08:00

范围说明

  • 上次评审记录 updatedAt2026-09-17T08:00:24Z(head f794aad21f);当前 head:4debaa26e(基于主干 8c7aac399,rebase 后全部 commit SHA 更新);当前 PR updatedAt2026-09-17T09:12:03Z
  • 增量独立核实(不采信作者声明):对 f794aad21f4debaa26esrc/cosh-ng 子树做 git tree blob 级全量对比,两侧各 1203 个 blob 中仅 1 个不同crates/cosh-shell/src/adapter/ecs_probe.rs882e4591bcd8d973a3),其余 1202 个 blob 完全一致。
  • 该文件新旧 blob 全文 diff:全部为 /// doc comment 行(+14 / −7),无任何非注释行变化。作者「注释-only」声明独立成立。

增量内容评审

  1. 上轮 P3 闭环(承重不变量锚定)keep_children_waitable 的 doc comment(ecs_probe.rs:113-128)现写明「按数值 PID/PGID 发信号以本进程为 probe 唯一回收者为前提」及其两条生产约束(不安装 SIGCHLD 回收 handler、不做通配 waitpid(-1)),并注明将来引入回收器需改用 pidfd 路径。与实现逐条一致:读处置 → 仅 SIG_IGN 归一化为 SIG_DFL → 非 ignore 处置一律不动。
  2. 上一版注释事实错误订正正确:原句「a caught disposition already retains zombies」不准确;新注释「caught handler 不跨 execve 继承,故继承处置只可能是 SIG_IGNSIG_DFL;非 ignore 一律不动以免误伤进程内合法使用者(如测试 wait-timeout)」符合 POSIX 语义。
  3. 交叉确认:独立评审人 kongche-jbw 的最新复审(#pullrequestreview-5233520324)对本增量的 range-diff 判断(前 6 提交等价、第 7 提交仅注释)与本轮 blob 级对比结论一致。

Findings(按 P0–P3 分级)

  • P0:无。
  • P1:无。
  • P2(存续;本轮独立核验其规范依据):同 PR 未补组件设计文档。本 PR 含跨 core/shell 的协议变更(cosh-core/src/registry/auth.rsauthorized: boolstatus: ready/not_ready + reason/error_code)与新增的探测/轮询/取消生命周期;独立核对 specs/documentation-standard.md §5(新 head 下 blob 3111742e1)原文:「when a PR introduces ... Architecture or protocol change → Component docs/design/」,且限定 same PR,属 MUST 规则;42 个变更文件均无 docs/ 改动。最小方向:在组件 docs/design/ 补一份最小设计说明(协议字段与 shell/core 同步升级要求、主要状态转移与各层时间预算、取消完成必须 join/reap、未知保存结果不自动重试、sole-reaper 信号安全前提),中英双语按仓库文档规范;无需 CHANGELOG。结论与 kongche-jbw 的 P2 一致。
  • P3(无新增):上轮 P3 已闭环。上轮存续观察项(configure 250ms 退出预算、每次探测重建 reqwest client、zone-id 字符校验、provider_type 归一化;后两项属基线既有语义)维持「建议单独提交并附 Fixes:/Supplements: 归因」的非阻塞口径。

验证与覆盖声明

  • 已执行(静态复核,head 4debaa26e):两版 head 的 src/cosh-ng 子树 blob 级全量对比(1203 vs 1203);ecs_probe.rs 新旧 blob 全文 diff;新注释与 keep_children_waitable 实现逐条语义核对;specs/documentation-standard.md §5 原文核对;registry/auth.rs patch 协议字段核对;新 head CI check-runs 核对(Test cosh-ngTest cosh-ng fast checksTest cosh-ng prebuilt actionBuild cosh-ng releaseCommit Message LintPR Checks 均 success)。
  • 未执行:未在本机重跑 fmt/clippy/test(本轮无代码变化,静态复核已覆盖注释改动);未独立复跑 kongche-jbw 报告的 232 项测试。
  • 未覆盖:真机 ECS 运行验证与 macOS 路径实测。
  • 剩余风险:P2 未补齐前,协议与生命周期语义仅存在于代码、注释与 PR 描述;sole-reaper 不变量的长期维持依赖后续评审纪律(现已锚定于代码注释)。

总结

rebase 后的本轮增量为注释-only,无行为变化;上轮 P3 闭环;无新增阻塞项。P2 文档项在仓库规范下为同 PR MUST,建议合并前补齐。PR 当前 REVIEW_REQUIRED,等待人工评审。

@SunnyQjm
SunnyQjm requested a review from casparant as a code owner September 17, 2026 09:41
@SunnyQjm

Copy link
Copy Markdown
Collaborator Author

跟进 #pullrequestreview-5233520324(kongche-jbw 完整复审)与 #issuecomment-5711958673(zhoukailong)。感谢两位对注释-only 增量的 blob/range-diff 级独立核实,以及对此前 SIGCHLD P1 的撤回确认。

[P2] 组件设计文档:已在本 PR 补齐

采纳。我先核实了 specs/documentation-standard.md §5 原文——"Architecture or protocol change → Component docs/design/",且 "documentation MUST be updated in the same PR"。本 PR 的 verify 协议从 authorized 改为 status: ready/not_ready + reason/error_code 属 protocol change,确落在这条 MUST 内;我此前"可独立文档 PR"的定性对这条规则有误,这里更正。

新增 src/cosh-ng/docs/design/ecs-auth-provisioning.md(候选 eb34a3191,独立 docs(cosh-ng) 提交),覆盖两位指出的全部要点:

  • registry auth prepare/verify/configure wire 契约与字段(status: ready/not_ready + reason ∈ {role_missing,credentials_expired}、失败 error_code ∈ 5 类 metadata_*、shell/core 必须同步升级);
  • ECS 轮询状态机(9 个 Stage)与各层时间预算(INTERVAL 2s / OPERATION_LIMIT 5s / WAIT_LIMIT 200s / configure 观察 12s / 取消回收 5s / metadata 3s);
  • 取消完成必须 join/reap 的零残留合同;未知保存结果不自动重发;
  • probe sole-reaper 信号安全前提(不装 SIGCHLD 回收 handler、不做通配 waitpid、handler 不跨 execve;将来引入回收器需改 pidfd)。

字段与常量均从源码核实,非从 spec 转抄。语言按 docs/design/ 现有惯例为英文:该目录 11 个文件中 10 个为纯英文,§1 双语强制集只含 repo/component root 与 docs/ standalone pages,不含 docs/design/ 设计说明;故不产出 _zh,也不动 CHANGELOG(§5:功能 PR 不写 CHANGELOG)。若维护者认为该设计说明需双语,请明示,我再补 _zh

历史项复核确认

  • P0/P1 无——SIGCHLD 自定义 reaper P1 经两位独立复核撤回(production 无通配回收器、handler 不跨 execve、wait-timeout 等待的是具体 Child 不会抢收 probe),sole-reaper 前提已锚定于 ecs_probe.rs doc comment。
  • 入口同步 prepare / SIG_IGN 复用窗口 / 取消完成语义 / 测试 facade 分类:维持已确认结论,不再重复改动。
  • P3 观察项(configure 250ms、每次重建 client、zone-id 校验、provider_type 归一化):后两项基线既有语义,维持"单独提交带归因"的非阻塞口径。

范围与门禁:docs/ 新增为独立提交,主干未动(base=head=8c7aac399),前 8 个提交中 1–7 不变、第 8 为纯新增文档;PR 差异 43 文件。等待当前候选 CI 重新验证,未自动合并。

@kongche-jbw kongche-jbw left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

增量复审:4debaa26eeed6137cd8a092f82113397cd8a6095 → eb34a319198f82066bdf782aca09291f77cd29c6,base/merge-base 仍为 8c7aac3994aa10b6ebef7d3d80f7d4e004fdf8e6

已阅读最新回复并独立核对:本轮仅新增 149 行设计文档,生产代码与测试均未变化。上轮“缺少同 PR 设计文档”的 P2 已解决;没有新增 P0/P1。不过文档新增两处 P2 语义偏差,已分别附在线上:configure 的执行/取消/Unknown 生命周期与 prepare 的手动模式回退边界。建议按当前实现修正文档,不需要为迁就文档改动代码。协议字段、同步升级要求、时间预算和 sole-reaper 前提等其余主要内容与实现吻合;当前规范没有要求这份组件设计文档必须另有中文版。

另建议合并前整理一下 commit 历史。当前 8 个提交中有几组属于同一逻辑变更的后续修正,可按仓库 AGENTS.md §13 的 fixup/autosquash 规则合并:

  • d1fd8130 修正了 d88a33ca 引入的输出行断言,建议合入 d88a33ca
  • a2f963f0 删除的是 6d054e67 在本 PR 内新增的两份验收 YAML,建议合入引入提交,避免历史里保留“先加入再移出产品树”的中间状态;外部验收资产仍可在 PR 中引用。
  • c4e7f4796a3e83044debaa26 中针对本 PR 新认证流程的回收、取消和信号所有权修正及对应回归测试,建议核对归因后 fixup 到引入它们的功能提交。确属 main 上既有缺陷的修正再保留独立提交和正确归因。
  • 设计文档可以保持独立 docs 提交,本轮文档纠正合入该 docs 提交即可;不要求把所有独立逻辑压成一个大提交。

整理后应核对最终 tree/diff 未意外变化,并以新 head 的 CI 为准。

验证:已完成新增文档全文与源码/现有测试对照、相对链接检查和 git diff --check;纯文档增量未重跑 Rust 测试。提交此评审时,当前 head 的 Docs Lint、Commit Message Lint、PR Checks 已通过,cosh-ng 测试、fast checks 和 release 构建仍在运行,未将其计为通过。

Comment thread src/cosh-ng/docs/design/ecs-auth-provisioning.md Outdated
Comment thread src/cosh-ng/docs/design/ecs-auth-provisioning.md Outdated
@zhoukailong

Copy link
Copy Markdown
Collaborator

🔄 Incremental Code Review

由 reviewer 自动评审生成。

评审时间: 2026-09-17T18:01:01+08:00
评审范围: PR #32984debaa26eeed6137cd8a092f82113397cd8a6095eb34a319198f82066bdf782aca09291f77cd29c6 的增量:1 个文件,+149/-0 行。
整体结论: REQUEST_CHANGES


Findings

P1 - High

  • [src/cosh-ng/crates/cosh-shell/tests/shell_host/native.rs:885] 必需的 shell_host 集成门禁在当前 PR head 失败
    • 问题本质:当前 Test cosh-ng CI 在 native::enhanced_shift_tab_toggles_shell_only_routing_without_restarting_bash 失败(shell_host:215 passed / 1 failed / 1 ignored)。该用例在固定 1 秒等待后断言第二次 Shift+Tab 已切换为 ,实际终端仍为 ,并将后续 /help 交给 Bash。无论归因于产品回归还是测试时序,该 PR 当前不能通过所需集成验证。
    • 可复现场景:在 PR head 的 Linux CI 运行 cargo test --package cosh-shell --test shell_host;失败日志定位到该断言。
    • 最小修复方向:在合并前使切换完成与测试观察具有确定性的同步契约,并确认 shell-only 路由的实际行为。
    • 应补回归测试:在 shell_host 层覆盖两种 bracketed-paste 配置下的连续 Shift+Tab 切换,并重跑完整 shell_host target。

P2 - Medium

  • [src/cosh-ng/docs/design/ecs-auth-provisioning.md:18] auth wire contract 漏写通用关联信封字段
    • 问题本质:设计文档将请求描述为仅含 domainaction 和参数,响应描述为仅含 successdataerror;但实际协议还强制要求请求 type: "registry_request"request_id,并要求响应 type: "registry_response" 且回显相同 request_idecs_probe.rs 会因类型或关联 ID 不匹配直接拒绝响应,因此按本文档实现的独立 shell/core peer 无法互操作。
    • 可复现场景:用文档所列字段构造请求,或返回不含/不匹配 request_id 的响应;parse_response 拒绝该响应。
    • 最小修复方向:在动作专属字段之前定义请求和响应的完整公共 JSONL 信封及关联规则。
    • 应补回归测试:在 protocol 层保留/补充对请求类型、响应类型及 request_id 回显的契约断言。

未发现本次文档增量中的 P0 问题。


验证与覆盖声明

  • 已执行验证:静态核对增量 diff、registry/auth.rsadapter/ecs_probe.rsauth/ecs_poll.rs 及 protocol fixture;查看 PR head CI。Test cosh-ng fast checksBuild cosh-ng release 通过,Test cosh-ng 失败(见 P1)。
  • 已覆盖角度:方案、协议功能、文档完整性、安全边界说明与测试门禁。
  • 未覆盖范围与剩余风险:未在本地运行 Cargo 测试;本轮仅复审最后的设计文档提交,未重新审查 PR 其余 42 个文件及 ECS 实机/取消时序行为;未验证 macOS 行为。

@SunnyQjm
SunnyQjm force-pushed the fix/cosh-3278-auth-ecs-polling branch from eb34a31 to 8c2f067 Compare September 17, 2026 10:56
@SunnyQjm

Copy link
Copy Markdown
Collaborator Author

跟进 #pullrequestreview-5233853275(kongche-jbw)。两条文档 P2 已订正,并按 §13 整理了提交历史。候选 8c2f06795

两处文档语义偏差:已订正(逐条见 inline 回复)

  • prepare 手动回退边界(line 38):改为明确 token 阶段 Unreachable/TimeoutOk(None)mode: manual 是非 ECS 环境的预期回退;仅有效 token 建立 ECS 身份后的 instance-id/zone GET 失败才作为 verify 错误上报。与 fetch_identity 及两条 prepare-manual 测试一致。
  • probe vs configure 生命周期(line 81 + summary):区分 Operation::Probe(独占可取消 EcsProbeTask)与 Operation::Configurecosh-auth-ecs-save worker → core_auth_configureregistry_query_classified,优先 live core、cancel() 在 Configure/Submitting 直接返回、不可取消);Unknown 改述为"保存 worker 12s 内未确认即进入、资源收回后才给返回入口";Summary 更正子进程创建者为 cosh-shell。

提交历史整理(AGENTS §13)

按建议将本 PR 内对新引入代码的修正 fixup 进引入提交,8 → 3 个提交:

  • chore(cosh-ng): [shell] align validation ← 原 align validation + CR 行断言修正(d1fd8130,两者同改 provider_handoff/foreground.rs
  • feat(cosh-ng): [core,shell] automate auth ← 原 automate auth + 验收 YAML 移除(a2f963f0,删的是本提交在 PR 内新增的两份)+ 回收/取消/信号所有权三处修正(c4e7f479/6a3e8304/4debaa26,均改本提交引入的认证文件)
  • docs(cosh-ng): [core,shell] document ECS auth protocol ← 设计文档 + 本轮两处订正

逐提交核实归因:五个被折叠提交全部修正本 PR 内新引入的内容,无 main 既有缺陷修复,故不保留独立提交。整理后用树哈希双验证:重写前后 tip 树哈希均为 551952814a3b086901e7ed711b44d2d61fcbde00零内容漂移origin/main...HEAD 差异 43 文件。base=merge-base 仍为 8c7aac399

CI 说明

前一候选 eb34a3191(纯文档)的 Test cosh-ng 首跑失败于 native::enhanced_shift_tab_toggles_shell_only_routing_without_restarting_bash——该测试不在本 PR diff在 main 上已存在shell_host/native.rs:837),是 PTY 时序敏感用例;而 eb34a3191 相对已全绿的 4debaa26e 仅多一个 markdown、零代码改动,故归因为与本 PR 无关的既有 PTY flake,如实保留该首跑失败记录,不声称其通过。当前候选 8c2f06795 的代码字节与 4debaa26e(CI 全绿)相同,仅提交结构与设计文档不同;等待新 head CI 重新验证,未自动合并。

Use equivalent fixed-size chunk iteration and move the pending request
vector without an extra allocation to satisfy the current Clippy rules.
Check the foreground command's complete output line without requiring
an incidental leading carriage return; keep handoff and once-only checks.

The carriage-return failure also reproduced on the unmodified baseline.
Crypto vectors, evidence requests and the foreground case were verified.
This does not change Gateway or suppress any lint or test.

Supplements: 703d4d4
Supplements: 1a93589
Fixes: 3100f22
Assisted-by: Qoder:0.2.5
Signed-off-by: SunnyQjm <mfeng@linux.alibaba.com>
Resolve first-provider identity from both template type and occupied
names, preserving editable-field rules across navigation and recovery.

Use one owned, cancellable registry probe at a time. The event pump
checks cancellation and deadlines before consuming readiness; task
completion requires joining the worker and reaping its temporary core.
Keep configure on its existing route, consume confirmed save results,
and never retry an uncertain mutation automatically.

Share bounded IMDSv2 token and credential validation across prepare,
verify, preflight and STS refresh. Metadata secrets stay in core;
expired credentials wait for refresh rather than requesting a new role.

Deterministic ownership/deadline tests and real ECS PTY cases cover
first-use naming, automatic readiness, persisted configuration and
cancellation followed by ordinary input and another auth flow.
Readiness does not claim SysOM service authorization or model access.

Reviews covered provider identity, task ownership, metadata safety and
code organization. Regressions exercise spawn failure reaping, shell
exit while saving, expired credentials and confirmed-ECS read failures.
The accepted validation scope is core/shell; pre-existing Gateway
workspace lint failures remain disclosed outside this change.

Supplements: 76d1d60
Assisted-by: Qoder:0.2.5
Signed-off-by: SunnyQjm <mfeng@linux.alibaba.com>
@SunnyQjm
SunnyQjm force-pushed the fix/cosh-3278-auth-ecs-polling branch from 8c2f067 to 72efd76 Compare September 17, 2026 11:08
@SunnyQjm

Copy link
Copy Markdown
Collaborator Author

跟进 #issuecomment-5712498625。

P2 registry 公共信封:已补齐

核实成立。候选 72efd7693ecs-auth-provisioning.md 现在先定义完整 JSONL 信封,再描述动作字段:

  • 请求:type: "registry_request"request_iddomain: "auth"actionparams
  • 响应:type: "registry_response"、原样回显同一个 request_idsuccess、可选 data/error
  • 明确 cosh-shell 会拒绝响应类型或关联 ID 不匹配;失败响应可同时携带结构化 data.error_code 与开发者错误串。

该说明与 InputMessage::RegistryRequestOutputMessage::RegistryResponseecs_probe.rs::parse_response 的强制校验逐项一致。本轮仅文档变化,无生产代码/测试变化。

P1 旧 head 的 shell_host 失败:不适用于当前候选

eb34a3191Test cosh-ng 首跑失败事实保留:唯一失败为 native::enhanced_shift_tab_toggles_shell_only_routing_without_restarting_bash(215 passed / 1 failed / 1 ignored)。归因已核实:

  • crates/cosh-shell/tests/shell_host/native.rs 不在本 PR diff;
  • 该用例已存在于 main(native.rs:837);
  • eb34a3191 相对 CI 全绿的 4debaa26e 仅新增一个 markdown,零 Rust 代码变化;
  • 失败断言依赖固定等待与 PTY 终端时序,属于既有 shell_host 环境/时序失败,不能由 docs-only 提交造成。

因此不改与本任务无关的 Shift+Tab 产品逻辑或测试等待,也不把旧失败豁免成通过。当前候选 72efd7693 已因文档订正与主干前移产生新 head,以其 CI 为权威;若同一用例再次失败,再按同 scope 重复证据处理。

历史与主干

文档订正已 fold 进 docs 提交,仍保持 3 提交结构。主干前移到 2508da624(仅 anolisa 文件,无 cosh-ng 重叠),rebase 前后 src/cosh-ng 树哈希同为 67980041cc20aacfaf3b2060acde80749f952d56,零内容漂移。等待新 head CI,未自动合并。

@zhoukailong

Copy link
Copy Markdown
Collaborator

🔄 Incremental Code Review

PR: #3298 feat(cosh-ng): [core,shell] automate auth
Reviewed head: 8c2f0679522c82cab53733023ec551f0a0f33e9e
Previous reviewed head: eb34a319198f82066bdf782aca09291f77cd29c6
Reviewed at: 2026-09-17
Scope: Incremental re-review after rebase/squash to three commits (chore(cosh-ng): [shell] align validation, feat(cosh-ng): [core,shell] automate auth, docs(cosh-ng): [core,shell] document ECS auth protocol). This is a static review; no local cargo commands or real ECS tests were executed.


Findings

P0 — None identified

No blocking correctness, safety, or wire-contract issues were found in the reviewed files.


P1 — Signal-disposition normalization has an unsynchronized race window

File: src/cosh-ng/crates/cosh-shell/src/adapter/ecs_probe.rs:129-147
Problem: keep_children_waitable() reads the inherited SIGCHLD disposition with sigaction and then writes SIG_DFL with libc::signal. Between the read and the write, any thread in the process that spawns a child will leave that child with the inherited SIG_IGN disposition, so the kernel auto-reaps it with no zombie. If that child happens to be the probe child (spawned by the worker thread immediately after this call), ProbeChild::drop may see ECHILD on try_wait and skip signaling — which is handled — but more importantly other concurrently spawned children can become zombies or be lost to wildcard waits. The design note correctly identifies the sole-reaper invariant, but the implementation relies on the absence of other child-spawning threads during a narrow window rather than enforcing it.
Fix direction: Serialize the read-and-set operation and, if possible, prevent other threads from spawning children across the window (e.g., a small critical section around the normalization). At minimum, document the exact race window and the set of threads that may spawn children in production. Add a regression test that spawns a child from another thread inside the window and asserts it remains waitable.
Test layer: protocol or raw_cli.


P2 — metadata_http_error is silently downgraded to a generic user message

File: src/cosh-ng/crates/cosh-shell/src/adapter/ecs_probe.rs:335-342
Problem: The parse_response match arm for metadata_http_error falls through to the catch-all "ECS credential check failed". The design doc (ecs-auth-provisioning.md) lists metadata_http_error as a distinct error code, and the other four codes get specific messages. Users therefore lose diagnostic detail for non-2xx metadata responses that are neither 401/403 nor timeouts/unreachable.
Fix direction: Add an explicit arm for metadata_http_error with a message such as "ECS instance metadata returned an unexpected HTTP error." and include it in the i18n catalog.
Test layer: protocol (ecs_probe.rs) or unit test in ecs_probe.rs.


P2 — Instance-ID cache side effect is undocumented in the design note

File: src/cosh-ng/crates/cosh-core/src/provider/sysom.rs:445-488
Problem: resolve_instance_id() writes a disk cache at ~/.copilot-shell/instance_id with a 3-hour TTL and persists negative results as an empty file. This is a cross-process, on-disk side effect that affects subsequent provider instances and is not mentioned in src/cosh-ng/docs/design/ecs-auth-provisioning.md. The PR description also does not disclose it. While the instance ID is not a credential, it is instance-identifying metadata, and cache invalidation behavior can surprise operators (e.g., an empty file suppressing metadata probes for three hours after a transient failure).
Fix direction: Add a "Metadata cache" subsection to the design doc describing the path, TTL, negative caching, and invalidation semantics.
Test layer: docs only; existing metadata_tests.rs already covers cache behavior.


P2 — EcsFlow::poll redraws on width changes without guarding capture re-arm

File: src/cosh-ng/crates/cosh-shell/src/auth/ecs_poll.rs:644-653
Problem: redraw() increments field_capture_revision unless the stage is Cancelling. The poll function calls redraw whenever changed is true OR when the terminal width changes. A terminal resize while the user is in the waiting panel will re-arm the input capture with the same scoped id. That is usually harmless, but if a resize occurs while the user has partially typed input into a free-text field, the capture revision bump could reset the capture and drop buffered input. This is a low-probability UI glitch rather than a correctness bug.
Fix direction: Only bump field_capture_revision when the auth phase or field actually changes, not on every redraw or width change. The existing changed guard already covers stage transitions; width changes should redraw chrome without re-arming capture.
Test layer: raw_cli or logic.


P2 — Re-review confirms MessageId segment is appended correctly

File: src/cosh-ng/crates/cosh-shell/src/i18n/message_id.rs:116
Observation: The new auth_ecs_ids segment is the last entry in collect_message_ids!, and the pinned ordinal assertions in src/cosh-ng/crates/cosh-shell/src/i18n/mod.rs:296-306 confirm the new IDs occupy the tail of the enum. This satisfies the "append-only" invariant. Both en/auth.rs and zh/auth.rs provide non-empty translations for every new ID.


Summary and remaining risks

  • The cross-component wire contract (prepare/verify/configure) is consistent between cosh-core/src/registry/auth.rs and cosh-shell/src/auth/ecs_poll.rs.
  • The IMDSv2 probe in cosh-core/src/provider/sysom/ecs_metadata.rs correctly rejects redirects, proxies, IMDSv1 fallback, and oversize/error bodies; credentials never leave the metadata owner.
  • Cancellation ownership in EcsProbeTask/ProbeChild and the "cancel complete before prompt" contract in EcsFlow are well designed and covered by protocol/raw_cli tests.
  • Remaining risks are the narrow SIGCHLD race, the undocumented instance-id cache, and the generic metadata_http_error user message. None of these block the feature, but they are worth addressing before the next release.

Verification and coverage statement

  • Executed: Static review of the key changed files (ecs_metadata.rs, ecs_probe.rs, ecs_poll.rs, ecs_poll_tests.rs, registry/auth.rs, preflight.rs, sysom.rs, metadata_tests.rs, tests/protocol/ecs_probe.rs, tests/raw_cli/auth_ecs.rs, default_id.rs, cosh_core_registry.rs, i18n catalogs, docs/design/ecs-auth-provisioning.md).
  • Not executed: cargo fmt --check, cargo clippy --all-targets, cargo test --workspace, real ECS PTY tests, or shell_host tests. This review was performed without a local checkout of the PR head.
  • Skipped: No other open PRs required review (review-requested:zkl123 returned empty; no issue comments mentioned zkl123; PR feat(cosh-ng): add WSL2 preview launcher from Git Bash #3143 had no update since last review).

Review produced by the reviewer automated scanner.

@SunnyQjm

Copy link
Copy Markdown
Collaborator Author

跟进 #issuecomment-5713398769。该评论评审的是旧 head 8c2f06795;当前 head 72efd7693 只在设计文档补齐 JSONL 信封并 rebase 到 2508da624,生产代码字节不变。逐项核实如下。

已修复:registry 公共信封 P2

ecs-auth-provisioning.md 已补齐完整 JSONL 信封:请求 type: registry_request / request_id / domain / action / params,响应 type: registry_response / 原样回显同一 request_id / success / 可选 data/error;明确 cosh-shell 对类型或关联 ID 不匹配直接拒绝。与 InputMessage::RegistryRequestOutputMessage::RegistryResponseecs_probe.rs::parse_response 一致。

P1 SIGCHLD read/set 窗口:不成立,不改代码

评论把 SIGCHLD 的僵尸策略误写成子进程在 spawn 时“继承”的属性。是否自动回收取决于父进程在子进程退出时的 process-global SIGCHLD disposition,不是给每个 child 在 spawn 时冻结一份父进程策略。

实现顺序也排除了 probe 命中该窗口:start_ecs_probe 在调用者线程同步执行 keep_children_waitable(),函数返回后才 thread::Builder::spawn worker,worker 内才创建 probe child;因此 probe 不可能在 sigaction 读/写之间被 spawn。另一个线程的既有 child 若恰在父进程仍为继承 SIG_IGN 的几条指令窗口内退出,只会按宿主原有语义被自动回收;这不是本修改新造的 zombie 或 probe PGID 风险。production 无通配 waitpid(-1) 已由两位 reviewer 独立核实。给 keep_children_waitable() 自己加锁也不能阻止未使用同一锁的其他 Command::spawn,不会增强该证明。

P2 metadata_http_error 显示文案:已知非阻塞限制,不扩展

wire 层仍保留独立 metadata_http_error,设计文档列的是协议分类,不承诺每类都有独立用户文案。UI 对该罕见“其他非成功 HTTP 状态”使用安全泛化的 ECS credential check failed,不泄漏上游状态/body;阶段标题与操作提示已本地化,部分技术详情仍英文/泛化已在 PR Risk 中披露。此前已决定本轮不扩展错误翻译框架,维持。

P2 instance-id 磁盘 cache:基线既有且不属于本设计边界

~/.copilot-shell/instance_id、3h TTL 与空文件负缓存均已存在于 origin/main 的同步 resolve_instance_id();本 PR 只是把调用从构造期移到 OnceCell 惰性解析并移除阻塞 TCP metadata,磁盘语义未新增。该 cache 服务 provider generate 请求体,不参与本文档覆盖的 registry auth prepare/verify 生命周期;因此不把基线 provider cache 扩写进 ECS auth provisioning 设计。若要把 cache 改为原子写/取消负缓存,应单独提交并对 main 既有代码做正确 Fixes: 归因。

P2 resize 重绘丢 free-text:所述丢失不成立

Stage::Editing + AuthPhase::FillingField 时 width-only redraw 的确会递增 revision 并产生新 capture id,但当前文本不会丢:每次编辑通过 record_field_edit 同步到 RuntimeAuthState.field_input;新的 RawInputCapture::TextQuestioninitial_text 设为该 field_inputauth/capture.rs:80-83);raw-input 的 CardInputState::apply_capture 在 capture id 变化时用 initial_text.clone() 重建 free_textraw_input/card_capture.rs:165-171)。因此 reviewer 所称“reset capture and drop buffered input”没有代码依据。未来可把 width-only repaint 优化成不 re-arm,但不是本 PR 的正确性修复。

当前门禁

当前 72efd7693 CI 首跑全绿、无复跑:Test cosh-ngTest cosh-ng fast checksTest cosh-ng prebuilt actionBuild cosh-ng release 均 SUCCESS,failed_jobs=[]rerun_history=[],PR MERGEABLE。无 P0/P1 未处理项,未自动合并。

@zhoukailong

Copy link
Copy Markdown
Collaborator

🔄 Incremental Code Review

由 reviewer 自动评审产生(自动化任务 anolisa-pr-review-scanner
Reviewed at: 2026-09-17T19:55:00+08:00

PR: #3298 feat(cosh-ng): [core,shell] automate auth
Reviewed head: 72efd76930de9a569677e8a6f0eba9fbd39d760f
Previously reviewed head: 8c2f0679522c82cab53733023ec551f0a0f33e9e

Scope

The PR was rebased onto the new main (2508da624d), which rewrote all three PR commits (new committer dates 11:07:41–43Z). A tree-to-tree diff between the old and new heads shows the cosh-ng code is byte-identical; the PR's own delta since the last review is exactly one file:

  • src/cosh-ng/docs/design/ecs-auth-provisioning.md — the "Registry auth wire contract" section was expanded from 2 sentences to a full JSONL/envelope specification (+30/−2).

The other three files that appear in the head-to-head diff (anolisa-cli raw.rs, raw_e2e.rs, anolisa-core distribution.rs) come from the new main commit 2508da624d, not from this PR. This is a static review; no cargo fmt / clippy / test was executed locally.


Findings

P0 — None identified

P1 — carried forward (code unchanged): signal-disposition normalization race window

File: src/cosh-ng/crates/cosh-shell/src/adapter/ecs_probe.rs:129-147
Unchanged since the last review; still open as previously reported. The read (sigaction, line 131) and write (libc::signal, line 141) of the SIGCHLD disposition remain unsynchronized. Note: the design doc's "Probe signal-safety invariant" section (unchanged this round) documents the sole-reaper invariant and the reasons for normalizing only SIG_IGN, but still does not document this read-then-write window itself.

P2 — NEW: the doc's response-validation claim does not hold on the short-lived fallback path

File: src/cosh-ng/docs/design/ecs-auth-provisioning.md:46-48 vs src/cosh-ng/crates/cosh-shell/src/adapter/cosh_core_registry.rs:221-244
Claim: "type must be registry_request / registry_response, and the response must echo the request's exact request_id; cosh-shell rejects a response whose type or correlation id differs."
Verified reality across the three shell-side paths:

  • Live core path — validated (is_registry_response_for, cosh_core_service/process.rs:362,387-390). ✔
  • Isolated ECS probe path — validated (parse_response, ecs_probe.rs:324-328). ✔
  • Short-lived fallback (registry_query_short) — not validated: the first non-empty stdout line is parsed as JSON and matched purely on success/data/error; type and request_id are never checked. ✘

For the auth domain, configure falls back to this path when no live runtime exists (registry_query_classified, cosh_core_registry.rs:57-74). The dedicated child makes exploitation implausible (one request in, first line out), so this is a contract-documentation inaccuracy rather than a practical vulnerability — but this document's stated purpose is to fix the cross-component contract, and a future implementer reading it would assume rejection is enforced on every path.
Fix direction: either narrow the doc claim to the paths that validate, or add the same type/request_id check to registry_query_short.
Test layer: lib (cosh_core_registry_tests.rs) — a fake registry process echoing a mismatched id must produce a Transport error.

P2 — carried forward (code unchanged): metadata_http_error downgraded to a generic message

File: src/cosh-ng/crates/cosh-shell/src/adapter/ecs_probe.rs:335-342
Confirmed still present in the new head: the match arms cover metadata_access_denied / metadata_timeout / metadata_unreachable / invalid_metadata_response, and metadata_http_error still falls to the catch-all "ECS credential check failed", although the design doc (line 94) lists it as a distinct error code.

P2 — carried forward (code unchanged): instance-ID disk cache undocumented

File: src/cosh-ng/crates/cosh-core/src/provider/sysom.rs:445-488
The ~/.copilot-shell/instance_id cache (3-hour TTL, negative results persisted as an empty file) remains absent from the design doc, whose sections otherwise enumerate the probe's observable side effects.

P2 — carried forward (code unchanged): capture re-arm on terminal resize

File: src/cosh-ng/crates/cosh-shell/src/auth/ecs_poll.rs:644-653
Unchanged; the field_capture_revision bump on width-change redraws is still unguarded.

P3 — NEW: "params defaults to an empty object" vs actual null default

File: src/cosh-ng/docs/design/ecs-auth-provisioning.md:48-49 vs src/cosh-ng/crates/cosh-core/src/protocol.rs:108-113
The request-side InputMessage::RegistryRequest field is #[serde(default)] params: Value, whose serde default is Value::Null, not {} (and registry_query_short passes the caller's Value verbatim — its own unit test sends Value::Null, which serializes as "params":null). Behaviorally equivalent today for every handler (.get() on null and on {} both yield None), so this is a wording nit in a contract doc that aims to be wire-exact.

Verified accurate (positive checks on the new doc text)

  • JSONL transport, one object per newline — confirmed on all writers (ecs_probe.rs request push, writeln! in registry_query_short, send_json in cosh_core_service/process.rs). ✔
  • Request envelope (type/request_id/domain/action/params) identical across all three request builders. ✔
  • Core echoes the request's request_id on every RegistryResponse and ignores non-registry_request input lines (fail-closed to a shell-side timeout). ✔
  • "Optional data / error fields are omitted when absent" — OutputMessage::RegistryResponse carries skip_serializing_if = "Option::is_none" on both. ✔
  • Failed-response shape (success:false + optional structured data with error_code + developer-facing error) matches ecs_probe_response / auth_registry_error / registry_error. ✔
  • Time-budget table matches the constants (INTERVAL=2s, OPERATION_LIMIT=5s, WAIT_LIMIT=200s, 12s configure observation; ecs_poll.rs:18-20,284). ✔

Summary and remaining risks

This increment is documentation-only and improves the contract's precision substantially. One new inaccuracy matters: the blanket "cosh-shell rejects a mismatched response" claim does not hold on the short-lived fallback path used by configure. All previously reported findings carry forward unchanged because the cosh-ng code is byte-identical to the last reviewed head. CI on the new head is green for the cosh-ng jobs (Test cosh-ng, Test cosh-ng fast checks, Build cosh-ng release).

Verification and coverage statement

  • Executed (static, at head 72efd76930): local partial clone; tree-to-tree diff old→new head; extraction and reading of ecs-auth-provisioning.md (old and new), cosh_core_registry.rs, cosh_core_service.rs, cosh_core_service/process.rs, ecs_probe.rs, ecs_poll.rs (constants), core registry/auth.rs, registry.rs, protocol.rs; CI check-runs for the new head.
  • Not executed: cargo fmt --check, cargo clippy --all-targets, cargo test --workspace (no code changed since the last review; CI results consulted instead).
  • Not covered: real ECS / PTY behavior; the three anolisa-cli/anolisa-core files that entered via the rebase belong to main's 2508da624d, not to this PR, and were not reviewed here.
  • Remaining risks: the P1 race window and the three carried-forward P2s remain open; the contract-doc overstatement could mislead future implementers.

Review produced by the reviewer automated scanner.

@kongche-jbw kongche-jbw left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Approve 当前 head 72efd76930de9a569677e8a6f0eba9fbd39d760f(base 2508da624d5f6ce3be77549681e4425c79541e99)。

完整评审及后续增量复核后,未发现仍成立的 P0/P1。上轮指出的 prepare 手动回退边界、probe/configure 生命周期两处文档 P2 已修正;提交历史已合理整理为 3 个提交。独立核对 eb34a3191 → 72efd7693,cosh-ng 生产代码与测试内容无变化,仅设计文档更新;当前 head 的 cosh-ng 测试、fast checks、prebuilt action 和 release 构建 CI 均通过,主测试 workflow 为首跑成功。

剩余两处文档问题接受后续补齐,明确不作为本次合并阻断:

  • P2,src/cosh-ng/docs/design/ecs-auth-provisioning.md:46–48:文档将响应 type/request_id 校验写成所有 shell 路径的统一保证。实际 live core 与独占 ECS probe 会校验,configure 的短进程 fallback 没有该校验。后续收窄文档陈述、准确注明各路径的校验范围即可。
  • P3,同文件 :48–49params 缺省值写成了空对象 {};实际 #[serde(default)] params: Valuenull。后续修正文案即可。

历史讨论中的 SIGCHLD 读写窗口 P1 不成立:keep_children_waitable() 完成后才创建 probe worker,评论所述 probe 在该窗口内启动的顺序与代码不符。此前关于自定义 reaper 的 P1 维持撤回。

验证边界不变:本轮为文档增量静态复核,已检查源码对应关系、相对链接、JSON 示例与 diff hygiene;未重跑 Rust 测试或真实 ECS 验收。以上剩余文档跟进不影响本次批准。

Add a component design note for the registry auth prepare/verify/configure
wire contract, the shell polling lifecycle and its time budgets, the
cancellation join/reap completion contract, the unknown-save no-resend rule,
and the probe sole-reaper signal-safety invariant. Required by
documentation-standard.md section 5 for the verify protocol change
(authorized -> status/reason/error_code); shell and core upgrade together.

Assisted-by: Qoder:0.2.5
Signed-off-by: SunnyQjm <mfeng@linux.alibaba.com>
@SunnyQjm
SunnyQjm force-pushed the fix/cosh-3278-auth-ecs-polling branch from 72efd76 to 85ecfbf Compare September 17, 2026 12:35
@SunnyQjm

Copy link
Copy Markdown
Collaborator Author

跟进 #pullrequestreview-5235502277(Approve)与 #issuecomment-5713886489。两处文档准确性问题已修正,候选 85ecfbf4f(仅设计文档变化,24 增 3 删)。

P2 响应校验范围写得过宽:已修正

核实成立,是我上一轮的表述错误。已按三条路径分别说明:

  • live core 传输与独占 ECS probe 会校验 typerequest_idcosh_core_service/process.rsadapter/ecs_probe.rs);
  • configure 在无 live runtime 时走的短进程 fallback 只按 success/data/error 解析首行非空 stdout,不复核关联字段(adapter/cosh_core_registry.rs);文档已注明该路径的独占子进程只承载一个请求,不存在第二个响应。

未按建议给 registry_query_short 补校验:那是本 PR 之外的既有通用路径(extensions/skills/hooks 共用),本轮只让文档与实现一致;如需加固应单独提交并对 main 既有代码做正确归因。

P3 params 默认值:已修正

InputMessage::RegistryRequest#[serde(default)] params: Value,默认是 Value::Null 而非 {}。文档改为"省略或 null 都被接受,读取方式与空对象等价",不再声称默认空对象。

顺带补齐 SIGCHLD 读写窗口的文档记录

reviewer 指出文档未记录该窗口本身。设计文档的 signal-safety 一节现明确:读/写两次调用非原子,但该窗口不可能影响 probe child——规范化在 start_ecs_probe 的调用者线程同步完成并在 spawn worker 之前返回;是否自动回收取决于子进程退出时父进程的处置,而非 spawn 时捕获的快照;给规范化加锁也不能约束不使用同一锁的其他 Command::spawn。P1 结论不变:不成立。

其余承接项

metadata_http_error 泛化文案、instance-id 磁盘 cache(基线既有、服务 provider generate 而非 auth registry 生命周期)、resize 时 capture re-arm(文本经 field_inputinitial_textapply_capture 链路保留,无输入丢失)维持上轮依据,不在本 PR 内改动。

范围与门禁:本轮 diff 仅 docs/design/ecs-auth-provisioning.md;提交历史仍为 3 个,rebase 前后 src/cosh-ng 树哈希同为 a2b0613954fb994b4722b91d1a1145e64000e033,生产代码与测试字节不变(前一候选 72efd7693 的 cosh-ng CI 首跑全绿)。等待新 head CI 复验,未自动合并。

@kongche-jbw
kongche-jbw merged commit 15aa76e into agentic-os-org:main Sep 17, 2026
25 checks passed
@SunnyQjm
SunnyQjm deleted the fix/cosh-3278-auth-ecs-polling branch September 17, 2026 16:10
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.

[cosh-ng] Provider 首配默认命名与 ECS RAM Role 自动认证

3 participants