fix(cosh-ng): [rpm] idempotent shells registration and fail-closed erase - #2599
fix(cosh-ng): [rpm] idempotent shells registration and fail-closed erase#2599SunnyQjm wants to merge 2 commits into
Conversation
|
PR number: #2599 Findings
未发现 blocking package/module/public API 组织问题：未新增 crate、root Open Questions
Validation
|
![qoderai[bot]](https://avatars.githubusercontent.com/in/1396158?s=60&v=4){kind=small}
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0f968bcaff
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Code Review: PR #2599 — fix(cosh-ng): [rpm] idempotent shells registration and fail-closed erase总体结论：未发现阻塞问题，可以合并 ✅PR 正确修复了 PKG-007 和 PKG-010 两个问题，方案设计合理，测试覆盖充分。以下是详细分析。 PKG-007 修复分析（
|
| 场景 | 旧行为 | 新行为 |
|---|---|---|
/etc/shells 无尾部换行 |
条目粘连（/bin/bash/usr/bin/cosh） |
自动补分隔换行 ✅ |
| 文件存在但为空 | nil concatenation crash |
or '' 兜底 ✅ |
子串匹配（/usr/bin/cosh-backup） |
误判为已注册 | true 参数 plain find + 尾部 \n 精确匹配 ✅ |
| 重复安装 | 重复追加 | 字节幂等 ✅ |
关键改进点：
f:seek('set')解决a+模式下读位置在文件末尾的问题f:read('*all') or ''修复了旧代码在空文件时nil..'\n'的 crash（旧 bug bonus fix）normalized:find(nl..cosh..nl, 1, true)使用 plain text 精确匹配，避免了 Lua pattern 的元字符问题
PKG-010 修复分析（%preun guard）
Fail-closed 设计正确：
getent失败 → exit 1 ✅getent返回空 → exit 1 ✅awk过滤失败 → exit 1 ✅- 用户仍引用 cosh 且 replacement 未就绪 → exit 1 并列出用户 ✅
- 升级（
$1 >= 1）→ 不阻塞 ✅ copilot-shell已接管/usr/bin/cosh→ 允许卸载 ✅
%define cosh_replacement_ready 宏设计合理，将 [ -x ... ] 检查和 rpm -q 所有权验证组合为共享谓词。
测试分析
测试质量很高：
packaged_spec_declares_login_shell_lifecycle_sections— 结构性回归保护，防止关键 section 被误删packaged_post_registers_shell_once_across_shells_variants— 7 个变体矩阵，通过真实 RPM Lua 解释器执行，覆盖文件缺失/空/有尾部换行/无尾部换行/已注册/重复注册/子串等场景packaged_post_skips_registration_when_shells_cannot_be_opened— 验证 fail-open 语义packaged_uninstall_blocks_while_passwd_references_cosh— 10 个场景矩阵，使用 bash fixture 模拟getent/rpm/awk的各种正常和异常行为
测试正确放在 raw_cli target 下，与 AGENTS.md 的分层规范一致。Linux-only 测试有 #[cfg(target_os = "linux")] 守卫，macOS 开发环境会正确跳过。
P2 观察（非阻塞，供参考）
P2: %postun Lua 脚本存在类似的尾部换行问题（已有问题，非本 PR 引入）
- 文件与行号：
src/cosh-ng/cosh-ng.spec.in%postunsection - 实际问题：
%postun的 Lua 代码使用io.lines()读取/etc/shells并重写。如果文件无尾部换行，最后一行会被io.lines()跳过（Lua 行为），导致卸载后丢失该条目。同时pairs(t)不保证顺序。 - 建议：这不是本 PR 引入的问题，但建议后续单独 issue 跟踪并修复，保持
%post和%postun的一致性。
P2: %preun awk pipeline 仅检测 awk 失败，未检测 printf 管道上游失败
- 文件与行号：
src/cosh-ng/cosh-ng.spec.in%preunif ! users=$(printf '%s\n' "$passwd_entries" | awk ...) - 实际问题：管道退出码取最后一个命令（
awk）。printf几乎不会在管道中失败，但在极端 I/O 错误场景下，printf失败 +awk成功会导致users为空，guard 被绕过。 - 建议：可考虑
set -o pipefail，但由于这是 RPM scriptlet 上下文（不一定使用 bash），实际风险极低，不阻塞。
P2: yum swap 场景下 %preun 的行为说明
- 实际问题：当执行
yum swap cosh-ng copilot-shell时，RPM 先移除 cosh-ng（%preun运行），再安装 copilot-shell。此时/usr/bin/cosh仍归 cosh-ng 所有，cosh_replacement_ready谓词为 false。如果有用户仍引用 cosh，swap 会被阻塞。 - 评估：这是 正确的 fail-closed 行为 — 用户应先迁移 shell 再执行 swap。
cosh_replacement_ready谓词的价值在于支持 copilot-shell 已独立接管/usr/bin/cosh的场景。建议在 PR 描述或 commit message 中明确说明这一行为，避免后续维护者困惑。
验证范围
- ✅ diff 逻辑正确性：手动验证所有边界情况
- ✅ 测试分层合规（AGENTS.md raw_cli target）
- ✅
%postLua 语义正确性（seek、read、write、close） - ✅
%preunshell 逻辑正确性（getent → awk → predicate 链） ⚠️ 未实际运行cargo test（需要 Linux + RPM 环境）⚠️ 未验证完整 RPM build + install/erase 机器周期（PR 已在容器中完成）
0f968bc to
569f1f5
Compare
|
PR number: #2599 Findings
未发现 blocking package/module/public API 组织问题：本 PR 未触碰 Open Questions
Validation
|
569f1f5 to
3df039e
Compare
|
第二轮机器评审与 @KaiLongZhou 评审的逐条回应（head 1. [P2]
2. [P3] scriptlet 提取的段落顺序依赖 — 采纳：结构锚定新增 awk 顺序断言（%preun < %post < %postun），段落重排会以可读断言失败暴露，并补注释说明切片依赖。 3. [P3] fast gate 的 shellcheck 硬依赖不对称 — 与事实不符：既有 4. [P3] PR body 与 diff 漂移 — body 在迁移当时已更新（Changes/Tests 已指向 5. @KaiLongZhou [P2] 6. @KaiLongZhou [P2] 7. @KaiLongZhou [P2] yum swap 语义说明 — 已在 PR body 补充：单事务 swap 中 rpm 先安装新提供者再跑旧包 8. Open question：rpm 事务内递归 验证：更新后的 harness 在 alinux3 容器（完整路径，含新宏展开断言）与 macOS host（skip 路径）均绿；shellcheck 干净；fmt/clippy/check-layout/check-test-inventory 重跑全绿。 |
|
PR number: #2599 Findings
未发现 blocking package/module/public API 组织问题：本 PR 未触碰 Open Questions
Validation
|
|
回应第三轮评审的 open question「CI 是否真正执行了 %post 完整矩阵（非 skip 路径）」——已按建议查 gate 日志实证：
存量问题 |
|
@SunnyQjm 这里卸载cosh软件包后，导致默认登录shell残留的问题当前已经不存在了，/etc/passwd里面注册为/usr/bin/cosh-login，如果/usr/bin/cosh不存在，则default到bash。 |
|
@KaiLongZhou 感谢指出。已在 AgenticOS 实例（ALinux4，cosh-ng 0.16.1）上实证核查，两个维度分别回应： 1. cosh-login 机制确认属实，但它在包外，且与 %preun 守卫正交互补 实例事实：
正交性实证（该实例真实 passwd）： 2. 关于"卸载守卫与注册逻辑耦合" 守卫的判定依据只有包自身拥有的两个事实：包 Provides 的 基于以上事实请您裁决：a) 按现状合入（两防线正交，零误伤实证如上）；b) 认为应随 cosh-login 方向系统化调整——那我把本 PR 缩窄到 PKG-007（%post 幂等），PKG-010 连同 cosh-login 包化、注册路径调整另立 issue 讨论。 |
|
经与 owner 侧确认，本 PR 决定按 a) 现状合入（保留 %preun 守卫）。补充"若撤守卫、只依赖 cosh-login"的缺口场景矩阵，作为该裁决的完整依据：
反向代价为零：cosh-login 注册形态下守卫判定 "登录形态统一收敛到 cosh-login + cosh-login 包化 + %post 注册路径调整"作为系统性方向，建议另立 issue 跟踪；在其落地前撤守卫会打开上表 1/2/3 的保护空窗。 @KaiLongZhou 如无进一步异议，烦请 approve 放行合入（CI 三轮全绿、评审意见均已闭环）。 |
kongche-jbw
left a comment
kongche-jbw
left a comment
There was a problem hiding this comment.
在最新 head 3df039ef 上复现到 1 个阻塞问题，因此请求修改。
验证环境为 ALinux4 arm64、RPM 4.18.2、Bash 5.2.15。默认运行新增 packaging harness 会稳定失败；仅关闭 Bash 5.2 默认启用的 patsub_replacement 后，同一 head 才能全部通过。当前 CI 全绿并不能覆盖这一 Bash 行为差异。
The %post lua scriptlet appended the login-shell registration with a bare write, so an /etc/shells file without a trailing newline glued the entry to the previous line (/bin/bash/usr/bin/cosh) and the membership probe never matched again, growing the file on every reinstall. Normalize the read content before probing: plain-find the exact newline-delimited entry, and prepend a separator newline when the existing content lacks a trailing one. Registration stays fail-open when /etc/shells cannot be opened. Cover the install/reinstall matrix (missing file, empty, trailing newline, missing trailing newline, existing registrations, substring non-registration) plus the fail-open path with the real RPM Lua interpreter in a dedicated packaging harness (tests/test-package-rpm.sh) wired into the fast/all test gates, skipping the rpm-backed matrix when rpm lua is unavailable. Fixes: 4ce37f9 ("chore(cosh-ng): add cargo config, nightly rpm build, and rust dev standard") Assisted-by: Qoder:1.24.2 Signed-off-by: SunnyQjm <mfeng@linux.alibaba.com>
The spec shipped no %preun, so rpm -e cosh-ng succeeded while passwd
entries still referenced %{_bindir}/cosh as their login shell,
leaving those users with a dangling shell and no warning.
Add a %preun guard that blocks plain erase ($1 = 0) and lists the
affected users on stderr. Enumeration stays fail-closed: a failing or
empty getent and a failing passwd filter all abort the erase. The
shared cosh_replacement_ready predicate keeps the documented
cosh-switch swap path working by allowing removal once
%{_bindir}/cosh is owned by copilot-shell. Upgrades ($1 >= 1) are
never blocked.
Extend the packaging harness with fixture-backed bash runs of the
extracted scriptlet across the guard matrix (referencing user, no
users, broken and empty enumeration, broken filter, replacement
readiness, swap, upgrade), pin the predicate's macro expansion to a
real-newline queryformat via rpm --define, and anchor the spec
lifecycle sections and their extraction order structurally. The
predicate splice disables patsub_replacement (a bash 5.2 default that
expands unquoted & in the replacement to the matched pattern) and
asserts the predicate lands verbatim in the generated scriptlet.
Fixes: 4ce37f9 ("chore(cosh-ng): add cargo config, nightly rpm build, and rust dev standard")
Assisted-by: Qoder:1.24.2
Signed-off-by: SunnyQjm <mfeng@linux.alibaba.com>
3df039e to
67373f6
Compare
|
PR number: #2599 Findings
未发现 blocking package/module/public API 组织问题：本 PR 未触碰 复核要点：
Open Questions
Validation
|
kongche-jbw
left a comment
kongche-jbw
left a comment
There was a problem hiding this comment.
Review baseline: 33ed384643c6...67373f6c420f
[P2] 为新 %preun 声明运行期工具依赖
src/cosh-ng/cosh-ng.spec.in:104 起的守卫依赖 getent 和 awk，但 spec 没有
对应的 Requires(preun)。在缺少任一工具的最小 RPM 系统中，cosh-ng 仍可安装，
随后即使没有用户引用 cosh，普通卸载也会 fail-closed 退出 1，必须手工补装工具
才能移除包。Possible direction: 为实际可执行文件提供者添加 qualified dependency，
或移除外部依赖；再用构建后的 RPM 核对 requires，并覆盖最小 installroot 的卸载。
3 participants
Summary
Fixes the two RPM scriptlet gaps tracked in #2543 (ALinux4 default
login-shell readiness audit, PKG-007 / PKG-010):
%postlua scriptlet appended the/etc/shellsregistration with a bare write. When the file lacked a trailing
newline the entry glued onto the previous line
(
/bin/bash/usr/bin/cosh), and the membership probe never matchedagain, appending another entry on every reinstall.
%preun, sorpm -e cosh-ngsucceeded (rc=0) while passwd entries still referenced
/usr/bin/coshas their login shell, leaving users with a danglingshell and no warning.
Changes
cosh-ng.spec.in%post: normalize the read content before themembership probe (plain
findof the exact newline-delimited entry)and prepend a separator newline when the existing content lacks a
trailing one. Registration stays fail-open when
/etc/shellscannot be opened (unchanged semantics).
cosh-ng.spec.in: add a shared%{cosh_replacement_ready}predicate and a
%preunguard that blocks plain erase ($1 = 0)while users still reference
/usr/bin/cosh, listing them on stderr.Enumeration is fail-closed (failing/empty
getent, failing filterall abort). The predicate keeps the documented
cosh-switch(yum swap) path working by allowing removal once
/usr/bin/coshisowned by copilot-shell (in a single swap transaction rpm installs
the new provider before running the old package's
%preun, so thepredicate observes the new owner). A plain erase while users still
reference the shell stays blocked by design. Upgrades (
$1 >= 1)are never blocked.
tests/test-package-rpm.sh(new packaging harness, mirroringtests/test-package-raw.sh): structural lifecycle-section anchors,%postinstall/reinstall matrix through the real RPM Luainterpreter (missing file, empty, trailing/missing trailing newline,
existing/duplicate registrations, substring non-registration),
%postfail-open behavior, and the%preunguard matrix viafixture-backed bash runs (referencing user, no users, broken/empty
enumeration, broken filter, replacement readiness, swap, upgrade).
scripts/run-test-gates.sh: newrun_rpm_packaginggate(shellcheck + execution) wired into the
fastandallgates,following the existing
run_raw_packagingpattern.Tests
tests/test-package-rpm.sh, run byrun_rpm_packaginginscripts/run-test-gates.sh fast|all. The RPM-Lua-backed%postmatrix skips (with an explicit SKIP notice) when the rpm lua
interpreter is unavailable (macOS dev hosts); the
%preunmatrixand structural anchors always run.
Review follow-up: the scriptlet coverage initially landed in
tests/raw_cli/passthrough.rs; per review it moved to this dedicatedpackaging harness, and
passthrough.rsis untouched now.Verification
Focused scope (run, green):
%postscriptlet viathe real rpm embedded Lua interpreter in an alinux3 container
(arm64): before — glued
/bin/bash/usr/bin/coshplus duplicategrowth on reinstall; after — single clean entry, byte-idempotent.
%preunacceptance in the same container with a realuseradd -s /usr/bin/coshuser: erase blocked (rc=1, user listed),upgrade allowed, clean erase allowed.
bash tests/test-package-rpm.shgreen in the alinux3 container(full matrix incl. rpm lua) and on the macOS host (skip path for
the
%postmatrix);shellcheck tests/test-package-rpm.shclean.cargo fmt --check,cargo clippy --workspace --all-targets -- -D warnings,crates/cosh-shell/scripts/check-layout.sh,scripts/check-test-inventory.sh— all green after rebase ontolatest main; additional
cargo clippy -p cosh-shell --all-targetson rust 1.97.1 clean.
Excluded scope (not run):
rpm -Uvh/rpm -emachine cycle. Scriptlet-levelacceptance was chosen for this PR; the issue's archived double-arch
full-package runs are the machine-level reference for the failing
baseline.
by CI).
Evidence
Hosted on a fork orphan branch (
pr-2599-assets), pinned by commit SHA:pkg007-fail-baseline.log
pkg007-pass-acceptance.log
pkg010-fail-baseline.log
pkg010-pass-acceptance.log
rust-anchors-linux-test.log
Closes #2543