fix(ci): restore green main — duplicate workflow key + missed spawn_subagent test ignore - #5193
Conversation
PR #5081 (9ac7b47) added a second `CARGO_NET_RETRY: "10"` to the `env:` block, duplicating the key #5115 already defined a few lines below. GitHub Actions rejects duplicate mapping keys at workflow-parse time, so every `reborn-tests.yml` run on main since #5081 has failed instantly with 0 jobs ("This run likely failed because of a workflow file issue"). Remove the duplicate; keep the commented original and the new CARGO_HTTP_MULTIPLEXING entry #5081 also added. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
PR #5175 (42307a7) disabled the spawn_subagent capability via the DISABLED_CAPABILITY_IDS deny filter and #[ignore]d the spawn tests in reborn_subagent_spawn_e2e.rs (5) and reborn_trace_first_party_tool_coverage.rs (1), but missed qa_subagent_capability_smoke_uses_child_run in reborn_qa_smoke_scenarios_e2e.rs. With the capability denied, the model's spawn_subagent call is rejected terminally, so the parent run fails with `driver_unavailable` instead of reaching BlockedDependentRun and the test times out — reddening Tests (all-features) on every main commit since #5175. Ignore it with the same TEMP marker; re-enable by emptying DISABLED_CAPABILITY_IDS. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Code Review
This pull request temporarily disables the qa_subagent_capability_smoke_uses_child_run end-to-end smoke test by adding an #[ignore] attribute with a descriptive message. There are no review comments to evaluate, and I have no additional feedback to provide.
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.
📝 WalkthroughWalkthroughUpdates reborn test gating and several test expectations across GitHub capability coverage, operator diagnostics, and Slack operator authorization. ChangesCI and test coverage updates
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 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 |
| # the previous one-job Cargo build serialization as the one-line fallback. | ||
| RUSTFLAGS: "-C linker=clang -C link-arg=--ld-path=/usr/bin/mold" | ||
| CARGO_HTTP_MULTIPLEXING: "false" | ||
| CARGO_NET_RETRY: "10" |
There was a problem hiding this comment.
Removed since this is a duplicate key from below causing the workflow to be skipped
|
|
||
| #[tokio::test] | ||
| #[ignore = "TEMP(disable-spawn-subagents): spawn_subagent temporarily disabled via capability deny filter; re-enable by emptying DISABLED_CAPABILITY_IDS"] | ||
| async fn qa_subagent_capability_smoke_uses_child_run() { |
There was a problem hiding this comment.
#5175 turned off spawn subagent and ignored a bunch of tests but forgot this one
…ailures Temporary: flip the crate-tests matrix to fail-fast:false so every reborn crate runs to completion in parallel and reports independently, instead of the first failure cancelling its ~60 siblings. Lets one CI run surface the full set of regressions that accumulated while reborn-tests was dead. Revert before merge. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
🚅 Deployed to the ironclaw-pr-5193 environment in ironclaw-ci-preview
|
#4859 ("complete operator setup state") consolidated the operator setup reason codes — a wired LLM config no longer emits operator_setup_profile_not_wired / operator_setup_webui_access_not_wired. The aggregate diagnostics contract test predates #4859 and was missed in that update; it merged unnoticed because reborn-tests has been dead since #5081. Assert the codes the path actually emits now (including operator_doctor_workspace_path_blocked) and drop the retired setup codes. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
#5171 ("fix: correct Reborn GitHub API requests") expanded the GitHub tool surface (issue labels/assignees, PR update, review-thread list/resolve/ unresolve, workflow run jobs/artifacts, rerun) from 35 to 48 tools, but the extension_v2_lifecycle_e2e contract test's hardcoded expected list and two count assertions were not updated. It merged unnoticed because reborn-tests has been dead since #5081. Sync the expected ids and counts to 48. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…nfig capability #5185 restricted the Slack admin routes to callers carrying the operator webui-config capability (only the admin webui-v2 token may mutate admin routes — intended, confirmed by the PR author). The four slack_host_beta admin-route tests predate that gate and use operator_caller(), which never set the flag, so they began returning 403 instead of 200. The forbidden path already has dedicated coverage (route_admin_rejects_operator_user_without_operator_capability); these four verify admin-route *logic* for an authorized admin, so give operator_caller the capability. Surfaced once reborn-tests resumed running (dead since #5081). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…merate failures" This reverts commit c17960a.
…_repos test #5171 ("fix: correct Reborn GitHub API requests" / make list_repos PAT scoped) made github.list_repos authenticated-user-only and dropped the `username` parameter (now uses a `type` affiliation filter). The bundled github WASM contract test still passed `{"username":"me","limit":2}`, which the rebuilt wasm now rejects as invalid_parameters. The expected request is already `/user/repos?per_page=2`, so just drop the removed `username` arg. Surfaced once reborn-tests resumed running (dead since #5081); was masked behind the github_v2 tool-list failure in the same crate. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…verage The reborn-tests.yml `env:` block declared `CARGO_NET_RETRY` twice, which GitHub Actions rejects as a duplicate mapping key — the workflow failed to parse (0s "workflow file issue" run on push) and blocked the pull_request checks from ever starting. Same class as #5193 / #5325 on main. Remove the duplicate; keep the commented occurrence. Address PR review feedback on the env-configurable turn-runner concurrency: - env_util: make `strict_env_var_parsed<T>` error message type-aware via `std::any::type_name::<T>()` instead of hardcoding "non-negative integer", which was misleading for the generic helper. - runtime/mod.rs: add caller-level coverage through `build_runtime_input` that `IRONCLAW_REBORN_RUNNER_WORKER_COUNT=0` reaches the built runtime input as `worker_count: None`; add blank/non-numeric fatal tests for a cap var; add a positive bounded `MAX_CONCURRENT_CONVERSATION_RUNS=2 -> Some(2)` test (the field defaults to None, so a silently-ignored override would otherwise pass). - profile_acceptance: add a `cfg(not(feature = "libsql"))` regression that `hosted_single_tenant_volume_build_input` returns `MissingLibsqlFeature`. Also gate two libsql-only test imports so the libsql-on CI build (webui-v2-beta pulls in libsql) compiles warning-free. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
#5265) * feat(reborn): add hosted single-tenant volume profile * Fail loud on hosted volume storage config * fix(reborn): authorize extension lifecycle catalog mounts * feat(reborn): env-configurable turn-runner concurrency with 0 = unlimited Make the Reborn turn-runner concurrency knobs overridable from the environment so a deployment can run with no global throttle to stress-test the database backend (e.g. libSQL) under high write concurrency. New env vars (highest-precedence layer over the `[runner]` config-file section), each with `0` meaning "unlimited": - IRONCLAW_REBORN_RUNNER_WORKER_COUNT - IRONCLAW_REBORN_RUNNER_MAX_CONCURRENT_RUNS_PER_USER - IRONCLAW_REBORN_RUNNER_MAX_CONCURRENT_TRIGGER_RUNS - IRONCLAW_REBORN_RUNNER_MAX_CONCURRENT_CONVERSATION_RUNS The per-user / per-origin caps already treated `None` as unlimited; this extends the same semantics to the global scheduler `worker_count`, which was previously a `NonZeroUsize` clamped to 32. It is now `Option<NonZeroUsize>` where `None` sizes the scheduler semaphore to `tokio::sync::Semaphore::MAX_PERMITS`, leaving the per-user / per-origin caps as the only concurrency bound. `worker_count` only feeds the scheduler semaphore (single scheduler task, no N-task pool), so an unbounded permit count spawns no extra tasks. Implementation notes: - `scheduler_permit_count()` maps the new `Option<NonZeroUsize>` to a permit count (None -> MAX_PERMITS). - `resolve_worker_count()` mirrors the existing `resolve_concurrency_cap()` (absent -> default, 0 -> unlimited, positive -> clamp to 32). - Env overrides apply even with no config file, using strict-presence semantics (set-but-blank / non-numeric is a fatal startup error). - Extracted the shared strict-env helpers into `runtime/env_util.rs` (moved out of `trigger_poller.rs`, no behavior change) and added a generic `strict_env_var_parsed<T>` used by the runner overrides. Tests: env-override precedence + 0=unlimited + clamp + fatal-blank for worker_count and caps; `scheduler_permit_count` None->MAX_PERMITS without panic; existing config-file tests guarded against env bleed. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(ci): drop duplicate workflow key; add runner env-override test coverage The reborn-tests.yml `env:` block declared `CARGO_NET_RETRY` twice, which GitHub Actions rejects as a duplicate mapping key — the workflow failed to parse (0s "workflow file issue" run on push) and blocked the pull_request checks from ever starting. Same class as #5193 / #5325 on main. Remove the duplicate; keep the commented occurrence. Address PR review feedback on the env-configurable turn-runner concurrency: - env_util: make `strict_env_var_parsed<T>` error message type-aware via `std::any::type_name::<T>()` instead of hardcoding "non-negative integer", which was misleading for the generic helper. - runtime/mod.rs: add caller-level coverage through `build_runtime_input` that `IRONCLAW_REBORN_RUNNER_WORKER_COUNT=0` reaches the built runtime input as `worker_count: None`; add blank/non-numeric fatal tests for a cap var; add a positive bounded `MAX_CONCURRENT_CONVERSATION_RUNS=2 -> Some(2)` test (the field defaults to None, so a silently-ignored override would otherwise pass). - profile_acceptance: add a `cfg(not(feature = "libsql"))` regression that `hosted_single_tenant_volume_build_input` returns `MissingLibsqlFeature`. Also gate two libsql-only test imports so the libsql-on CI build (webui-v2-beta pulls in libsql) compiles warning-free. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * test(reborn): drop hosted-volume from dev-only readiness assertion after merge The merge of origin/main adopted main's readiness model where `HostedSingleTenantVolume` is a *preview* diagnostic (`HostedSingleTenantVolumePreview` / `Warning`), not a blocking dev-only profile. The merged test file kept this branch's stale `dev_only_profiles_are_visible_non_production_in_readiness` variant, which still grouped `HostedSingleTenantVolume` with the dev-only profiles and asserted `DevOnlyProfile` / `Blocking` — contradicting the adopted code and main's dedicated `hosted_single_tenant_volume_is_visible_as_preview_readiness` test. Restore main's loop (LocalDev + LocalDevYolo only); the volume profile's preview readiness is covered by the separate test. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat(reborn): trust explicit runner worker_count, drop the 32 clamp `worker_count` is the global scheduler-semaphore permit count. The feature already treats `0` as the "unlimited" sentinel (sizes the semaphore to `Semaphore::MAX_PERMITS`), but a positive value was silently clamped to `MAX_WORKER_COUNT = 32` — so an operator asking for e.g. 64 got 32 with no error, and the only way past 32 was to go fully unlimited. That asymmetry half-defeats the env-configurable knob and diverged from the per-user / trigger / conversation caps, which are passed through verbatim. Remove the clamp and the `MAX_WORKER_COUNT` constant: `Some(n)` now resolves to exactly `n`. This is safe — the permit count just sizes a semaphore counter; runner tasks are still only spawned per claimed run, so a large value degrades smoothly toward the `0` = unlimited regime. Update the two clamp tests to assert verbatim pass-through (512 -> 512) and refresh the `.env.example` note. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * refactor(reborn): apply runner worker_count env override via a sibling helper `runner_settings` applied the three concurrency caps through `apply_cap_env_override` but hand-inlined the `worker_count` override, so the env-override block read asymmetrically and invited the question "why is worker_count special?". The only reason is the type split (worker_count is usize/NonZeroUsize scheduler permits; caps are u32/NonZeroU32) — stable Rust can't express one helper generic over NonZero<T> (ZeroablePrimitive is unstable). Add a sibling `apply_worker_count_env_override` so all four env overrides read as uniform helper calls. Behavior-preserving (runner tests unchanged). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(reborn): reject runner worker_count above the semaphore ceiling Removing the 32-worker clamp let any positive `worker_count` flow verbatim into `scheduler_permit_count` → `TurnRunSchedulerConfig::with_max_concurrent_runs` (which only floors at 1) → `tokio::sync::Semaphore::new(...)`. Tokio's `Semaphore::new` panics above `Semaphore::MAX_PERMITS`, so a malformed config/env value (e.g. `usize::MAX`) crashed startup instead of failing validation — violating the repo's fail-loud boundary rule. `resolve_worker_count` now returns a Result and rejects values above `tokio::sync::Semaphore::MAX_PERMITS` as a config error, while still accepting `1..=MAX_PERMITS` verbatim and keeping `0` as the explicit unlimited sentinel (the unlimited path sizes the semaphore to exactly `MAX_PERMITS`, which Tokio accepts). Both the config-file and env-override paths funnel through it. Adds regression tests for `MAX_PERMITS + 1` on both paths, and fixes the stale `RunnerSection.worker_count` doc that still promised the 32-worker clamp. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(reborn): harden runner worker_count validation + dedup env helpers Addresses the review round on the worker_count work: - High: oversized worker_count could still panic direct composition callers. `DefaultPlannedRuntimeConfig.worker_count` is public, so a caller bypassing the CLI could reach `Semaphore::new(n > MAX_PERMITS)`. `scheduler_permit_count` now saturates at `tokio::sync::Semaphore::MAX_PERMITS` as an infallible backstop (defense-in-depth), with a regression test. - Medium: an oversized config-file worker_count rejected startup before the higher-precedence env override could win. Move the ceiling check off the per-layer resolution into a single `ensure_worker_count_within_ceiling` gate applied to the FINAL merged value, after env precedence. `resolve_worker_count` is pure layering again. Tests: env override rescues an oversized config value; the exact `MAX_PERMITS` boundary is accepted on both config + env paths. - Medium: `runtime/env_util.rs` duplicated the strict-presence + truncation helpers already in `operator_env.rs`. Move `strict_env_var_parsed` into `operator_env.rs` beside its siblings, repoint `runner_settings`, and delete the duplicate module. - Low: add an oversized-invalid-value test asserting the parse error truncates the echoed value. - Low: rename the process-wide test env lock `TRIGGER_ENV_LOCK` / `lock_trigger_env` to scope-neutral `RUNTIME_ENV_LOCK` / `lock_runtime_env`. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: serrrfirat <f@nuff.tech> Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Bug 1 - a typo in a CI config file.
PR #5081 added
CARGO_NET_RETRY: "10"to the reborn-tests.yml workflow's env: block but that exact key was already there a few lines down. YAML doesn't allow the same key twice in one block, so GitHub rejected the whole workflow before runninganything.Bug 2 - a forgotten test when a feature was switched off.
PR #5175 deliberately turned off the
spawn_subagentcapability. When you disable a feature, you also have to skip the tests that exercise it. They #[ignore]-d the 6 obvious spawn tests in two files - but missed a 7th one,qa_subagent_capability_smoke_uses_child_run, which lives in a general QA-smoke test file rather than the spawn-specific file.Bugs 3-6 weren't new - they were hidden. Because Bug 1 stopped
reborn-testsfrom running for ~2 days, the reborn crate test suites never executed, so a handful of regressions from other PRs merged unnoticed. Fixing Bug 1 un-blinded the workflow and surfaced them:Bug 3 - a stale test after operator setup was reworked.
PR #4859 reworked operator setup so a wired LLM config no longer reports the old
operator_setup_profile_not_wired/operator_setup_webui_access_not_wiredreasons (they were consolidated intooperator_setup_service_not_wired). It updated most tests but missedoperator_diagnostics_aggregates_status_setup_and_config_reasons, which still asserted the retired reason codes.Bug 4 - a hardcoded list that didn't grow with the feature.
PR #5171 added ~13 new GitHub tools (issue labels/assignees, PR review threads, workflow rerun, etc.), taking the surface from 35 to 48. The
github_v2_package_discovers_and_publishes_issue_hot_catalogcontract test hardcodes the expected tool list and its count, and wasn't updated.Bug 5 - a test using a parameter the API no longer accepts.
PR #5171 also made
github.list_reposauthenticated-user-only and dropped theusernameparameter. The bundled-GitHub-WASM contract test still passed{"username":"me","limit":2}, which the rebuilt wasm now rejects as invalid parameters. The expected request was already/user/repos?per_page=2, so theusernamearg just needed to go.Bug 6 - a test that didn't get the new admin token.
PR #5185 intentionally restricted the Slack admin routes so only the admin webui-v2 token (the
operator_webui_configcapability) can change admin settings. Four older admin-route tests used a plain operator caller without that capability, so they correctly started returning 403 instead of 200. Confirmed intended by the author - the fix is to give the test caller the new capability, not to change the route.