feat(multi-tenancy): per-session workspace_root into praxis tool execution (BRO-1491)#1771
Open
broomva wants to merge 2 commits into
Open
Conversation
…ution (BRO-1491) Autonomous arc implementation — VPS Life-governor first dispatch (BRO-1744). Threads per-session manifest.workspace_root through ToolContext -> bridge -> praxis FsPort scoping + session-unique manifest keys + BashTool cwd, per the ticket design notes. 27 files, BRO-1490 follow-up. ARC-AUTHORED + UNVALIDATED: the arc died before its own test run completed. CI is the validator — do not merge until green + reviewed. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
📝 WalkthroughWalkthroughThis PR threads a per-session ChangesPer-session workspace root scoping
Estimated code review effort: 4 (Complex) | ~75 minutes Sequence Diagram(s)sequenceDiagram
participant Adapter as ArcanHarnessAdapter
participant Bridge as ArcanHarnessBridge
participant Tool as WriteFileTool/BashTool
participant FsPort as effective_fs/LocalFs.scoped
participant Lago as LagoTrackedFs/FsTracker
Adapter->>Adapter: build ToolContext with workspace_root from request
Adapter->>Bridge: arcan_ctx_to_proto(ctx)
Bridge->>Tool: execute(ctx with workspace_root)
Tool->>FsPort: effective_fs(base, ctx)
FsPort->>FsPort: scoped(workspace_root)
Tool->>Lago: write/read via scoped FsPort
Lago->>Lago: manifest_key(path) rebased to session prefix
Lago->>Lago: reconcile_bounded_scoped(walk_root, key_prefix)
Lago-->>Tool: FileWrite/FileDelete events scoped to session
Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
crates/arcan/arcan/tests/praxis_integration.rs (1)
447-452: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueComment overstates what the assertion checks. The
assert_ne!just re-compares the two on-disk contents (already asserted equal to their expected values above); it does not exercise a session-A read that traverses into session B, so it doesn't demonstrate boundary rejection. Consider adjusting the comment (or dropping the redundant assert) to avoid implying isolation coverage that lives in thetracked_fsunit tests.🤖 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/arcan/arcan/tests/praxis_integration.rs` around lines 447 - 452, The current comment in the praxis integration test overstates what the trailing assertion verifies, since the assert_ne! only compares the two files’ on-disk contents and does not test a session-A read crossing into session B. Update the surrounding comment near the a_file/b_file assertions to describe only what is actually checked, or remove the redundant assert_ne! if it adds no value; keep the real boundary-isolation coverage in the tracked_fs tests and avoid implying this test exercises it.crates/lago/lago-fs/src/tracker.rs (1)
240-248: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winScoped reconcile disables hash reuse — re-hashes the whole session tree on every command.
Passing
Manifest::new()as the previous view meanssnapshot_bounded's size+mtime fast path never fires, so every file in the session workspace is re-read and re-putinto the blob store on each exec-path reconcile.reconcile_boundedavoids this via the manifest reuse hint. WithRemoteBlobBackend(LAGO_URL set) this becomes a network put per file per shell command. You can rebuild the reuse hint by strippingkey_prefixoff the current subtree:⚡ Restore hash reuse for the scoped walk
// Previous view keyed relative to walk_root (`/rel`) so snapshot_bounded // can reuse unchanged hashes instead of re-reading + re-putting every file. let prev = { let manifest = self.manifest.lock().unwrap(); let mut m = std::collections::BTreeMap::new(); for (k, e) in manifest.subtree(key_prefix) { if let Some(rel) = k.strip_prefix(key_prefix) { let mut e = e.clone(); e.path = rel.to_string(); m.insert(rel.to_string(), e); } } Manifest::from_entries(m) }; let raw = snapshot::snapshot_bounded(walk_root, &prev, self.blob_store.as_ref(), limits)?;Confirm whether the scoped exec path runs against remote lagod in production, which would amplify the per-command cost.
🤖 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/lago/lago-fs/src/tracker.rs` around lines 240 - 248, Restore hash reuse in the scoped exec reconcile path by replacing the unconditional Manifest::new() passed to snapshot::snapshot_bounded in Tracker::reconcile_bounded with a previous-view manifest built from the current subtree under key_prefix. Use the existing manifest lock and subtree lookup to strip key_prefix from keys, clone each entry, and rebase entry.path to the walk_root-relative path so snapshot_bounded can hit its size+mtime fast path instead of re-hashing and re-putting every file. Keep the change localized to the session workspace snapshot logic in tracker.rs and preserve the existing snapshot_bounded call shape.
🤖 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.
Nitpick comments:
In `@crates/arcan/arcan/tests/praxis_integration.rs`:
- Around line 447-452: The current comment in the praxis integration test
overstates what the trailing assertion verifies, since the assert_ne! only
compares the two files’ on-disk contents and does not test a session-A read
crossing into session B. Update the surrounding comment near the a_file/b_file
assertions to describe only what is actually checked, or remove the redundant
assert_ne! if it adds no value; keep the real boundary-isolation coverage in the
tracked_fs tests and avoid implying this test exercises it.
In `@crates/lago/lago-fs/src/tracker.rs`:
- Around line 240-248: Restore hash reuse in the scoped exec reconcile path by
replacing the unconditional Manifest::new() passed to snapshot::snapshot_bounded
in Tracker::reconcile_bounded with a previous-view manifest built from the
current subtree under key_prefix. Use the existing manifest lock and subtree
lookup to strip key_prefix from keys, clone each entry, and rebase entry.path to
the walk_root-relative path so snapshot_bounded can hit its size+mtime fast path
instead of re-hashing and re-putting every file. Keep the change localized to
the session workspace snapshot logic in tracker.rs and preserve the existing
snapshot_bounded call shape.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: bd3ecf3f-66ea-4775-8009-7c32e33b4832
📒 Files selected for processing (27)
crates/aios/aios-protocol/src/tool.rscrates/arcan/arcan-aios-adapters/src/gating_middleware.rscrates/arcan/arcan-aios-adapters/src/haima_middleware.rscrates/arcan/arcan-aios-adapters/src/tools.rscrates/arcan/arcan-core/src/runtime.rscrates/arcan/arcan-harness/src/bridge.rscrates/arcan/arcan-lago/src/learning.rscrates/arcan/arcan-lago/src/memory_tools.rscrates/arcan/arcan-lago/src/policy_middleware.rscrates/arcan/arcan-lago/src/reconciling_tool.rscrates/arcan/arcan-lago/src/tracked_fs.rscrates/arcan/arcan-lago/tests/end_to_end.rscrates/arcan/arcan-opsis/src/tools.rscrates/arcan/arcan-praxis/src/registry.rscrates/arcan/arcan-spaces/src/middleware.rscrates/arcan/arcan-spaces/src/tools.rscrates/arcan/arcan/src/main.rscrates/arcan/arcan/src/shell.rscrates/arcan/arcan/tests/praxis_integration.rscrates/lago/lago-fs/src/manifest.rscrates/lago/lago-fs/src/tracker.rscrates/nous/nous-middleware/src/middleware.rscrates/praxis/praxis-core/src/fs_port.rscrates/praxis/praxis-core/src/local_fs.rscrates/praxis/praxis-tools/src/edit.rscrates/praxis/praxis-tools/src/fs.rscrates/praxis/praxis-tools/src/shell.rs
…ad-per-session-manifestworkspace_root-into-praxis-tool
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
1 participant
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.
What
Threads per-session
manifest.workspace_rootthrough the arcan→praxis tool-execution path so concurrent sessions get isolated workspaces (multi-tenancy isolation). Implements the BRO-1490 follow-up per the ticket's code-verified design notes:ToolContextfield +bridge.rscopy-through, praxisFsPortscoping, session-uniquelago-fsmanifest keys, andBashToolcwd following the session workspace.27 files, +1093/−120 across arcan-{aios-adapters,core,harness,lago}, praxis-{core,tools}, lago-fs, plus
praxis_integration.rs+end_to_end.rstests.Autonomous arc-authored — the first live dispatch of the VPS Life-governor (BRO-1744). The arc completed the code but died before its own test run (the arc-stall failure mode being fixed in BRO-1831). So this is arc-authored + arc-unvalidated: CI is the validator, and this needs human/P20 review before merge. Do not merge until CI green + reviewed.
Acceptance (from the ticket)
Two concurrent sessions write
artifacts/receipt.txt; each lands in its own{data_dir}/sessions/<id>/artifacts/, two distinct FileWrite events with session-unique manifest paths, neither session reads the other's file.Linear: BRO-1491 · recovered via BRO-1744 · stall-fix BRO-1831
Summary by CodeRabbit
New Features
Bug Fixes