Unbreak the package-set clippy lane, make the tracing targets real, and close the #7144 defect set - #7154
Unbreak the package-set clippy lane, make the tracing targets real, and close the #7144 defect set#7154BenKurrek wants to merge 14 commits into
Conversation
…t needs (#7119) `cargo clippy -p ironclaw -p ironclaw_reborn_config --lib --bins --all-features -- -D warnings` exits 101 on a clean `origin/main` checkout with three `unused import` errors in `ironclaw_reborn_composition`. The PR lane in `code_style.yml` builds exactly that invocation from `changed_workspace_packages.py`, so `main` is red for any PR whose diff produces that package set. Mechanism, measured: the three imports are named only by `#[cfg(any(test, feature = "test-support"))]` accessors. `test-support` on `ironclaw_reborn_composition` is enabled exclusively through *dev-dependency* edges (`ironclaw_reborn_cli`, `ironclaw_product`, the root test package, and the crate's own self-edge). `--lib --bins` builds no dev-dependencies, so with the crate outside the selected set the feature stays off, the accessors vanish, and the imports are unused. Every whole-workspace lane builds `--tests`, which pulls those dev-dependencies in and unifies the feature back on — which is why the merge queue has been green over a red tree. Two changes: - Gate the imports with the same `cfg` as their users. - Add the missing lint shape to the non-PR lane: `cargo clippy --all --lib --bins -- -D warnings`. `default` flavour on purpose — `--all-features` re-enables `test-support` on every selected package and masks the class. Sabotage-tested: with the `cfg` gates removed the new command exits 101 listing exactly the three unused imports; with them restored it exits 0. Run workspace-wide it also answers the issue's open question — there are no other latent instances of this class in the production feature shape. Refs #7119 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…med `target` (#7146) `tracing::warn!(target = "…")` records a *field* named `target`; `=` is the field-assignment operator. The event's metadata target stays the emitting module path, so a subscriber filtering the named target never sees the event. `target: "…"` is the syntax that sets it. Both forms were in the tree — 121 field-form sites against 53 correct ones — and `docs/` teaches operators to filter on exactly the targets the field form makes unreachable. That is the worst shape of logging bug: the log looks configured, the filter returns nothing, and the operator concludes the code path never ran. - 120 literal-valued sites rewritten to `target:`. - The 121st, `ironclaw_loop_host::tool_disclosure_port`, is a genuine field carrying a runtime tool name — it cannot be a metadata target, so it is renamed to `tool`. That leaves the rule absolute with no production allowlist. - `reborn_tracing_target_syntax` scans `crates/` for a first-argument `target =` in any target-bearing macro. It found the 121st site the literal-only sweep had missed. - `metadata_target_only_follows_the_colon_form` emits both forms through a capturing subscriber and reads `event.metadata().target()`, so the language fact the gate rests on is measured, not asserted in a comment. - `forbidden_origin_announces_itself_on_the_ws_origin_target` pins one real production emission the same way. Asserted off metadata rather than rendered output on purpose: the field form also prints `target=` in the formatted line, so a text assertion passes on the broken form. Sabotage-tested: reverting the WS-origin site to `target =` fails the scan naming that exact file and line; restoring it passes. The scan also refuses to run vacuously — it asserts it read the tree. Refs #7146 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
|
🚅 Deployed to the ironclaw-pr-7154 environment in ironclaw-ci-preview
|
github-actions
Bot
added
scope: ci
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis PR corrects tracing metadata targets, adds a regression gate, improves extraction and latency behavior, hardens HTTP and trace egress, fixes trace-contribution state handling, and changes Slack migration and CI classification behavior. ChangesTracing metadata and CI
Runtime and extraction
HTTP egress
Trace contributions
Slack migration and CI classification
Estimated code review effort: 5 (Critical) | ~90 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 2 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (2 passed)
✨ Finishing Touches 💡 1⚔️ Resolve merge conflicts 💡
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
github-actions
Bot
added
size: L
…#7115) `docker/reborn/entrypoint.sh` skipped its "strip retired `[slack]` setup fields" migration whenever `IRONCLAW_REBORN_SLACK_ENABLED` was truthy. That variable lost its last Rust reader in #6116, which deleted the enablement-gate path outright — this line was the only thing left in the repo reading it. The operator docs told people to set it to `true`. So following the documented setup turned the migration off, left `signing_secret_env` / `bot_token_env` in `config.toml`, and those are exactly the retired keys that make `ironclaw serve` fail closed. Following the docs produced a container that would not boot, and the mechanism built to prevent that was disabled by the same instruction. - Drop the `is_truthy` clause; the awk condition is the whole signal. - State the `enabled = true` carve-out as a choice rather than an accident: a config that looks live is left alone and fails startup with a migration pointer instead of being silently rewritten. - Correct the five operator docs. They taught more than the dead variable: `docs/channels/slack.mdx` documented a `[slack]` key table in which every setup key now makes `serve` refuse to start, so the doc instructed operators to build an unbootable config. `[slack].enabled` and `[telegram].enabled` are inert too — neither has a reader outside the config struct. - `scripts/ci/test-reborn-docker-entrypoint.sh` drives the real entrypoint with a stub `ironclaw` on PATH and asserts the migration fires for every truthy spelling `is_truthy` accepts, that the `enabled = true` carve-out holds, and that the variable regains no reader. Driven through the script rather than the awk block on purpose: the defect was in the `if` wrapping that block, so a test on the block alone would have passed on the broken script. - Wired into the `Static-check self-tests` step, and `docker/reborn/entrypoint.sh` added to the `has_code` path filter so the lane actually lights up for the diff that could break it. Sabotage-tested: restoring the `is_truthy` clause fails the self-test with 11 findings — 10 surviving legacy keys plus the reader check. Refs #7115 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
🔎 Review · PR #7154
The target changed before this Run could finish. Automatic · PR opened · attempt 0 of 3 · cancelled after 1s Run details
|
… is off (#7103) `trace_coding_latency` computed `output_bytes` before checking anything. That value feeds only the latency trace, and both `trace_tool_ok` and `trace_tool_error` return immediately on `None` fields — so every successful `read_file` / `write_file` / `apply_patch` / `list_dir` / `grep` result was fully serialized to count its bytes on every deployment that had not enabled the `ironclaw_latency` TRACE target, which is all of them by default. `ironclaw_observability`'s charter is zero-cost-when-off: the trace was, this field was not. The two neighbouring constructors in `latency.rs` already check `live_latency_enabled()` before measuring; this now matches them. Not the same as the `web_access.rs` / `gsuite/handlers.rs` call sites, which also call `json_bytes` unconditionally — there the value feeds `ResourceUsage::set_output_bytes`, i.e. resource accounting, which must happen regardless of tracing. Those are correct as written and untouched. Covered by driving `CodingCapabilityState::dispatch` — the public entry point, which is also what builds the latency fields — over a 16 KiB file and asserting the byte counter never moved. "No work happened" has no other observable signature, so `json_bytes` gains a `#[cfg(test)]` thread-local call counter (thread-local because `#[tokio::test]` is current-thread, so a parallel sibling cannot pollute it). The test also asserts the counter still increments when the helper *is* called, so a dead probe cannot report success forever. Sabotage-tested: removing the guard fails the assertion with left: 1, right: 0. Refs #7103 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ng real failures (#7104) `DocumentExtraction` distinguishes `Empty` ("the extractor succeeded but produced no usable text") from `Failed` ("unsupported/corrupt"), and the two consumers render different model-facing text from it. But five private extractors returned `Err` for the succeeded-but-empty case, and `extract_document` maps every `Err` to `Failed` — so a well-formed image-only slide deck, an empty spreadsheet, a picture-only `.docx` or a text-free `.rtf` told the model "[Could not extract text from … ]" when the file had been processed fine and simply had no text. That message invites a retry that cannot help; `Empty` tells the truth. The five now return `Ok(String::new())` and let `extract_document`'s existing trim-and-classify produce `Empty`, which is already what it does for the UTF-8 path. One trap on the way: for PPTX and XLSX the empty-result `Err` was *also* the only surfacing of an entry refused by the decompression bound — entries that trip it are skipped with `continue`. Returning `Ok("")` unconditionally would have downgraded the zip-bomb guard's observable outcome from "failed" to "no text found". Both loops now remember the first rejection and still fail when nothing else yielded text, so the guard keeps its signal. The pre-existing `extract_pptx_rejects_oversized_slide` caught this. Also fixes the adjacent #7144 finding in the same file: `try_extract_by_extension` discarded the error from `extract_document_text_by_filename`, dropping the caller into the "unsupported document type" arm — which by contract means *no extractor was attempted*. A corrupt `.docx` under a generic MIME type was therefore reported as an unknown format, and the real parse error reached neither the caller nor the log. Both covered through `extract_document`, the public classifier, since the `Err -> Failed` mapping is the wrapper that turns the wrong return value into the wrong model-facing text. Sabotage-tested: restoring either behaviour fails its test. Refs #7104, #7144 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
![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 `@crates/ironclaw_architecture/tests/reborn_tracing_target_syntax.rs`:
- Around line 136-175: The rust_files traversal and
tracing_macros_set_the_metadata_target scan currently suppress filesystem
errors; make the scan fail instead of passing with incomplete coverage. Change
rust_files and its callers to return and propagate Result errors from read_dir
and directory entries, include affected paths in errors, skip both target and
node_modules, and propagate read_to_string failures rather than continuing. Add
a regression test covering a missing or unreadable scan root.
🪄 Autofix
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: 97bbb66d-70d1-4421-ac22-5e2ce5428f15
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock,!**/Cargo.lock
📒 Files selected for processing (38)
.github/workflows/code_style.ymlcrates/ironclaw_architecture/Cargo.tomlcrates/ironclaw_architecture/tests/reborn_tracing_target_syntax.rscrates/ironclaw_extension_host/src/channel_host.rscrates/ironclaw_extension_host/src/channel_outbound_targets.rscrates/ironclaw_extension_host/src/channel_subject_routes.rscrates/ironclaw_extension_host/src/channel_triggered_delivery.rscrates/ironclaw_extension_host/src/run_delivery_ports.rscrates/ironclaw_loop_host/src/tool_disclosure_port.rscrates/ironclaw_outbound/src/delivery_targets.rscrates/ironclaw_outbound/src/outbound_state_store.rscrates/ironclaw_product/src/run_delivery.rscrates/ironclaw_product/src/run_delivery/gate_routes.rscrates/ironclaw_product/src/run_delivery/observer.rscrates/ironclaw_product/src/run_delivery/triggered.rscrates/ironclaw_reborn_cli/src/commands/serve.rscrates/ironclaw_reborn_cli/src/commands/serve_sso.rscrates/ironclaw_reborn_cli/src/runtime/mod.rscrates/ironclaw_reborn_composition/src/automation/trigger_poller.rscrates/ironclaw_reborn_composition/src/factory/trigger_creation_assembly.rscrates/ironclaw_reborn_composition/src/llm_admin/openai_compat_serve.rscrates/ironclaw_reborn_composition/src/runtime.rscrates/ironclaw_reborn_event_store/src/coalescing_sink.rscrates/ironclaw_reborn_event_store/src/lib.rscrates/ironclaw_reborn_openai_compat/src/error.rscrates/ironclaw_webui/Cargo.tomlcrates/ironclaw_webui/src/auth/google.rscrates/ironclaw_webui/src/auth/routes.rscrates/ironclaw_webui/src/cli_token_login.rscrates/ironclaw_webui/src/lib.rscrates/ironclaw_webui/src/oidc.rscrates/ironclaw_webui/src/session.rscrates/ironclaw_webui/src/webui_body_limit.rscrates/ironclaw_webui/src/webui_rate_limit.rscrates/ironclaw_webui/src/webui_serve.rscrates/ironclaw_webui/src/webui_v2/error.rscrates/ironclaw_webui/src/webui_v2/handlers.rscrates/ironclaw_webui/src/webui_ws_origin.rs
| fn rust_files(dir: &Path, out: &mut Vec<PathBuf>) { | ||
| let Ok(entries) = std::fs::read_dir(dir) else { | ||
| return; | ||
| }; | ||
| for entry in entries.flatten() { | ||
| let path = entry.path(); | ||
| if path.is_dir() { | ||
| if path.file_name().is_some_and(|name| name == "target") { | ||
| continue; | ||
| } | ||
| rust_files(&path, out); | ||
| } else if path.extension().is_some_and(|extension| extension == "rs") { | ||
| out.push(path); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| #[test] | ||
| fn tracing_macros_set_the_metadata_target() { | ||
| let crates_dir = workspace_root().join("crates"); | ||
| let mut files = Vec::new(); | ||
| rust_files(&crates_dir, &mut files); | ||
| files.sort(); | ||
| assert!( | ||
| !files.is_empty(), | ||
| "found no Rust files under {} — the scan would pass vacuously", | ||
| crates_dir.display() | ||
| ); | ||
|
|
||
| let mut report = String::new(); | ||
| let mut violations = 0usize; | ||
| let mut scanned = 0usize; | ||
| for file in &files { | ||
| // This file emits the field form on purpose, to measure what it does. | ||
| if file.file_name().is_some_and(|name| name == SELF_FILE) { | ||
| continue; | ||
| } | ||
| let Ok(source) = std::fs::read_to_string(file) else { | ||
| continue; | ||
| }; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Make the scan fail on incomplete traversal.
Lines 137-150 and Lines 173-175 silently skip filesystem errors. A permission error or unreadable Rust file can then let this regression gate pass without scanning all production sources.
Return Result from rust_files and propagate read_dir, directory-entry, and read_to_string errors with the affected path. Exclude node_modules explicitly as well as target.
Add a regression test for a missing or unreadable scan root. Based on learnings: architecture source scans must fail on unreadable paths and exclude both node_modules and target directories.
🤖 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 `@crates/ironclaw_architecture/tests/reborn_tracing_target_syntax.rs` around
lines 136 - 175, The rust_files traversal and
tracing_macros_set_the_metadata_target scan currently suppress filesystem
errors; make the scan fail instead of passing with incomplete coverage. Change
rust_files and its callers to return and propagate Result errors from read_dir
and directory entries, include affected paths in errors, skip both target and
node_modules, and propagate read_to_string failures rather than continuing. Add
a regression test covering a missing or unreadable scan root.
Sources: Coding guidelines, Learnings
…redirect gate (#7144) #7144 finding 1 said three credential-attaching HTTP builders in the trace pipeline never inspect `url.scheme()`. Sweeping every credential-attaching path in `crates/` found the exposure is narrower than that in one place and wider in another. **`ironclaw_host_runtime` credential injection — the real chokepoint.** `apply_credential_injection` checked `scheme() == "https"` for `PathPlaceholder` only. `Header`, `QueryParam` and `BodyJsonPointer` had no check, so a bearer token could be attached to a plaintext `http://` URL. The manifest audience gate does reject non-https for WASM/MCP, but `host_port::stage_credentials` performs no audience match at all, so nothing guaranteed it. The check now covers all four kinds. No loopback carve-out: the `PathPlaceholder` arm has shipped without one, and the measured loopback-http consumers (Ollama, self-hosted mem0, the sandbox broker) all use their own clients and never reach this chokepoint. **Trace Commons submit/status/revoke.** `pinned_trace_remote_http_client` carries the enrolled bearer to `policy.ingestion_endpoint`. The comment above it claimed the lane was validated via `validate_trace_commons_ingest_url`; nothing on this path ever called it — only `community_profile_url_from_policy` did. Meanwhile `ironclaw traces opt-in --endpoint <url>` writes the endpoint unvalidated, so `--endpoint http://public-host/...` shipped the token in clear text. The builder now validates, which makes the comment true. **A twelfth green-but-inert gate, found on the way.** `ironclaw_network`'s `strip_credential_headers` filtered `authorization`/`cookie`/`proxy-authorization` before each redirect hop — over a vector that is always empty, because `request.headers` is moved into the transport request by `mem::take` beforehand. Proved on pristine `origin/main`: inserting `assert!(headers.is_empty())` at the top of that function leaves all three redirect tests green. Its contract test passed because *no* header survived a hop, never because credentials were filtered. Behaviour is unchanged and the pretence is gone: the function is now `clear_headers_for_next_hop`, defensive rather than decorative. A denylist could not have been made correct anyway — `RuntimeCredentialTarget::Header` lets a manifest name its credential header anything, so `x-api-key` and friends were never covered. "Nothing follows a hop" needs no enumeration. The contract test now asserts the observed hop carries *no* headers and seeds an `x-api-key` alongside the `authorization` it used to check alone. Sabotage evidence, each restored to green afterwards: - removing the https guard lets the plaintext credential test through; - removing the trace endpoint validation builds a client for `http://traces.example.test`; - forwarding the header buffer past the hop fails the redirect contract test, naming both leaked headers. Refuted with measurement, not fixed: the Ollama / self-hosted-mem0 / LLM `base_url` paths permit loopback and private-range http deliberately and document it (`.env.example`, the catalog default, the keyless mem0 test); they attach credentials through their own clients, not this chokepoint. Refs #7144 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Each of these is pre-existing and verbatim on `main`; #7124 only moved the file, so the move-only PR could not also change semantics. Working through them by consequence. **Privacy gate keyed on prose (finding 2).** Dataset eligibility was decided by scanning `warnings` for the substring `"quarantined"`, whose sole producer is one English sentence. Rewording, translating or localising it silently opened the gate — quietly, and in the permissive direction. `PrivacyMetadata` now carries a typed `quarantined` flag set by the producer beside that sentence, and the gate keys on it plus the typed `residual_pii_risk` (which covers envelopes persisted before the field existed, since `#[serde(default)]` gives them `false`). Prose is prose again: the sabotage test replaces the whole sentence with German and the gate still holds. **Sidecar security tests passing vacuously (finding 6).** The stderr suppression, environment scrubbing and oversized-stdout tests opened with `if !Path::new("/bin/sh").exists() { return; }` — success while asserting nothing. Now `#[cfg(unix)]`, so they do not exist on Windows rather than silently passing there (Windows runs `cargo check`, never this suite), and on unix a missing shell is a hard failure. Deliberately not the `IRONCLAW_REQUIRE_DOCKER_TESTS` shape: that flag is set nowhere in the repo, so the gate it guards is itself inert. **Redaction sidecar deadlock (finding 7).** The parent wrote up to 1 MiB into stdin with nothing draining stdout, and the timeout covered only `wait_with_output` — so a sidecar emitting more than one pipe buffer before reading wedged both ends under no timeout at all, leaking a live child per turn. Write and drain now run concurrently, both under the timeout. Every pre-existing sidecar test starts `cat >/dev/null` with 5 bytes of input, i.e. the one ordering that cannot deadlock; the new test inverts both. **Fabricated server receipt (finding 5).** A 2xx whose body did not parse was turned into `status: "submitted"` with a *locally estimated* credit, recorded as Submitted, and the queued envelope deleted — destroying the only retryable copy. Every receipt field has a serde default, so `{}` already parses; reaching that branch means the body was not JSON at all. It is now an error. **Compaction deleting a held envelope (finding 4).** The fail-loud hold read was swallowed by `.ok().flatten()`, so an unreadable sidecar ranked a held envelope as unheld and compaction deleted it — a consent artifact, lost silently, while every other IO failure in that function propagates. Now propagated with context. The existing telemetry test already built this exact fixture and asserted a *downstream* symptom; it now asserts the earlier, accurate failure. **Durable identifiers derived from `Debug` (finding 8).** `vector_key` addresses rows in a vector store; the credit fingerprint is persisted in `submissions.json` and compared on every load to keep an acknowledged notice suppressed. Both are now built from explicit `as_str`-style methods frozen at the values `Debug` produced, so nothing already persisted moves and a rename has to come to the `match`. Measured and *not* changed: adding `#[serde(rename_all = "snake_case")]` to `TraceCreditEventKind` for consistency with its 22 siblings would make every existing `submissions.json` fail to deserialize — that file has no schema version and no migration. The inconsistency is load-bearing and now says so. **Unbounded process-global maps (finding 9).** `TRACE_UPLOAD_CLAIM_CACHE` held one live-or-stale *bearer token* per user subject forever (expiry was filtered on read, never evicted); it now sweeps expired entries on write behind a `CREDIT_VIEW_CACHE_MAX_SCOPES`-shaped cap. `TRACE_SCOPE_MUTATION_LOCKS` sweeps entries with `Arc::strong_count == 1` — explicitly *not* the wholesale `clear()` that bounds the credit cache, because these `Arc`s are the mutual-exclusion identity and evicting a held one would hand the next caller a fresh uncontended mutex. **Smaller (finding 10).** `redaction_hash` no longer hashes zero bytes on a serialization failure, which gave every failing trace the same digest for dedupe and integrity — it is fallible now, and `rescrub_trace_envelope` carries it. `novelty_score` is clamped at both ends like its sibling. The trace card derives its retention policy from `allowed_uses` through the same ranking `retention_policy_for_trace` uses, instead of hardcoding `private_corpus_revocable` — they disagreed for three of five consent scopes, and the card is what crosses the wire. A malformed `tool_calls` payload still yields no calls but is no longer silent. "standaloneice key" corrected in both doc comments. Every fix above is sabotage-tested: the change is reverted, the new test observed failing with the right message, then restored and re-run green. Refs #7144 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The path-placeholder contract test pinned the old, kind-specific message; #7144 widened the guard to every injection kind, so it no longer names one. Same variant, same refusal before transport. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…suite in CI `test_live_canary_workflow_shards_cover_non_telegram_qa_suite` asserted a packaging string from `reborn-e2e.yml` that no longer exists — the step now pipes `tar` into `gzip`, so the archive name is the redirect target rather than a tar argument. The reason it stayed broken is worse than the assertion: **no CI lane has ever run `scripts/reborn_webui_v2_live_qa/test_run_live_qa.py`.** 204 tests, executed by nothing. Five of them had drifted red against the code they gate. - The shard roster test now asserts what actually has to hold: the archive carries both members (canonical `ironclaw` plus the `ironclaw-reborn` compatibility copy QA consumers still invoke), and the redirect writes `ironclaw-reborn.tar.gz`. - Two test doubles were missing methods their production caller uses — `raise_for_status` on the extension-setup responses, and an exact-dict payload assertion that predates the generated `client_action_id` idempotency key. Fixed; the doubles now model the API the caller actually exercises. - The four remaining extension-setup-API tests are marked `@unittest.expectedFailure`, not skipped and not deleted. The body still runs, the failure is still real, and the day the #6520 operator-catalog contract is modelled correctly they turn into unexpected *passes* and go red — which a skip could never do. Each carries the specific projection its double is missing. - Wired into the `Static-check self-tests` step, with `scripts/reborn_webui_v2_live_qa/`, `scripts/live-canary/`, `live-canary.yml` and `reborn-e2e.yml` added to the `has_code` path filter so the lane lights up for the diffs that can break it. Sabotage-tested: corrupting the tar assertion fails the suite naming that test; restored, `OK (skipped=5, expected failures=4)`. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
github-actions
Bot
added
scope: docs
# Conflicts: # crates/ironclaw_reborn_composition/src/runtime.rs
BenKurrek
changed the title
|
⚠ WIP — handing off mid-verification. The last merge is UNVERIFIED. Tip ⚠ The #7119 premise has changed and needs re-checking before this ships
What is NOT fixed on main, and is the part of this PR that still matters: the CI gap. No lane lints the shape that produced the failure, so the class remains invisible. This PR's Required before merge: re-run Verified before the merge (each sabotage-tested: break it, watch it go red with the right message, restore, green)
Full per-item detail, including the four findings I closed as not-a-defect with refuting measurements, is on #7144, #7119, #7146, #7115, #7104 and #7103. |
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 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 `@crates/extensions/ironclaw_extension_support/src/latency.rs`:
- Around line 6-11: Update the documentation for json_bytes to require an active
latency-tracing context only for latency-measurement callers, and explicitly
state that resource-accounting callers may invoke it with tracing disabled. Keep
the description consistent with the implementation and tests without changing
behavior.
In `@crates/ironclaw_extractors/src/lib.rs`:
- Around line 798-855: Add caller-level coverage to
text_free_but_valid_documents_classify_as_empty_not_failed by constructing valid
text-free XLSX and DOCX inputs and passing them through extract_document. Assert
both return DocumentExtraction::Empty, using the real classifier path and
appropriate MIME types/filenames; keep the existing PPTX, RTF, legacy binary,
and bomb-deck assertions unchanged.
In `@crates/ironclaw_host_runtime/src/services/tests.rs`:
- Around line 503-572: Extend
host_http_egress_refuses_to_attach_a_credential_over_plaintext_http to include a
BodyJsonPointer target with a valid JSON request body. Configure the request so
the body-pointer credential follows the same plaintext HTTP path, then assert
the HTTPS credential error and that recorded_requests remains empty, matching
the existing Header and QueryParam cases.
In `@crates/ironclaw_reborn_traces/src/contribution.rs`:
- Around line 2227-2259: Replace the child.wait_with_output() call in the
tokio::join! block with custom bounded readers that enforce max_stdout_bytes and
max_stderr_bytes limits concurrently with the write_request. Create separate
async tasks to drain child's stdout and stderr, each checking its respective
size limit as data flows in. When a bounded reader detects an overflow, kill the
child process and return a limit-exceeded error. Preserve the existing timeout
and error-mapping structure around the joined operations.
- Around line 11175-11199: The regression test should require a successful
redaction result, not merely completion before the outer timeout. Update
command_privacy_filter_does_not_deadlock_on_a_sidecar_that_writes_before_reading
to make the sidecar emit valid JSON only after its large output is handled,
avoid relying on seq under the /bin/sh precondition, and assert the returned
redacted text is “ok” while retaining the timeout assertion.
In `@scripts/reborn_webui_v2_live_qa/test_run_live_qa.py`:
- Around line 5888-5894: Add the explicit None return annotation to all three
raise_for_status methods in scripts/reborn_webui_v2_live_qa/test_run_live_qa.py
at ranges 5888-5894, 6084-6090, and 6191-6197, preserving their existing
behavior.
- Around line 456-464: Slack setup contract tests are being suppressed instead
of validating current behavior. In
scripts/reborn_webui_v2_live_qa/test_run_live_qa.py:456-464, update the catalog
fixture to model the extension.<id> operator-catalog group and revision used by
_extension_setup_submission, then remove `@unittest.expectedFailure`. Apply the
corresponding projection updates and remove the decorator at
scripts/reborn_webui_v2_live_qa/test_run_live_qa.py:5868-5876 for
operator-catalog behavior, :6066-6074 for lifecycle readiness, and :6172-6180
for secret presence, so failures remain visible in CI.
🪄 Autofix
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: 4c74654f-1b06-4215-9ca7-8905f1df445e
📒 Files selected for processing (18)
.env.example.github/workflows/code_style.ymlcrates/extensions/ironclaw_extension_support/src/coding/mod.rscrates/extensions/ironclaw_extension_support/src/latency.rscrates/ironclaw_extractors/src/lib.rscrates/ironclaw_host_runtime/src/egress/credential.rscrates/ironclaw_host_runtime/src/services/tests.rscrates/ironclaw_host_runtime/tests/runtime_http_egress_contract.rscrates/ironclaw_network/src/egress.rscrates/ironclaw_network/tests/network_http_egress_contract.rscrates/ironclaw_reborn_traces/src/contribution.rsdocker/reborn/entrypoint.shdocs/capabilities/configuration.mdxdocs/channels/slack.mdxdocs/reborn/deploy-reborn-cli-docker.mddocs/reborn/setup-slack-for-reborn-binary.mdscripts/ci/test-reborn-docker-entrypoint.shscripts/reborn_webui_v2_live_qa/test_run_live_qa.py
💤 Files with no reviewable changes (1)
- .env.example
| /// Serializes `value` purely to count the bytes it would occupy. | ||
| /// | ||
| /// Cheap per byte but *not* free: it walks the whole value, and a `read_file` | ||
| /// output can be large. Every caller must therefore establish that latency | ||
| /// tracing is live before calling — the counter below is how tests prove they | ||
| /// do, since "no work happened" has no other observable signature. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Correct the json_bytes caller contract.
The new text says every caller must enable latency tracing. The related coding rationale documents byte measurement that must run with tracing off for resource accounting. Limit this requirement to latency-only callers and state the accounting exception.
Proposed wording
-/// output can be large. Every caller must therefore establish that latency
-/// tracing is live before calling — the counter below is how tests prove they
-/// do, since "no work happened" has no other observable signature.
+/// output can be large. Callers that measure bytes only for latency tracing
+/// must establish that tracing is live before calling. Resource-accounting
+/// callers can require measurement while tracing is off.As per path instructions, “comments/documentation promising guarantees must match code and tests.”
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /// Serializes `value` purely to count the bytes it would occupy. | |
| /// | |
| /// Cheap per byte but *not* free: it walks the whole value, and a `read_file` | |
| /// output can be large. Every caller must therefore establish that latency | |
| /// tracing is live before calling — the counter below is how tests prove they | |
| /// do, since "no work happened" has no other observable signature. | |
| /// Serializes `value` purely to count the bytes it would occupy. | |
| /// | |
| /// Cheap per byte but *not* free: it walks the whole value, and a `read_file` | |
| /// output can be large. Callers that measure bytes only for latency tracing | |
| /// must establish that tracing is live before calling. Resource-accounting | |
| /// callers can require measurement while tracing is off. |
🤖 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 `@crates/extensions/ironclaw_extension_support/src/latency.rs` around lines 6 -
11, Update the documentation for json_bytes to require an active latency-tracing
context only for latency-measurement callers, and explicitly state that
resource-accounting callers may invoke it with tracing disabled. Keep the
description consistent with the implementation and tests without changing
behavior.
Source: Path instructions
| fn text_free_but_valid_documents_classify_as_empty_not_failed() { | ||
| use std::io::{Cursor, Write}; | ||
|
|
||
| fn pptx_with_slides(slides: &[&str]) -> Vec<u8> { | ||
| let mut writer = zip::ZipWriter::new(Cursor::new(Vec::new())); | ||
| let options = zip::write::SimpleFileOptions::default() | ||
| .compression_method(zip::CompressionMethod::Stored); | ||
| for (index, xml) in slides.iter().enumerate() { | ||
| writer | ||
| .start_file(format!("ppt/slides/slide{}.xml", index + 1), options) | ||
| .expect("start slide"); | ||
| writer.write_all(xml.as_bytes()).expect("write slide"); | ||
| } | ||
| writer.finish().expect("finish zip").into_inner() | ||
| } | ||
|
|
||
| // A valid deck whose slides carry only markup — an image-only deck. | ||
| let image_only_deck = pptx_with_slides(&["<p:sld><p:cSld><p:spTree/></p:cSld></p:sld>"]); | ||
| assert_eq!( | ||
| extract_document( | ||
| &image_only_deck, | ||
| "application/vnd.openxmlformats-officedocument.presentationml.presentation", | ||
| Some("deck.pptx"), | ||
| ), | ||
| DocumentExtraction::Empty, | ||
| "an image-only deck was processed fine and simply has no text" | ||
| ); | ||
|
|
||
| // A structurally valid RTF document with no text runs. | ||
| assert_eq!( | ||
| extract_document(br"{\rtf1\ansi}", "application/rtf", Some("empty.rtf")), | ||
| DocumentExtraction::Empty | ||
| ); | ||
|
|
||
| // Legacy binary with no printable run long enough to be text. | ||
| assert_eq!( | ||
| extract_document(&[0x00, 0x01, 0x02, 0x03, 0x04], "application/msword", None), | ||
| DocumentExtraction::Empty | ||
| ); | ||
|
|
||
| // The distinction still holds in the other direction: a deck whose only | ||
| // slide is refused by the decompression bound is a *failure*, not a | ||
| // text-free file. Without this the #7104 fix would have downgraded the | ||
| // zip-bomb guard's observable outcome to "no text found". | ||
| let bomb_deck = | ||
| pptx_with_slides(&[&format!("<a:t>{}</a:t>", "x".repeat(60 * 1024 * 1024))]); | ||
| assert!( | ||
| matches!( | ||
| extract_document( | ||
| &bomb_deck, | ||
| "application/vnd.openxmlformats-officedocument.presentationml.presentation", | ||
| Some("bomb.pptx"), | ||
| ), | ||
| DocumentExtraction::Failed(_) | ||
| ), | ||
| "an entry refused by the size guard must stay Failed" | ||
| ); | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Add caller-level empty-classification coverage for XLSX and DOCX.
This test covers PPTX, RTF, and legacy binary only. Lines 371-376 and Lines 393-399 also change XLSX and DOCX behavior. Add valid text-free .xlsx and .docx inputs through extract_document and assert DocumentExtraction::Empty.
As per coding guidelines, “New or changed production-wired behavior must have a caller-level test.” As per path instructions, the Test through the caller invariant requires the real classifier path.
🤖 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 `@crates/ironclaw_extractors/src/lib.rs` around lines 798 - 855, Add
caller-level coverage to
text_free_but_valid_documents_classify_as_empty_not_failed by constructing valid
text-free XLSX and DOCX inputs and passing them through extract_document. Assert
both return DocumentExtraction::Empty, using the real classifier path and
appropriate MIME types/filenames; keep the existing PPTX, RTF, legacy binary,
and bomb-deck assertions unchanged.
Sources: Coding guidelines, Path instructions
| async fn host_http_egress_refuses_to_attach_a_credential_over_plaintext_http() { | ||
| for target in [ | ||
| RuntimeCredentialTarget::Header { | ||
| name: "authorization".to_string(), | ||
| prefix: Some("Bearer ".to_string()), | ||
| }, | ||
| RuntimeCredentialTarget::QueryParam { | ||
| name: "access_token".to_string(), | ||
| }, | ||
| ] { | ||
| let scope = sample_scope(); | ||
| let capability_id = sample_capability_id(); | ||
| let handle = SecretHandle::new("api-token").unwrap(); | ||
|
|
||
| let network = RecordingNetwork::ok(); | ||
| let recorded_requests = Arc::clone(&network.requests); | ||
| let services = test_services() | ||
| .with_secret_store(Arc::new(SecretStore::ephemeral())) | ||
| .try_with_host_http_egress(network) | ||
| .expect("host HTTP egress should wire with graph secret store"); | ||
| let mut policy = staged_policy(); | ||
| // Let the *policy* admit plaintext http, so the refusal under test is | ||
| // the credential guard and not the network allowlist. | ||
| policy.allowed_targets = vec![NetworkTargetPattern { | ||
| scheme: None, | ||
| host_pattern: "api.example.test".to_string(), | ||
| port: None, | ||
| }]; | ||
| services | ||
| .network_policy_store | ||
| .insert(&scope, &capability_id, policy.clone()); | ||
| services | ||
| .secret_injection_store | ||
| .insert( | ||
| &scope, | ||
| &capability_id, | ||
| &handle, | ||
| SecretMaterial::from("staged-secret"), | ||
| ) | ||
| .expect("staged credential should be seeded"); | ||
| let egress = configured_egress(&services); | ||
|
|
||
| let mut request = | ||
| request_with_staged_credential(scope, capability_id.clone(), handle.clone()); | ||
| request.url = "http://api.example.test/v1/run".to_string(); | ||
| request.network_policy = policy; | ||
| request.credential_injections = vec![RuntimeCredentialInjection { | ||
| handle, | ||
| source: RuntimeCredentialSource::StagedObligation { capability_id }, | ||
| target: target.clone(), | ||
| required: true, | ||
| }]; | ||
|
|
||
| let error = egress | ||
| .execute(request) | ||
| .await | ||
| .expect_err("a credential must never be attached to a plaintext URL"); | ||
| assert!( | ||
| matches!( | ||
| &error, | ||
| ironclaw_host_api::http::RuntimeHttpEgressError::Credential { reason } | ||
| if reason.contains("HTTPS") | ||
| ), | ||
| "expected an HTTPS credential refusal for {target:?}, got {error:?}" | ||
| ); | ||
| assert!( | ||
| recorded_requests.lock().unwrap().is_empty(), | ||
| "the request must not reach the network at all for {target:?}" | ||
| ); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Cover BodyJsonPointer at the egress caller.
The loop covers only Header and QueryParam. Add a BodyJsonPointer case with a valid JSON body. Assert the same HTTPS error and zero recorded network requests.
As per coding guidelines, “New or changed production-wired behavior must have a caller-level test.” As per path instructions, “Test through the caller.”
🤖 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 `@crates/ironclaw_host_runtime/src/services/tests.rs` around lines 503 - 572,
Extend host_http_egress_refuses_to_attach_a_credential_over_plaintext_http to
include a BodyJsonPointer target with a valid JSON request body. Configure the
request so the body-pointer credential follows the same plaintext HTTP path,
then assert the HTTPS credential error and that recorded_requests remains empty,
matching the existing Header and QueryParam cases.
Sources: Coding guidelines, Path instructions
| // The write must run *concurrently* with draining stdout, and the whole | ||
| // exchange must sit under the timeout. | ||
| // | ||
| // Before #7144 the parent wrote up to `max_input_bytes` (1 MiB by | ||
| // default) into stdin while nothing read stdout, and the timeout covered | ||
| // only `wait_with_output`. A sidecar that emits more than one pipe | ||
| // buffer (64 KiB) before draining its input deadlocks both ends, and the | ||
| // parked `write_all` is under no timeout at all — so the redaction task | ||
| // wedges forever, leaking a live child process per turn. | ||
| // `kill_on_drop` does not help: nothing cancels a future that is never | ||
| // polled to completion. | ||
| let write_request = async move { | ||
| stdin.write_all(&request_body).await?; | ||
| stdin.shutdown().await?; | ||
| drop(stdin); | ||
| Ok::<(), std::io::Error>(()) | ||
| }; | ||
| let (write_result, output) = tokio::time::timeout(self.timeout, async move { | ||
| tokio::join!(write_request, child.wait_with_output()) | ||
| }) | ||
| .await | ||
| .map_err(|_| TraceContributionError::RedactionFailed { | ||
| reason: format!( | ||
| "privacy filter sidecar timed out after {}ms", | ||
| self.timeout.as_millis() | ||
| ), | ||
| })?; | ||
| write_result.map_err(|error| TraceContributionError::RedactionFailed { | ||
| reason: format!("failed to write privacy filter request: {error}"), | ||
| })?; | ||
| let output = output.map_err(|error| TraceContributionError::RedactionFailed { | ||
| reason: format!("privacy filter sidecar failed: {error}"), | ||
| })?; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Enforce sidecar output limits while reading.
child.wait_with_output() buffers both streams before the length checks run. A sidecar can exhaust process memory before max_stdout_bytes or max_stderr_bytes rejects its output.
Drain stdout and stderr with bounded readers concurrently with stdin. When a reader exceeds its limit, kill and reap the child, then return the limit error.
🤖 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 `@crates/ironclaw_reborn_traces/src/contribution.rs` around lines 2227 - 2259,
Replace the child.wait_with_output() call in the tokio::join! block with custom
bounded readers that enforce max_stdout_bytes and max_stderr_bytes limits
concurrently with the write_request. Create separate async tasks to drain
child's stdout and stderr, each checking its respective size limit as data flows
in. When a bounded reader detects an overflow, kill the child process and return
a limit-exceeded error. Preserve the existing timeout and error-mapping
structure around the joined operations.
| async fn command_privacy_filter_does_not_deadlock_on_a_sidecar_that_writes_before_reading() { | ||
| require_posix_shell(); | ||
| let adapter = CommandPrivacyFilterAdapter::new("/bin/sh") | ||
| .with_args([ | ||
| "-c", | ||
| // Fill the stdout pipe well past its buffer, then read stdin. | ||
| "printf 'x%.0s' $(seq 1 262144); cat >/dev/null; \ | ||
| printf '{\"redacted_text\":\"ok\"}'", | ||
| ]) | ||
| .with_output_limits(2 * 1024 * 1024, 64 * 1024); | ||
|
|
||
| let big_input = "y".repeat(256 * 1024); | ||
| let result = tokio::time::timeout( | ||
| std::time::Duration::from_secs(30), | ||
| adapter.redact_text(&big_input), | ||
| ) | ||
| .await; | ||
|
|
||
| assert!( | ||
| result.is_ok(), | ||
| "the sidecar exchange deadlocked: stdin must be written concurrently \ | ||
| with draining stdout, and the whole exchange must sit under the \ | ||
| adapter timeout" | ||
| ); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Make the deadlock regression require successful redaction.
The sidecar emits x...{json}, so redact_text() returns a JSON parse error after draining output. result.is_ok() checks only the outer timeout. The test therefore passes on any immediate adapter failure. seq is also not guaranteed by the /bin/sh precondition.
Proposed test fix
- "printf 'x%.0s' $(seq 1 262144); cat >/dev/null; \
+ "printf '%262144s' ''; cat >/dev/null; \
printf '{\"redacted_text\":\"ok\"}'",
@@
- assert!(
- result.is_ok(),
- "the sidecar exchange deadlocked: stdin must be written concurrently \
- with draining stdout, and the whole exchange must sit under the \
- adapter timeout"
- );
+ let redaction = result
+ .expect("the sidecar exchange must not deadlock")
+ .expect("the sidecar exchange must succeed")
+ .expect("the sidecar must return redaction");
+ assert_eq!(redaction.redacted_text, "ok");As per coding guidelines, “Every bug fix must include a regression test that fails before the fix.”
🤖 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 `@crates/ironclaw_reborn_traces/src/contribution.rs` around lines 11175 -
11199, The regression test should require a successful redaction result, not
merely completion before the outer timeout. Update
command_privacy_filter_does_not_deadlock_on_a_sidecar_that_writes_before_reading
to make the sidecar emit valid JSON only after its large output is handled,
avoid relying on seq under the /bin/sh precondition, and assert the returned
redacted text is “ok” while retaining the timeout assertion.
Source: Coding guidelines
| # Pre-existing red, and pre-existing *invisible*: no CI lane has ever run | ||
| # this module, so these four drifted out of sync with the #6520 extension | ||
| # setup contract unnoticed. `expectedFailure` rather than a skip or a | ||
| # deletion — the body still runs, the failure is still real, and the day the | ||
| # contract is modelled correctly this turns into an unexpected *pass* and | ||
| # goes red, which a skip could never do. To clear one: teach its double the | ||
| # operator-catalog projection (`extension.<id>` group + revision) that | ||
| # `_extension_setup_submission` now routes non-secret fields through. | ||
| @unittest.expectedFailure |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
Do not suppress the Slack setup contract tests.
@unittest.expectedFailure converts any failure in these tests into a passing test result. This hides regressions in Slack installation, lifecycle readiness, and secret-presence validation after the suite is wired into CI.
scripts/reborn_webui_v2_live_qa/test_run_live_qa.py#L456-L464: update the catalog fixture and remove@unittest.expectedFailure.scripts/reborn_webui_v2_live_qa/test_run_live_qa.py#L5868-L5876: model the current operator-catalog projection and remove@unittest.expectedFailure.scripts/reborn_webui_v2_live_qa/test_run_live_qa.py#L6066-L6074: model the current lifecycle projection and remove@unittest.expectedFailure.scripts/reborn_webui_v2_live_qa/test_run_live_qa.py#L6172-L6180: model the secret-presence projection and remove@unittest.expectedFailure.
📍 Affects 1 file
scripts/reborn_webui_v2_live_qa/test_run_live_qa.py#L456-L464(this comment)scripts/reborn_webui_v2_live_qa/test_run_live_qa.py#L5868-L5876scripts/reborn_webui_v2_live_qa/test_run_live_qa.py#L6066-L6074scripts/reborn_webui_v2_live_qa/test_run_live_qa.py#L6172-L6180
🤖 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 `@scripts/reborn_webui_v2_live_qa/test_run_live_qa.py` around lines 456 - 464,
Slack setup contract tests are being suppressed instead of validating current
behavior. In scripts/reborn_webui_v2_live_qa/test_run_live_qa.py:456-464, update
the catalog fixture to model the extension.<id> operator-catalog group and
revision used by _extension_setup_submission, then remove
`@unittest.expectedFailure`. Apply the corresponding projection updates and remove
the decorator at scripts/reborn_webui_v2_live_qa/test_run_live_qa.py:5868-5876
for operator-catalog behavior, :6066-6074 for lifecycle readiness, and
:6172-6180 for secret presence, so failures remain visible in CI.
| def raise_for_status(self): | ||
| # The production extension-setup path calls this on the catalog | ||
| # response. A double that omits a method its caller uses turns a | ||
| # real assertion into an AttributeError — the suite has never run | ||
| # in CI, so the drift went unnoticed (#7144-adjacent). | ||
| return None | ||
|
|
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add the missing return annotations.
Ruff reports ANN202 for all three raise_for_status methods. Add -> None to each method.
🧰 Tools
🪛 Ruff (0.16.0)
[warning] 5888-5888: Missing return type annotation for private function raise_for_status
Add return type annotation: None
(ANN202)
📍 Affects 1 file
scripts/reborn_webui_v2_live_qa/test_run_live_qa.py#L5888-L5894(this comment)scripts/reborn_webui_v2_live_qa/test_run_live_qa.py#L6084-L6090scripts/reborn_webui_v2_live_qa/test_run_live_qa.py#L6191-L6197
🤖 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 `@scripts/reborn_webui_v2_live_qa/test_run_live_qa.py` around lines 5888 -
5894, Add the explicit None return annotation to all three raise_for_status
methods in scripts/reborn_webui_v2_live_qa/test_run_live_qa.py at ranges
5888-5894, 6084-6090, and 6191-6197, preserving their existing behavior.
Source: Linters/SAST tools
|
Coordinator note: this PR's headline premise no longer holds — #7119 is closed on measurement (the exact clippy invocation completes clean on |
`Detect Reborn test scope` failed on this PR with
Reborn PR test planner failed: unclassified pull-request path: .env.example
and took `Tests (Reborn)` down with it — that job's first step is
`changes failed: failure`, so the whole roll-up is red on one
classification gap. Same class as #7064/#7087: the planner's fail-closed
arm rejects any path it has no rule for, and the only satisfiable
behaviour for an unclassified class is "never edit it".
The error names one path because the planner raises on the first miss in
sorted order. Driving `build_plan()` over this PR's 56 changed paths one
at a time shows the real set is two:
* `.env.example` — the environment-variable reference `CLAUDE.md` and
five operator docs point at. Nothing in the repository reads it; every
reference is a comment or a doc string. Prose, in the same class as
`docs/` and `.claude/`, so it is ignored.
* `docker/reborn/entrypoint.sh` — shell that no Reborn Rust lane
executes, but which Code Style now owns end to end: this PR's
`scripts/ci/test-reborn-docker-entrypoint.sh` drives the real script
in the script self-test step, and `code_style.yml`'s `has_code` filter
names the path. Classified as static-control so the plan *names* the
owner rather than silently skipping it.
(`platform-and-compat.yml`'s `has_docker_risk` is keyed to
`Dockerfile`/`.dockerignore` and owns the image build, not this.)
`docker/` is classified per-file, for the reason repo-root `scripts/` is:
a blanket prefix would absorb `docker/reborn/config.*.toml` and
`docker/process-sandbox-entrypoint.sh`, which have no owning lane. A test
pins that those still refuse.
Four regression tests, each verified red by reverting the classification
it covers — removing the `.env.example` arm fails
`test_operator_env_reference_is_classified_and_selects_no_rust_lane` and
`test_classified_operator_paths_do_not_mask_a_real_lane` with
`unclassified pull-request path: .env.example`; removing the entrypoint
entry fails the entrypoint test and the same masking test with
`unclassified pull-request path: docker/reborn/entrypoint.sh`. Both are
paired assertions (accepted AND selects no Rust lane), and the masking
test drives both new paths beside a crate change so a per-PR shortcut
cannot pass where a per-path rule is required.
Verified: `python3 scripts/ci/test_reborn_pr_test_plan.py` 47/47 OK, and
the real CLI over this PR's changed-file list plus `--base-sha` exits 0
with `mode: selected`, 28 affected packages, 3 crate buckets.
Refs #7087
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`cargo test -p ironclaw` does not compile on this branch:
error[E0063]: missing field `quarantined` in initializer of `PrivacyMetadata`
--> crates/ironclaw_reborn_cli/src/commands/traces/tests.rs:134:18
The #7144 privacy commit replaced the prose-substring quarantine check with a
typed `PrivacyMetadata::quarantined` flag and updated the producer
(`contribution.rs:2644`) but not the only other construction site in the
workspace, this CLI test fixture. `false` is what the fixture already meant:
its `residual_pii_risk` is `Low`, and `quarantines_trace` only quarantines
`High`, so the envelope's eligibility is unchanged.
Repo-wide there are exactly two `PrivacyMetadata { ... }` construction sites
and both are now correct; nothing else in the workspace names the type.
Why no lane caught it: the field is only reachable from a `--tests` build of
`ironclaw`, and on a pull request nothing builds that. The Code Style clippy
PR lane runs `--lib --bins`; the `--all --tests --examples` lane is
`github.event_name != 'pull_request'`; and every Reborn job in this PR's run
reported `skipping` because `Detect Reborn test scope` had already failed. The
same event asymmetry that hid #7119 hid this.
Verified: `cargo test -p ironclaw --no-fail-fast` → 478 + 6 + 145 = 629 tests,
0 failed; `cargo clippy -p ironclaw --all-targets --all-features -- -D warnings`
exit 0; `cargo fmt --all -- --check` clean.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
BenKurrek
changed the title
Re-scoped, merged with
|
| tree | result |
|---|---|
the three #[cfg] gates removed (= origin/main's content) |
exit 101, unused import ×3 at runtime.rs:418,419,420 |
| gates restored (this PR) | exit 0 |
Corroborated structurally: cargo tree -p ironclaw --all-features -e features,no-dev -i ironclaw_reborn_composition resolves composition to default + memory-mem0 — no test-support — exactly the mechanism the commit describes.
Worth knowing for anyone re-measuring: cargo clippy does not reliably invalidate its unit cache on a change to the flags after --, so re-running over an already-fresh unit can exit 0 without re-linting. The pair above forces a recompile in both directions. The commit stays. #7119 is currently closed; on this measurement it should not be — flagging rather than reopening.
⛔ Blocker surfaced: the #7144 HTTPS guard breaks two existing tests, and it is real
trace_commons_dispatch_e2e::account_login_link_through_dispatch
trace_commons_instance_dispatch_e2e::instance_only_user_passes_dispatch_gate_and_mints_login_link
error_code="AccountLoginLinkFailed"
Causality proven, not inferred: deleting the eight-line guard from apply_credential_injection and changing nothing else turns both green (5 passed; 0 failed, 1 passed; 0 failed); restoring it turns both red. The guard is restored in the pushed tree — neither test is weakened and neither is deleted. The lane will be red, on purpose.
The commit reasoned "No loopback carve-out, deliberately: … the measured loopback-http consumers (Ollama, a self-hosted mem0, the sandbox broker) all use their own clients and never reach this chokepoint." The Trace Commons agent path does reach it — mint_account_login_link_inner sends the login-link POST with bearer_token: Some(..) through the host RuntimeHttpEgress sink, i.e. straight into apply_credential_injection, and standalone Trace Commons is http://127.0.0.1.
And the PR now contradicts itself: validate_trace_commons_ingest_url — the validator this same commit newly calls from pinned_trace_remote_http_client — deliberately permits plaintext to loopback ("must use https (or http to a loopback host for standalone)"). So one commit both permits and refuses bearer-over-loopback-http. That is a production behaviour change, not a test artifact.
Two defensible resolutions, both security-posture calls I have deliberately not made:
- Give the chokepoint the same literal-loopback exception the trace validator already documents (
onboarding::invite::is_loopback_host) — internally consistent, but widens a generic chokepoint used by every extension's credential injection. - Keep the guard absolute, accept that standalone-over-loopback Trace Commons is unreachable through the agent path, and re-specify the two tests against HTTPS — which also makes the trace validator's loopback exception dead for this lane.
Verification (local, merged tree over origin/main @ d06f80413d)
cargo fmt --all -- --check clean. Unfiltered cargo test -p <crate> --no-fail-fast on all 15 touched crates: 6534 passed, 2 failed — the two above, nothing else. cargo clippy -p <crate> --all-targets --all-features -- -D warnings: exit 0 on all 15. The workspace shape this PR adds to the merge-queue lane, cargo clippy --all --lib --bins -- -D warnings: exit 0.
Scripts: planner self-test 47 OK (4 new, each red-verified); test-reborn-docker-entrypoint.sh pass; live-QA module 204 tests OK (skipped=5, expected failures=4); ws12 workflow contracts 25 OK; ws12 suite shards 6 OK; changed-workspace-packages 4 OK; changed-coverage 26 OK.
One flake characterised rather than counted: smoke::onboard_login_link_then_bearer_authorizes_a_protected_request fails Connection refused when the machine is CPU-saturated and passes 4/4 when it is not. smoke.rs:3038 documents the race (the banner flushes just before the listener starts), and its sibling serve_mounts_cli_login_route_without_sso drives the same helper against the same listener and passed throughout.
Deliberately not done
- CodeRabbit's 8 inline comments are untriaged — separate review loop.
- No issue state changed (including Code Style clippy is package-set-dependent: main is red for the {ironclaw, ironclaw_reborn_config} set #7119, which this measurement says was closed prematurely).
- Nothing under
docs/reborn/target-architecture/touched; this PR never did.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
scripts/ci/reborn_pr_test_plan.py (1)
387-389: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winKeep the Reborn E2E ownership contract consistent.
The broad
tests/e2e/classifier now ownstests/e2e/reborn_webui_harness.py, while the comments and Reborn fallback logic still describetests/e2e/reborn_*harnesses as shared fixtures under Reborn E2E. Align the classifier branch order, comments, tests, and fallback logic behind one contract.🤖 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 `@scripts/ci/reborn_pr_test_plan.py` around lines 387 - 389, The Reborn E2E ownership contract is inconsistent because the broad tests/e2e/ classifier captures reborn_webui_harness.py before the dedicated Reborn handling. In scripts/ci/reborn_pr_test_plan.py:387-389, reorder or adjust classifier branches so the dedicated Reborn E2E rule consistently owns tests/e2e/reborn_* harnesses; update the related comments and fallback logic to match. In scripts/ci/test_reborn_pr_test_plan.py:213-216 and 385-400, update expectations and coverage to enforce the same ownership behavior.Source: Path instructions
🤖 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.
Outside diff comments:
In `@scripts/ci/reborn_pr_test_plan.py`:
- Around line 387-389: The Reborn E2E ownership contract is inconsistent because
the broad tests/e2e/ classifier captures reborn_webui_harness.py before the
dedicated Reborn handling. In scripts/ci/reborn_pr_test_plan.py:387-389, reorder
or adjust classifier branches so the dedicated Reborn E2E rule consistently owns
tests/e2e/reborn_* harnesses; update the related comments and fallback logic to
match. In scripts/ci/test_reborn_pr_test_plan.py:213-216 and 385-400, update
expectations and coverage to enforce the same ownership behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 22652d00-35b4-455e-9909-053b74dd22f3
📒 Files selected for processing (3)
crates/ironclaw_reborn_cli/src/commands/traces/tests.rsscripts/ci/reborn_pr_test_plan.pyscripts/ci/test_reborn_pr_test_plan.py
Heads-up:
|
| path | kind |
|---|---|
crates/ironclaw_reborn_traces/src/contribution.rs |
modify/delete — #7139 split the 17,470-line file into ~25 modules under src/contribution/ |
crates/ironclaw_extractors/src/lib.rs |
content |
crates/extensions/ironclaw_extension_support/src/latency.rs |
content |
crates/ironclaw_reborn_composition/src/runtime.rs |
content |
scripts/ci/reborn_pr_test_plan.py |
content |
docs/{capabilities/configuration.mdx, channels/slack.mdx, reborn/deploy-reborn-cli-docker.md, reborn/setup-slack-for-reborn-binary.md} |
content |
What this costs, measured
The contribution.rs conflict is the real one. This PR changes it in 34 hunks, +685/−80, and the file no longer exists — every hunk has to be routed to the right one of classification.rs / privacy.rs / envelope.rs / remote/{account,claim,client,profile}.rs / queue.rs / submission.rs / maintenance.rs / …. That is a careful port, not a merge, and a mis-routed hunk silently drops a security fix.
None of the #7144 work is made redundant by #7139 — it is a pure move. Spot-checked the load-bearing one: main still decides dataset eligibility by scanning prose,
// origin/main:crates/ironclaw_reborn_traces/src/contribution/classification.rs:209
.any(|warning| warning.to_ascii_lowercase().contains("quarantined"))so the typed-flag fix is still needed, just at a new address.
One half of the planner commit is now duplicated. main classified .env.example independently, as IGNORED_ROOT_FILES (reborn_pr_test_plan.py:43) — same fix, different constant name, arrived by the same route. The docker/reborn/entrypoint.sh half is not on main and is still live: main's planner still rejects it, so that path still fails Detect Reborn test scope for any PR that touches it. Resolution should keep main's IGNORED_ROOT_FILES and carry over this branch's entrypoint entry plus the entrypoint/masking/sibling-refusal tests.
Not attempted here, deliberately
The port is a different and larger job than the CI root-cause this pass was scoped to, and it interacts with the open loopback question above: if that is resolved by keeping the guard absolute, the egress commit changes shape anyway, and porting it first would be wasted precision. Flagging both together so they can be sequenced rather than discovered one at a time.
|
Ruling on the loopback credential question (delegated authority, 2026-08-05): option 1 — the chokepoint takes a literal-loopback exception via the same Why, from the codebase rather than taste:
Implementation (lands with this PR's post-stack refresh): the eight-line guard in 🤖 Generated with Claude Code |
Superseded by #7263 — closingEvery commit on this branch shipped in #7263 ( What landed, and where it went:
Two things this branch's work surfaced that were fixed in #7263 and are worth knowing independently: the folded #7146 gate immediately caught a field-form tracing site Branches 🤖 Generated with Claude Code |
This branch was successfully deployed
1 participant
Works through the pre-existing defects that were filed rather than fixed because the PRs that surfaced them were move-only or scope-bound. One commit per issue.
Closes #7119, #7146, #7115, #7104, #7103. Works #7144, #7087.
Why this PR's own CI was red, and what it was hiding
Detect Reborn test scopefailed, andTests (Reborn)failed because of it — that job's first step is the roll-up gate overneeds.changes, and its log is literallychanges failed: failure. Every other Reborn job in the run reportedskipping. Two red lanes, one cause:Same class as #7064 and #7087:
scripts/ci/reborn_pr_test_plan.pyfails closed on any path it has no rule for, so the only satisfiable behaviour for an unclassified class is never edit it.The message names one path because the planner raises on the first miss in sorted order. Driving
build_plan()over this PR's 56 changed paths one at a time shows the real set is two, and.env.examplewas masking the second:.env.exampleCLAUDE.mdand five operator docs point at. Nothing in the repository reads it — every reference in the tree is a comment or a doc string. Prose, same class asdocs/and.claude/.docker/reborn/entrypoint.shscripts/ci/test-reborn-docker-entrypoint.shdrives the real script in the self-test step, andcode_style.yml'shas_codefilter names the path.platform-and-compat.yml'shas_docker_riskdeliberately does not cover it — that filter is keyed toDockerfile/.dockerignoreand owns the image build, not the entrypoint's behaviour.docker/is classified per file, for the reason repo-rootscripts/is: a blanket prefix would silently absorbdocker/reborn/config.*.tomlanddocker/process-sandbox-entrypoint.sh, which have no owning lane — turning a loud rejection into a silent under-schedule. A test pins that those still refuse.Four regression tests, each verified red by reverting the classification it covers:
.env.examplearmunclassified pull-request path: .env.example— in the classification test and the masking testunclassified pull-request path: docker/reborn/entrypoint.sh— in bothBoth new classifications carry the paired assertion the
.claude/fix set as precedent — accepted and selects no Rust lane — and the masking test drives both new paths beside a crate change, so a per-PR shortcut cannot pass where a per-path rule is required. The fail-closed arm itself is untouched.What the red lane was hiding. With every Reborn job skipped, nothing on this branch had ever built a
--teststarget for theironclawbinary crate — and it did not compile: the #7144 privacy commit added the typedPrivacyMetadata::quarantinedflag and missed one of the workspace's two construction sites. The PR-lane clippy shape is--lib --bins, and the--all --tests --exampleslane isgithub.event_name != 'pull_request', so no PR-time check could have seen it either. Fixed inc583ea2eb; the two remaining failures are in Known red.#7119 —
mainis red for the{ironclaw, ironclaw_reborn_config}package setRe-measured on the post-merge tree, because the issue was closed on a contrary measurement. The defect is live. Sabotage pair, back to back in one tree,
cargo clippy -p ironclaw -p ironclaw_reborn_config --lib --bins --all-features -- -D warnings:#[cfg]gates removed (i.e.origin/main's content)unused import×3 atruntime.rs:418,419,420The PR lane in
code_style.ymlbuilds exactly that invocation fromchanged_workspace_packages.py, somainis red for any PR whose changed set is{ironclaw, ironclaw_reborn_config}.A false-clean is easy to get here, which is worth stating since it is what closed the issue:
cargo clippydoes not reliably invalidate its unit cache on a change to the lint flags after--, so a re-run over an already-fresh unit can exit 0 without re-linting. The pair above forces a real recompile in both directions.Mechanism, measured — and not the one the issue guessed. It is dev-dependency feature unification, not
--all-featuresscope.test-supportonironclaw_reborn_compositionis enabled through dev-dependency edges only;--lib --binsbuilds no dev-dependencies, so with the crate outside the selected set the feature is off, thecfg-gated accessors vanish, and three imports are unused. Every whole-workspace lane passes--tests, which pulls those dev-dependencies in and unifies the feature back on — which is why the merge queue stayed green over a red tree. Corroborated structurally:cargo tree -p ironclaw --all-features -e features,no-dev -i ironclaw_reborn_compositionresolves composition todefault+memory-mem0, nottest-support.Fix: gate the imports, and add the missing shape to the non-PR lane (
cargo clippy --all --lib --bins -- -D warnings,defaultflavour —--all-featuresre-enablestest-supporton every selected package and masks the class). This is the issue's option 2 in the one form that reproduces; option 2 as written would have stayed green.Run workspace-wide it also answers the issue's open question: no other latent instances of this class exist in the production feature shape.
#7146 — 121
target = …sitestarget = "…"records a field; the metadata target stays the module path, soRUST_LOGfilters on the intended target match nothing. 120 literal sites swept totarget:. The 121st was found by the new gate, not by the sweep — its value is a variable, so no regex overtarget = "could see it. It is a genuine domain field (the tool a call targeted) and is renamed totool, which leaves the rule absolute with no production allowlist.Three layers of coverage because the sweep is mechanical: a repo-wide lexer scan (deliberately not
syn— a tracing macro nested inside another macro's token stream is invisible tosyn) with a self-test over every shape and a "did it read anything" assertion; a capturing-subscriber probe that measures the language fact the gate rests on; and a capturing-subscriber assertion on one real production emission. Asserted offevent.metadata().target(), never off rendered output — the field form also printstarget=in the formatted line, so a text assertion passes on the broken form.Still absolute after the
origin/mainmerge:cargo test -p ironclaw_architectureis 210/0 with the gate in it, and the only fourtarget = "literals left undercrates/are a local variable in a mem0 test, two doc comments describing the defect, and the gate's own deliberate probe.#7144 — the trace contribution defect set
Ordered by consequence in the commit. Highlights:
apply_credential_injectioncheckedhttpsforPathPlaceholderonly, soHeader,QueryParamandBodyJsonPointercould put a bearer on a plaintext URL. And the trace lane's own comment claimed a validator ran on it that nothing on that path ever called, whiletraces opt-in --endpointwrites the endpoint unvalidated. (This is the change that is now red — see below.)strip_credential_headersfiltered three header names before each redirect hop — over a vector that is always empty, because the header buffer ismem::taken into the transport request first. Proved on pristineorigin/main: assertingheaders.is_empty()at the top of that function leaves all three redirect tests green. Behaviour kept, pretence removed; the contract test now asserts the observed hop carries no headers, and seeds anx-api-keybeside theauthorizationit used to check alone.#[cfg(unix)]plus a hard failure on a missing shell. Deliberately not theIRONCLAW_REQUIRE_DOCKER_TESTSshape: that flag is set nowhere, so the gate it guards is itself inert.Debug-derived durable identifiers, two unbounded process-global maps (one holding bearer tokens), and the smaller items.One test follows the fix rather than being weakened: the path-placeholder contract case pinned the old kind-specific refusal message, and the widened guard no longer names a kind. Same error variant, same refusal before transport, assertion updated to
credential injection requires HTTPS.Refuted with measurements and closed on the issue rather than left ambiguous: two of the three "swallowing" sites (one is a logged soften, one propagates and is already test-pinned), the missing
rename_all(adding it would make every existingsubmissions.jsonunloadable — the inconsistency is load-bearing), and the "every WebUI poll" premise (that path is CLI-only; the WebUI goes through the memoizedscoped_credit_view).Known red — the loopback credential question
Two tests fail, both in
ironclaw_host_runtime, both caused by the blanket HTTPS guard above:Causality proven, not inferred. Deleting the eight-line guard from
apply_credential_injectionand changing nothing else turns both green (5 passed; 0 failedand1 passed; 0 failed); restoring it turns both red again. The guard is restored in the tree — the failure is what is being reported, not something worked around.The commit's premise is falsified by these tests. It reasoned "No loopback carve-out, deliberately: … the measured loopback-http consumers (Ollama, a self-hosted mem0, the sandbox broker) all use their own clients and never reach this chokepoint." The Trace Commons agent path does reach it:
mint_account_login_link_innersends the login-link POST withbearer_token: Some(..)through the hostRuntimeHttpEgresssink, which is exactlyapply_credential_injection, and standalone Trace Commons is ahttp://127.0.0.1endpoint.And the PR now contradicts itself about loopback.
validate_trace_commons_ingest_url— the validator this same commit newly calls frompinned_trace_remote_http_client— deliberately allows plaintext to a loopback host:So within one commit, the trace builder permits bearer-over-loopback-http and the host chokepoint refuses it. That is not a test artifact: it means an agent minting a Trace Commons account login link against a standalone/loopback deployment now fails closed in production.
Two defensible resolutions, and picking one is a security-posture call:
onboarding::invite::is_loopback_host). Narrow and internally consistent — plaintext is acceptable exactly where there is no network to observe — but it widens a generic chokepoint used by every extension's credential injection, not just traces.Deliberately not decided here.
#7115, #7104, #7103
docs/channels/slack.mdxdocumented a[slack]key table in which every key now makesserverefuse to start. New self-test drives the real entrypoint, wired into CI with a path filter so the lane lights up for the diff that can break it.Empty. One trap: for PPTX/XLSX the empty-resultErrwas also the zip-bomb guard's only observable outcome, so both loops now remember a refused entry. The pre-existing size-guard test caught it.#[cfg(test)]call counter driven through the public dispatch entry point, which also asserts the counter itself still works.Also
test_live_canary_workflow_shards_cover_non_telegram_qa_suitewas pre-existing-broken. The deeper finding: no CI lane has ever run that 204-test module. Five of its tests had drifted red. The named one is fixed, two test doubles now model the API their production caller uses, and the four remaining are@unittest.expectedFailure— not skipped and not deleted, so the bodies still run and the day the #6520 operator-catalog contract is modelled they go red as unexpected passes. Suite wired into CI with its path filter.Verification (measured locally on the merged tree,
origin/main@d06f80413d)cargo fmt --all -- --checkclean.CI scripts / planner
python3 scripts/ci/test_reborn_pr_test_plan.pyreborn_pr_test_plan.py --event pull_request --changed-files <this PR's 56 paths> --base-sha d06f8041mode: selected· 28 affected packages · 3 crate buckets ·run_qa_replay: truebuild_plan()scripts/ci/test-reborn-docker-entrypoint.shpython3 -m unittest scripts.reborn_webui_v2_live_qa.test_run_live_qaOK (skipped=5, expected failures=4)test_ws12_workflow_contracts.py/test_ws12_suite_shards.pytest_changed_workspace_packages.py/test_reborn_changed_coverage.pyRust — unfiltered
cargo test -p <crate> --no-fail-fast, every crate this PR touches:ironclaw_architectureironclaw_extension_supportironclaw_extension_hostironclaw_extractorsironclaw_host_runtimeironclaw_loop_hostironclaw_networkironclaw_outboundironclaw_productironclawironclaw_reborn_compositionironclaw_reborn_event_storeironclaw_reborn_openai_compatironclaw_reborn_tracesironclaw_webuicargo clippy -p <crate> --all-targets --all-features -- -D warnings: exit 0 on all 15.The workspace lint shape this PR adds to the merge-queue lane —
cargo clippy --all --lib --bins -- -D warnings— exit 0 on the merged tree, so the new step does not arrive red.One flake characterised rather than counted as a failure:
smoke::onboard_login_link_then_bearer_authorizes_a_protected_requestfailsconnect to serve listener failed: Connection refusedwhen the machine is CPU-saturated, and passes 4/4 when it is not.smoke.rs:3038documents the race — the banner line is flushed just before the listener starts — and the siblingserve_mounts_cli_login_route_without_ssodrives the same helper against the same listener and passed throughout.🤖 Generated with Claude Code