fix(deps): preflight runtime dependencies - #2557
Conversation
37bdd24 to
796dd4d
Compare
796dd4d to
b437ae7
Compare
kongche-jbw
changed the title
95e98b9 to
05f48a5
Compare
|
@codex review |
![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: 05f48a5ee2
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
899421a to
35746f1
Compare
Forrest-ly
left a comment
Forrest-ly
left a comment
There was a problem hiding this comment.
Code review for commit 35746f1 — approving.
What I verified
- Ran
tests/test-build-all-runtime-deps.shlocally against this commit: 28/28 pass. - Ran
runtime_dependencies_for_manifestagainst all 8 real component manifests: emits exactly the expected 15 records; agent-memory and os-skills correctly contribute zero. - Preflight ordering confirmed in code and tests: platform capabilities are checked before user source setup; system mode blocks platform-capability / language-runtime blockers before any package mutation; one deduplicated APT transaction preceded by
apt-get update(failure stops before mutation); re-probe after install; install phase re-verifies before any component file lands. - The two earlier automated findings (sec-core
systemdexclusion for source builds, APT refresh before the aggregated install) are both addressed in this commit with regression tests. - User mode never calls
as_rootfor runtime packages; system probes use the fixedRUNTIME_SYSTEM_PATH, so a user-local nvm Node cannot satisfy the system contract; the emitted retry command is absolute and preserves--system/--deps-only/--ignore-deps(covered for--component memory). - agent-memory code is untouched; it participates via the generic manifest-driven preflight (it declares no dependencies), so no new requirements are introduced for memory installs.
- The ws-ckpt probe change (
btrfs version→mkfs.btrfs --version) matches what the removed installer actually required (cmd_exists mkfs.btrfs).
Non-blocking suggestions
scripts/build-all.sh:2128,2150— thelocal -nnamerefs are the script's first bash ≥ 4.3 dependency. On bash 4.2 hosts (CentOS/Anolis 7 era) the preflight would error out instead of reporting. Likely acceptable given the ws-ckpt/sight kernel floors and EL7 EOL, but worth confirming EL7 is out of support scope, or documenting a minimum bash version.- User-visible behavior change worth a release note: missing runtime dependencies now hard-fail user-mode installs, where bwrap/gpg/jq previously only warned. Intentional per the PR body, but users will notice.
- The awk TOML reader assumes single-line
packages = { ... }inline tables and no|character in any field. Current manifests comply and the 15-record assertion guards against drift, but consider a one-line comment nearruntime_dependencies_for_manifestspelling out these constraints so future manifest edits don't silently drop records.
Nice work on the aggregated reporting, the strict order-of-operations guarantees, and the test coverage.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 35746f1d87
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| while IFS= read -r component; do | ||
| manifest="$(runtime_manifest_path "$component")" || return 1 | ||
| dependencies="$(runtime_dependencies_for_manifest "$component" "$manifest")" || return 1 |
There was a problem hiding this comment.
With the default user-mode invocation on a host where btrfs is neither registered nor loadable, this loop includes the default ws-ckpt component and its btrfs platform dependency, so the initial platform-only preflight aborts before any dependency setup or build. However, src/ws-ckpt/Makefile explicitly makes INSTALL_PROFILE=user a no-op that installs no ws-ckpt files, so its runtime requirements are irrelevant in this mode; filter dependencies by the selected install profile before probing them.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 70d65071. Runtime dependencies are now collected only for components that install files in the selected profile, so user mode skips all ws-ckpt daemon runtime and btrfs platform probes while system mode retains them. A regression test covers both default user selection and explicit system ws-ckpt selection.
35746f1 to
a463f7b
Compare
Forrest-ly
left a comment
Forrest-ly
left a comment
There was a problem hiding this comment.
Code review for commit a463f7bf (re-review after force-push squash) — approving.
What I verified
P1 fix (skip runtime checks for user-ineligible components). The delta vs the previous head 35746f1d is exactly the fix plus one regression test: a new runtime_install_components() filter (user mode drops ws-ckpt) now feeds both selected_runtime_dependencies and source_build_runtime_dependencies. I confirmed this is correct:
src/ws-ckpt/MakefilemakesINSTALL_PROFILE=userinstall a genuine no-op ("User mode: ws-ckpt requires a systemd/root daemon; skipping install."), and its manifest declares[component.layout] modes = ["system"], so its daemon runtime/btrfs requirements are irrelevant in user mode.- Ran the real (non-stubbed) collection logic on a simulated btrfs-less host (empty
/proc/filesystemsentry + emptyRUNTIME_SYSTEM_PATHsomodprobeis unavailable): user mode with default components collects 0 platform-capability gaps (previously aborted before any dependency setup), while system mode with--component ws-ckptstill correctly reportsws-ckpt|btrfs [platform-capability]as missing. - System mode with default components retains all three ws-ckpt records (
btrfs-progswith themkfs.btrfs --versionprobe,rsync,btrfsplatform capability). - The filter is correctly scoped:
sightis the only other mode-restricted component, but its user profile genuinely installs files (user PREFIX,INSTALL_SYSTEMD=0), so it rightly remains unfiltered.
Tests. Ran tests/test-build-all-runtime-deps.sh against this commit locally: 29/29 pass, including the new test_user_skips_ws_ckpt_noop_install_dependencies covering both directions (user default omits ws-ckpt; explicit system ws-ckpt keeps it).
Earlier P2s remain satisfied in the squashed diff. sec-core systemd is excluded from source-build dependencies (runtime_dependency_for_source_build), and DEB system preflight refreshes APT indexes before the single deduplicated transaction with a hard stop on refresh failure. Both retain their regression assertions.
Full squashed single-commit diff reviewed (all 5 files, +1300/-55): manifest-driven awk parser with key-based (not order-based) reads, source-build adapters (openssl1.1→openssl, sec-core nodejs→node >=20), two-phase user preflight (platform-only → source setup → full), system preflight blocking platform/language-runtime blockers before any package mutation, fixed RUNTIME_SYSTEM_PATH contract for system installs (nvm can't satisfy it), absolute retry commands preserving --system/--deps-only/--ignore-deps, and the guarded main enabling the test harness to source the script. CI changes are consistent: the sec-core source-build jobs preinstall the runtime contract (or let system-mode preflight install it on a clean apt state, which exercises the new aggregated path), and the new check-build-runtime-deps job wires the test suite into CI. agent-memory participates via its manifest, which declares no dependencies — no new requirements for memory installs.
Non-blocking notes (no action required)
- The user-mode
ws-ckptexemption is hardcoded; if more mode-restricted components appear, deriving the filter from[component.layout] modeswould generalize it. Current approach is explicit, commented, and test-covered, so this is fine for now. - Carry-over from the previous review, still present and still acceptable:
local -nnamerefs require bash >= 4.3, and missing system packages now hard-fail user-mode installs (previously warn-only). Both were already acknowledged.
a463f7b to
ecdcf96
Compare
- Aggregate manifests and block system changes before dependency setup. - Preserve user-local source provisioning, then re-probe every runtime. - Parse manifest keys safely and batch native system package installs. Fixes: 6e4a741 ("refactor(build): introduce unified build workflow") Signed-off-by: kongche-jbw <kongche.jbw@alibaba-inc.com>
ecdcf96 to
70d6507
Compare
Forrest-ly
left a comment
Forrest-ly
left a comment
There was a problem hiding this comment.
Code review 通过（re-review @ 70d6507）。
对比上一轮已批准的 head a463f7b，70d6507 的变更仅限 .github/workflows/sec-core-source-code-build.yaml（+17/-2），两处 CI 改进均正确：
UV_PYTHON_INSTALL_MIRROR指向官方 astral-sh python-build-standalone 源：scripts/build-all.sh中_configure_uv_mirror使用${UV_PYTHON_INSTALL_MIRROR:-<nju mirror>}，workflow 级 env 在 CI 中正确生效，本地构建默认值不受影响，与 PR body 描述一致。- 构建失败时打印
target/build.log最后 200 行（::group::折叠）并保留原始退出码：日志路径与LOG_FILE="$OUTPUT_DIR/build.log"（OUTPUT_DIR=$PROJECT_ROOT/target）一致；在 GitHub Actions 默认bash -e下逐句验证（|| build_status=$?捕获、if (( ... ))条件上下文、tail ... || echo兜底、exit "$build_status"），成功/失败两条路径行为均正确，用户模式成功后追加$GITHUB_PATH的顺序也保留。
同时确认：
scripts/build-all.sh、manifests、tests 在两个 head 之间无任何变更，上轮验证过的逻辑原样保留；bot 的 P1 修复（user 模式跳过不安装文件的 ws-ckpt 的 runtime 依赖收集）在最终 head 仍成立。- 本地回归
./tests/test-build-all-runtime-deps.sh在 70d6507 上 29/29 通过；YAML 解析与bash -n均通过。 - CI（70d6507）：Source Build (Ubuntu 22.04 / Alinux4)、Source Build System Install、Check build runtime dependencies、Test anolisa 等全部 SUCCESS，无 failure。
代码质量良好，未发现新问题，approve。
2 participants
Why
build-all.shcould build or begin installation before discovering missing runtimedependencies such as
bwrap. It also checked only sec-core, so users could hitthe same one-at-a-time failure pattern in other selected components.
What changed
and report all missing capabilities together.
and mapping source-specific Node and OpenSSL requirements.
such as nvm, then re-probe every runtime and report remaining gaps once.
mutation, refresh APT indexes for DEB hosts, install native packages in one
deduplicated transaction, and re-probe.
user nvm never satisfies the system contract, and no external repository is added.
mkfs.btrfsandmodprobe.\n- Skip ws-ckpt runtime probes in user mode because its user install profile is a no-op.--deps-onlyand--ignore-deps.Related issue
Related to #2520. The direct
linux-sandboxdiagnostic remains separate.User / Agent impact
Default user builds retain automatic Node setup through nvm. Missing runtime
dependencies are still reported together before component files are installed.
System installs require language runtimes to be present in the fixed system PATH;
ordinary native packages may be installed automatically after all blockers pass.
Risk and compatibility
System mode may refresh APT metadata and invoke the detected package manager once
for missing native runtime packages, but it never configures NodeSource or another
external repository. User mode never invokes privileged runtime package
installation; its existing user-local source dependency installers remain available.
Validation
bash -n scripts/build-all.sh tests/test-build-all-runtime-deps.sh./tests/test-build-all-runtime-deps.sh(29/29)cargo test -p anolisa-core manifest --locked(93 passed)deb/rpminline-table keysmkfs.btrfsmkfs.btrfsis unavailable\n- User installs omit ws-ckpt daemon runtime and platform requirementstarget/build.log\n-git diff --checkDocumentation and rollback
No documentation changes are required. Revert commit
70d65071to restore theprevious component-specific behavior.