ci(reborn): run the full reborn_cli dependency closure on every PR - #5110
Conversation
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe reborn-tests CI workflow is updated to build ChangesReborn CI: closure-based package matrix and fallback feature flags
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
github-actions
Bot
added
scope: ci
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the CI packaging script scripts/ci/package-feature-flags.sh to introduce a fallback mechanism that automatically detects and enables default and libsql features for Rust crates without explicit recipes. It also maintains a list of specific crates that should remain flag-free. The review feedback suggests simplifying the fallback_feature_flags function by offloading the filtering and formatting logic entirely to jq, which eliminates the need for multiple subshells and complex Bash array manipulation.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| local feature_list | ||
| feature_list="$( | ||
| jq -r --arg package "${package}" ' | ||
| .packages[] | ||
| | select(.name == $package) | ||
| | .features | ||
| | keys[] | ||
| ' <<< "${metadata}" | ||
| )" | ||
|
|
||
| local features=() | ||
| if printf '%s\n' "${feature_list}" | grep -Fxq "default"; then | ||
| features+=("default") | ||
| fi | ||
| if printf '%s\n' "${feature_list}" | grep -Fxq "libsql"; then | ||
| features+=("libsql") | ||
| fi | ||
|
|
||
| if [ "${#features[@]}" -gt 0 ]; then | ||
| local IFS=, | ||
| printf '%s\n' "--features ${features[*]}" | ||
| fi |
There was a problem hiding this comment.
We can simplify this function significantly by performing the entire feature filtering and formatting logic directly inside jq. This avoids creating multiple subshells (printf and grep twice) and simplifies the Bash array/IFS manipulation, making the script cleaner, faster, and more maintainable.
| local feature_list | |
| feature_list="$( | |
| jq -r --arg package "${package}" ' | |
| .packages[] | |
| | select(.name == $package) | |
| | .features | |
| | keys[] | |
| ' <<< "${metadata}" | |
| )" | |
| local features=() | |
| if printf '%s\n' "${feature_list}" | grep -Fxq "default"; then | |
| features+=("default") | |
| fi | |
| if printf '%s\n' "${feature_list}" | grep -Fxq "libsql"; then | |
| features+=("libsql") | |
| fi | |
| if [ "${#features[@]}" -gt 0 ]; then | |
| local IFS=, | |
| printf '%s\n' "--features ${features[*]}" | |
| fi | |
| jq -r --arg package "${package}" ' | |
| .packages[] | |
| | select(.name == $package) | |
| | .features | |
| | keys | |
| | map(select(. == "default" or . == "libsql")) | |
| | if length > 0 then "--features " + join(",") else empty end | |
| ' <<< "${metadata}" |
|
🚅 Deployed to the ironclaw-pr-5110 environment in ironclaw-ci-preview
|
7d2342b to
bbf71e9
Compare
PR CI's reborn-tests matrix was a name-prefix allowlist of 21 reborn/ product-family crates. But only 10 of those overlap the actual `ironclaw_reborn_cli` dependency closure (53 workspace crates) — so 43 crates the shipped Reborn binary links (auth, host_runtime, skills, first_party_extensions, extensions, dispatcher, llm, safety, memory, network, turns, host_api, loop_support, threads, ...) ran their own test suites ONLY via the nightly/manual closure path, never on a PR. They were exercised on PR only indirectly by the 4 root integration partitions, which don't run those crates' own unit/contract tests. That gap is exactly why the bugs fixed in #5105/#5108 (host_runtime github surface, skills TOCTOU, gsuite wrong-account egress, loop_support/ threads/auth) slipped through normal PR CI and only surfaced when the closure was run by hand. This makes the closure the default PR matrix — "run everything on every PR": - package-matrix: discover the union of the existing allowlist and the `cargo tree -p ironclaw_reborn_cli -e normal,build` closure ∩ workspace members (64 crates). Union (not replace) so non-closure reborn-family crates — channel adapters, webui_v2 — are never dropped from coverage. - package-feature-flags.sh: derive fallback features (default/libsql when declared) for closure crates without an explicit recipe; keep the previously-allowlisted no-flag crates flag-free so their behavior is unchanged. The 3 closure reds this would have caught are already fixed on main (#5105, #5108), so the closure should be green — this PR's own CI run is the 64/64 verification. Tradeoff: 21 -> 64 parallel crate jobs raises compute and, if runner concurrency is capped, may raise wall-clock as jobs queue. Follow-ups: (1) build-once `nextest archive` + shard to cut redundant compiles (spike #5086); (2) bake a few runs, then promote reborn-tests to a required check; (3) cut v1 (`test.yml` / Tests (all-features), ~29m) so the gate drops to ~10-12m. Automated agent-authored. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…he closure host_runtime's integration tests (tests/) link the lib as a normal dependency, so cfg(test) is false there and the deterministic test-mode behavior they assert is gated behind `feature = "test-support"`. The generic default/libsql fallback runs the lib in production mode, so give host_runtime an explicit `--features test-support,libsql` recipe — libsql exercises the embedded-DB paths without needing a Postgres server (which the crate-tests job does not provision). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
bbf71e9 to
857355d
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/reborn-tests.yml:
- Around line 142-145: The error message in the condition checking
closure_packages is misleading because it doesn't distinguish between cargo tree
failing versus returning an empty result. Modify the script to separately
capture and check the exit status of the cargo tree command that generates
closure_packages. If cargo tree exits with a non-zero status, output an error
message indicating the tool failure. Only check for empty or "[]" results if
cargo tree succeeded, and keep the current message for that case.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: ff1bde2e-3b48-4994-bf7f-32b98b592496
📒 Files selected for processing (2)
.github/workflows/reborn-tests.ymlscripts/ci/package-feature-flags.sh
| if [ -z "${closure_packages}" ] || [ "${closure_packages}" = "[]" ]; then | ||
| echo "No Reborn CLI workspace dependency closure crates discovered" >&2 | ||
| exit 1 | ||
| fi |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
Misleading error message on cargo tree failure.
If cargo tree fails, the script will hit this error message, but "No Reborn CLI workspace dependency closure crates discovered" suggests a dependency-graph issue rather than a tool failure. Consider checking cargo tree exit status separately or clarifying the message.
Clearer error handling
+ if ! cargo tree -p ironclaw_reborn_cli -e normal,build --prefix none >/dev/null 2>&1; then
+ echo "cargo tree failed for ironclaw_reborn_cli" >&2
+ exit 1
+ fi
+
closure_packages="$(
comm -12 \🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/reborn-tests.yml around lines 142 - 145, The error message
in the condition checking closure_packages is misleading because it doesn't
distinguish between cargo tree failing versus returning an empty result. Modify
the script to separately capture and check the exit status of the cargo tree
command that generates closure_packages. If cargo tree exits with a non-zero
status, output an error message indicating the tool failure. Only check for
empty or "[]" results if cargo tree succeeded, and keep the current message for
that case.
This branch was successfully deployed
1 participant
What
Make PR CI's
reborn-testsmatrix run the fullironclaw_reborn_clidependency closure (64 crates) instead of the 21-crate name-prefix allowlist.Why
Only 10 of the 21 allowlisted crates overlap the actual closure. 43 closure crates the shipped Reborn binary links —
auth,host_runtime,skills,first_party_extensions,extensions,dispatcher,llm,safety,memory,network,turns,host_api,loop_support,threads, … — ran their own test suites only via the nightly/manual closure path, never on a PR (only exercised indirectly by the 4 root integration partitions).That's exactly the gap that let the bugs in #5105 / #5108 (github surface, skills TOCTOU, gsuite wrong-account egress, loop_support/threads/auth) through normal PR CI — they only surfaced when the closure was run by hand.
How
cargo tree -p ironclaw_reborn_cli -e normal,build∩ workspace members = 64 crates. Union (not replace) so channel adapters / webui_v2 are never dropped.default/libsqlwhen declared) for closure crates without an explicit recipe; previously-allowlisted no-flag crates kept flag-free (behavior unchanged).Verification
The 3 closure reds this catches are already fixed on main (#5105, #5108), so the closure should be green. This PR's own CI run is the 64/64 verification — hence draft.
Tradeoff / follow-ups
21 → 64 parallel crate jobs raises compute and, if runner concurrency is capped, may raise wall-clock as jobs queue.
nextest archive+ shard to cut redundant compiles (spike ci(spike): experimental full-suite gate - nextest archive + mold + sccache + sharding #5086)reborn-teststo a required checktest.yml/ Tests (all-features), ~29m) → gate drops to ~10–12mAutomated agent-authored.