test(reborn): storage-mode audit + operator LLM-config tier-2 coverage - #6131
Closed
henrypark133 wants to merge 2 commits into
Closed
henrypark133 wants to merge 2 commits into
henrypark133 wants to merge 2 commits into
Conversation
…ory/LibSql Storage-mode audit (docs/plans/2026-07-15-reborn-tier2-extension-plan.md §1): result_read_continues_a_durable_result_byte_exactly reads a byte-exact continuation slice back from the durable-preview seam, which is backed by the group's real thread service — the same RootFilesystem StorageMode selects — but the test rode the InMemory default only. Parametrize it the backend_matrix.rs way so the byte-offset round trip is proven against LibSql's real SQL storage too, not just InMemory's Vec<u8> staging store. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
railway-app
Bot
temporarily deployed
to
ironclaw-ci-preview / ironclaw-pr-6131
July 15, 2026 22:24
Destroyed
github-actions
Bot
added
size: XS
📝 WalkthroughWalkthroughThe integration suite adds end-to-end operator LLM-provider CRUD coverage with API-key redaction assertions and parameterizes durable result continuation coverage across in-memory and LibSql storage modes. ChangesIntegration coverage
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Test
participant webui_v2_router
participant RebornLlmConfigService
Test->>webui_v2_router: Upsert provider with api_key
webui_v2_router->>RebornLlmConfigService: Store provider
RebornLlmConfigService-->>Test: Redacted provider response
Test->>webui_v2_router: Fetch providers
webui_v2_router->>RebornLlmConfigService: Read providers
RebornLlmConfigService-->>Test: api_key_set provider view
Test->>webui_v2_router: Delete provider
webui_v2_router->>RebornLlmConfigService: Remove provider
Test->>webui_v2_router: Delete provider again
webui_v2_router-->>Test: 404 Not Found
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request parameterizes the result_read_continues_a_durable_result_byte_exactly integration test in tests/integration/tool_call.rs using rstest. The test is now executed against both StorageMode::InMemory and StorageMode::LibSql backends to verify that the byte-offset continuation behaves correctly across different storage implementations. There are no review comments, so I have no 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.
Contributor
Coverage ratchetReborn integration-tier coverageLine coverage (Reborn crates): 85.83% — 305266 / 355677 lines Per-crate breakdown (63 crates, lowest-covered first)
This table itself is informational and never gates the PR on its own — not the percentage, not the per-crate holes, not the 0-coverage callout. A separate coverage ratchet (dry-run until enforce=true; see tests/integration/coverage-floor.toml) can fail the build on specific configured floors. Exemptions (3 entry/entries excluded from the accounting above)
|
|
🚅 Deployed to the ironclaw-pr-6131 environment in ironclaw-ci-preview
|
Storage-mode audit plan (§5b): "Operator API has zero tier-2 coverage: LLM-provider CRUD (including the api_key never echoed back redaction invariant) ... no tests/integration/ file touches any of it." Adds a dedicated new bin driving the real webui_v2 router + real RebornLlmConfigService (not a stub) through upsert-with-key -> assert no key value anywhere in the response + api_key_set flips true -> GET re-fetch (redaction holds on read, not just write) -> delete -> assert gone -> delete again -> assert 404 (the unknown-provider-id branch, not covered by any existing crate-tier or contract-suite test). Requires enabling `root-llm-provider` on the ironclaw_reborn_composition dev-dependency (root Cargo.toml) so RebornRuntimeInput::with_boot_config and the real LLM-config service exist in the int-tier build at all. Dev-dependency only: the shipped ironclaw-reborn binary already carries this feature via ironclaw_reborn_cli's own default, so this changes zero production behavior — verified with a clean build and unchanged 21/21 webui_v2_product_api.rs pass before vs. after enabling it. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
railway-app
Bot
temporarily deployed
to
ironclaw-ci-preview / ironclaw-pr-6131
July 15, 2026 23:20
Destroyed
github-actions
Bot
added
scope: dependencies
henrypark133
changed the title
![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 `@tests/integration/operator_llm_config.rs`:
- Around line 51-53: Keep the TempDir created in operator_router_with_llm_config
alive for the full HTTP scenario by returning it alongside the Router (or
otherwise transferring ownership) and updating every caller to retain it until
requests and provider writes complete. Apply the same lifetime fix to the
corresponding setup at the other referenced location, while preserving
production composition and the existing seam assertions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: f660428b-ce1c-4c53-aa68-ca8c9003a4df
📒 Files selected for processing (2)
Cargo.tomltests/integration/operator_llm_config.rs
Comment on lines
+51
to
+53
| async fn operator_router_with_llm_config() -> Router { | ||
| let root = tempdir().expect("runtime storage tempdir"); | ||
| let storage_root = root.path().join("local-dev"); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Keep the runtime TempDir alive through the HTTP scenario.
root drops when this helper returns, before provider writes begin. This leaves the router backed by deleted paths on Unix or failed cleanup on Windows.
Proposed fix
-async fn operator_router_with_llm_config() -> Router {
- let root = tempdir().expect("runtime storage tempdir");
+async fn operator_router_with_llm_config(root: &tempfile::TempDir) -> Router {
let storage_root = root.path().join("local-dev"); let secret_key = "sk-test-should-never-appear-in-any-response";
- let router = operator_router_with_llm_config().await;
+ let runtime_root = tempdir().expect("runtime storage tempdir");
+ let router = operator_router_with_llm_config(&runtime_root).await;As per coding guidelines, production-wired behavior must use production composition and assert a meaningful seam.
Also applies to: 108-108
🤖 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 `@tests/integration/operator_llm_config.rs` around lines 51 - 53, Keep the
TempDir created in operator_router_with_llm_config alive for the full HTTP
scenario by returning it alongside the Router (or otherwise transferring
ownership) and updating every caller to retain it until requests and provider
writes complete. Apply the same lifetime fix to the corresponding setup at the
other referenced location, while preserving production composition and the
existing seam assertions.
Source: Coding guidelines
Collaborator
Author
|
Closing as stale — no activity in over three weeks. The branch is untouched; reopen if this is still needed. |
This branch was successfully deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Lane 1 of a 4-lane parallel extension to the Reborn tier-2 integration harness. Two pieces now in this PR:
reborn_*bins for assertions reading back persisted state without opting intoStorageMode::LibSql.api_keynever echoed back redaction invariant) ... notests/integration/file touches any of it." Closed that specific gap.Two other requested follow-ups were investigated and NOT implemented — see "Scope note" below for why.
1. Storage-mode audit
Method: re-verified against current
mainthat 4 of 50 bins already opt into LibSql (backend_matrix.rs,reopen_resume_through_gate.rs,webui_v2_product_api.rs,group_approvals'sapprovals_group_libsql_e2e). Went file-by-file through the remaining 82 files: keyword grep (reopen|restart|rehydrat|cold_get|survive|durab|reload|fresh|storage_root_for_test|open_local_dev_|byte_exact|offset|libsql), then for every hit, traced the actual store being asserted against back to its construction to determine whether it's genuinely gated byRebornIntegrationHarness'sStorageMode, or a separate always-on-disk local-dev store.Finding — one genuine candidate:
tests/integration/tool_call.rs::result_read_continues_a_durable_result_byte_exactly. Tracedinstall_durable_capability_io→group.rs'sgroup_thread_harness.service→base.composite→build_storage_composite(self.storage, ...)— i.e..with_durable_capability_io_file_tools()'s content storage genuinely round-trips through whichever backendStorageModeselects. This test reads a byte-exact continuation slice at a specific offset from persisted content — the one test in the suite doing that — and rodeInMemoryonly.Change: parametrized it with
#[rstest] #[case(StorageMode::InMemory)] #[case(StorageMode::LibSql)], followingtests/integration/backend_matrix.rs's established convention exactly.Left alone (with reasoning): sibling test in the same file (backend-agnostic truncation decision, no new signal from LibSql); approval-request/trigger-repository/extension-installation/outbound-preferences/secret-store reopen tests (all confirmed always-on-disk local-dev stores independent of
StorageMode, several self-documented "C-DURABLE");group_multiuser/scenario_turn_state_isolation_across_actors.rs(tracedFilesystemTurnStateStore::get_run_state— the scope-isolation check delegates to backend-agnostic shared code); all*_cross_thread/*_isolationgroup scenarios (same-process sharedArc, proving live visibility not durability);auth_failure.rs/top-levelsecrets.rs(feature-gated or bypassStorageModeentirely).2. Operator API — LLM-provider CRUD (new)
New file:
tests/integration/operator_llm_config.rs(dedicated bin —webui_v2_product_api.rsis already 1300+ lines covering a different set of route families; operator/LLM-config is its own first-class, zero-to-one area).Drives the REAL
webui_v2_router+ REALRebornLlmConfigService(not a stub), reusing the exact runtime-construction pattern already established 4× inwebui_v2_product_api.rs(RebornBuildInput::local_dev→build_reborn_runtime→build_webui_services), plus one new step:.with_boot_config(...)onRebornRuntimeInput, which is what makes the WebUI facade actually wire the real LLM-config service (crates/ironclaw_reborn_composition/src/webui/facade.rs'sbuild_llm_config_serviceonly fires when a boot config is present).One cohesive test (
upsert_llm_provider_never_echoes_api_key_then_get_then_delete) covers the full CRUD path: upsert a provider with an API key → assert the key value never appears anywhere in the response andapi_key_setflipstrue→ GET re-fetch → same redaction assertion on read (not just write) → delete → assert gone → delete again → assert404(the unknown-provider-id branch — not covered by any existing crate-tier unit test or thewebui_v2_handlers_contract.rscontract suite, which only exercises 403-unauthorized and happy-delete for this handler).One infrastructure line required, not a
.rsproduction change:root-llm-providerwas missing from the rootCargo.toml'sironclaw_reborn_compositiondev-dependency feature list — without it,.with_boot_configand the real LLM-config service don't exist in thetests/integrationbuild at all. Added it, with a rationale comment matching the file's own established convention for that line. Verified dev-dependency-only and behavior-neutral: the shippedironclaw-rebornbinary (ironclaw_reborn_cli) already hasdefault = ["root-llm-provider"], so production behavior is unchanged. Confirmed with a clean build and the existingwebui_v2_product_api.rssuite passing identically (21/21, same tests) before vs. after enabling the feature.Scope note — two items investigated, not implemented
Filesystem/project-browsing API tier-2 coverage (also flagged as a zero-to-one gap): investigated and found genuinely blocked without a production crate-source change. The concrete reader types (
ProjectScopedFilesystemReader,MountScopedFilesystemReader) and theRebornRuntimeaccessors that would hand back an instance arepub(crate)toironclaw_reborn_composition—tests/integrationis a separate crate and can't name either. The one already-public test-support escape hatch (local_dev_profile_filesystem_for_test) only exposes the rawRootFilesystem, which would mean reimplementing the production reader's path-scoping/traversal-protection logic in test code — testing a reimplementation, not the real production code, which would be worse than no coverage. Closing this for real needs a new#[cfg(feature = "test-support")]accessor incrates/ironclaw_reborn_composition/src/factory.rs(precedented by 3 sibling_for_testaccessors in that exact file, but still a production-crate diff). Not implementing per the test-only constraint on this PR; reporting instead.golden_payload.rs:232-242's two "NOT implemented — blocked" compaction gaps (classify_compaction_message,ActiveTaskPreservingCompactionStrategy::should_compact): read the comment and both functions directly. Turns out both are already genuinely covered at tier-2 through a real turn —tests/integration/http_matcher.rs::multi_tool_turn_survives_failed_forced_compaction_after_resultsscripts enough content to trip the real ~8,000-token force-compaction threshold and reach both functions, asserted viaassert_compaction_failed_since(..., "security rejected").golden_payload.rs's own docstring states its design constraint ("curated scenario set... add a scenario only when an existing one can't absorb it") — the multi-thousand-token transcript needed would produce an unreviewable golden snapshot, so that file's exclusion is a correct, deliberate scope decision already satisfied elsewhere, not an actual gap. Nothing to add for the two named functions. (A smaller, separate, genuinely-real gap exists — only theInclude/security-reject disposition ofclassify_compaction_messageis exercised through a real turn, the other 3 dispositions are unit-tested only — but closing it needs new test-support harness scaffolding to seed specific message statuses, a distinct piece of design work, not a one-scenario add. Flagging as a follow-up, not bolting it onto this PR.)Verification
cargo test --test reborn_integration_tool_call— 28/28 pass, including both newStorageModecases.cargo test --test reborn_integration_operator_llm_config— 13/13 pass; mutation-tested the core assertion (flipped an expected value, confirmed the test fails for the right reason with the real provider snapshot in the panic message) to confirm it's genuinely discriminating, not vacuous.cargo test --test reborn_integration_webui_v2_product_api --test reborn_integration_webui_v2_router_smoke— unchanged 21+1/22 pass before/after theroot-llm-providerfeature flip, confirming zero behavioral effect on existing coverage.cargo test -p ironclaw_architecture— 34/34 pass (dependency/composition boundaries unaffected).cargo fmt --check/cargo clippy --all-featureson all touched files — clean, zero warnings.thermo-nuclear-code-quality-reviewbefore implementing (twice total across the two work items), and the final diffs after (twice) — no structural blockers either pass.code-reviewskill in local mode on both diffs. First pass (storage-mode parametrization): zero findings. Second pass (operator API + Cargo.toml): 2 actionable findings, both addressed — a missing 404-unknown-provider negative-path assertion (added by extending the existing test) and a missing build-cost rationale comment on the Cargo.toml feature addition (added, matching the file's own convention).🤖 Generated with Claude Code