[codex] Add Reborn multi-tenant isolation contract tests - #3890
serrrfirat wants to merge 4 commits into
Conversation
github-actions
Bot
added
size: XS
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds extensive contract tests across multiple crates to verify that system operations—such as event replaying, filesystem access, idempotency, egress policies, and process management—are strictly partitioned by tenant, user, and project scopes. These tests ensure that data and metadata remain isolated and cannot be accessed via guessed identifiers. A correction was suggested for a test mock where a raw string literal contained unnecessary backslash escapes, resulting in malformed JSON.
| let network = RecordingNetwork::ok(NetworkHttpResponse { | ||
| status: 200, | ||
| headers: vec![], | ||
| body: br#"{\"ok\":true}"#.to_vec(), |
There was a problem hiding this comment.
The mock response body contains unnecessary backslash escapes for double quotes within a raw byte string literal (br#"..."#). In Rust, raw strings do not process backslash escapes, so the resulting bytes will literally contain \". This results in malformed JSON (e.g., {\"ok\":true}) which may cause issues if the body is ever parsed by a consumer expecting valid JSON. Ensuring valid JSON prevents client-side parsing failures.
| body: br#"{\"ok\":true}"#.to_vec(), | |
| body: br#"{"ok":true}"#.to_vec(), |
References
- API endpoints must return a valid JSON body on success to ensure consistency and prevent client-side parsing failures.
serrrfirat
left a comment
serrrfirat
left a comment
There was a problem hiding this comment.
🤖 Multi-Agent Code Review
PR: #3890 — [codex] Add Reborn multi-tenant isolation contract tests
Commit: 3128058 | Reviewers: security, bugs, performance, tests, conventions
Summary
This PR adds multi-tenant isolation contract tests across 8 crates. The tests verify that tenant/user/project scoping prevents cross-boundary data access in events, filesystem, host runtime, HTTP egress, MCP, memory, processes, and approval resolution.
Findings by Category
| Category | Count | Critical | High | Medium | Low | Nit |
|---|---|---|---|---|---|---|
| 🛡️ Security | 0 | - | - | - | - | - |
| 🐛 Bugs | 0 | - | - | - | - | - |
| ⚡ Performance | 2 | - | - | - | 1 | 1 |
| 🧪 Tests | 0 | - | - | - | - | - |
| 📏 Conventions | 0 | - | - | - | - | - |
Total actionable findings: 0 (all remaining are Low/Nit in test-only code)
⚡ Performance Findings
-
[Low/70]
crates/ironclaw_memory/tests/memory_backend_contract.rs— Document write clones bytes on every update pathSearchableMemoryRepository::write_document clones bytes to_vec() even on overwrites. Test-only, bounded data, no practical impact.
💡 Store bytes as Cow<[u8]> or Vec directly; only clone when value changes. -
[Nit/50]
crates/ironclaw_memory/tests/memory_backend_contract.rs— search_documents does String::from_utf8_lossy per documentEach search call converts every document from bytes to lossy UTF8 string then runs contains(). O(n) per search. Fine for tests with few documents.
💡 No change needed. No realistic test data size makes this a bottleneck.
Verdict
✅ No Critical, High, or Medium findings. The PR is clean. The two Low/Nit performance observations are in test-helper code (SearchableMemoryRepository) and have no impact on production behavior or CI reliability.
Review generated by multi-agent code-review skill (5 parallel reviewers + intent analysis). 2026-05-22T15:38:53.886Z
|
|
||
| async fn search_documents( | ||
| &self, | ||
| scope: &MemoryDocumentScope, |
There was a problem hiding this comment.
[Low/70] ⚡ Document write clones bytes on every update path
SearchableMemoryRepository::write_document clones bytes to_vec() even on overwrites. Test-only, bounded data, no practical impact.
💡 Store bytes as Cow<[u8]> or Vec directly; only clone when value changes.
| path: path.clone(), | ||
| score: 1.0 / (index + 1) as f32, | ||
| snippet: String::from_utf8_lossy(bytes).to_string(), | ||
| full_text_rank: Some((index + 1) as u32), |
There was a problem hiding this comment.
[Nit/50] ⚡ search_documents does String::from_utf8_lossy per document
Each search call converts every document from bytes to lossy UTF8 string then runs contains(). O(n) per search. Fine for tests with few documents.
💡 No change needed. No realistic test data size makes this a bottleneck.
henrypark133
left a comment
henrypark133
left a comment
There was a problem hiding this comment.
Multi-Agent Code Review — PR #3890
Intent: Add Reborn crate-level contract tests for multi-tenant isolation across 8 subsystems (filesystem, events, host-runtime, MCP, memory, processes, run-state, HTTP egress) without touching legacy src/ or E2E tests.
PR Type: test | Confidence: high | Scope: ironclaw_events, ironclaw_filesystem, ironclaw_host_runtime, ironclaw_mcp, ironclaw_memory, ironclaw_processes, ironclaw_run_state
Summary
| Reviewer | Findings |
|---|---|
| Security | 2 (Medium) |
| Bugs | 1 (Low) |
| Performance | 0 |
| Tests | 3 (1 High, 2 Medium) |
| Conventions | 0 |
| Design | 1 (Low) |
Verdict: REQUEST_CHANGES — 1 High-confidence gap (mock-the-boundary anti-pattern in memory scope test) and 1 Medium-confidence security gap (ReplayGap error leaking cursor value across scope boundary).
Security
f-security-1 — Medium (75%) — crates/ironclaw_events/tests/durable_log_contract.rs
ReplayGap error leaks alice's cursor value to bob's call site
The test correctly verifies that bob gets no entries from alice's stream. However, EventError::ReplayGap includes requested: EventCursor and earliest: EventCursor in its #[error(...)] format string (crates/ironclaw_events/src/error.rs:17). A wrong-scope caller who receives this error can determine that alice has an event at exactly the cursor value they guessed — which is cross-tenant information disclosure. The approval_resolution_contract.rs test correctly checks !rendered.contains(...) for opaque errors; the same check is absent here.
Fix: Add assert!(!err.to_string().contains(&alice_entry.cursor.to_string())) after the ReplayGap match, or ensure the ReplayGap error format string omits cursor values for cross-stream access.
f-security-2 — Medium (65%) — crates/ironclaw_host_runtime/tests/host_runtime_contract.rs
cancel_work returns Ok (empty sets) for wrong-scope — inconsistent denial signal
The test verifies guessed-scope cancel produces an empty result. However, Ok with empty sets is an ambiguous response: a caller cannot distinguish "no work in your scope" (legitimate) from "you guessed a wrong-scope ID" (isolation enforcement). The ProcessHost analogous test uses ProcessError::UnknownProcess (explicit error). The contract spec states "wrong-scope access must look unknown" — while empty-Ok satisfies opacity, it doesn't satisfy fail-closed consistency. Needs either an explicit error variant or an inline comment justifying why silent-empty-Ok is the intended contract shape for cancel specifically.
Tests (critical)
f-tests-1 — High (75%) — crates/ironclaw_memory/tests/memory_backend_contract.rs
SearchableMemoryRepository implements the scope isolation it's supposed to test (mock-the-boundary anti-pattern)
The new SearchableMemoryRepository (lines 527–604) manually filters search_documents by path.scope() == scope. The test therefore verifies only that RepositoryMemoryBackend passes context.scope() to repository.search_documents() — which it does (backend.rs:678) — but the isolation is enforced by the test mock itself. The production InMemoryMemoryDocumentRepository has no search_documents implementation (inherits the unsupported-error default). The real production enforcement lives in FilesystemMemoryDocumentRepository::search_documents (repo/filesystem.rs:823), which this test never exercises.
Per .claude/rules/testing.md ("Test Through the Caller, Not Just the Helper"): a test that exercises a mock which implements the isolation is not sufficient regression coverage for the production repository path.
Fix: Add filesystem_memory_document_repository_search_does_not_cross_tenant_scope using FilesystemMemoryDocumentRepository backed by LocalFilesystem (as used by many other tests in the file), verifying cross-scope searches return empty results.
f-tests-2 — Medium (65%) — crates/ironclaw_mcp/tests/mcp_adapter_contract.rs
MCP scope partitioning test verifies pass-through only, not session isolation at cross-tenant boundary
The test verifies two calls with different tenant/project scopes produce two separate client requests with correct scope fields. It does not verify that session IDs are distinct across these scopes. The existing concrete_mcp_http_client_scopes_session_ids_per_invocation test covers user-scoped session isolation within the same tenant; the new cross-tenant test should extend this to verify that cross-tenant calls cannot share session state.
Fix: Assert that requests[0] and requests[1] either have no shared session ID headers, or extend the test to verify McpRuntime creates separate sessions for separate tenant scopes (per ironclaw_mcp/CLAUDE.md:5).
f-tests-3 — Medium (60%) — crates/ironclaw_host_runtime/tests/host_runtime_contract.rs
Idempotency scope collision test doesn't verify each dispatch carries its correct scope
default_runtime_same_idempotency_key_in_different_scopes_does_not_collide asserts dispatcher.count() == 3 (3 calls made, not collapsed). It does not verify each dispatch was made with its correct tenant scope. A buggy implementation that dispatches all 3 with the last scope (project_b) would still pass. The MCP partitioning test correctly asserts requests[0].scope == alice_scope; the same rigor is missing here.
Fix: Switch to RecordingDispatcher (already used in the cancel test at line 232) and assert the scope of each dispatched request matches the corresponding tenant context.
Bugs
f-bugs-1 — Low (55%) — crates/ironclaw_run_state/tests/approval_resolution_contract.rs
Opaque-error assertion uses hardcoded string that may diverge from fixture
Line 84 asserts !rendered.contains("echo.say") where "echo.say" is the capability from the approval_request() fixture (line 253). If the fixture capability is changed in a future refactor, the assertion passes trivially even if the error starts leaking capability IDs. The reason-string assertion at line 83 is more fragile-resistant (it uses the reason string directly from the fixture). Consider capturing the capability from the fixture to keep the assertion coupled to it.
Design
f-design-1 — Low (55%) — crates/ironclaw_memory/tests/memory_backend_contract.rs
SearchableMemoryRepository duplicates InMemoryMemoryDocumentRepository with added search
The 77-line SearchableMemoryRepository (lines 527–604) reimplements read_document, write_document, and list_documents from InMemoryMemoryDocumentRepository with the only addition being search_documents. Adding search_documents directly to InMemoryMemoryDocumentRepository would eliminate this duplication and make the canonical test repository more useful. This is also relevant to the f-tests-1 gap above.
What's working well
- Coverage is broad: 8 distinct subsystems, each with a targeted cross-scope denial test.
- The process host test (
process_host_hides_guessed_process_from_other_scope) is exemplary: it tests all four operations (status, records, subscribe, kill), verifies the timeout on cancellation does NOT fire for wrong-scope, and confirms the owner can still cancel after all guessed-scope probes. - The approval opacity test correctly validates error message content (
!rendered.contains(reason)) — this is the right pattern for all isolation tests. - The filesystem tests cover both workspace paths and attachment paths, and handle both
Ok(empty)andErr(NotFound)as acceptable responses for listing. - HTTP egress cross-scope policy test verifies that staged policies for other tenants/users/projects are not used, AND that those staged policies remain available to their actual owners.
Inline Findings (anchors could not be resolved against diff hunks; surfacing in body)
crates/ironclaw_events/tests/durable_log_contract.rs (diff position 257)
SECURITY [Medium / 75% confidence] f-security-1
ReplayGap error leaks alice's cursor value to bob's call site
The test at line 33-42 uses expect_err and then asserts on the ReplayGap variant fields including requested == alice_entry.cursor. The EventError::ReplayGap error includes requested: EventCursor and earliest: EventCursor in its #[error(...)] format string (crates/ironclaw_events/src/error.rs:17). This means the error message rendered to a wrong-scope caller contains alice's cursor value verbatim. A cross-tenant caller who receives this error can determine that alice has an event at exactly that cursor position — the test verifies fail-closed (no entries returned) but does not verify that the error message itself is opaque. The approval test (approval_resolution_contract.rs:82-84) correctly checks !rendered.contains(...) for secret content in error strings; the same check is missing here.
Fix: Add assertion assert!(!err.to_string().contains(&alice_entry.cursor.to_string())) after the ReplayGap pattern match, or verify the error format string omits cursor values for cross-stream access.
Anchor: crates/ironclaw_events/src/error.rs:17
crates/ironclaw_host_runtime/tests/host_runtime_contract.rs (diff position 765)
SECURITY [Medium / 65% confidence] f-security-2
cancel_work returns Ok (empty sets) for wrong-scope — silent success may mask denial
The test verifies that a guessed-scope cancel returns Ok with cancel.cancelled.is_empty(), cancel.already_terminal.is_empty(), and cancel.unsupported.is_empty(). While the test verifies the work item is not cancelled (line 286-292), returning Ok with empty sets to a cross-tenant caller is a weaker denial signal than an explicit Err(RuntimeError::Unauthorized) or a not_found variant. A caller cannot distinguish between 'no work to cancel' (legitimate empty result for own scope) and 'you guessed a wrong-scope ID' (isolation enforcement). The contract spec in ironclaw_run_state/CLAUDE.md says 'wrong-scope access must look unknown' — empty-ok satisfies opacity but not explicit denial. The process host test uses ProcessError::UnknownProcess (explicit error) for the analogous operation, creating inconsistency.
Fix: Either return an explicit error variant (e.g., RuntimeError::WorkNotFound) for any cross-scope cancel attempt, or add a comment justifying why silent-empty-Ok is the correct contract shape here.
Anchor: crates/ironclaw_run_state/CLAUDE.md:4
crates/ironclaw_memory/tests/memory_backend_contract.rs (diff position 1154)
TESTS [High / 75% confidence] f-tests-1
SearchableMemoryRepository implements the isolation it's supposed to test
The test repository_memory_backend_search_does_not_cross_tenant_user_or_project_scope introduces a custom SearchableMemoryRepository that manually filters search_documents by path.scope() == scope. This means the test is verifying that RepositoryMemoryBackend.search() passes context.scope() as the scope argument to repository.search_documents() — which it does (backend.rs:678) — but the isolation is enforced by the test mock itself, not by the production repository implementations.
The production in-memory repository (InMemoryMemoryDocumentRepository) has NO search_documents implementation (it inherits the default which returns an error). The real enforcement lives in FilesystemMemoryDocumentRepository::search_documents (repo/filesystem.rs:823). The new test never exercises that implementation against cross-scope queries — it only exercises the mock.
This is the 'mock-the-boundary' anti-pattern documented in .claude/rules/testing.md: the test passes the scope to a mock that honors it, but doesn't verify the production repository honors it. If a future FilesystemMemoryDocumentRepository implementation ever adds a search path that ignores the scope argument, this test would still pass.
Fix: Add filesystem_memory_document_repository_search_does_not_cross_tenant_scope test using FilesystemMemoryDocumentRepository backed by a real LocalFilesystem (as done elsewhere in the file), verifying cross-scope search returns empty results.
Anchor: .claude/rules/testing.md:29
crates/ironclaw_mcp/tests/mcp_adapter_contract.rs (diff position 85)
TESTS [Medium / 65% confidence] f-tests-2
MCP scope partitioning test only verifies scopes pass-through, not that session/state cannot leak
The test mcp_runtime_keeps_same_tool_name_calls_partitioned_by_scope verifies that two calls with different scopes produce two separate client requests with correct scope fields. However, MCP runtimes maintain session state keyed per-invocation (as shown by the existing concrete_mcp_http_client_scopes_session_ids_per_invocation test). The new test does not verify that session IDs are distinct across scopes, nor that a bob-scoped call cannot observe or share session state with an alice-scoped call. The existing session isolation test at line 340 covers user-scoped isolation within the same tenant; the new test introduces cross-tenant calls but doesn't check session isolation at that boundary.
Fix: Add assertion that requests[0] and requests[1] do not share session IDs or headers that would indicate a shared session, or extend the test to verify McpRuntime creates separate sessions for separate tenant scopes.
Anchor: crates/ironclaw_mcp/CLAUDE.md:5
crates/ironclaw_host_runtime/tests/host_runtime_contract.rs (diff position 409)
TESTS [Medium / 60% confidence] f-tests-3
Idempotency scope collision test does not verify dispatcher payloads carry correct scope per call
The test default_runtime_same_idempotency_key_in_different_scopes_does_not_collide asserts dispatcher.count() == 3, confirming 3 dispatch calls were made (not collapsed). However, it does not verify that each dispatch was made with the correct scope. A buggy implementation could dispatch all 3 with the last scope (e.g., project_b) and still count 3. The CountingDispatcher only records count, not payloads. The analogous test for MCP (mcp_runtime_keeps_same_tool_name_calls_partitioned_by_scope) correctly asserts requests[0].scope == alice_scope and requests[1].scope == bob_scope, setting a better pattern.
Fix: Switch to RecordingDispatcher (already used in default_runtime_status_and_cancel_do_not_reveal_guessed_ids_from_other_scope) and assert that each dispatched request carries the expected tenant scope.
Anchor: crates/ironclaw_host_runtime/tests/host_runtime_contract.rs:231
crates/ironclaw_run_state/tests/approval_resolution_contract.rs (diff position 54)
BUGS [Low / 55% confidence] f-bugs-1
Opaque-error test asserts against hardcoded capability ID string that may change
Line 84 asserts !rendered.contains("echo.say") where echo.say is the capability ID in the approval_request() fixture (line 253). If the fixture capability changes, the test passes trivially even if the error message starts leaking capability IDs. The reason-string assertion at line 83 (!rendered.contains("tenant1 secret approval reason")) is parameterized to the fixture correctly. The capability assertion should be made robust against fixture changes by reading the expected capability from the fixture rather than repeating the string inline.
Fix: Capture the capability ID from the fixture: let action_cap = approval_request(invocation_id).action; assert!(!rendered.contains(action_cap.capability_str())); — or keep the current approach with a comment noting the string must match the fixture.
Anchor: crates/ironclaw_run_state/tests/approval_resolution_contract.rs:253
crates/ironclaw_memory/tests/memory_backend_contract.rs (diff position 1154)
DESIGN [Low / 55% confidence] f-design-1
SearchableMemoryRepository duplicates InMemoryMemoryDocumentRepository with added search
The new SearchableMemoryRepository (lines 527-604) is a near-duplicate of the existing InMemoryMemoryDocumentRepository (repo/in_memory.rs), differing only in adding search_documents. It re-implements read_document, write_document, and list_documents using the same Vec<(MemoryDocumentPath, Vec<u8>)> pattern. An alternative approach would be to add a search_documents implementation directly to InMemoryMemoryDocumentRepository that performs in-memory substring matching with scope filtering — both serving tests and eliminating the duplication.
Fix: Consider adding search_documents to InMemoryMemoryDocumentRepository directly so the contract test can use the canonical test repository rather than an ad-hoc duplicate.
Anchor: crates/ironclaw_memory/src/repo/in_memory.rs:24
Review: REQUEST CHANGESThanks for the breadth here — 8 subsystems with named cross-scope tests is the right shape, and the process-host test ( Three issues that should land before merge, plus some smaller notes. HighMemory scope test exercises a mock that implements its own isolation The new Please add MediumIdempotency cross-scope test can't catch "dispatched with wrong scope" bugs
Malformed JSON in fixture body body: br#"{\"ok\":true}"#.to_vec(),Raw byte strings don't process escapes — actual bytes are Low
Note on automated reviewThe pre-posted multi-agent review's f-security-1 (ReplayGap leaks alice's cursor) is a false positive: The f-security-2 (cancel returns empty-Ok vs QuestionIs there a planned trait-level contract harness (like |
…s from #3890) (#3918) * test: introduce trait-level contract test harness pattern Establishes a reusable harness for testing trait contracts across multiple impls in a single suite. Across recent PR review (#3890, #3887, #3908) the same antipattern recurred: a trait has multiple impls with isolation/durability/CAS invariants every impl must honor, but tests only cover one impl — often a mock that quietly implements its own invariants. Per .claude/rules/testing.md ("Test Through the Caller, Not Just the Helper"), a contract test against one impl proves only that impl, not the contract. This is a scaffolding change: it picks one trait (MemoryDocumentRepository, the trait at issue in #3890's HIGH finding) and wires both existing impls to a shared contract suite. The shape — not the breadth — is the point. Follow-ups can extend the suite and port other traits (IdempotencyLedger, CheckpointStateStore, ProcessStore, ApprovalRequestStore, ...) onto the same pattern. The harness lives in crates/ironclaw_memory/src/contract_tests.rs as pub async fn contracts taking a factory closure. The contract_test! macro expands to one #[tokio::test] per contract named <impl_label>::<contract_name> for clear failure attribution. Contracts covered in this scaffold: - round_trip_returns_written_bytes - writes_isolated_across_scopes (the #3890 invariant) - list_documents_honors_scope - search_documents_isolated_across_scopes (the exact #3890 HIGH) Existing tests in memory_backend_contract.rs and memory_filesystem_contract.rs are intentionally not deleted; deduplication is a follow-up. * ci: gate contract_tests behind feature to hide harness panics from production scan The `scripts/check_no_panics.py` scanner flagged 25 panic-style calls (`.expect`, `.unwrap`, `assert*!`) inside `crates/ironclaw_memory/src/contract_tests.rs`. These are intentional — it is a test harness — but the module was unconditionally `pub mod`, so the scanner (which walks added lines in changed files and only treats `#[cfg(...test...)]`-gated items as test context) saw them as production code. Fix: - Add a `contract-tests` cargo feature on `ironclaw_memory`. - Gate `pub mod contract_tests;` in `lib.rs` behind `#[cfg(any(test, feature = "contract-tests"))]`. - Gate every item inside `contract_tests.rs` with the same cfg so the no-panics scanner recognizes the file as test-only context (the scanner only sees the file's own attributes, not the module declaration in `lib.rs`). - Add a self dev-dependency on `ironclaw_memory` with the `contract-tests` feature enabled so the per-impl integration test files in `tests/` (which import the harness and the `contract_test!` macro) keep compiling. Production builds (`cargo build -p ironclaw_memory`) no longer compile the harness; `cargo test -p ironclaw_memory` continues to run all 4 contracts for both the in-memory and filesystem repository impls. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
@henrypark133 @zmanian @serrrfirat @gemini-code-assist I addressed the review feedback in Addressed comments
Not changed
Validation passed
|
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds scope/tenant isolation regression tests across eight subsystems (durable log, filesystem, host runtime, HTTP egress, MCP, memory search, process host, approval resolution), validates isolation at the e2e public gateway level (chat threads, approvals, SSE events), refactors ChangesScope isolation contract regression tests
Unrelated minor fixes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![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_mcp/tests/mcp_adapter_contract.rs`:
- Around line 549-567: The test setup for cross-tenant validation is confounded
by changing the user_id alongside the tenant_id and project_id. To properly
isolate and test cross-tenant/project session partition, keep the same user
identifier (e.g., "user1") in both scopes while only varying the tenant_id and
project_id in the cross_tenant_scope. Modify the second entry in the loop to use
the same user string in the query parameter as the first scope, ensuring that
the scope change tests only tenant/project isolation without the confounding
variable of a different user_id.
🪄 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: c505119b-347a-43f2-aac7-b688038c707c
📒 Files selected for processing (10)
crates/ironclaw_events/tests/durable_log_contract.rscrates/ironclaw_filesystem/tests/db_root_filesystem_contract.rscrates/ironclaw_host_runtime/tests/host_runtime_contract.rscrates/ironclaw_host_runtime/tests/runtime_http_egress_contract.rscrates/ironclaw_mcp/tests/mcp_adapter_contract.rscrates/ironclaw_memory/tests/memory_backend_contract.rscrates/ironclaw_processes/tests/process_store_contract.rscrates/ironclaw_run_state/tests/approval_resolution_contract.rscrates/ironclaw_webui_v2_static/static/js/pages/automations/lib/automations-presenters.jsdocs/plans/2026-06-06-subagent-compaction-impl.md
💤 Files with no reviewable changes (2)
- docs/plans/2026-06-06-subagent-compaction-impl.md
- crates/ironclaw_webui_v2_static/static/js/pages/automations/lib/automations-presenters.js
| let mut cross_tenant_scope = sample_scope_for_user("user2"); | ||
| cross_tenant_scope.tenant_id = TenantId::new("tenant2").unwrap(); | ||
| cross_tenant_scope.project_id = Some(ProjectId::new("project2").unwrap()); | ||
|
|
||
| for (scope, query) in [ | ||
| (sample_scope_for_user("user1"), "user1"), | ||
| (cross_tenant_scope, "user2"), | ||
| ] { | ||
| client | ||
| .call_tool(McpClientRequest { | ||
| provider: ExtensionId::new("github-mcp").unwrap(), | ||
| capability_id: CapabilityId::new("github-mcp.search").unwrap(), | ||
| scope: sample_scope_for_user(user), | ||
| scope, | ||
| transport: "http".to_string(), | ||
| command: None, | ||
| args: vec![], | ||
| url: Some("https://mcp.example.test/mcp".to_string()), | ||
| input: json!({"query": user}), | ||
| input: json!({"query": query}), | ||
| max_output_bytes: 4096, |
There was a problem hiding this comment.
Cross-tenant assertion is currently confounded by user change
This setup changes tenant/project and user, so session partition can still pass if keying is user-only. Keep user_id the same across the two scopes and vary only tenant/project to validate tenant/project isolation directly.
As per coding guidelines, “Preserve tenant/user/agent/project/mission/thread scope on … event records”; this test should isolate the tenant/project axis when claiming cross-tenant coverage.
🤖 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_mcp/tests/mcp_adapter_contract.rs` around lines 549 - 567,
The test setup for cross-tenant validation is confounded by changing the user_id
alongside the tenant_id and project_id. To properly isolate and test
cross-tenant/project session partition, keep the same user identifier (e.g.,
"user1") in both scopes while only varying the tenant_id and project_id in the
cross_tenant_scope. Modify the second entry in the loop to use the same user
string in the query parameter as the first scope, ensuring that the scope change
tests only tenant/project isolation without the confounding variable of a
different user_id.
Source: Coding guidelines
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/e2e/scenarios/test_multi_tenant_isolation.py`:
- Around line 33-41: The _set_http_tool_approval function directly uses
httpx.AsyncClient for making HTTP requests, which is inconsistent with other
helper functions like _create_thread and _send_chat that use api_post from
helpers. Either extend the api_post helper to support PUT requests by adding a
method parameter or create a new api_put helper function, then refactor
_set_http_tool_approval to use this helper instead of httpx.AsyncClient
directly. This will ensure uniform error handling and timeout behavior across
all HTTP interactions in the test.
- Around line 113-210: Both test_chat_thread_history_and_list_are_user_scoped
and test_approval_gate_resolution_is_user_scoped invoke _send_chat, which
requires LLM interaction but currently do not explicitly pin the LLM provider,
risking silent fallback to NearAI and test instability. Before each _send_chat
call in both functions, add a setup step that calls the /api/settings API
endpoint to explicitly configure llm_backend=openai_compatible,
openai_compatible_base_url set to the mock LLM server URL (ironclaw_server), and
selected_model=mock-model. This can be done by adding an API call early in each
test (after _two_member_users and before the first _send_chat) or by creating a
shared fixture that both tests use.
🪄 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: 7acb5630-4fd4-4158-a9f0-412f0106ebfb
📒 Files selected for processing (1)
tests/e2e/scenarios/test_multi_tenant_isolation.py
| async def _set_http_tool_approval(base_url: str, token: str) -> None: | ||
| async with httpx.AsyncClient() as client: | ||
| response = await client.put( | ||
| f"{base_url}/api/settings/tools/http", | ||
| headers={"Authorization": f"Bearer {token}"}, | ||
| json={"state": "ask_each_time"}, | ||
| timeout=15, | ||
| ) | ||
| assert response.status_code == 200, response.text |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
Consider extracting PUT support to helpers for consistency.
This helper uses httpx.AsyncClient directly while _create_thread and _send_chat use api_post from helpers. If api_post (or a new api_put) supported PUT, you'd gain uniform error handling and timeout behavior across all HTTP helpers.
🤖 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/e2e/scenarios/test_multi_tenant_isolation.py` around lines 33 - 41, The
_set_http_tool_approval function directly uses httpx.AsyncClient for making HTTP
requests, which is inconsistent with other helper functions like _create_thread
and _send_chat that use api_post from helpers. Either extend the api_post helper
to support PUT requests by adding a method parameter or create a new api_put
helper function, then refactor _set_http_tool_approval to use this helper
instead of httpx.AsyncClient directly. This will ensure uniform error handling
and timeout behavior across all HTTP interactions in the test.
| async def test_chat_thread_history_and_list_are_user_scoped(ironclaw_server): | ||
| """Bob must not list or read Alice's gateway thread by guessed id.""" | ||
| alice, bob = await _two_member_users(ironclaw_server) | ||
| thread_id = await _create_thread(ironclaw_server, alice["token"]) | ||
| marker = f"alice-private-thread-{uuid.uuid4().hex}" | ||
|
|
||
| await _send_chat(ironclaw_server, alice["token"], thread_id, marker) | ||
| alice_history = await _wait_for_history( | ||
| ironclaw_server, | ||
| alice["token"], | ||
| thread_id, | ||
| expected_user_input=marker, | ||
| ) | ||
| assert any(marker in (turn.get("user_input") or "") for turn in alice_history["turns"]) | ||
|
|
||
| bob_history = await api_get( | ||
| ironclaw_server, | ||
| f"/api/chat/history?thread_id={thread_id}", | ||
| token=bob["token"], | ||
| timeout=10, | ||
| ) | ||
| assert bob_history.status_code == 404, bob_history.text | ||
|
|
||
| bob_threads = await api_get( | ||
| ironclaw_server, | ||
| "/api/chat/threads", | ||
| token=bob["token"], | ||
| timeout=10, | ||
| ) | ||
| assert bob_threads.status_code == 200, bob_threads.text | ||
| bob_thread_ids = { | ||
| item["id"] for item in bob_threads.json().get("threads", []) | ||
| } | ||
| assistant = bob_threads.json().get("assistant_thread") | ||
| if assistant: | ||
| bob_thread_ids.add(assistant["id"]) | ||
| assert thread_id not in bob_thread_ids | ||
|
|
||
|
|
||
| async def test_approval_gate_resolution_is_user_scoped(ironclaw_server): | ||
| """Bob must not resolve Alice's pending approval gate by request id.""" | ||
| alice, bob = await _two_member_users(ironclaw_server) | ||
| await _set_http_tool_approval(ironclaw_server, alice["token"]) | ||
| thread_id = await _create_thread(ironclaw_server, alice["token"]) | ||
|
|
||
| await _send_chat( | ||
| ironclaw_server, | ||
| alice["token"], | ||
| thread_id, | ||
| f"make approval post cross-user-{uuid.uuid4().hex[:8]}", | ||
| ) | ||
| pending_history = await _wait_for_history( | ||
| ironclaw_server, | ||
| alice["token"], | ||
| thread_id, | ||
| expect_pending=True, | ||
| ) | ||
| request_id = pending_history["pending_gate"]["request_id"] | ||
|
|
||
| bob_approval = await api_post( | ||
| ironclaw_server, | ||
| "/api/chat/approval", | ||
| token=bob["token"], | ||
| json={ | ||
| "request_id": request_id, | ||
| "action": "approve", | ||
| "thread_id": thread_id, | ||
| }, | ||
| timeout=15, | ||
| ) | ||
| assert bob_approval.status_code in (403, 404, 409), bob_approval.text | ||
|
|
||
| still_pending = await _wait_for_history( | ||
| ironclaw_server, | ||
| alice["token"], | ||
| thread_id, | ||
| expect_pending=True, | ||
| ) | ||
| assert still_pending["pending_gate"]["request_id"] == request_id | ||
|
|
||
| cleanup = await api_post( | ||
| ironclaw_server, | ||
| "/api/chat/approval", | ||
| token=alice["token"], | ||
| json={ | ||
| "request_id": request_id, | ||
| "action": "deny", | ||
| "thread_id": thread_id, | ||
| }, | ||
| timeout=15, | ||
| ) | ||
| assert cleanup.status_code == 202, cleanup.text | ||
| await _wait_for_history( | ||
| ironclaw_server, | ||
| alice["token"], | ||
| thread_id, | ||
| expect_pending=False, | ||
| ) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Pin LLM provider explicitly to prevent silent fallback.
Per tests/e2e/scenarios/test_*.py guideline: "Pin the LLM provider explicitly via /api/settings/... by writing llm_backend=openai_compatible, openai_compatible_base_url=<mock_llm_url>, and selected_model=mock-model... to prevent silent fallback to NearAI."
Each test calls _send_chat, which hits /api/chat/send (LLM interaction). Add a fixture or setup step that pins llm_backend and selected_model before invoking _send_chat to harden these isolation tests against environment drift.
🤖 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/e2e/scenarios/test_multi_tenant_isolation.py` around lines 113 - 210,
Both test_chat_thread_history_and_list_are_user_scoped and
test_approval_gate_resolution_is_user_scoped invoke _send_chat, which requires
LLM interaction but currently do not explicitly pin the LLM provider, risking
silent fallback to NearAI and test instability. Before each _send_chat call in
both functions, add a setup step that calls the /api/settings API endpoint to
explicitly configure llm_backend=openai_compatible, openai_compatible_base_url
set to the mock LLM server URL (ironclaw_server), and selected_model=mock-model.
This can be done by adding an API call early in each test (after
_two_member_users and before the first _send_chat) or by creating a shared
fixture that both tests use.
Source: Coding guidelines
…s from nearai#3890) (nearai#3918) * test: introduce trait-level contract test harness pattern Establishes a reusable harness for testing trait contracts across multiple impls in a single suite. Across recent PR review (nearai#3890, nearai#3887, nearai#3908) the same antipattern recurred: a trait has multiple impls with isolation/durability/CAS invariants every impl must honor, but tests only cover one impl — often a mock that quietly implements its own invariants. Per .claude/rules/testing.md ("Test Through the Caller, Not Just the Helper"), a contract test against one impl proves only that impl, not the contract. This is a scaffolding change: it picks one trait (MemoryDocumentRepository, the trait at issue in nearai#3890's HIGH finding) and wires both existing impls to a shared contract suite. The shape — not the breadth — is the point. Follow-ups can extend the suite and port other traits (IdempotencyLedger, CheckpointStateStore, ProcessStore, ApprovalRequestStore, ...) onto the same pattern. The harness lives in crates/ironclaw_memory/src/contract_tests.rs as pub async fn contracts taking a factory closure. The contract_test! macro expands to one #[tokio::test] per contract named <impl_label>::<contract_name> for clear failure attribution. Contracts covered in this scaffold: - round_trip_returns_written_bytes - writes_isolated_across_scopes (the nearai#3890 invariant) - list_documents_honors_scope - search_documents_isolated_across_scopes (the exact nearai#3890 HIGH) Existing tests in memory_backend_contract.rs and memory_filesystem_contract.rs are intentionally not deleted; deduplication is a follow-up. * ci: gate contract_tests behind feature to hide harness panics from production scan The `scripts/check_no_panics.py` scanner flagged 25 panic-style calls (`.expect`, `.unwrap`, `assert*!`) inside `crates/ironclaw_memory/src/contract_tests.rs`. These are intentional — it is a test harness — but the module was unconditionally `pub mod`, so the scanner (which walks added lines in changed files and only treats `#[cfg(...test...)]`-gated items as test context) saw them as production code. Fix: - Add a `contract-tests` cargo feature on `ironclaw_memory`. - Gate `pub mod contract_tests;` in `lib.rs` behind `#[cfg(any(test, feature = "contract-tests"))]`. - Gate every item inside `contract_tests.rs` with the same cfg so the no-panics scanner recognizes the file as test-only context (the scanner only sees the file's own attributes, not the module declaration in `lib.rs`). - Add a self dev-dependency on `ironclaw_memory` with the `contract-tests` feature enabled so the per-impl integration test files in `tests/` (which import the harness and the `contract_test!` macro) keep compiling. Production builds (`cargo build -p ironclaw_memory`) no longer compile the harness; `cargo test -p ironclaw_memory` continues to run all 4 contracts for both the in-memory and filesystem repository impls. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
3 participants
Summary
Adds Reborn crate-level contract coverage for the next multi-tenancy isolation cases without touching legacy
src/or top-level E2E tests.Coverage includes:
Validation
Passed:
cargo test -p ironclaw_events --test durable_log_contractcargo test -p ironclaw_filesystem --features libsql --test db_root_filesystem_contractcargo test -p ironclaw_memory --test memory_backend_contractcargo test -p ironclaw_run_state --test approval_resolution_contractcargo test -p ironclaw_host_runtime --test host_runtime_contract default_runtime_same_idempotency_key_in_different_scopes_does_not_collidecargo test -p ironclaw_host_runtime --test host_runtime_contract default_runtime_status_and_cancel_do_not_reveal_guessed_ids_from_other_scopecargo test -p ironclaw_host_runtime --test runtime_http_egress_contract host_http_egress_does_not_use_policy_staged_for_other_tenant_user_or_projectcargo test -p ironclaw_mcp --test mcp_adapter_contract mcp_runtime_keeps_same_tool_name_calls_partitioned_by_scopecargo test -p ironclaw_processes --test process_store_contract process_host_hides_guessed_process_from_other_scopegit diff --checkNotes:
mcp_adapter_contract,process_store_contract, andruntime_http_egress_contractsuites still have unrelated existing failures on this reborn base; the added tests pass directly.