Added: Linux bubblewrap sandbox for shell command execution#69
Added: Linux bubblewrap sandbox for shell command execution#69
Conversation
WalkthroughAdds a new Linux-only crate, Possibly related PRs
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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.
Actionable comments posted: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/llm-coding-tools-core/src/tools/bash/blocking_impl.rs (1)
68-137:⚠️ Potential issue | 🟠 MajorBound the blocking pipe-drain wait after timeout.
After a timeout, this path always joins the stdout/stderr reader threads. If
child.kill()fails or a descendant keeps one pipe open, those joins can outlive the requested timeout indefinitely, soTimeoutWithKillFailureis not returned promptly. The tokio runner already caps post-kill drain time; blocking mode needs the same bounded behavior.
🧹 Nitpick comments (2)
src/.cargo/verify.ps1 (1)
89-89: Minor: Use named parameter for clarity.The array concatenation with
@("doc") + $docArgsuses a positional parameter. Consider using the explicit splatting pattern for consistency with other invocations.♻️ Suggested improvement
- Invoke-LoggedCommand "cargo" @("doc") + $docArgs + Invoke-LoggedCommand "cargo" (@("doc") + $docArgs)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/.cargo/verify.ps1` at line 89, Update the Invoke-LoggedCommand call to use a named parameter for argument splatting instead of positional array concatenation: replace the positional invocation Invoke-LoggedCommand "cargo" @("doc") + $docArgs with a call that passes the argument array via the explicit named parameter (e.g., using -Args or the parameter name that Invoke-LoggedCommand accepts) and combine @("doc") with $docArgs into the array you splat; refer to Invoke-LoggedCommand and $docArgs when making this change.src/llm-coding-tools-bubblewrap/src/profile/builder.rs (1)
73-545: Splitbuilder.rs; it is already over the module-size limit.Validation, shell resolution, and argv construction all live in one 500+ line production module now, which makes the builder path harder to audit and maintain. Pulling those pieces into smaller modules would keep the public builder API easier to navigate.
As per coding guidelines,
Keep modules under 500 lines (excluding tests); split if larger.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/llm-coding-tools-bubblewrap/src/profile/builder.rs` around lines 73 - 545, The file is too large and should be split: extract validation logic (validate_builder, validate_credential_file_mounts, credential_dest_is_allowed, validate_* helpers) into a new validation module; extract sandbox layout and shell resolution (builder_sandbox_layout, resolve_shell_for_builder, SandboxLayout helper) into a layout/module; and extract argv construction (build_static_args, arg_capacity_for, push_bind, push_symlinks, push_env_args, push_same_path_bind(s), push_tmpfs_mounts, push_tmp_mount, push_file_mounts) into an args/module. Keep the Builder struct and new/with_* methods in the original module, make the moved functions pub(crate) as needed, update mod declarations and use paths, and run cargo check to fix visibility/imports so the build and tests still compile. Ensure tests and any callers reference the relocated symbols through the new modules.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@SANDBOX-PROFILES.md`:
- Line 4: Update the incorrect feature flag name in the setup guidance: replace
occurrences of `linux-sandbox` with the correct flag `linux-bubblewrap` in the
sentence referencing `llm-coding-tools-bubblewrap` and any nearby guidance so
the docs match the implementation; search for any other mentions of
`linux-sandbox` in the same doc and update them to `linux-bubblewrap` to avoid
configuration failures.
In `@src/.cargo/verify.ps1`:
- Line 35: The script assigns to $isLinux using $IsLinux which shadows the
automatic readonly $IsLinux variable (case-insensitive) and will error on
PowerShell Core; rename the local variable (e.g. to $runOnLinux) and update all
later references from $isLinux to $runOnLinux so you no longer attempt to assign
to the automatic $IsLinux and the runtime check ($IsLinux -eq $true) result is
stored in a safe mutable variable.
In `@src/.cargo/verify.sh`:
- Around line 83-87: The workspace packaging step always runs and may try to
package the Linux-only crate llm-coding-tools-bubblewrap on non-Linux systems;
change the cargo package invocation so non-Linux runs exclude that crate. Update
the script around the IS_LINUX check to either (a) only run cargo package
--workspace on Linux, or (b) add --exclude llm-coding-tools-bubblewrap when
IS_LINUX is not true (modify the run_cmd call that invokes "cargo package
--workspace --allow-dirty --quiet" to include the conditional exclusion). Ensure
you reference the IS_LINUX variable, the run_cmd wrapper, and the crate name
llm-coding-tools-bubblewrap when making the change.
In `@src/llm-coding-tools-bubblewrap/Cargo.toml`:
- Around line 11-14: Update the Cargo.toml features so the tokio async mode is
enabled by default: change the features table to set default = ["tokio"] while
keeping the existing feature entries tokio = ["dep:process-wrap",
"process-wrap/tokio1", "process-wrap/process-group"] and blocking =
["dep:process-wrap", "process-wrap/std", "process-wrap/process-group"]
unchanged, so the crate defaults to the tokio feature but still allows opting
into blocking.
In `@src/llm-coding-tools-bubblewrap/README.md`:
- Around line 155-160: Remove the unused Markdown reference definitions for
Availability, Availability::detect, Profile, Builder, Preset, and TmpBacking
from the README (the lines that define [`Availability`],
[`Availability::detect`], [`Profile`], [`Builder`], [`Preset`], and
[`TmpBacking`]); delete those reference anchor lines so markdownlint no longer
reports unused reference definitions while leaving any actually-used references
intact.
In `@src/llm-coding-tools-bubblewrap/src/probe.rs`:
- Around line 95-103: The find_binary_on_path function should ignore
non-absolute PATH entries to prevent resolving binaries from the current
directory or other relative paths; update the loop that iterates
env::split_paths(&path) (in find_binary_on_path) to skip any dir where
dir.is_absolute() is false (also skip empty entries) before joining name and
checking candidate.is_file(), so only absolute directories are considered when
constructing candidate.into_boxed_path().
In `@src/llm-coding-tools-bubblewrap/src/profile/builder.rs`:
- Around line 708-721: The test
public_bot_preset_rejects_nonexistent_workspace_directory currently uses shared
paths (/tmp/workspace, /tmp/home, /tmp/cache) which may exist; change it so the
workspace path passed to Builder::public_bot is guaranteed-missing (e.g.,
generate a unique random or temporary path that does not exist or
create-and-drop a TempDir then use that path) while keeping home/cache valid if
needed; update the call to Builder::public_bot to use that non-existent
workspace path and leave the rest of the test flow (with_availability, build,
unwrap_err and the assertion about "workspace host directory") unchanged.
In `@src/llm-coding-tools-bubblewrap/src/profile/presets.rs`:
- Around line 188-196: path_entry_allowed currently allows relative PATH entries
for Preset::TrustedMaintenance, which lets `.`/workspace-local bins bypass the
deny-list; update path_entry_allowed (the Preset::TrustedMaintenance branch) to
first reject non-absolute paths (use Path::is_absolute or equivalent) and return
false for relative entries before running the TRUSTED_DENY_PREFIXES prefix check
so only absolute paths are evaluated against the deny-list.
- Around line 94-97: The tmpfs overlay list passed to with_tmpfs_overlays
contains a file path (Path::new("/etc/shadow")) which is invalid for Bubblewrap
--tmpfs (expects directories); remove "/etc/shadow" from the Arc::from([...])
passed to with_tmpfs_overlays in presets.rs and instead implement a file-level
protection for /etc/shadow (for example, add a separate ro-bind of an empty file
or a dedicated file-overlay mechanism using --ro-bind to a created empty file)
so that /home stays as a tmpfs directory while /etc/shadow is handled via a file
bind-mount.
In `@src/llm-coding-tools-bubblewrap/src/profile/validation.rs`:
- Around line 57-67: The validators (e.g., validate_absolute_path) currently
accept paths with `..` components which can bypass later lexical allowlist
checks; update validate_absolute_path and the other path validators (the ones
around lines 81–103 and 189–205 that validate cache_root, mount paths, and
overlay paths) to first normalize the input path (using std::fs::canonicalize
where appropriate or a safe lexical clean for non-existing paths) and operate on
that normalized Path before returning Ok/Err, and also update allowlist checks
in layout.rs and builder.rs to normalize both the candidate path and the
allowlist prefixes before using starts_with/strip_prefix so comparisons are done
on cleaned, equivalent paths.
In `@src/llm-coding-tools-bubblewrap/src/test_helpers.rs`:
- Around line 31-79: The helpers replace_path and prepend_path call unsafe
env-var APIs but expose a safe API; change both to unsafe fn replace_path(path:
&Path) -> PathGuard and unsafe fn prepend_path(path: &Path) -> PathGuard so the
unsafety contract (no concurrent PATH access) is enforced by the type system,
update all call sites to wrap calls in unsafe blocks (or provide a safe wrapper
in tests that holds the process-wide mutex), and leave PathGuard and
PathGuard::capture unchanged; ensure the function docs still describe the
precondition and note callers must uphold the no-concurrent-access guarantee.
In `@src/llm-coding-tools-core/src/path/allowed.rs`:
- Around line 28-29: The comment in allowed.rs has awkward phrasing ("read
`SANDBOX-PROFILES.md` to read more about Sandboxing on Linux"); update the doc
comment to a concise, grammatically correct sentence such as "See
SANDBOX-PROFILES.md for details on sandboxing on Linux." — change the sentence
in the comment block in src/llm-coding-tools-core/src/path/allowed.rs to use
this simplified wording.
In `@src/llm-coding-tools-serdesai/src/bash.rs`:
- Around line 133-136: Update the documentation for with_linux_bwrap and call():
change the #[doc] / "Errors" section on with_linux_bwrap so it no longer claims
it returns an execution error (since with_linux_bwrap only stores the mode), and
either remove that error clause or replace it with a note that any
missing/unusable bwrap will cause an execution error when call() is invoked;
ensure the call() function's docstring includes the execution error description
about missing/unusable bwrap so the failure is documented at the point it can
actually occur.
---
Nitpick comments:
In `@src/.cargo/verify.ps1`:
- Line 89: Update the Invoke-LoggedCommand call to use a named parameter for
argument splatting instead of positional array concatenation: replace the
positional invocation Invoke-LoggedCommand "cargo" @("doc") + $docArgs with a
call that passes the argument array via the explicit named parameter (e.g.,
using -Args or the parameter name that Invoke-LoggedCommand accepts) and combine
@("doc") with $docArgs into the array you splat; refer to Invoke-LoggedCommand
and $docArgs when making this change.
In `@src/llm-coding-tools-bubblewrap/src/profile/builder.rs`:
- Around line 73-545: The file is too large and should be split: extract
validation logic (validate_builder, validate_credential_file_mounts,
credential_dest_is_allowed, validate_* helpers) into a new validation module;
extract sandbox layout and shell resolution (builder_sandbox_layout,
resolve_shell_for_builder, SandboxLayout helper) into a layout/module; and
extract argv construction (build_static_args, arg_capacity_for, push_bind,
push_symlinks, push_env_args, push_same_path_bind(s), push_tmpfs_mounts,
push_tmp_mount, push_file_mounts) into an args/module. Keep the Builder struct
and new/with_* methods in the original module, make the moved functions
pub(crate) as needed, update mod declarations and use paths, and run cargo check
to fix visibility/imports so the build and tests still compile. Ensure tests and
any callers reference the relocated symbols through the new modules.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a61dd94c-d1e4-43c3-8dc4-905915cce980
⛔ Files ignored due to path filters (1)
src/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (45)
.github/workflows/rust.ymlREADME.MDSANDBOX-PROFILES.mdsrc/.cargo/verify.ps1src/.cargo/verify.shsrc/Cargo.tomlsrc/llm-coding-tools-bubblewrap/ARCHITECTURE.mdsrc/llm-coding-tools-bubblewrap/Cargo.tomlsrc/llm-coding-tools-bubblewrap/README.mdsrc/llm-coding-tools-bubblewrap/src/error.rssrc/llm-coding-tools-bubblewrap/src/lib.rssrc/llm-coding-tools-bubblewrap/src/path_util.rssrc/llm-coding-tools-bubblewrap/src/probe.rssrc/llm-coding-tools-bubblewrap/src/profile/builder.rssrc/llm-coding-tools-bubblewrap/src/profile/layout.rssrc/llm-coding-tools-bubblewrap/src/profile/mod.rssrc/llm-coding-tools-bubblewrap/src/profile/presets.rssrc/llm-coding-tools-bubblewrap/src/profile/types.rssrc/llm-coding-tools-bubblewrap/src/profile/validation.rssrc/llm-coding-tools-bubblewrap/src/test_helpers.rssrc/llm-coding-tools-bubblewrap/src/wrap/blocking.rssrc/llm-coding-tools-bubblewrap/src/wrap/command.rssrc/llm-coding-tools-bubblewrap/src/wrap/mod.rssrc/llm-coding-tools-bubblewrap/src/wrap/tokio.rssrc/llm-coding-tools-core/Cargo.tomlsrc/llm-coding-tools-core/README.mdsrc/llm-coding-tools-core/examples/system_prompt/mock_tools.rssrc/llm-coding-tools-core/src/context/tool_prompt/mod.rssrc/llm-coding-tools-core/src/context/tool_prompt/tool_sections.rssrc/llm-coding-tools-core/src/lib.rssrc/llm-coding-tools-core/src/path/allowed.rssrc/llm-coding-tools-core/src/system_prompt.rssrc/llm-coding-tools-core/src/tools/bash/blocking_impl.rssrc/llm-coding-tools-core/src/tools/bash/mod.rssrc/llm-coding-tools-core/src/tools/bash/tokio_impl.rssrc/llm-coding-tools-core/src/tools/mod.rssrc/llm-coding-tools-serdesai/Cargo.tomlsrc/llm-coding-tools-serdesai/README.mdsrc/llm-coding-tools-serdesai/examples/serdesai-agents.rssrc/llm-coding-tools-serdesai/examples/serdesai-basic.rssrc/llm-coding-tools-serdesai/examples/serdesai-sandboxed-bash.rssrc/llm-coding-tools-serdesai/examples/serdesai-sandboxed.rssrc/llm-coding-tools-serdesai/examples/serdesai-task.rssrc/llm-coding-tools-serdesai/src/bash.rssrc/llm-coding-tools-serdesai/src/lib.rs
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Build and Test (Async/Tokio) (ubuntu-latest, x86_64-unknown-linux-gnu)
- GitHub Check: Build and Test (Async/Tokio) (windows-latest, x86_64-pc-windows-msvc)
- GitHub Check: Build and Test (Async/Tokio) (macos-latest, aarch64-apple-darwin)
- GitHub Check: Build and Test (Blocking) (macos-latest, aarch64-apple-darwin)
- GitHub Check: Build and Test (Blocking) (windows-latest, x86_64-pc-windows-msvc)
🧰 Additional context used
📓 Path-based instructions (2)
src/**/*.rs
📄 CodeRabbit inference engine (src/AGENTS.md)
src/**/*.rs: Preallocate collections when size is known or estimable usingString::with_capacity(estimated_len),Vec::with_capacity(count), orBufReader::with_capacity(size, reader)
Prefer&str/&[T]returns over owned types when lifetime allows
UseCow<'_, str>for conditional ownership (e.g.,String::from_utf8_lossy)
Use&'static strfor compile-time constant strings
Reuse buffers by using.clear()and reusingVec/Stringinstead of reallocating
Use const generics for compile-time branching (e.g.,<const LINE_NUMBERS: bool>)
Use#[inline]on small, hot-path functions
Prefercoreoverstdwhere possible (e.g.,core::memoverstd::mem)
Stream data instead of loading entire files when possible
Usememchrfor fast byte searching over manual iteration
Keep modules under 500 lines (excluding tests); split if larger
Placeusestatements inside functions only for#[cfg]conditional compilation
Document public items with///and add examples in docs where helpful
Use//!for module-level documentation
Focus comments on 'why' not 'what'
Use [TypeName] rustdoc links in documentation, not backticks
Files:
src/llm-coding-tools-serdesai/examples/serdesai-agents.rssrc/llm-coding-tools-core/src/path/allowed.rssrc/llm-coding-tools-core/examples/system_prompt/mock_tools.rssrc/llm-coding-tools-serdesai/examples/serdesai-sandboxed.rssrc/llm-coding-tools-serdesai/examples/serdesai-basic.rssrc/llm-coding-tools-bubblewrap/src/error.rssrc/llm-coding-tools-bubblewrap/src/path_util.rssrc/llm-coding-tools-serdesai/examples/serdesai-task.rssrc/llm-coding-tools-core/src/context/tool_prompt/mod.rssrc/llm-coding-tools-core/src/context/tool_prompt/tool_sections.rssrc/llm-coding-tools-bubblewrap/src/wrap/tokio.rssrc/llm-coding-tools-bubblewrap/src/wrap/blocking.rssrc/llm-coding-tools-core/src/lib.rssrc/llm-coding-tools-core/src/system_prompt.rssrc/llm-coding-tools-serdesai/src/lib.rssrc/llm-coding-tools-core/src/tools/mod.rssrc/llm-coding-tools-bubblewrap/src/wrap/mod.rssrc/llm-coding-tools-bubblewrap/src/profile/mod.rssrc/llm-coding-tools-bubblewrap/src/lib.rssrc/llm-coding-tools-serdesai/examples/serdesai-sandboxed-bash.rssrc/llm-coding-tools-bubblewrap/src/wrap/command.rssrc/llm-coding-tools-core/src/tools/bash/blocking_impl.rssrc/llm-coding-tools-serdesai/src/bash.rssrc/llm-coding-tools-bubblewrap/src/profile/presets.rssrc/llm-coding-tools-core/src/tools/bash/mod.rssrc/llm-coding-tools-core/src/tools/bash/tokio_impl.rssrc/llm-coding-tools-bubblewrap/src/test_helpers.rssrc/llm-coding-tools-bubblewrap/src/probe.rssrc/llm-coding-tools-bubblewrap/src/profile/layout.rssrc/llm-coding-tools-bubblewrap/src/profile/types.rssrc/llm-coding-tools-bubblewrap/src/profile/builder.rssrc/llm-coding-tools-bubblewrap/src/profile/validation.rs
src/**/Cargo.toml
📄 CodeRabbit inference engine (src/AGENTS.md)
src/**/Cargo.toml: Enabletokiofeature (default) for async mode with tokio runtime, orblockingfeature for sync/blocking mode. Theasyncandblockingfeatures are mutually exclusive - enabling both causes a compile error.
Prefer performance-oriented crates such asparking_lotoverstd::syncandmemchrfor byte search
Keep dependency footprint minimal
Files:
src/Cargo.tomlsrc/llm-coding-tools-bubblewrap/Cargo.tomlsrc/llm-coding-tools-serdesai/Cargo.tomlsrc/llm-coding-tools-core/Cargo.toml
🧠 Learnings (6)
📚 Learning: 2026-03-17T21:06:53.909Z
Learnt from: CR
Repo: Sewer56/llm-coding-tools PR: 0
File: src/AGENTS.md:0-0
Timestamp: 2026-03-17T21:06:53.909Z
Learning: Applies to src/**/Cargo.toml : Keep dependency footprint minimal
Applied to files:
src/Cargo.toml.github/workflows/rust.ymlsrc/llm-coding-tools-bubblewrap/Cargo.tomlsrc/llm-coding-tools-serdesai/Cargo.tomlsrc/llm-coding-tools-core/Cargo.toml
📚 Learning: 2026-03-17T21:06:53.909Z
Learnt from: CR
Repo: Sewer56/llm-coding-tools PR: 0
File: src/AGENTS.md:0-0
Timestamp: 2026-03-17T21:06:53.909Z
Learning: Applies to src/**/Cargo.toml : Enable `tokio` feature (default) for async mode with tokio runtime, or `blocking` feature for sync/blocking mode. The `async` and `blocking` features are mutually exclusive - enabling both causes a compile error.
Applied to files:
README.MDsrc/llm-coding-tools-core/README.mdsrc/llm-coding-tools-core/Cargo.toml
📚 Learning: 2026-03-17T21:06:53.909Z
Learnt from: CR
Repo: Sewer56/llm-coding-tools PR: 0
File: src/AGENTS.md:0-0
Timestamp: 2026-03-17T21:06:53.909Z
Learning: Applies to src/**/*.rs : Document public items with `///` and add examples in docs where helpful
Applied to files:
src/llm-coding-tools-serdesai/examples/serdesai-basic.rssrc/llm-coding-tools-serdesai/README.mdsrc/llm-coding-tools-bubblewrap/src/lib.rssrc/llm-coding-tools-serdesai/examples/serdesai-sandboxed-bash.rssrc/llm-coding-tools-bubblewrap/src/test_helpers.rs
📚 Learning: 2026-03-17T21:06:53.909Z
Learnt from: CR
Repo: Sewer56/llm-coding-tools PR: 0
File: src/AGENTS.md:0-0
Timestamp: 2026-03-17T21:06:53.909Z
Learning: After making changes to source code, always run `.cargo/verify.sh` (`.cargo/verify.ps1` on Windows) before returning to the user
Applied to files:
.github/workflows/rust.ymlsrc/.cargo/verify.ps1src/.cargo/verify.sh
📚 Learning: 2026-03-17T21:06:53.909Z
Learnt from: CR
Repo: Sewer56/llm-coding-tools PR: 0
File: src/AGENTS.md:0-0
Timestamp: 2026-03-17T21:06:53.909Z
Learning: Applies to src/**/*.rs : Use `//!` for module-level documentation
Applied to files:
src/llm-coding-tools-serdesai/README.mdsrc/llm-coding-tools-bubblewrap/src/lib.rs
📚 Learning: 2026-03-17T21:06:53.909Z
Learnt from: CR
Repo: Sewer56/llm-coding-tools PR: 0
File: src/AGENTS.md:0-0
Timestamp: 2026-03-17T21:06:53.909Z
Learning: Applies to src/**/*.rs : Place `use` statements inside functions only for `#[cfg]` conditional compilation
Applied to files:
src/llm-coding-tools-bubblewrap/src/lib.rs
🪛 markdownlint-cli2 (0.22.0)
src/llm-coding-tools-bubblewrap/README.md
[warning] 155-155: Link and image reference definitions should be needed
Unused link or image reference definition: "availability"
(MD053, link-image-reference-definitions)
[warning] 159-159: Link and image reference definitions should be needed
Unused link or image reference definition: "preset"
(MD053, link-image-reference-definitions)
[warning] 160-160: Link and image reference definitions should be needed
Unused link or image reference definition: "tmpbacking"
(MD053, link-image-reference-definitions)
🪛 PSScriptAnalyzer (1.25.0)
src/.cargo/verify.ps1
[error] 35-35: Starting from PowerShell 6.0, the Variable 'isLinux' cannot be assigned any more since it is a readonly automatic variable that is built into PowerShell, please use a different name.
(PSAvoidAssignmentToAutomaticVariable)
[warning] 38-38: File 'verify.ps1' uses Write-Host. Avoid using Write-Host because it might not work in all hosts, does not work when there is no host, and (prior to PS 5.0) cannot be suppressed, captured, or redirected. Instead, use Write-Output, Write-Verbose, or Write-Information.
(PSAvoidUsingWriteHost)
[warning] 47-47: File 'verify.ps1' uses Write-Host. Avoid using Write-Host because it might not work in all hosts, does not work when there is no host, and (prior to PS 5.0) cannot be suppressed, captured, or redirected. Instead, use Write-Output, Write-Verbose, or Write-Information.
(PSAvoidUsingWriteHost)
[warning] 56-56: File 'verify.ps1' uses Write-Host. Avoid using Write-Host because it might not work in all hosts, does not work when there is no host, and (prior to PS 5.0) cannot be suppressed, captured, or redirected. Instead, use Write-Output, Write-Verbose, or Write-Information.
(PSAvoidUsingWriteHost)
[warning] 65-65: File 'verify.ps1' uses Write-Host. Avoid using Write-Host because it might not work in all hosts, does not work when there is no host, and (prior to PS 5.0) cannot be suppressed, captured, or redirected. Instead, use Write-Output, Write-Verbose, or Write-Information.
(PSAvoidUsingWriteHost)
[warning] 73-73: File 'verify.ps1' uses Write-Host. Avoid using Write-Host because it might not work in all hosts, does not work when there is no host, and (prior to PS 5.0) cannot be suppressed, captured, or redirected. Instead, use Write-Output, Write-Verbose, or Write-Information.
(PSAvoidUsingWriteHost)
[warning] 76-76: File 'verify.ps1' uses Write-Host. Avoid using Write-Host because it might not work in all hosts, does not work when there is no host, and (prior to PS 5.0) cannot be suppressed, captured, or redirected. Instead, use Write-Output, Write-Verbose, or Write-Information.
(PSAvoidUsingWriteHost)
[warning] 80-80: File 'verify.ps1' uses Write-Host. Avoid using Write-Host because it might not work in all hosts, does not work when there is no host, and (prior to PS 5.0) cannot be suppressed, captured, or redirected. Instead, use Write-Output, Write-Verbose, or Write-Information.
(PSAvoidUsingWriteHost)
[warning] 94-94: File 'verify.ps1' uses Write-Host. Avoid using Write-Host because it might not work in all hosts, does not work when there is no host, and (prior to PS 5.0) cannot be suppressed, captured, or redirected. Instead, use Write-Output, Write-Verbose, or Write-Information.
(PSAvoidUsingWriteHost)
[warning] 97-97: File 'verify.ps1' uses Write-Host. Avoid using Write-Host because it might not work in all hosts, does not work when there is no host, and (prior to PS 5.0) cannot be suppressed, captured, or redirected. Instead, use Write-Output, Write-Verbose, or Write-Information.
(PSAvoidUsingWriteHost)
[warning] 103-103: File 'verify.ps1' uses Write-Host. Avoid using Write-Host because it might not work in all hosts, does not work when there is no host, and (prior to PS 5.0) cannot be suppressed, captured, or redirected. Instead, use Write-Output, Write-Verbose, or Write-Information.
(PSAvoidUsingWriteHost)
[info] 89-89: Cmdlet 'Invoke-LoggedCommand' has positional parameter. Please use named parameters instead of positional parameters when calling a command.
(PSAvoidUsingPositionalParameters)
🔇 Additional comments (33)
src/llm-coding-tools-bubblewrap/src/path_util.rs (1)
10-15: Looks good: normalization fallback is explicit and safe.The canonicalize-or-preserve behavior is clear and aligns with the function contract.
src/llm-coding-tools-serdesai/examples/serdesai-agents.rs (1)
19-19: Model constant update is clean and scoped.No functional concerns in this file for the model-id switch.
src/llm-coding-tools-serdesai/examples/serdesai-sandboxed.rs (1)
24-24: Scoped constant update looks good.The change is isolated to model selection and doesn’t affect runtime flow here.
src/llm-coding-tools-serdesai/examples/serdesai-task.rs (1)
24-24: Constant change is tidy and non-invasive.No review concerns in this segment.
src/llm-coding-tools-core/examples/system_prompt/mock_tools.rs (1)
164-167: Good update to the newToolPrompt::Bashshape.This keeps the mock aligned with the new prompt variant without changing intended behavior.
src/llm-coding-tools-core/src/context/tool_prompt/mod.rs (1)
38-41:ToolPrompt::Bashmigration is consistent here.The new struct variant and the updated matcher in
record()are correctly aligned.Also applies to: 120-120
src/Cargo.toml (1)
4-10: LGTM!The workspace member list is correctly updated to include the new
llm-coding-tools-bubblewrapcrate. The multi-line formatting improves readability and makes future additions cleaner.src/llm-coding-tools-serdesai/examples/serdesai-basic.rs (2)
10-12: LGTM!Good addition of documentation clarifying that sandboxing is not enabled in this example and pointing users to the
serdesai-sandboxexample for restricted execution.
51-51: LGTM!The change from
BashTool::new()toBashTool::host()correctly reflects the new explicit API for host-mode (non-sandboxed) execution, maintaining backward-compatible semantics while making the execution mode clear.README.MD (1)
21-22: LGTM!Documentation updates are comprehensive and consistent:
- Workspace crate listing includes the new bubblewrap crate
- Shell feature description accurately notes optional Linux sandboxing
- Feature flags section documents
linux-bubblewrap- Quick Start includes the new dependency
- Documentation section links to both the crate README and operator guide
Also applies to: 30-30, 43-43, 55-55, 82-84
src/llm-coding-tools-serdesai/src/lib.rs (1)
18-23: LGTM!The re-exports are well-structured:
BashExecutionModeandexecute_command_with_modeenable mode-aware bash execution at the serdesai API level- The
profilemodule is correctly gated with bothfeature = "linux-bubblewrap"andtarget_os = "linux", ensuring compile-time safety on non-Linux platformssrc/llm-coding-tools-bubblewrap/src/error.rs (1)
1-16: LGTM!Clean error type design using
thiserror. The two variants cover the main failure modes:
InvalidPathfor sandbox path validation failuresExecutionfor bwrap/shell availability issuesThe
{0}format pattern keeps error messages flexible while avoiding redundant prefixes.src/llm-coding-tools-bubblewrap/Cargo.toml (1)
16-19: LGTM!Dependencies follow the coding guidelines:
parking_lotfor synchronization (preferred overstd::sync)thiserrorfor ergonomic error handlingprocess-wrapis optional and only pulled in when async/blocking execution is neededThe footprint is minimal and each dependency serves a clear purpose. Based on learnings: "Keep dependency footprint minimal" and "Prefer performance-oriented crates such as
parking_lotoverstd::sync"..github/workflows/rust.yml (3)
68-79: LGTM!Comprehensive semver-checks for the Linux-only crate:
- Correctly gated to
ubuntu-latestonly- Tests all three feature combinations: default (no features),
tokio, andblocking- Uses
--only-explicit-featuresto avoid mutually exclusive feature conflicts
92-94: LGTM!Documentation and linting correctly gated to Linux runners only. The feature flags match each job's context (
tokiofor async,blockingfor blocking mode).Also applies to: 105-107, 153-155, 163-165
187-191: LGTM!Publishing
llm-coding-tools-bubblewrapfirst ensures it's available on crates.io before dependent crates (llm-coding-tools-corewithlinux-bubblewrapfeature) are published.src/llm-coding-tools-core/README.md (2)
89-109: LGTM!Excellent documentation for the Linux shell sandboxing feature:
- Clear description of what the sandbox constrains (filesystem, environment, network)
- Concise comparison of the two profiles with appropriate security guidance
- Smart default choice (Public Bot) with explicit call-out to evaluate fit
- Link to comprehensive operator guide
218-218: LGTM!Example correctly updated from
BashTool::new()toBashTool::host()to reflect the new explicit API for host-mode execution.src/llm-coding-tools-serdesai/Cargo.toml (2)
46-50: LGTM!The feature wiring is correct:
linux-bubblewrapproperly enables the optional dependency and forwards the feature tollm-coding-tools-core. This follows the idiomatic pattern for conditional feature propagation.
58-60: LGTM!The optional dependency declaration is appropriate—it keeps the dependency footprint minimal by only including the bubblewrap crate when explicitly enabled. Based on learnings: "Keep dependency footprint minimal".
src/llm-coding-tools-core/src/context/tool_prompt/tool_sections.rs (1)
22-25: LGTM!The updated
write_bash_sectioncorrectly handles the new structuredToolPrompt::Bashvariant. The conditional guidance lines are appended in a logical order (sandbox context first, then network restriction), and the implementation cleanly extends the existing pattern.Also applies to: 44-61
src/llm-coding-tools-core/src/tools/mod.rs (1)
17-19: LGTM!The conditional re-export of
linux_bwrap_profileis correctly gated by both the feature flag and target OS. The unconditional export ofBashExecutionModeandexecute_command_with_modeis appropriate since these types support both host and sandboxed execution modes.src/llm-coding-tools-bubblewrap/src/wrap/tokio.rs (1)
1-33: LGTM!The Tokio async wrapper is well-implemented:
- Proper error propagation from
wrap_command- Correct I/O configuration (null stdin, piped stdout/stderr)
- Process group leadership for proper sandbox child management
- Good documentation with
# Errorssectionsrc/llm-coding-tools-core/src/lib.rs (1)
33-42: LGTM!The crate-level re-exports correctly expose the new mode-aware bash execution API (
BashExecutionMode,execute_command_with_mode) and conditionally export the Linux sandbox profile types. The feature gating is consistent with the internal module exports.src/llm-coding-tools-core/src/system_prompt.rs (2)
468-475: LGTM!The
BuiltInBashToolcorrectly uses the new structuredToolPrompt::Bashvariant with explicitnetwork_disabledandsandboxedfields for host mode testing.
1198-1243: LGTM!Comprehensive test coverage for the conditional Bash guidance lines. The test validates both sandboxed (with network disabled) and host configurations, asserting that conditional lines appear only when expected. The count assertion on the network line prevents accidental duplication.
src/llm-coding-tools-bubblewrap/src/wrap/mod.rs (1)
1-26: LGTM!Well-structured module with clear documentation following the
//!convention. The feature-gated submodule exposure (blocking/tokio) and public re-exports are appropriately organized for the crate's dual async/blocking support pattern.src/llm-coding-tools-bubblewrap/src/wrap/blocking.rs (1)
18-33: Good blocking adapter implementation.Clear delegation, correct stdio setup, and explicit
ProcessGroup::leader()wrapping make this path consistent and predictable.src/llm-coding-tools-serdesai/README.md (1)
82-103: Linux sandboxing docs are clear and actionable.The feature-flag section and profile guidance communicate security posture and defaults well.
src/llm-coding-tools-bubblewrap/src/profile/mod.rs (1)
9-18: Nice module boundary and re-export surface.This keeps internals private while exposing a concise public API.
src/llm-coding-tools-bubblewrap/src/lib.rs (1)
3-4: Linux-only gate is explicit and correctly enforced.Compile-time guarding here makes platform constraints unambiguous.
src/llm-coding-tools-core/Cargo.toml (1)
16-38: Feature/dependency wiring looks solid for opt-in Linux sandboxing.The integration keeps default footprint lean while enabling runtime-specific forwarding when requested.
Also applies to: 100-105
src/llm-coding-tools-bubblewrap/ARCHITECTURE.md (1)
8-156: Great architecture write-up.The module map and lifecycle diagrams make the sandbox behavior and integration points easy to reason about.
Sewer56
force-pushed
the
bwrap-support-new
branch
from
6818f43 to
244d5f1
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/llm-coding-tools-core/src/tools/bash/blocking_impl.rs (1)
105-123:⚠️ Potential issue | 🟠 MajorTimeout can still hang on the blocking path.
After a timeout, this code always joins the stdout/stderr reader threads. If
child.kill()fails, or the child keeps either pipe open, thoseread_to_end()calls never finish and the timeout is never returned.src/llm-coding-tools-core/src/tools/bash/tokio_impl.rsalready avoids this with a bounded grace wait; this path needs the same treatment.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/llm-coding-tools-core/src/tools/bash/blocking_impl.rs` around lines 105 - 123, The blocking path can hang waiting on stdout_thread/stderr_thread after a timeout because join() blocks if readers never finish; update the timeout branch (where WaitOutcome::TimedOut is created after child.kill()) to mirror tokio_impl's bounded grace behavior: after attempting to kill the child (child.kill()), close the child's stdio pipes (take and drop child.stdout/child.stderr / close underlying handles) to force readers to see EOF, then wait a short bounded grace period for stdout_thread and stderr_thread to join (e.g., spin/poll with a small sleep loop up to X ms), and if they still haven't finished return TimedOut with the kill_error without blocking forever; reference stdout_thread, stderr_thread, child.kill(), and WaitOutcome::TimedOut when making the change.
🧹 Nitpick comments (3)
src/llm-coding-tools-serdesai/examples/serdesai-sandboxed-bash.rs (1)
37-44: Consider documenting the empty default behavior more clearly.The
API_KEY_VALUEconstant is empty andget_api_key()falls back to it, which then triggers the exit at line 53-56. This is fine but could confuse readers who might expect a placeholder value.📝 Optional: Make the fallback intent clearer
- const API_KEY_VALUE: &str = ""; + /// Empty by default - forces users to set the env var. + const API_KEY_VALUE: &str = "";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/llm-coding-tools-serdesai/examples/serdesai-sandboxed-bash.rs` around lines 37 - 44, The default API key constant API_KEY_VALUE is an empty string which makes get_api_key() silently fall back to an empty value and later trigger the exit path; clarify intent by either replacing API_KEY_VALUE with a descriptive placeholder (e.g. "<UNSET_API_KEY>") or adding a short doc comment above API_KEY_VALUE and get_api_key() that explains the empty default and that the program will exit later if no env var is set; ensure references to API_KEY_NAME, API_KEY_VALUE, and get_api_key() are updated so readers understand the fallback behavior.src/llm-coding-tools-serdesai/src/bash.rs (1)
203-233: Consider simplifying the cfg-gated helpers.Both
bash_prompt_network_disabledandbash_prompt_sandboxedhave parallel#[cfg]blocks with the non-Linux path just returningfalse. This is correct but could be slightly more concise.♻️ Optional: Consolidate with a single expression
#[inline] fn bash_prompt_sandboxed(mode: &BashExecutionMode) -> bool { - #[cfg(all(feature = "linux-bubblewrap", target_os = "linux"))] - { - matches!(mode, BashExecutionMode::LinuxBwrap(_)) - } - - #[cfg(not(all(feature = "linux-bubblewrap", target_os = "linux")))] - { - let _ = mode; - false - } + #[cfg(all(feature = "linux-bubblewrap", target_os = "linux"))] + return matches!(mode, BashExecutionMode::LinuxBwrap(_)); + #[cfg(not(all(feature = "linux-bubblewrap", target_os = "linux")))] + { let _ = mode; false } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/llm-coding-tools-serdesai/src/bash.rs` around lines 203 - 233, Simplify the dual #[cfg] blocks by making each function return a single expression that uses cfg!(all(feature = "linux-bubblewrap", target_os = "linux")): when true evaluate the matches! check against BashExecutionMode (for bash_prompt_network_disabled check BashExecutionMode::LinuxBwrap(config) and NetworkPolicy::Disabled; for bash_prompt_sandboxed check BashExecutionMode::LinuxBwrap(_)), otherwise return false; update bash_prompt_network_disabled and bash_prompt_sandboxed to use this single-expression approach so the non-Linux branch no longer needs its own #[cfg] block and the mode parameter can be ignored only in the else path.src/llm-coding-tools-bubblewrap/src/profile/presets.rs (1)
154-181:inherited_pathperforms defensive filtering.The function properly:
- Falls back to
DEFAULT_SANDBOX_PATHwhen host PATH is unset- Normalizes paths before checking
- Deduplicates entries
- Falls back again if all entries are filtered
One minor observation: the duplicate check at lines 170-172 uses linear search. For typical PATH lengths this is fine, but for very long PATHs a
HashSetwould be more efficient.♻️ Optional: Use HashSet for deduplication on large PATHs
+use std::collections::HashSet; fn inherited_path(preset: Preset) -> String { let Some(path) = std::env::var_os("PATH") else { return DEFAULT_SANDBOX_PATH.to_string(); }; - let mut entries = Vec::new(); + let mut seen = HashSet::new(); + let mut entries = Vec::new(); for entry in std::env::split_paths(&path) { let entry = normalize_path(&entry); if !path_entry_allowed(preset, &entry) { continue; } let value = entry.to_string_lossy(); if value.is_empty() { continue; } let value = value.into_owned(); - if entries.iter().any(|existing| existing == &value) { + if !seen.insert(value.clone()) { continue; } entries.push(value); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/llm-coding-tools-bubblewrap/src/profile/presets.rs` around lines 154 - 181, The duplicate-check in inherited_path currently uses entries.iter().any(...) which is O(n^2) for many PATH entries; replace this with a HashSet to track seen entries: create a HashSet<String> (e.g., seen) alongside entries, check seen.contains(&value) instead of iter().any(...), insert into seen when pushing to entries, and use entries.join(":") as before; update references in inherited_path to use the new seen set for deduplication while preserving normalization, filtering via path_entry_allowed, and the DEFAULT_SANDBOX_PATH fallbacks.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/rust.yml:
- Around line 159-162: Update the GitHub Actions step that runs the blocking
bubblewrap tests ("Test bubblewrap (Blocking)") so the cargo invocation disables
default features before enabling the blocking feature; specifically modify the
run command string used in that step (currently "cargo test -p
llm-coding-tools-bubblewrap --features blocking") to insert
"--no-default-features" immediately before "--features blocking" to ensure tokio
(a default) is not enabled; apply the same change to the other bubblewrap
blocking test steps that use the same run pattern.
In `@src/.cargo/verify.ps1`:
- Around line 67-68: The blocking test invokes Invoke-LoggedCommand for the
crate llm-coding-tools-bubblewrap with the "blocking" feature but does not
disable default features, so the default "tokio" feature remains enabled; update
the Invoke-LoggedCommand call that passes @("test", "-p",
"llm-coding-tools-bubblewrap", "--features", "blocking", "--quiet") to include
"--no-default-features" (i.e., add the flag to the argument array for that
Invoke-LoggedCommand) so the blocking feature is tested in isolation from the
default tokio feature.
- Line 89: The PowerShell command invocation is treating the + operator as a
literal token so @("doc") and @("package") are passed alone; fix the calls to
Invoke-LoggedCommand by forcing expression mode for array concatenation: wrap
the concatenation expressions that combine @("doc") with $docArgs and
@("package") with $pkgArgs in parentheses so the arrays are
evaluated/concatenated before being passed to Invoke-LoggedCommand (locate the
calls to Invoke-LoggedCommand that use @("doc") + $docArgs and @("package") +
$pkgArgs and change them to use parenthesized concatenation).
---
Outside diff comments:
In `@src/llm-coding-tools-core/src/tools/bash/blocking_impl.rs`:
- Around line 105-123: The blocking path can hang waiting on
stdout_thread/stderr_thread after a timeout because join() blocks if readers
never finish; update the timeout branch (where WaitOutcome::TimedOut is created
after child.kill()) to mirror tokio_impl's bounded grace behavior: after
attempting to kill the child (child.kill()), close the child's stdio pipes (take
and drop child.stdout/child.stderr / close underlying handles) to force readers
to see EOF, then wait a short bounded grace period for stdout_thread and
stderr_thread to join (e.g., spin/poll with a small sleep loop up to X ms), and
if they still haven't finished return TimedOut with the kill_error without
blocking forever; reference stdout_thread, stderr_thread, child.kill(), and
WaitOutcome::TimedOut when making the change.
---
Nitpick comments:
In `@src/llm-coding-tools-bubblewrap/src/profile/presets.rs`:
- Around line 154-181: The duplicate-check in inherited_path currently uses
entries.iter().any(...) which is O(n^2) for many PATH entries; replace this with
a HashSet to track seen entries: create a HashSet<String> (e.g., seen) alongside
entries, check seen.contains(&value) instead of iter().any(...), insert into
seen when pushing to entries, and use entries.join(":") as before; update
references in inherited_path to use the new seen set for deduplication while
preserving normalization, filtering via path_entry_allowed, and the
DEFAULT_SANDBOX_PATH fallbacks.
In `@src/llm-coding-tools-serdesai/examples/serdesai-sandboxed-bash.rs`:
- Around line 37-44: The default API key constant API_KEY_VALUE is an empty
string which makes get_api_key() silently fall back to an empty value and later
trigger the exit path; clarify intent by either replacing API_KEY_VALUE with a
descriptive placeholder (e.g. "<UNSET_API_KEY>") or adding a short doc comment
above API_KEY_VALUE and get_api_key() that explains the empty default and that
the program will exit later if no env var is set; ensure references to
API_KEY_NAME, API_KEY_VALUE, and get_api_key() are updated so readers understand
the fallback behavior.
In `@src/llm-coding-tools-serdesai/src/bash.rs`:
- Around line 203-233: Simplify the dual #[cfg] blocks by making each function
return a single expression that uses cfg!(all(feature = "linux-bubblewrap",
target_os = "linux")): when true evaluate the matches! check against
BashExecutionMode (for bash_prompt_network_disabled check
BashExecutionMode::LinuxBwrap(config) and NetworkPolicy::Disabled; for
bash_prompt_sandboxed check BashExecutionMode::LinuxBwrap(_)), otherwise return
false; update bash_prompt_network_disabled and bash_prompt_sandboxed to use this
single-expression approach so the non-Linux branch no longer needs its own
#[cfg] block and the mode parameter can be ignored only in the else path.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0eab1914-aefa-4d06-8229-d5ec829e2c2c
⛔ Files ignored due to path filters (1)
src/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (45)
.github/workflows/rust.ymlREADME.MDSANDBOX-PROFILES.mdsrc/.cargo/verify.ps1src/.cargo/verify.shsrc/Cargo.tomlsrc/llm-coding-tools-bubblewrap/ARCHITECTURE.mdsrc/llm-coding-tools-bubblewrap/Cargo.tomlsrc/llm-coding-tools-bubblewrap/README.mdsrc/llm-coding-tools-bubblewrap/src/error.rssrc/llm-coding-tools-bubblewrap/src/lib.rssrc/llm-coding-tools-bubblewrap/src/path_util.rssrc/llm-coding-tools-bubblewrap/src/probe.rssrc/llm-coding-tools-bubblewrap/src/profile/builder.rssrc/llm-coding-tools-bubblewrap/src/profile/layout.rssrc/llm-coding-tools-bubblewrap/src/profile/mod.rssrc/llm-coding-tools-bubblewrap/src/profile/presets.rssrc/llm-coding-tools-bubblewrap/src/profile/types.rssrc/llm-coding-tools-bubblewrap/src/profile/validation.rssrc/llm-coding-tools-bubblewrap/src/test_helpers.rssrc/llm-coding-tools-bubblewrap/src/wrap/blocking.rssrc/llm-coding-tools-bubblewrap/src/wrap/command.rssrc/llm-coding-tools-bubblewrap/src/wrap/mod.rssrc/llm-coding-tools-bubblewrap/src/wrap/tokio.rssrc/llm-coding-tools-core/Cargo.tomlsrc/llm-coding-tools-core/README.mdsrc/llm-coding-tools-core/examples/system_prompt/mock_tools.rssrc/llm-coding-tools-core/src/context/tool_prompt/mod.rssrc/llm-coding-tools-core/src/context/tool_prompt/tool_sections.rssrc/llm-coding-tools-core/src/lib.rssrc/llm-coding-tools-core/src/path/allowed.rssrc/llm-coding-tools-core/src/system_prompt.rssrc/llm-coding-tools-core/src/tools/bash/blocking_impl.rssrc/llm-coding-tools-core/src/tools/bash/mod.rssrc/llm-coding-tools-core/src/tools/bash/tokio_impl.rssrc/llm-coding-tools-core/src/tools/mod.rssrc/llm-coding-tools-serdesai/Cargo.tomlsrc/llm-coding-tools-serdesai/README.mdsrc/llm-coding-tools-serdesai/examples/serdesai-agents.rssrc/llm-coding-tools-serdesai/examples/serdesai-basic.rssrc/llm-coding-tools-serdesai/examples/serdesai-sandboxed-bash.rssrc/llm-coding-tools-serdesai/examples/serdesai-sandboxed.rssrc/llm-coding-tools-serdesai/examples/serdesai-task.rssrc/llm-coding-tools-serdesai/src/bash.rssrc/llm-coding-tools-serdesai/src/lib.rs
✅ Files skipped from review due to trivial changes (13)
- src/llm-coding-tools-serdesai/examples/serdesai-agents.rs
- src/llm-coding-tools-core/src/path/allowed.rs
- src/Cargo.toml
- src/llm-coding-tools-serdesai/examples/serdesai-task.rs
- src/llm-coding-tools-core/src/context/tool_prompt/tool_sections.rs
- README.MD
- src/llm-coding-tools-bubblewrap/Cargo.toml
- src/llm-coding-tools-bubblewrap/src/error.rs
- src/llm-coding-tools-bubblewrap/src/wrap/mod.rs
- src/llm-coding-tools-bubblewrap/src/lib.rs
- src/llm-coding-tools-bubblewrap/README.md
- SANDBOX-PROFILES.md
- src/llm-coding-tools-bubblewrap/src/test_helpers.rs
🚧 Files skipped from review as they are similar to previous changes (17)
- src/llm-coding-tools-serdesai/examples/serdesai-sandboxed.rs
- src/llm-coding-tools-bubblewrap/src/path_util.rs
- src/llm-coding-tools-core/src/system_prompt.rs
- src/llm-coding-tools-core/README.md
- src/llm-coding-tools-core/src/lib.rs
- src/llm-coding-tools-serdesai/src/lib.rs
- src/llm-coding-tools-bubblewrap/src/profile/mod.rs
- src/llm-coding-tools-serdesai/README.md
- src/llm-coding-tools-serdesai/Cargo.toml
- src/llm-coding-tools-bubblewrap/ARCHITECTURE.md
- src/.cargo/verify.sh
- src/llm-coding-tools-core/Cargo.toml
- src/llm-coding-tools-core/src/context/tool_prompt/mod.rs
- src/llm-coding-tools-bubblewrap/src/profile/validation.rs
- src/llm-coding-tools-bubblewrap/src/profile/layout.rs
- src/llm-coding-tools-bubblewrap/src/profile/types.rs
- src/llm-coding-tools-bubblewrap/src/probe.rs
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Build and Test (Blocking) (windows-latest, x86_64-pc-windows-msvc)
- GitHub Check: Build and Test (Async/Tokio) (windows-latest, x86_64-pc-windows-msvc)
- GitHub Check: Build and Test (Async/Tokio) (ubuntu-latest, x86_64-unknown-linux-gnu)
- GitHub Check: Build and Test (Async/Tokio) (macos-latest, aarch64-apple-darwin)
🧰 Additional context used
📓 Path-based instructions (1)
src/**/*.rs
📄 CodeRabbit inference engine (src/AGENTS.md)
src/**/*.rs: Preallocate collections when size is known or estimable usingString::with_capacity(estimated_len),Vec::with_capacity(count), orBufReader::with_capacity(size, reader)
Prefer&str/&[T]returns over owned types when lifetime allows
UseCow<'_, str>for conditional ownership (e.g.,String::from_utf8_lossy)
Use&'static strfor compile-time constant strings
Reuse buffers by using.clear()and reusingVec/Stringinstead of reallocating
Use const generics for compile-time branching (e.g.,<const LINE_NUMBERS: bool>)
Use#[inline]on small, hot-path functions
Prefercoreoverstdwhere possible (e.g.,core::memoverstd::mem)
Stream data instead of loading entire files when possible
Usememchrfor fast byte searching over manual iteration
Keep modules under 500 lines (excluding tests); split if larger
Placeusestatements inside functions only for#[cfg]conditional compilation
Document public items with///and add examples in docs where helpful
Use//!for module-level documentation
Focus comments on 'why' not 'what'
Use [TypeName] rustdoc links in documentation, not backticks
Files:
src/llm-coding-tools-core/examples/system_prompt/mock_tools.rssrc/llm-coding-tools-serdesai/examples/serdesai-basic.rssrc/llm-coding-tools-core/src/tools/mod.rssrc/llm-coding-tools-bubblewrap/src/wrap/tokio.rssrc/llm-coding-tools-bubblewrap/src/wrap/blocking.rssrc/llm-coding-tools-core/src/tools/bash/blocking_impl.rssrc/llm-coding-tools-bubblewrap/src/wrap/command.rssrc/llm-coding-tools-serdesai/src/bash.rssrc/llm-coding-tools-core/src/tools/bash/tokio_impl.rssrc/llm-coding-tools-bubblewrap/src/profile/presets.rssrc/llm-coding-tools-serdesai/examples/serdesai-sandboxed-bash.rssrc/llm-coding-tools-core/src/tools/bash/mod.rssrc/llm-coding-tools-bubblewrap/src/profile/builder.rs
🧠 Learnings (3)
📚 Learning: 2026-03-17T21:06:53.934Z
Learnt from: CR
Repo: Sewer56/llm-coding-tools PR: 0
File: src/AGENTS.md:0-0
Timestamp: 2026-03-17T21:06:53.934Z
Learning: Applies to src/**/*.rs : Document public items with `///` and add examples in docs where helpful
Applied to files:
src/llm-coding-tools-serdesai/examples/serdesai-basic.rssrc/llm-coding-tools-serdesai/examples/serdesai-sandboxed-bash.rs
📚 Learning: 2026-03-17T21:06:53.934Z
Learnt from: CR
Repo: Sewer56/llm-coding-tools PR: 0
File: src/AGENTS.md:0-0
Timestamp: 2026-03-17T21:06:53.934Z
Learning: Applies to src/**/Cargo.toml : Keep dependency footprint minimal
Applied to files:
.github/workflows/rust.yml
📚 Learning: 2026-03-17T21:06:53.934Z
Learnt from: CR
Repo: Sewer56/llm-coding-tools PR: 0
File: src/AGENTS.md:0-0
Timestamp: 2026-03-17T21:06:53.934Z
Learning: After making changes to source code, always run `.cargo/verify.sh` (`.cargo/verify.ps1` on Windows) before returning to the user
Applied to files:
.github/workflows/rust.ymlsrc/.cargo/verify.ps1
🪛 PSScriptAnalyzer (1.25.0)
src/.cargo/verify.ps1
[warning] 38-38: File 'verify.ps1' uses Write-Host. Avoid using Write-Host because it might not work in all hosts, does not work when there is no host, and (prior to PS 5.0) cannot be suppressed, captured, or redirected. Instead, use Write-Output, Write-Verbose, or Write-Information.
(PSAvoidUsingWriteHost)
[warning] 47-47: File 'verify.ps1' uses Write-Host. Avoid using Write-Host because it might not work in all hosts, does not work when there is no host, and (prior to PS 5.0) cannot be suppressed, captured, or redirected. Instead, use Write-Output, Write-Verbose, or Write-Information.
(PSAvoidUsingWriteHost)
[warning] 56-56: File 'verify.ps1' uses Write-Host. Avoid using Write-Host because it might not work in all hosts, does not work when there is no host, and (prior to PS 5.0) cannot be suppressed, captured, or redirected. Instead, use Write-Output, Write-Verbose, or Write-Information.
(PSAvoidUsingWriteHost)
[warning] 65-65: File 'verify.ps1' uses Write-Host. Avoid using Write-Host because it might not work in all hosts, does not work when there is no host, and (prior to PS 5.0) cannot be suppressed, captured, or redirected. Instead, use Write-Output, Write-Verbose, or Write-Information.
(PSAvoidUsingWriteHost)
[warning] 73-73: File 'verify.ps1' uses Write-Host. Avoid using Write-Host because it might not work in all hosts, does not work when there is no host, and (prior to PS 5.0) cannot be suppressed, captured, or redirected. Instead, use Write-Output, Write-Verbose, or Write-Information.
(PSAvoidUsingWriteHost)
[warning] 76-76: File 'verify.ps1' uses Write-Host. Avoid using Write-Host because it might not work in all hosts, does not work when there is no host, and (prior to PS 5.0) cannot be suppressed, captured, or redirected. Instead, use Write-Output, Write-Verbose, or Write-Information.
(PSAvoidUsingWriteHost)
[warning] 80-80: File 'verify.ps1' uses Write-Host. Avoid using Write-Host because it might not work in all hosts, does not work when there is no host, and (prior to PS 5.0) cannot be suppressed, captured, or redirected. Instead, use Write-Output, Write-Verbose, or Write-Information.
(PSAvoidUsingWriteHost)
[warning] 94-94: File 'verify.ps1' uses Write-Host. Avoid using Write-Host because it might not work in all hosts, does not work when there is no host, and (prior to PS 5.0) cannot be suppressed, captured, or redirected. Instead, use Write-Output, Write-Verbose, or Write-Information.
(PSAvoidUsingWriteHost)
[warning] 97-97: File 'verify.ps1' uses Write-Host. Avoid using Write-Host because it might not work in all hosts, does not work when there is no host, and (prior to PS 5.0) cannot be suppressed, captured, or redirected. Instead, use Write-Output, Write-Verbose, or Write-Information.
(PSAvoidUsingWriteHost)
[warning] 108-108: File 'verify.ps1' uses Write-Host. Avoid using Write-Host because it might not work in all hosts, does not work when there is no host, and (prior to PS 5.0) cannot be suppressed, captured, or redirected. Instead, use Write-Output, Write-Verbose, or Write-Information.
(PSAvoidUsingWriteHost)
[info] 89-89: Cmdlet 'Invoke-LoggedCommand' has positional parameter. Please use named parameters instead of positional parameters when calling a command.
(PSAvoidUsingPositionalParameters)
[info] 106-106: Cmdlet 'Invoke-LoggedCommand' has positional parameter. Please use named parameters instead of positional parameters when calling a command.
(PSAvoidUsingPositionalParameters)
🔇 Additional comments (28)
src/llm-coding-tools-core/examples/system_prompt/mock_tools.rs (1)
158-169: LGTM!The
MockBashToolcorrectly returns the updated struct variant matching the newToolPrompt::Bash { network_disabled, sandboxed }signature. Usingfalse, falseappropriately represents the backward-compatible default (non-sandboxed, network-enabled) behavior for example prompt previews.src/llm-coding-tools-serdesai/examples/serdesai-basic.rs (1)
51-51: Explicit host mode is clearer here.
BashTool::host()makes the example’s unsandboxed behavior obvious and lines up with the warning in the module docs.src/llm-coding-tools-core/src/tools/mod.rs (1)
17-19: LGTM.The Linux-only gate keeps the sandbox helper off unsupported targets while still exposing the mode-aware Bash API.
src/llm-coding-tools-bubblewrap/src/wrap/tokio.rs (1)
18-32: LGTM.This matches the blocking adapter’s stdio and process-group setup, so both execution paths get the same isolation and cleanup behavior.
src/llm-coding-tools-bubblewrap/src/wrap/blocking.rs (1)
18-32: LGTM.This stays in lockstep with the Tokio adapter, which keeps blocking and async sandbox startup behavior aligned.
src/llm-coding-tools-bubblewrap/src/wrap/command.rs (1)
268-283: Remove this comment — PATH is properly restored bySandboxFixture.The test creates a
SandboxFixturewhich holds aPathGuardthat captures the originalPATHat fixture creation and automatically restores it when the fixture is dropped at test end. The subsequentenv::set_varcall modifiesPATHduring the test, but cleanup happens whenfixturegoes out of scope. No additional guard is needed.src/llm-coding-tools-serdesai/src/bash.rs (4)
1-25: Well-structured conditional documentation.The module-level documentation uses
cfg_attrto conditionally include Linux-specific sandbox documentation only when the feature is enabled. This keeps the docs relevant to the current build configuration.
54-68: LGTM on the struct and Default impl.The explicit
modefield with a customDefaultimpl delegating tohost()maintains backward compatibility while enabling the new execution mode API.
111-137: Documentation forwith_linux_bwrapis now correct.The previous review flagged an incorrect
# Errorssection claiming this method returns an execution error. That has been appropriately removed — the current documentation correctly notes that failures surface when the tool executes a command, not when storing the mode.
359-383: Tests cover the new mode and context behavior.The added tests verify that
BashTool::new()defaults to host mode and that theToolContextcorrectly reportsnetwork_disabled: false, sandboxed: falsefor host execution.src/llm-coding-tools-serdesai/examples/serdesai-sandboxed-bash.rs (2)
75-87: LGTM on profile and tool construction.The example correctly demonstrates creating a
public_botprofile with a bind-mounted/tmpdirectory, then configuring aBashToolwith the sandbox profile, timeout, and default workdir.
119-133: Event handling is clean and demonstrates streaming usage.The match arms handle the key agent events appropriately, clearing the buffer on new requests and logging tool calls and responses with incrementing IDs.
src/llm-coding-tools-core/src/tools/bash/tokio_impl.rs (3)
91-126: Clean mode-aware routing with backward compatibility.The
execute_commandfunction now delegates toexecute_command_with_modewithHostmode, preserving the existing API. The mode routing correctly handles the Linux-bubblewrap case with proper error mapping.
128-188: Well-designed shared execution implementation.
run_wrapped_commandproperly handles:
- Concurrent pipe draining to prevent deadlocks on large output
- Timeout with proper process tree cleanup
- Grace period for pipe drain after timeout kill
- Buffered output preservation even on timeout
The
biasedselect is a good choice for consistent timeout behavior.
190-221: LGTM on host wrap construction.The
build_host_wrapfunction correctly:
- Validates workdir before command setup
- Uses platform-appropriate shell (
cmdon Windows,bashon Unix)- Wraps with
JobObjecton Windows andProcessGroup::leader()on Unix for proper process tree managementsrc/llm-coding-tools-bubblewrap/src/profile/presets.rs (3)
94-98: Previous issues have been addressed.The
/etc/shadowmasking now correctly useswith_file_overlaysto bind-mount/dev/nullover the file (rather than incorrectly using--tmpfson a file path), and/homeremains as the only tmpfs overlay for directory-level masking.
189-200: Relative PATH entries are now properly rejected.The
path_entry_allowedfunction forTrustedMaintenancenow includesentry.is_absolute()check (line 195), preventing relative paths like.orbinfrom bypassing the deny-list. ThePublicBotbranch implicitly rejects relative paths since they can't match absolute prefixes.
305-332: Tests useunsafeforenv::set_var— this is required on Rust 1.80+.The
unsafeblocks aroundenv::set_varare necessary since Rust 1.80 deprecated safe mutation of environment variables due to thread-safety concerns. The#[serial]attribute ensures these tests don't run concurrently.src/llm-coding-tools-core/src/tools/bash/mod.rs (4)
1-33: Comprehensive module documentation.The module docs clearly describe the public API, Linux sandbox options, and error conditions. The conditional
cfg_attrblocks keep Linux-specific docs visible only when relevant.
55-62:BashExecutionModeenum is well-designed.The enum uses
#[default]onHostfor backward compatibility, and theLinuxBwrapvariant is properly feature-gated. UsingArc<Profile>allows sharing profiles across tool instances efficiently.
107-126:validate_workdirprovides clear, actionable error messages.The function distinguishes between:
- Non-absolute paths
- Paths that exist but aren't directories
- Paths that don't exist
This helps users quickly diagnose configuration issues.
176-184: Error mapping is exhaustive.Per the context snippet from
src/llm-coding-tools-bubblewrap/src/error.rs,LinuxBwrapErrorhas exactly two variants:InvalidPathandExecution. The match is exhaustive and preserves error semantics correctly.src/llm-coding-tools-bubblewrap/src/profile/builder.rs (6)
25-75: Excellent documentation with examples.The struct-level docs include
no_runexamples showing both baseline and preset builder usage. The notes section clarifies the role of each parameter.
201-233:build()follows a clear validation-then-construct pattern.The method:
- Ensures cache-root subdirs exist
- Validates all builder fields
- Resolves bwrap backend
- Resolves visible shell
- Precomputes static args
- Constructs the final
ProfileThis ordering ensures all validation happens before any expensive operations.
426-507: Capacity calculation matches actual arg count.The
arg_capacity_forfunction carefully accounts for:
- Fixed slots (die-with-parent, new-session, dev, proc, etc.)
- Conditional slots (clearenv, unshare-net)
- Per-env-var slots (3 each: --setenv, name, value)
- Various mount types with their slot counts
The tests at lines 646-697 verify that
arg_capacity_forequalsargs.len()for different configurations, catching any drift.
502-506: Verifyfixed_slotscount matches actual fixed arguments.The
fixed_slotsvalue of12accounts for specific bwrap flags. Let me trace throughbuild_static_args:
--die-with-parent,--new-session= 2--dev,/dev= 2--proc,/proc= 2- Synthetic home bind = 3
- Workspace bind = 3
That's 12, but there's also
--unshare-net(conditional) and--clearenv(conditional) which are already accounted for separately. The/tmpmount is intmp_slots. This looks correct.
734-749: Test now uses a guaranteed non-existent workspace path.The previous review flagged that shared paths like
/tmp/workspacemight exist. The current implementation creates aTempDir, sets up validhomeandcachedirectories within it, then references aworkspace_does_not_existpath that is guaranteed not to exist (since it's never created).
363-389: Credential file validation is thorough.The validation:
- Checks source and dest are absolute paths
- Verifies source exists and is readable
- Confirms source is a regular file (not directory/symlink)
- Ensures dest is within allowed sandbox paths (synthetic home, workspace, or cache root)
This prevents credentials from being exposed outside the intended sandbox boundaries.
Sewer56
force-pushed
the
bwrap-support-new
branch
from
244d5f1 to
b6b5677
Compare
Introduce `llm-coding-tools-bubblewrap`, a new Linux-only crate that builds sandbox profiles, probes the host for a working `bwrap` binary, and produces wrapped command lines that confine shell execution to an isolated filesystem and network namespace. Two preset profiles are provided: - **Public Bot** - strictest containment for untrusted input. Network disabled, minimal read-only system mounts, synthetic home, memory-backed `/tmp`, cleared environment. - **Trusted Maintenance** - broader defaults for trusted CI/CD and build work. Network enabled, full host `/` read-only, disk-backed `/tmp`, credential file mounts. The `linux-bubblewrap` feature flag wires the new crate into `llm-coding-tools-core` and `llm-coding-tools-serdesai`: - `BashExecutionMode` enum selects host or sandboxed execution. - `execute_command_with_mode` routes through the chosen backend. - `BashTool::host()` and `BashTool::with_linux_bwrap(profile)` replace the old `BashTool::new()` default. - The system prompt conditionally notes sandboxing and network status. - A `serdesai-sandboxed-bash` example demonstrates the full flow. Other changes: - `SANDBOX-PROFILES.md` - operator guide, security model, and pre-deployment checklist for the two profiles. - CI workflow and `verify.sh`/`verify.ps1` updated to build, test, lint, and docs-check the new crate on Linux; skipped on other platforms. - `README.md` and per-crate READMEs updated with sandboxing docs, feature flags, and updated examples. - Existing `BashTool::new()` preserved as a backward-compatible alias for `BashTool::host()`.
…ind-mount `--tmpfs` only works on directories, so mounting it on a file path like `/etc/shadow` caused a runtime crash. Introduce a `FileOverlay` type that bind-mounts a host file (e.g. `/dev/null`) over any sandbox path, and use it to mask `/etc/shadow` in the TrustedMaintenance preset.
Sewer56
force-pushed
the
bwrap-support-new
branch
from
b6b5677 to
3333ff6
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (6)
src/.cargo/verify.ps1 (3)
89-89:⚠️ Potential issue | 🟠 MajorArray concatenation not evaluated in command mode.
In PowerShell command mode, the
+operator is treated as a literal argument, not an operator. Only@("doc")is passed to the function while+ $docArgsare passed as separate literal arguments. This causescargo docto run without the critical flags like--workspace,--document-private-items,--no-deps, and--quiet.Wrap the concatenation in parentheses to force expression mode evaluation.
🐛 Proposed fix
- Invoke-LoggedCommand "cargo" @("doc") + $docArgs + Invoke-LoggedCommand "cargo" (@("doc") + $docArgs)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/.cargo/verify.ps1` at line 89, The command invocation uses Invoke-LoggedCommand with @("doc") + $docArgs but in PowerShell command mode the + is treated as a literal; change the call so the array concatenation is evaluated by wrapping the expression in parentheses (e.g., Invoke-LoggedCommand "cargo" ( @("doc") + $docArgs )) so that $docArgs is correctly appended and flags like --workspace/--document-private-items/--no-deps/--quiet are passed to cargo; update the invocation that references Invoke-LoggedCommand, @("doc"), and $docArgs accordingly.
67-68:⚠️ Potential issue | 🟠 MajorAdd
--no-default-featuresto properly isolate blocking feature tests.The blocking test on line 68 still enables
tokioby default. Sincedefault = ["tokio"]inllm-coding-tools-bubblewrap, running--features blockingwithout--no-default-featuresattempts to compile with both features enabled, which are mutually exclusive.🐛 Proposed fix
- Invoke-LoggedCommand "cargo" @("test", "-p", "llm-coding-tools-bubblewrap", "--features", "tokio", "--quiet") - Invoke-LoggedCommand "cargo" @("test", "-p", "llm-coding-tools-bubblewrap", "--features", "blocking", "--quiet") + Invoke-LoggedCommand "cargo" @("test", "-p", "llm-coding-tools-bubblewrap", "--no-default-features", "--features", "tokio", "--quiet") + Invoke-LoggedCommand "cargo" @("test", "-p", "llm-coding-tools-bubblewrap", "--no-default-features", "--features", "blocking", "--quiet")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/.cargo/verify.ps1` around lines 67 - 68, The blocking-feature test currently runs Invoke-LoggedCommand for package llm-coding-tools-bubblewrap with "--features", "blocking" but does not disable defaults (default = ["tokio"]), causing tokio and blocking to be enabled together; update the second Invoke-LoggedCommand invocation that runs cargo test for llm-coding-tools-bubblewrap with "--features", "blocking" to also include "--no-default-features" so the blocking feature is tested in isolation.
106-106:⚠️ Potential issue | 🟠 MajorSame array concatenation issue for cargo package.
Same issue as line 89—the
+operator is not evaluated in command mode, so$pkgArgsis not concatenated with the array.🐛 Proposed fix
- Invoke-LoggedCommand "cargo" @("package") + $pkgArgs + Invoke-LoggedCommand "cargo" (@("package") + $pkgArgs)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/.cargo/verify.ps1` at line 106, The Invoke-LoggedCommand call is trying to concatenate @("package") and $pkgArgs using + in command mode which does not evaluate, so $pkgArgs is not passed; fix by pre-building the argument array (combine @("package") with $pkgArgs into a single array variable) and then call Invoke-LoggedCommand "cargo" with that combined array variable instead of using + inline; update the Invoke-LoggedCommand invocation and references to @("package") and $pkgArgs accordingly (same pattern used to fix the earlier occurrence around the other cargo invocation)..github/workflows/rust.yml (3)
164-168:⚠️ Potential issue | 🟠 MajorAdd
--no-default-featuresto blocking test for proper feature isolation.Line 168 runs
cargo test -p llm-coding-tools-bubblewrap --features blockingwithout--no-default-features. Since the crate's default includestokio, both features will be enabled simultaneously, which are mutually exclusive and may cause compilation conflicts.🐛 Proposed fix
- name: Test bubblewrap (Blocking) if: matrix.os == 'ubuntu-latest' working-directory: src - run: cargo test -p llm-coding-tools-bubblewrap --features blocking + run: cargo test -p llm-coding-tools-bubblewrap --no-default-features --features blocking🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/rust.yml around lines 164 - 168, The blocking test invocation currently runs "cargo test -p llm-coding-tools-bubblewrap --features blocking" which enables default features (including tokio) and conflicts with the blocking feature; update that command to pass --no-default-features so only the blocking feature is enabled (i.e., run cargo test -p llm-coding-tools-bubblewrap --no-default-features --features blocking) to enforce feature isolation for the blocking test.
190-192:⚠️ Potential issue | 🟠 MajorAdd
--no-default-featuresto blocking clippy.Line 191's
cargo clippyfor bubblewrap uses--features blockingwithout--no-default-features, inconsistent with lines 188-189 which correctly use--no-default-featuresfor the other crates.🐛 Proposed fix
if [ "${{ matrix.os }}" = "ubuntu-latest" ]; then - cargo clippy -p llm-coding-tools-bubblewrap --features blocking --target ${{ matrix.target }} -- -D warnings + cargo clippy -p llm-coding-tools-bubblewrap --no-default-features --features blocking --target ${{ matrix.target }} -- -D warnings fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/rust.yml around lines 190 - 192, The cargo clippy invocation for the llm-coding-tools-bubblewrap job uses --features blocking but is missing --no-default-features; update the clippy command that runs when matrix.os is ubuntu-latest (the line invoking cargo clippy -p llm-coding-tools-bubblewrap --features blocking --target ${{ matrix.target }} -- -D warnings) to include --no-default-features so it matches the other crates' invocations and disables defaults while enabling the blocking feature.
179-181:⚠️ Potential issue | 🟠 MajorAdd
--no-default-featuresto blocking doc generation.Line 180's
cargo docfor bubblewrap uses--features blockingwithout--no-default-features, inconsistent with lines 177-178 which correctly use--no-default-featuresforllm-coding-tools-coreandllm-coding-tools-models-dev.🐛 Proposed fix
if [ "${{ matrix.os }}" = "ubuntu-latest" ]; then - cargo doc -p llm-coding-tools-bubblewrap --features blocking --document-private-items --no-deps --target ${{ matrix.target }} + cargo doc -p llm-coding-tools-bubblewrap --no-default-features --features blocking --document-private-items --no-deps --target ${{ matrix.target }} fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/rust.yml around lines 179 - 181, The cargo doc invocation for llm-coding-tools-bubblewrap currently uses --features blocking but omits --no-default-features; update the cargo doc command that targets llm-coding-tools-bubblewrap in the CI workflow to include --no-default-features alongside --features blocking so it matches the llm-coding-tools-core and llm-coding-tools-models-dev invocations and builds docs with only the specified blocking feature.
🧹 Nitpick comments (1)
src/llm-coding-tools-bubblewrap/src/profile/presets.rs (1)
209-215: Preallocate the fixed-size preset buffers.Both loops have a known upper bound, so
Vec::with_capacity(...)can avoid needless growth during preset construction.As per coding guidelines, Preallocate collections when size is known or estimable using `String::with_capacity(estimated_len)`, `Vec::with_capacity(count)`, or `BufReader::with_capacity(size, reader)`.♻️ Proposed refactor
fn public_bot_read_only_mounts() -> Arc<[Box<Path>]> { - let mut mounts = Vec::new(); + let mut mounts = Vec::with_capacity(PUBLIC_BOT_PREFIXES.len()); for path in PUBLIC_BOT_PREFIXES { let path = PathBuf::from(path); if path.exists() { mounts.push(path.into_boxed_path()); } }fn public_bot_compat_symlinks() -> Arc<[Symlink]> { - let mut symlinks = Vec::new(); + let mut symlinks = Vec::with_capacity(3); for (target, link_path, required_path) in [ ("usr/bin", "/bin", "/usr/bin"), ("usr/lib", "/lib", "/usr/lib"), ("usr/sbin", "/sbin", "/usr/sbin"),Also applies to: 233-245
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/llm-coding-tools-bubblewrap/src/profile/presets.rs` around lines 209 - 215, The preset-construction loops (notably in public_bot_read_only_mounts) create Vecs without preallocating despite a known upper bound; change Vec::new() to Vec::with_capacity(PUBLIC_BOT_PREFIXES.len()) in public_bot_read_only_mounts and do the analogous with_capacity(...) in the other preset-construction loop in this module that builds mounts (the sibling mount constructor later in the file) so the vectors are preallocated to the known max size before pushing PathBufs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/.cargo/verify.sh`:
- Around line 59-60: The blocking-mode test enables both default tokio and
blocking features; update the second cargo test invocation that targets
llm-coding-tools-bubblewrap (the run_cmd line that uses --features blocking) to
add --no-default-features so it runs with only the blocking feature (i.e.,
change the test command for llm-coding-tools-bubblewrap --features blocking to
include --no-default-features to isolate the blocking feature from the default
tokio feature).
In `@src/llm-coding-tools-bubblewrap/src/profile/presets.rs`:
- Around line 95-98: The preset uses FileOverlay::new with a character device
(currently "/dev/null"), but types.rs documents overlay sources must be regular
files; update the preset in presets.rs so FileOverlay::new is given a regular
file source (e.g., an actual file path or temporary empty file) or, if the
intent is to allow device special files, update the FileOverlay validation and
the documented contract in types.rs to accept character devices and adjust
trusted_maintenance() accordingly so the runtime and API contract remain
consistent. Ensure you change either the caller (presets.rs) or the
contract/validation (types.rs and trusted_maintenance())—not both
inconsistently—so tests/validation will pass.
- Around line 217-223: The fallback branch wrongly re-adds the same
PUBLIC_BOT_PREFIXES that were just checked and found absent; instead, when
mounts.is_empty() use a true fallback such as mounting the filesystem root or a
safer, existing path. Replace the three
Box::from(Path::new("/usr/bin"))/("/usr/lib")/("/lib64") entries with a single
Box::from(Path::new("/")) (or another validated existing path) so the mounts
fallback provides a real, present mount point; update the logic around the
mounts variable in presets.rs where mounts.is_empty() is checked.
In `@src/llm-coding-tools-core/src/context/tool_prompt/mod.rs`:
- Around line 38-41: The public enum variant ToolPrompt::Bash exposes two new
public fields network_disabled and sandboxed but lacks documentation; add
triple-slash doc comments (///) above each field in the Bash variant that
briefly describe their semantics (e.g., network_disabled: whether the bash
execution must be run without network access; sandboxed: whether the execution
must be confined to a sandbox/limited filesystem and resources), mention
default/expected behavior and any implications for implementers, and include a
short usage example or note if both can be combined or are mutually exclusive to
help downstream users.
In `@src/llm-coding-tools-serdesai/README.md`:
- Around line 93-96: Rewrite the two profile descriptions to be clearer and
grammatically simple: for "Public Bot" replace the sentence with a short,
unambiguous line such as "Public Bot: accessible to anyone; strict
containment—no host filesystem access, a synthetic home, memory-backed /tmp,
network disabled, and a sanitized PATH." For "Trusted Maintenance" replace with
a short line like "Trusted Maintenance: for trusted operators; read-only host
root with writable overlays, disk-backed /tmp, sanitized PATH, and network
enabled." Update the corresponding headings "Public Bot" and "Trusted
Maintenance" in the README to use these concise, parallel bullets so the
security implications are easy to parse.
---
Duplicate comments:
In @.github/workflows/rust.yml:
- Around line 164-168: The blocking test invocation currently runs "cargo test
-p llm-coding-tools-bubblewrap --features blocking" which enables default
features (including tokio) and conflicts with the blocking feature; update that
command to pass --no-default-features so only the blocking feature is enabled
(i.e., run cargo test -p llm-coding-tools-bubblewrap --no-default-features
--features blocking) to enforce feature isolation for the blocking test.
- Around line 190-192: The cargo clippy invocation for the
llm-coding-tools-bubblewrap job uses --features blocking but is missing
--no-default-features; update the clippy command that runs when matrix.os is
ubuntu-latest (the line invoking cargo clippy -p llm-coding-tools-bubblewrap
--features blocking --target ${{ matrix.target }} -- -D warnings) to include
--no-default-features so it matches the other crates' invocations and disables
defaults while enabling the blocking feature.
- Around line 179-181: The cargo doc invocation for llm-coding-tools-bubblewrap
currently uses --features blocking but omits --no-default-features; update the
cargo doc command that targets llm-coding-tools-bubblewrap in the CI workflow to
include --no-default-features alongside --features blocking so it matches the
llm-coding-tools-core and llm-coding-tools-models-dev invocations and builds
docs with only the specified blocking feature.
In `@src/.cargo/verify.ps1`:
- Line 89: The command invocation uses Invoke-LoggedCommand with @("doc") +
$docArgs but in PowerShell command mode the + is treated as a literal; change
the call so the array concatenation is evaluated by wrapping the expression in
parentheses (e.g., Invoke-LoggedCommand "cargo" ( @("doc") + $docArgs )) so that
$docArgs is correctly appended and flags like
--workspace/--document-private-items/--no-deps/--quiet are passed to cargo;
update the invocation that references Invoke-LoggedCommand, @("doc"), and
$docArgs accordingly.
- Around line 67-68: The blocking-feature test currently runs
Invoke-LoggedCommand for package llm-coding-tools-bubblewrap with "--features",
"blocking" but does not disable defaults (default = ["tokio"]), causing tokio
and blocking to be enabled together; update the second Invoke-LoggedCommand
invocation that runs cargo test for llm-coding-tools-bubblewrap with
"--features", "blocking" to also include "--no-default-features" so the blocking
feature is tested in isolation.
- Line 106: The Invoke-LoggedCommand call is trying to concatenate @("package")
and $pkgArgs using + in command mode which does not evaluate, so $pkgArgs is not
passed; fix by pre-building the argument array (combine @("package") with
$pkgArgs into a single array variable) and then call Invoke-LoggedCommand
"cargo" with that combined array variable instead of using + inline; update the
Invoke-LoggedCommand invocation and references to @("package") and $pkgArgs
accordingly (same pattern used to fix the earlier occurrence around the other
cargo invocation).
---
Nitpick comments:
In `@src/llm-coding-tools-bubblewrap/src/profile/presets.rs`:
- Around line 209-215: The preset-construction loops (notably in
public_bot_read_only_mounts) create Vecs without preallocating despite a known
upper bound; change Vec::new() to Vec::with_capacity(PUBLIC_BOT_PREFIXES.len())
in public_bot_read_only_mounts and do the analogous with_capacity(...) in the
other preset-construction loop in this module that builds mounts (the sibling
mount constructor later in the file) so the vectors are preallocated to the
known max size before pushing PathBufs.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5e10b793-d54f-4cc4-8d5a-b4a438c98826
⛔ Files ignored due to path filters (1)
src/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (45)
.github/workflows/rust.ymlREADME.MDSANDBOX-PROFILES.mdsrc/.cargo/verify.ps1src/.cargo/verify.shsrc/Cargo.tomlsrc/llm-coding-tools-bubblewrap/ARCHITECTURE.mdsrc/llm-coding-tools-bubblewrap/Cargo.tomlsrc/llm-coding-tools-bubblewrap/README.mdsrc/llm-coding-tools-bubblewrap/src/error.rssrc/llm-coding-tools-bubblewrap/src/lib.rssrc/llm-coding-tools-bubblewrap/src/path_util.rssrc/llm-coding-tools-bubblewrap/src/probe.rssrc/llm-coding-tools-bubblewrap/src/profile/builder.rssrc/llm-coding-tools-bubblewrap/src/profile/layout.rssrc/llm-coding-tools-bubblewrap/src/profile/mod.rssrc/llm-coding-tools-bubblewrap/src/profile/presets.rssrc/llm-coding-tools-bubblewrap/src/profile/types.rssrc/llm-coding-tools-bubblewrap/src/profile/validation.rssrc/llm-coding-tools-bubblewrap/src/test_helpers.rssrc/llm-coding-tools-bubblewrap/src/wrap/blocking.rssrc/llm-coding-tools-bubblewrap/src/wrap/command.rssrc/llm-coding-tools-bubblewrap/src/wrap/mod.rssrc/llm-coding-tools-bubblewrap/src/wrap/tokio.rssrc/llm-coding-tools-core/Cargo.tomlsrc/llm-coding-tools-core/README.mdsrc/llm-coding-tools-core/examples/system_prompt/mock_tools.rssrc/llm-coding-tools-core/src/context/tool_prompt/mod.rssrc/llm-coding-tools-core/src/context/tool_prompt/tool_sections.rssrc/llm-coding-tools-core/src/lib.rssrc/llm-coding-tools-core/src/path/allowed.rssrc/llm-coding-tools-core/src/system_prompt.rssrc/llm-coding-tools-core/src/tools/bash/blocking_impl.rssrc/llm-coding-tools-core/src/tools/bash/mod.rssrc/llm-coding-tools-core/src/tools/bash/tokio_impl.rssrc/llm-coding-tools-core/src/tools/mod.rssrc/llm-coding-tools-serdesai/Cargo.tomlsrc/llm-coding-tools-serdesai/README.mdsrc/llm-coding-tools-serdesai/examples/serdesai-agents.rssrc/llm-coding-tools-serdesai/examples/serdesai-basic.rssrc/llm-coding-tools-serdesai/examples/serdesai-sandboxed-bash.rssrc/llm-coding-tools-serdesai/examples/serdesai-sandboxed.rssrc/llm-coding-tools-serdesai/examples/serdesai-task.rssrc/llm-coding-tools-serdesai/src/bash.rssrc/llm-coding-tools-serdesai/src/lib.rs
✅ Files skipped from review due to trivial changes (17)
- src/Cargo.toml
- src/llm-coding-tools-serdesai/examples/serdesai-agents.rs
- src/llm-coding-tools-serdesai/examples/serdesai-sandboxed.rs
- src/llm-coding-tools-serdesai/examples/serdesai-task.rs
- src/llm-coding-tools-bubblewrap/Cargo.toml
- src/llm-coding-tools-bubblewrap/src/error.rs
- src/llm-coding-tools-core/README.md
- src/llm-coding-tools-bubblewrap/src/lib.rs
- src/llm-coding-tools-bubblewrap/src/wrap/mod.rs
- src/llm-coding-tools-core/src/path/allowed.rs
- src/llm-coding-tools-bubblewrap/src/profile/mod.rs
- src/llm-coding-tools-bubblewrap/README.md
- src/llm-coding-tools-bubblewrap/ARCHITECTURE.md
- SANDBOX-PROFILES.md
- src/llm-coding-tools-serdesai/src/bash.rs
- src/llm-coding-tools-bubblewrap/src/probe.rs
- src/llm-coding-tools-bubblewrap/src/profile/validation.rs
🚧 Files skipped from review as they are similar to previous changes (16)
- src/llm-coding-tools-serdesai/examples/serdesai-basic.rs
- src/llm-coding-tools-bubblewrap/src/path_util.rs
- src/llm-coding-tools-serdesai/src/lib.rs
- src/llm-coding-tools-core/src/context/tool_prompt/tool_sections.rs
- src/llm-coding-tools-bubblewrap/src/wrap/tokio.rs
- src/llm-coding-tools-bubblewrap/src/wrap/blocking.rs
- src/llm-coding-tools-core/examples/system_prompt/mock_tools.rs
- src/llm-coding-tools-core/Cargo.toml
- README.MD
- src/llm-coding-tools-core/src/lib.rs
- src/llm-coding-tools-core/src/tools/bash/blocking_impl.rs
- src/llm-coding-tools-serdesai/Cargo.toml
- src/llm-coding-tools-serdesai/examples/serdesai-sandboxed-bash.rs
- src/llm-coding-tools-core/src/tools/bash/tokio_impl.rs
- src/llm-coding-tools-bubblewrap/src/test_helpers.rs
- src/llm-coding-tools-bubblewrap/src/profile/builder.rs
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Build and Test (Async/Tokio) (macos-latest, aarch64-apple-darwin)
- GitHub Check: Build and Test (Blocking) (macos-latest, aarch64-apple-darwin)
- GitHub Check: Build and Test (Blocking) (windows-latest, x86_64-pc-windows-msvc)
- GitHub Check: Build and Test (Async/Tokio) (ubuntu-latest, x86_64-unknown-linux-gnu)
- GitHub Check: Build and Test (Async/Tokio) (windows-latest, x86_64-pc-windows-msvc)
🧰 Additional context used
📓 Path-based instructions (1)
src/**/*.rs
📄 CodeRabbit inference engine (src/AGENTS.md)
src/**/*.rs: Preallocate collections when size is known or estimable usingString::with_capacity(estimated_len),Vec::with_capacity(count), orBufReader::with_capacity(size, reader)
Prefer&str/&[T]returns over owned types when lifetime allows
UseCow<'_, str>for conditional ownership (e.g.,String::from_utf8_lossy)
Use&'static strfor compile-time constant strings
Reuse buffers by using.clear()and reusingVec/Stringinstead of reallocating
Use const generics for compile-time branching (e.g.,<const LINE_NUMBERS: bool>)
Use#[inline]on small, hot-path functions
Prefercoreoverstdwhere possible (e.g.,core::memoverstd::mem)
Stream data instead of loading entire files when possible
Usememchrfor fast byte searching over manual iteration
Keep modules under 500 lines (excluding tests); split if larger
Placeusestatements inside functions only for#[cfg]conditional compilation
Document public items with///and add examples in docs where helpful
Use//!for module-level documentation
Focus comments on 'why' not 'what'
Use [TypeName] rustdoc links in documentation, not backticks
Files:
src/llm-coding-tools-core/src/context/tool_prompt/mod.rssrc/llm-coding-tools-core/src/tools/mod.rssrc/llm-coding-tools-core/src/system_prompt.rssrc/llm-coding-tools-core/src/tools/bash/mod.rssrc/llm-coding-tools-bubblewrap/src/profile/types.rssrc/llm-coding-tools-bubblewrap/src/wrap/command.rssrc/llm-coding-tools-bubblewrap/src/profile/presets.rssrc/llm-coding-tools-bubblewrap/src/profile/layout.rs
🧠 Learnings (4)
📚 Learning: 2026-03-17T21:06:53.934Z
Learnt from: CR
Repo: Sewer56/llm-coding-tools PR: 0
File: src/AGENTS.md:0-0
Timestamp: 2026-03-17T21:06:53.934Z
Learning: After making changes to source code, always run `.cargo/verify.sh` (`.cargo/verify.ps1` on Windows) before returning to the user
Applied to files:
src/.cargo/verify.ps1src/.cargo/verify.sh.github/workflows/rust.yml
📚 Learning: 2026-03-17T21:06:53.934Z
Learnt from: CR
Repo: Sewer56/llm-coding-tools PR: 0
File: src/AGENTS.md:0-0
Timestamp: 2026-03-17T21:06:53.934Z
Learning: Applies to src/**/Cargo.toml : Enable `tokio` feature (default) for async mode with tokio runtime, or `blocking` feature for sync/blocking mode. The `async` and `blocking` features are mutually exclusive - enabling both causes a compile error.
Applied to files:
src/.cargo/verify.ps1.github/workflows/rust.yml
📚 Learning: 2026-03-17T21:06:53.934Z
Learnt from: CR
Repo: Sewer56/llm-coding-tools PR: 0
File: src/AGENTS.md:0-0
Timestamp: 2026-03-17T21:06:53.934Z
Learning: Applies to src/**/Cargo.toml : Prefer performance-oriented crates such as `parking_lot` over `std::sync` and `memchr` for byte search
Applied to files:
.github/workflows/rust.yml
📚 Learning: 2026-03-17T21:06:53.934Z
Learnt from: CR
Repo: Sewer56/llm-coding-tools PR: 0
File: src/AGENTS.md:0-0
Timestamp: 2026-03-17T21:06:53.934Z
Learning: Applies to src/**/Cargo.toml : Keep dependency footprint minimal
Applied to files:
.github/workflows/rust.yml
🪛 PSScriptAnalyzer (1.25.0)
src/.cargo/verify.ps1
[warning] 38-38: File 'verify.ps1' uses Write-Host. Avoid using Write-Host because it might not work in all hosts, does not work when there is no host, and (prior to PS 5.0) cannot be suppressed, captured, or redirected. Instead, use Write-Output, Write-Verbose, or Write-Information.
(PSAvoidUsingWriteHost)
[warning] 47-47: File 'verify.ps1' uses Write-Host. Avoid using Write-Host because it might not work in all hosts, does not work when there is no host, and (prior to PS 5.0) cannot be suppressed, captured, or redirected. Instead, use Write-Output, Write-Verbose, or Write-Information.
(PSAvoidUsingWriteHost)
[warning] 56-56: File 'verify.ps1' uses Write-Host. Avoid using Write-Host because it might not work in all hosts, does not work when there is no host, and (prior to PS 5.0) cannot be suppressed, captured, or redirected. Instead, use Write-Output, Write-Verbose, or Write-Information.
(PSAvoidUsingWriteHost)
[warning] 65-65: File 'verify.ps1' uses Write-Host. Avoid using Write-Host because it might not work in all hosts, does not work when there is no host, and (prior to PS 5.0) cannot be suppressed, captured, or redirected. Instead, use Write-Output, Write-Verbose, or Write-Information.
(PSAvoidUsingWriteHost)
[warning] 73-73: File 'verify.ps1' uses Write-Host. Avoid using Write-Host because it might not work in all hosts, does not work when there is no host, and (prior to PS 5.0) cannot be suppressed, captured, or redirected. Instead, use Write-Output, Write-Verbose, or Write-Information.
(PSAvoidUsingWriteHost)
[warning] 76-76: File 'verify.ps1' uses Write-Host. Avoid using Write-Host because it might not work in all hosts, does not work when there is no host, and (prior to PS 5.0) cannot be suppressed, captured, or redirected. Instead, use Write-Output, Write-Verbose, or Write-Information.
(PSAvoidUsingWriteHost)
[warning] 80-80: File 'verify.ps1' uses Write-Host. Avoid using Write-Host because it might not work in all hosts, does not work when there is no host, and (prior to PS 5.0) cannot be suppressed, captured, or redirected. Instead, use Write-Output, Write-Verbose, or Write-Information.
(PSAvoidUsingWriteHost)
[warning] 94-94: File 'verify.ps1' uses Write-Host. Avoid using Write-Host because it might not work in all hosts, does not work when there is no host, and (prior to PS 5.0) cannot be suppressed, captured, or redirected. Instead, use Write-Output, Write-Verbose, or Write-Information.
(PSAvoidUsingWriteHost)
[warning] 97-97: File 'verify.ps1' uses Write-Host. Avoid using Write-Host because it might not work in all hosts, does not work when there is no host, and (prior to PS 5.0) cannot be suppressed, captured, or redirected. Instead, use Write-Output, Write-Verbose, or Write-Information.
(PSAvoidUsingWriteHost)
[warning] 108-108: File 'verify.ps1' uses Write-Host. Avoid using Write-Host because it might not work in all hosts, does not work when there is no host, and (prior to PS 5.0) cannot be suppressed, captured, or redirected. Instead, use Write-Output, Write-Verbose, or Write-Information.
(PSAvoidUsingWriteHost)
[info] 89-89: Cmdlet 'Invoke-LoggedCommand' has positional parameter. Please use named parameters instead of positional parameters when calling a command.
(PSAvoidUsingPositionalParameters)
[info] 106-106: Cmdlet 'Invoke-LoggedCommand' has positional parameter. Please use named parameters instead of positional parameters when calling a command.
(PSAvoidUsingPositionalParameters)
🔇 Additional comments (12)
src/llm-coding-tools-serdesai/README.md (3)
21-23: Quick Start API usage is aligned with current tool constructors.The updated snippet matches the current public API (
EditTool::new,BashTool::host,WebFetchTool::new) and avoids stale usage.Also applies to: 36-38
64-66: Tool variants example correctly reflectsallowed::EditToolconstruction.The alias-based
AllowedEditTool::new(resolver.clone())usage is accurate for the current constructor shape.Also applies to: 76-76
179-181: Good addition of a feature-flagged sandbox example and root project reference link.This makes discovery of
linux-bubblewrapusage much easier for users.Also applies to: 193-193
src/llm-coding-tools-core/src/context/tool_prompt/mod.rs (1)
120-120: Good adaptation for the new Bash variant shape.The
ToolPrompt::Bash { .. }pattern keepsToolPromptFactsbehavior intact while supporting the expanded variant payload.src/llm-coding-tools-core/src/system_prompt.rs (2)
468-475: Test fixture update looks correct.Using
ToolPrompt::Bash { network_disabled: false, sandboxed: false }inBuiltInBashToolaligns the fixture with the new API without changing intended behavior.
1198-1243: Strong regression coverage for conditional bash prompt lines.This test cleanly verifies both branches (
sandboxed + network_disabledvs host mode), including the single-occurrence check for the network-disabled line.src/.cargo/verify.ps1 (1)
38-108:Write-Hostusage is acceptable for this verification script.The static analysis warnings about
Write-Hostcan be safely ignored here. This is an interactive verification script where direct console output is the intended behavior, and the script is not designed to run in headless/non-host environments.src/.cargo/verify.sh (1)
73-90: LGTM!The conditional exclusion logic for documentation generation and package validation correctly handles the Linux-only
llm-coding-tools-bubblewrapcrate. TheDOC_ARGSandPKG_ARGSarrays properly add--excludeon non-Linux platforms..github/workflows/rust.yml (3)
43-47: LGTM!The Linux-gated bubblewrap test step correctly uses
matrix.os == 'ubuntu-latest'and explicitly specifies--features tokio. Whiletokiois the default feature, explicitly stating it is harmless and makes the intent clear.
85-96: LGTM!The semver checks for
llm-coding-tools-bubblewrapcorrectly use--only-explicit-featuresto avoid enabling mutually exclusive features simultaneously. The three separate runs (no features,tokio,blocking) properly cover the feature matrix.
214-218: LGTM!Adding
src/llm-coding-tools-bubblewrapto the publish paths is correct. Since thepublish-cratejob runs onubuntu-latest, the Linux-only crate will be published successfully alongside the other workspace crates.src/llm-coding-tools-bubblewrap/src/profile/layout.rs (1)
159-173: The review comment identifies correct code behavior—strip_prefixis indeed lexical and does not normalize..components—but mischaracterizes the threat model. Per the library design documented in learnings, theworkdirparameter passed towrap_commandcomes from trusted operator code (the library caller), not from untrusted input. Untrusted input (LLM-generated shell commands) enters only through the command string parameter, not through the path parameters. Path normalization and..-component hardening are therefore not required; the code correctly assumes that the operator provides valid paths.> Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/llm-coding-tools-core/src/tools/bash/tokio_impl.rs (1)
131-163:⚠️ Potential issue | 🟠 MajorA fast leader exit can still bypass the timeout guarantee.
Once
child.wait()resolves, this helper awaits both drain tasks without any bound. A command that backgrounds a child and exits, such assleep 100 &, keeps stdout/stderr open in the descendant, so this call can hang long aftertimeouthas elapsed. Consider using the same grace-period path here, or keeping the deadline alive until both pipes hit EOF.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/llm-coding-tools-core/src/tools/bash/tokio_impl.rs` around lines 131 - 163, The current run_wrapped_command can hang awaiting await_pipe_drain_task after child.wait() if a backgrounded descendant keeps pipes open; fix by bounding the drain wait with the same deadline/grace logic: after child.wait() (in run_wrapped_command) compute the remaining time (or use the configured grace period) and await await_pipe_drain_task(stdout_task) and await_pipe_drain_task(stderr_task) under a tokio::time::timeout; if the timeout expires, cancel or abort the drain tasks and collect whatever buffered output is available, then proceed to return the BashOutput/ToolResult. Reference run_wrapped_command, child.wait, spawn_pipe_drain_task, and await_pipe_drain_task when implementing the timed/abortable drain wait.
♻️ Duplicate comments (3)
src/.cargo/verify.sh (1)
59-60:⚠️ Potential issue | 🟠 MajorThe blocking bubblewrap feature check is still not isolated.
llm-coding-tools-bubblewrapdefaults totokio, so--features blockinghere ends up combining both runtimes. That means this block can miss the exact mutually-exclusive configuration failure it is supposed to catch.🐛 Suggested fix
- run_cmd cargo test -p llm-coding-tools-bubblewrap --features tokio --quiet - run_cmd cargo test -p llm-coding-tools-bubblewrap --features blocking --quiet + run_cmd cargo test -p llm-coding-tools-bubblewrap --no-default-features --features tokio --quiet + run_cmd cargo test -p llm-coding-tools-bubblewrap --no-default-features --features blocking --quietVerify by inspecting the crate feature defaults and this test block:
#!/bin/bash set -e echo "== bubblewrap features ==" rg -n -A12 '^\[features\]' src/llm-coding-tools-bubblewrap/Cargo.toml echo echo "== verify.sh linux-bubblewrap block ==" sed -n '57,63p' src/.cargo/verify.shExpected:
default = ["tokio"]in the bubblewrap manifest, and the blocking test line here missing--no-default-features. Based on learnings: "Enabletokiofeature (default) for async mode with tokio runtime, orblockingfeature for sync/blocking mode. Theasyncandblockingfeatures are mutually exclusive - enabling both causes a compile error."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/.cargo/verify.sh` around lines 59 - 60, The blocking-feature test currently enables `--features blocking` while the crate `llm-coding-tools-bubblewrap` defaults to `tokio`, so both runtimes are active; update the `run_cmd` invocation that runs `cargo test -p llm-coding-tools-bubblewrap --features blocking --quiet` to disable default features (add `--no-default-features`) so the test compiles with only the `blocking` feature active and thus correctly verifies the mutually-exclusive configuration.src/.cargo/verify.ps1 (1)
67-68:⚠️ Potential issue | 🟠 MajorThe PowerShell blocking bubblewrap check is still not isolated.
This has the same problem as
verify.sh:--features blockingleaves bubblewrap's defaulttokiofeature enabled, so the script does not exercise a pure blocking build.🐛 Suggested fix
- Invoke-LoggedCommand "cargo" @("test", "-p", "llm-coding-tools-bubblewrap", "--features", "tokio", "--quiet") - Invoke-LoggedCommand "cargo" @("test", "-p", "llm-coding-tools-bubblewrap", "--features", "blocking", "--quiet") + Invoke-LoggedCommand "cargo" @("test", "-p", "llm-coding-tools-bubblewrap", "--no-default-features", "--features", "tokio", "--quiet") + Invoke-LoggedCommand "cargo" @("test", "-p", "llm-coding-tools-bubblewrap", "--no-default-features", "--features", "blocking", "--quiet")Verify by inspecting the crate feature defaults and the PowerShell test block:
#!/bin/bash set -e echo "== bubblewrap features ==" rg -n -A12 '^\[features\]' src/llm-coding-tools-bubblewrap/Cargo.toml echo echo "== verify.ps1 linux-bubblewrap block ==" sed -n '65,71p' src/.cargo/verify.ps1Expected:
default = ["tokio"]in the bubblewrap manifest, and the blocking test line here missing--no-default-features. Based on learnings: "Enabletokiofeature (default) for async mode with tokio runtime, orblockingfeature for sync/blocking mode. Theasyncandblockingfeatures are mutually exclusive - enabling both causes a compile error."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/.cargo/verify.ps1` around lines 67 - 68, The blocking bubblewrap test currently still enables default features; update the second Invoke-LoggedCommand call that runs cargo test for llm-coding-tools-bubblewrap so the blocking build is isolated by adding the --no-default-features flag before the --features "blocking" argument (i.e., change the Invoke-LoggedCommand invocation that contains "-p", "llm-coding-tools-bubblewrap", "--features", "blocking" to include "--no-default-features" so the blocking feature is built without the default "tokio" feature)..github/workflows/rust.yml (1)
179-191:⚠️ Potential issue | 🟠 MajorBlocking doc/clippy still turn
tokioback on.The test step was fixed, but these two commands still run
llm-coding-tools-bubblewrapwithblockingplus its defaulttokiofeature. That either breaks the Linux job or stops validating the blocking-only surface you meant to cover.Suggested fix
- cargo doc -p llm-coding-tools-bubblewrap --features blocking --document-private-items --no-deps --target ${{ matrix.target }} + cargo doc -p llm-coding-tools-bubblewrap --no-default-features --features blocking --document-private-items --no-deps --target ${{ matrix.target }}- cargo clippy -p llm-coding-tools-bubblewrap --features blocking --target ${{ matrix.target }} -- -D warnings + cargo clippy -p llm-coding-tools-bubblewrap --no-default-features --features blocking --target ${{ matrix.target }} -- -D warningsBased on learnings, "Enable
tokiofeature (default) for async mode with tokio runtime, orblockingfeature for sync/blocking mode. Theasyncandblockingfeatures are mutually exclusive - enabling both causes a compile error."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/rust.yml around lines 179 - 191, The bubblewrap targets are being built with the default tokio feature still enabled because the commands pass --features blocking without disabling default features; update both the cargo doc invocation (llm-coding-tools-bubblewrap) and the cargo clippy invocation that reference llm-coding-tools-bubblewrap to include --no-default-features so only the blocking feature is enabled (i.e., add --no-default-features before --features blocking in the cargo doc and cargo clippy commands).
🧹 Nitpick comments (2)
src/llm-coding-tools-serdesai/Cargo.toml (1)
58-60: Adddefault-features = falseto the serdesai bubblewrap dependency.Serdesai only uses the
profilemodule fromllm-coding-tools-bubblewrap(re-exports and type imports inlib.rsandbash.rs). Inheriting the dependency's default runtime features is unnecessary. Core already appliesdefault-features = falseto the same dependency.♻️ Suggested manifest change
-llm-coding-tools-bubblewrap = { version = "0.1.0", path = "../llm-coding-tools-bubblewrap", optional = true } +llm-coding-tools-bubblewrap = { version = "0.1.0", path = "../llm-coding-tools-bubblewrap", optional = true, default-features = false }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/llm-coding-tools-serdesai/Cargo.toml` around lines 58 - 60, The llm-coding-tools-bubblewrap dependency in the serdesai Cargo.toml is pulling in default runtime features; update the dependency entry for llm-coding-tools-bubblewrap (the dependency named "llm-coding-tools-bubblewrap" used by the serdesai crate) to add default-features = false so the crate does not inherit the bubblewrap crate's default features (match how core already sets default-features = false).src/llm-coding-tools-bubblewrap/src/profile/builder.rs (1)
25-570: SplitBuilderhelpers into smaller modules.Validation, shell resolution, and static-argv assembly are all living in one file now. Pulling those helpers into submodules would make the new sandbox surface easier to audit and keep this module back under the repo limit.
As per coding guidelines, "Keep modules under 500 lines (excluding tests); split if larger."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/llm-coding-tools-bubblewrap/src/profile/builder.rs` around lines 25 - 570, The Builder file is too large and should be split: extract validation helpers (validate_builder, validate_credential_file_mounts, credential_dest_is_allowed, and all validate_* helpers they call) into a new validation module; extract sandbox shell logic (resolve_shell_for_builder, builder_sandbox_layout and first-shell-lookup helpers) into a shell module; and extract static argv assembly (build_static_args, arg_capacity_for, push_bind/push_* helpers) into an args module. Move the functions into their respective modules, keep their signatures (adjust visibility to pub(crate) as needed), update builder.rs to call validation::validate_builder(...), shell::resolve_shell_for_builder(...), and args::build_static_args(...), add mod declarations and any required use/imports, and run tests to ensure no visibility/import regressions. Ensure credential and path types (Builder, TmpBacking, FileOverlay, EnvVar, Symlink, FileMount, SandboxLayout types used) are imported or referenced via crate:: paths and re-export any helper types if other modules need them.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/llm-coding-tools-bubblewrap/src/profile/builder.rs`:
- Around line 397-400: resolve_shell_for_builder currently uses
layout.path_visible(shell) and returns the host-side candidate path, which fails
at runtime when the shell is only reachable via a remap; change the selection to
call layout.classify(shell) and persist the sandbox-side mapped path (the
classified/mapped Path) so that the value returned by resolve_shell_for_builder
(and ultimately profile.shell() passed to wrap_command) is the executable path
inside the sandbox; update the logic around first_shell_candidate_matching /
builder_sandbox_layout to convert the matching host candidate into its
classified sandbox path before returning.
In `@src/llm-coding-tools-bubblewrap/src/profile/layout.rs`:
- Around line 79-85: The current classify() logic in layout.rs incorrectly
treats read_only_mounts the same as read_write_mounts by returning
PathMapping::SamePath when either list has a prefix match; change this so
read_only_mounts are not considered SamePath when there are file overlays or
tmpfs entries that will be mounted later and thus mask RO mounts at runtime.
Specifically, update the predicate used in classify() (the iterator over
read_only_mounts and read_write_mounts) to only return PathMapping::SamePath for
read_write_mounts, and add a separate check that if any overlay path (file
overlays or tmpfs) is a prefix of entry then classify it as overlaid/hidden
instead of SamePath; reference read_only_mounts, read_write_mounts, classify(),
and PathMapping::SamePath when making the change.
---
Outside diff comments:
In `@src/llm-coding-tools-core/src/tools/bash/tokio_impl.rs`:
- Around line 131-163: The current run_wrapped_command can hang awaiting
await_pipe_drain_task after child.wait() if a backgrounded descendant keeps
pipes open; fix by bounding the drain wait with the same deadline/grace logic:
after child.wait() (in run_wrapped_command) compute the remaining time (or use
the configured grace period) and await await_pipe_drain_task(stdout_task) and
await_pipe_drain_task(stderr_task) under a tokio::time::timeout; if the timeout
expires, cancel or abort the drain tasks and collect whatever buffered output is
available, then proceed to return the BashOutput/ToolResult. Reference
run_wrapped_command, child.wait, spawn_pipe_drain_task, and
await_pipe_drain_task when implementing the timed/abortable drain wait.
---
Duplicate comments:
In @.github/workflows/rust.yml:
- Around line 179-191: The bubblewrap targets are being built with the default
tokio feature still enabled because the commands pass --features blocking
without disabling default features; update both the cargo doc invocation
(llm-coding-tools-bubblewrap) and the cargo clippy invocation that reference
llm-coding-tools-bubblewrap to include --no-default-features so only the
blocking feature is enabled (i.e., add --no-default-features before --features
blocking in the cargo doc and cargo clippy commands).
In `@src/.cargo/verify.ps1`:
- Around line 67-68: The blocking bubblewrap test currently still enables
default features; update the second Invoke-LoggedCommand call that runs cargo
test for llm-coding-tools-bubblewrap so the blocking build is isolated by adding
the --no-default-features flag before the --features "blocking" argument (i.e.,
change the Invoke-LoggedCommand invocation that contains "-p",
"llm-coding-tools-bubblewrap", "--features", "blocking" to include
"--no-default-features" so the blocking feature is built without the default
"tokio" feature).
In `@src/.cargo/verify.sh`:
- Around line 59-60: The blocking-feature test currently enables `--features
blocking` while the crate `llm-coding-tools-bubblewrap` defaults to `tokio`, so
both runtimes are active; update the `run_cmd` invocation that runs `cargo test
-p llm-coding-tools-bubblewrap --features blocking --quiet` to disable default
features (add `--no-default-features`) so the test compiles with only the
`blocking` feature active and thus correctly verifies the mutually-exclusive
configuration.
---
Nitpick comments:
In `@src/llm-coding-tools-bubblewrap/src/profile/builder.rs`:
- Around line 25-570: The Builder file is too large and should be split: extract
validation helpers (validate_builder, validate_credential_file_mounts,
credential_dest_is_allowed, and all validate_* helpers they call) into a new
validation module; extract sandbox shell logic (resolve_shell_for_builder,
builder_sandbox_layout and first-shell-lookup helpers) into a shell module; and
extract static argv assembly (build_static_args, arg_capacity_for,
push_bind/push_* helpers) into an args module. Move the functions into their
respective modules, keep their signatures (adjust visibility to pub(crate) as
needed), update builder.rs to call validation::validate_builder(...),
shell::resolve_shell_for_builder(...), and args::build_static_args(...), add mod
declarations and any required use/imports, and run tests to ensure no
visibility/import regressions. Ensure credential and path types (Builder,
TmpBacking, FileOverlay, EnvVar, Symlink, FileMount, SandboxLayout types used)
are imported or referenced via crate:: paths and re-export any helper types if
other modules need them.
In `@src/llm-coding-tools-serdesai/Cargo.toml`:
- Around line 58-60: The llm-coding-tools-bubblewrap dependency in the serdesai
Cargo.toml is pulling in default runtime features; update the dependency entry
for llm-coding-tools-bubblewrap (the dependency named
"llm-coding-tools-bubblewrap" used by the serdesai crate) to add
default-features = false so the crate does not inherit the bubblewrap crate's
default features (match how core already sets default-features = false).
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9602ffc0-7a56-423a-bdb8-e044ad0c7fb2
⛔ Files ignored due to path filters (1)
src/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (45)
.github/workflows/rust.ymlREADME.MDSANDBOX-PROFILES.mdsrc/.cargo/verify.ps1src/.cargo/verify.shsrc/Cargo.tomlsrc/llm-coding-tools-bubblewrap/ARCHITECTURE.mdsrc/llm-coding-tools-bubblewrap/Cargo.tomlsrc/llm-coding-tools-bubblewrap/README.mdsrc/llm-coding-tools-bubblewrap/src/error.rssrc/llm-coding-tools-bubblewrap/src/lib.rssrc/llm-coding-tools-bubblewrap/src/path_util.rssrc/llm-coding-tools-bubblewrap/src/probe.rssrc/llm-coding-tools-bubblewrap/src/profile/builder.rssrc/llm-coding-tools-bubblewrap/src/profile/layout.rssrc/llm-coding-tools-bubblewrap/src/profile/mod.rssrc/llm-coding-tools-bubblewrap/src/profile/presets.rssrc/llm-coding-tools-bubblewrap/src/profile/types.rssrc/llm-coding-tools-bubblewrap/src/profile/validation.rssrc/llm-coding-tools-bubblewrap/src/test_helpers.rssrc/llm-coding-tools-bubblewrap/src/wrap/blocking.rssrc/llm-coding-tools-bubblewrap/src/wrap/command.rssrc/llm-coding-tools-bubblewrap/src/wrap/mod.rssrc/llm-coding-tools-bubblewrap/src/wrap/tokio.rssrc/llm-coding-tools-core/Cargo.tomlsrc/llm-coding-tools-core/README.mdsrc/llm-coding-tools-core/examples/system_prompt/mock_tools.rssrc/llm-coding-tools-core/src/context/tool_prompt/mod.rssrc/llm-coding-tools-core/src/context/tool_prompt/tool_sections.rssrc/llm-coding-tools-core/src/lib.rssrc/llm-coding-tools-core/src/path/allowed.rssrc/llm-coding-tools-core/src/system_prompt.rssrc/llm-coding-tools-core/src/tools/bash/blocking_impl.rssrc/llm-coding-tools-core/src/tools/bash/mod.rssrc/llm-coding-tools-core/src/tools/bash/tokio_impl.rssrc/llm-coding-tools-core/src/tools/mod.rssrc/llm-coding-tools-serdesai/Cargo.tomlsrc/llm-coding-tools-serdesai/README.mdsrc/llm-coding-tools-serdesai/examples/serdesai-agents.rssrc/llm-coding-tools-serdesai/examples/serdesai-basic.rssrc/llm-coding-tools-serdesai/examples/serdesai-sandboxed-bash.rssrc/llm-coding-tools-serdesai/examples/serdesai-sandboxed.rssrc/llm-coding-tools-serdesai/examples/serdesai-task.rssrc/llm-coding-tools-serdesai/src/bash.rssrc/llm-coding-tools-serdesai/src/lib.rs
✅ Files skipped from review due to trivial changes (13)
- src/llm-coding-tools-serdesai/examples/serdesai-sandboxed.rs
- src/Cargo.toml
- src/llm-coding-tools-core/src/path/allowed.rs
- src/llm-coding-tools-core/examples/system_prompt/mock_tools.rs
- src/llm-coding-tools-bubblewrap/src/error.rs
- src/llm-coding-tools-bubblewrap/src/wrap/mod.rs
- src/llm-coding-tools-bubblewrap/Cargo.toml
- src/llm-coding-tools-bubblewrap/src/lib.rs
- src/llm-coding-tools-bubblewrap/ARCHITECTURE.md
- SANDBOX-PROFILES.md
- src/llm-coding-tools-bubblewrap/src/profile/mod.rs
- src/llm-coding-tools-bubblewrap/README.md
- src/llm-coding-tools-bubblewrap/src/probe.rs
🚧 Files skipped from review as they are similar to previous changes (17)
- src/llm-coding-tools-serdesai/examples/serdesai-agents.rs
- src/llm-coding-tools-serdesai/examples/serdesai-task.rs
- README.MD
- src/llm-coding-tools-core/src/context/tool_prompt/mod.rs
- src/llm-coding-tools-core/src/context/tool_prompt/tool_sections.rs
- src/llm-coding-tools-bubblewrap/src/path_util.rs
- src/llm-coding-tools-core/src/lib.rs
- src/llm-coding-tools-bubblewrap/src/wrap/tokio.rs
- src/llm-coding-tools-serdesai/README.md
- src/llm-coding-tools-bubblewrap/src/wrap/blocking.rs
- src/llm-coding-tools-core/Cargo.toml
- src/llm-coding-tools-core/README.md
- src/llm-coding-tools-core/src/tools/bash/blocking_impl.rs
- src/llm-coding-tools-bubblewrap/src/profile/presets.rs
- src/llm-coding-tools-serdesai/examples/serdesai-sandboxed-bash.rs
- src/llm-coding-tools-bubblewrap/src/profile/validation.rs
- src/llm-coding-tools-bubblewrap/src/test_helpers.rs
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build and Test (Async/Tokio) (windows-latest, x86_64-pc-windows-msvc)
- GitHub Check: Build and Test (Async/Tokio) (macos-latest, aarch64-apple-darwin)
🧰 Additional context used
📓 Path-based instructions (2)
src/**/*.rs
📄 CodeRabbit inference engine (src/AGENTS.md)
src/**/*.rs: Preallocate collections when size is known or estimable usingString::with_capacity(estimated_len),Vec::with_capacity(count), orBufReader::with_capacity(size, reader)
Prefer&str/&[T]returns over owned types when lifetime allows
UseCow<'_, str>for conditional ownership (e.g.,String::from_utf8_lossy)
Use&'static strfor compile-time constant strings
Reuse buffers by using.clear()and reusingVec/Stringinstead of reallocating
Use const generics for compile-time branching (e.g.,<const LINE_NUMBERS: bool>)
Use#[inline]on small, hot-path functions
Prefercoreoverstdwhere possible (e.g.,core::memoverstd::mem)
Stream data instead of loading entire files when possible
Usememchrfor fast byte searching over manual iteration
Keep modules under 500 lines (excluding tests); split if larger
Placeusestatements inside functions only for#[cfg]conditional compilation
Document public items with///and add examples in docs where helpful
Use//!for module-level documentation
Focus comments on 'why' not 'what'
Use [TypeName] rustdoc links in documentation, not backticks
Files:
src/llm-coding-tools-serdesai/examples/serdesai-basic.rssrc/llm-coding-tools-core/src/tools/mod.rssrc/llm-coding-tools-serdesai/src/lib.rssrc/llm-coding-tools-serdesai/src/bash.rssrc/llm-coding-tools-core/src/tools/bash/mod.rssrc/llm-coding-tools-core/src/system_prompt.rssrc/llm-coding-tools-bubblewrap/src/profile/layout.rssrc/llm-coding-tools-bubblewrap/src/profile/types.rssrc/llm-coding-tools-core/src/tools/bash/tokio_impl.rssrc/llm-coding-tools-bubblewrap/src/wrap/command.rssrc/llm-coding-tools-bubblewrap/src/profile/builder.rs
src/**/Cargo.toml
📄 CodeRabbit inference engine (src/AGENTS.md)
src/**/Cargo.toml: Enabletokiofeature (default) for async mode with tokio runtime, orblockingfeature for sync/blocking mode. Theasyncandblockingfeatures are mutually exclusive - enabling both causes a compile error.
Prefer performance-oriented crates such asparking_lotoverstd::syncandmemchrfor byte search
Keep dependency footprint minimal
Files:
src/llm-coding-tools-serdesai/Cargo.toml
🧠 Learnings (8)
📓 Common learnings
Learnt from: Sewer56
Repo: Sewer56/llm-coding-tools PR: 69
File: src/llm-coding-tools-bubblewrap/src/profile/validation.rs:57-67
Timestamp: 2026-03-28T02:14:00.864Z
Learning: In `src/llm-coding-tools-bubblewrap/src/profile/` (Rust, llm-coding-tools-bubblewrap crate), the `Builder` API paths (workspace, synthetic_home, cache_root, mount lists, overlays, etc.) are always set by trusted application/operator code — the library consumer is the trusted party. Path normalization and `..`-component hardening in validators like `validate_absolute_path` is therefore NOT required to defend against traversal attacks. Untrusted input (LLM-generated shell commands) only enters through `wrap_command`/`execute_command_with_mode`, not through the `Builder`.
📚 Learning: 2026-03-17T21:06:53.934Z
Learnt from: CR
Repo: Sewer56/llm-coding-tools PR: 0
File: src/AGENTS.md:0-0
Timestamp: 2026-03-17T21:06:53.934Z
Learning: Applies to src/**/*.rs : Document public items with `///` and add examples in docs where helpful
Applied to files:
src/llm-coding-tools-serdesai/examples/serdesai-basic.rs
📚 Learning: 2026-03-28T02:14:00.864Z
Learnt from: Sewer56
Repo: Sewer56/llm-coding-tools PR: 69
File: src/llm-coding-tools-bubblewrap/src/profile/validation.rs:57-67
Timestamp: 2026-03-28T02:14:00.864Z
Learning: In `src/llm-coding-tools-bubblewrap/src/profile/` (Rust, llm-coding-tools-bubblewrap crate), the `Builder` API paths (workspace, synthetic_home, cache_root, mount lists, overlays, etc.) are always set by trusted application/operator code — the library consumer is the trusted party. Path normalization and `..`-component hardening in validators like `validate_absolute_path` is therefore NOT required to defend against traversal attacks. Untrusted input (LLM-generated shell commands) only enters through `wrap_command`/`execute_command_with_mode`, not through the `Builder`.
Applied to files:
src/llm-coding-tools-core/src/tools/mod.rssrc/llm-coding-tools-serdesai/src/lib.rssrc/llm-coding-tools-serdesai/src/bash.rssrc/llm-coding-tools-core/src/tools/bash/mod.rssrc/llm-coding-tools-serdesai/Cargo.tomlsrc/llm-coding-tools-core/src/system_prompt.rs.github/workflows/rust.ymlsrc/llm-coding-tools-core/src/tools/bash/tokio_impl.rssrc/llm-coding-tools-bubblewrap/src/wrap/command.rs
📚 Learning: 2026-03-17T21:06:53.934Z
Learnt from: CR
Repo: Sewer56/llm-coding-tools PR: 0
File: src/AGENTS.md:0-0
Timestamp: 2026-03-17T21:06:53.934Z
Learning: After making changes to source code, always run `.cargo/verify.sh` (`.cargo/verify.ps1` on Windows) before returning to the user
Applied to files:
src/.cargo/verify.sh.github/workflows/rust.ymlsrc/.cargo/verify.ps1
📚 Learning: 2026-03-17T21:06:53.934Z
Learnt from: CR
Repo: Sewer56/llm-coding-tools PR: 0
File: src/AGENTS.md:0-0
Timestamp: 2026-03-17T21:06:53.934Z
Learning: Applies to src/**/Cargo.toml : Enable `tokio` feature (default) for async mode with tokio runtime, or `blocking` feature for sync/blocking mode. The `async` and `blocking` features are mutually exclusive - enabling both causes a compile error.
Applied to files:
src/.cargo/verify.sh.github/workflows/rust.ymlsrc/.cargo/verify.ps1
📚 Learning: 2026-03-17T21:06:53.934Z
Learnt from: CR
Repo: Sewer56/llm-coding-tools PR: 0
File: src/AGENTS.md:0-0
Timestamp: 2026-03-17T21:06:53.934Z
Learning: Applies to src/**/Cargo.toml : Keep dependency footprint minimal
Applied to files:
src/llm-coding-tools-serdesai/Cargo.toml.github/workflows/rust.yml
📚 Learning: 2026-03-28T02:14:00.864Z
Learnt from: Sewer56
Repo: Sewer56/llm-coding-tools PR: 69
File: src/llm-coding-tools-bubblewrap/src/profile/validation.rs:57-67
Timestamp: 2026-03-28T02:14:00.864Z
Learning: In the `src/llm-coding-tools-bubblewrap/src/profile/` module (Rust, `llm-coding-tools-bubblewrap` crate), reviewers should assume the `Builder` API’s path fields (`workspace`, `synthetic_home`, `cache_root`, mount lists, overlays, etc.) are set only by trusted application/operator code. Therefore, absence of traversal hardening/normalization in validator helpers (e.g., `validate_absolute_path`) should not be treated as a traversal vulnerability by itself.
However, this applies only if the call graph guarantees that untrusted/LLM-generated shell inputs never reach these `Builder` path fields. Untrusted input is expected to enter only via `wrap_command` / `execute_command_with_mode`. During review, verify the trust boundary: if any LLM/untrusted data can influence the `Builder` path fields or mount/overlay sources, then path normalization/`..`-component hardening and traversal-safe checks are required and should be flagged.
Applied to files:
src/llm-coding-tools-bubblewrap/src/profile/layout.rssrc/llm-coding-tools-bubblewrap/src/profile/types.rssrc/llm-coding-tools-bubblewrap/src/profile/builder.rs
📚 Learning: 2026-03-17T21:06:53.934Z
Learnt from: CR
Repo: Sewer56/llm-coding-tools PR: 0
File: src/AGENTS.md:0-0
Timestamp: 2026-03-17T21:06:53.934Z
Learning: Applies to src/**/Cargo.toml : Prefer performance-oriented crates such as `parking_lot` over `std::sync` and `memchr` for byte search
Applied to files:
.github/workflows/rust.yml
🪛 PSScriptAnalyzer (1.25.0)
src/.cargo/verify.ps1
[warning] 38-38: File 'verify.ps1' uses Write-Host. Avoid using Write-Host because it might not work in all hosts, does not work when there is no host, and (prior to PS 5.0) cannot be suppressed, captured, or redirected. Instead, use Write-Output, Write-Verbose, or Write-Information.
(PSAvoidUsingWriteHost)
[warning] 47-47: File 'verify.ps1' uses Write-Host. Avoid using Write-Host because it might not work in all hosts, does not work when there is no host, and (prior to PS 5.0) cannot be suppressed, captured, or redirected. Instead, use Write-Output, Write-Verbose, or Write-Information.
(PSAvoidUsingWriteHost)
[warning] 56-56: File 'verify.ps1' uses Write-Host. Avoid using Write-Host because it might not work in all hosts, does not work when there is no host, and (prior to PS 5.0) cannot be suppressed, captured, or redirected. Instead, use Write-Output, Write-Verbose, or Write-Information.
(PSAvoidUsingWriteHost)
[warning] 65-65: File 'verify.ps1' uses Write-Host. Avoid using Write-Host because it might not work in all hosts, does not work when there is no host, and (prior to PS 5.0) cannot be suppressed, captured, or redirected. Instead, use Write-Output, Write-Verbose, or Write-Information.
(PSAvoidUsingWriteHost)
[warning] 73-73: File 'verify.ps1' uses Write-Host. Avoid using Write-Host because it might not work in all hosts, does not work when there is no host, and (prior to PS 5.0) cannot be suppressed, captured, or redirected. Instead, use Write-Output, Write-Verbose, or Write-Information.
(PSAvoidUsingWriteHost)
[warning] 76-76: File 'verify.ps1' uses Write-Host. Avoid using Write-Host because it might not work in all hosts, does not work when there is no host, and (prior to PS 5.0) cannot be suppressed, captured, or redirected. Instead, use Write-Output, Write-Verbose, or Write-Information.
(PSAvoidUsingWriteHost)
[warning] 80-80: File 'verify.ps1' uses Write-Host. Avoid using Write-Host because it might not work in all hosts, does not work when there is no host, and (prior to PS 5.0) cannot be suppressed, captured, or redirected. Instead, use Write-Output, Write-Verbose, or Write-Information.
(PSAvoidUsingWriteHost)
[warning] 94-94: File 'verify.ps1' uses Write-Host. Avoid using Write-Host because it might not work in all hosts, does not work when there is no host, and (prior to PS 5.0) cannot be suppressed, captured, or redirected. Instead, use Write-Output, Write-Verbose, or Write-Information.
(PSAvoidUsingWriteHost)
[warning] 97-97: File 'verify.ps1' uses Write-Host. Avoid using Write-Host because it might not work in all hosts, does not work when there is no host, and (prior to PS 5.0) cannot be suppressed, captured, or redirected. Instead, use Write-Output, Write-Verbose, or Write-Information.
(PSAvoidUsingWriteHost)
[warning] 108-108: File 'verify.ps1' uses Write-Host. Avoid using Write-Host because it might not work in all hosts, does not work when there is no host, and (prior to PS 5.0) cannot be suppressed, captured, or redirected. Instead, use Write-Output, Write-Verbose, or Write-Information.
(PSAvoidUsingWriteHost)
🔇 Additional comments (8)
src/llm-coding-tools-serdesai/examples/serdesai-basic.rs (2)
51-51: Explicit host-mode registration is clearer.Using
BashTool::host()makes the unrestricted execution model obvious in this example.
11-12:⚠️ Potential issue | 🟡 MinorFix incorrect example reference in documentation.
The module documentation references
serdesai-sandbox, but the actual example file is namedserdesai-sandboxed.rs. Update the reference so users can correctly runcargo run --example serdesai-sandboxed.⛔ Skipped due to learnings
Learnt from: CR Repo: Sewer56/llm-coding-tools PR: 0 File: src/AGENTS.md:0-0 Timestamp: 2026-03-17T21:06:53.934Z Learning: Applies to src/**/*.rs : Document public items with `///` and add examples in docs where helpfulsrc/llm-coding-tools-core/src/tools/mod.rs (1)
17-19: Clean top-level re-export surface.This keeps mode-aware bash execution reachable from
toolswithout forcing downstream crates to import fromtools::bashdirectly.src/llm-coding-tools-serdesai/src/lib.rs (1)
18-23: Good facade update.Re-exporting the execution mode and Linux profile module here keeps the serdesAI surface aligned with the new sandboxed bash builder.
src/llm-coding-tools-core/src/system_prompt.rs (1)
468-475: Nice regression coverage for the Bash prompt flags.This locks in both the expanded
ToolPrompt::Bashshape and the conditional text that should appear only for sandboxed execution.Also applies to: 1198-1243
src/llm-coding-tools-serdesai/src/bash.rs (2)
64-137: The constructor split makes execution mode explicit.Keeping
new()as an alias while addinghost()andwith_linux_bwrap()is a clean migration path.
170-243: The prompt metadata now matches runtime behavior.Deriving
network_disabledandsandboxedfromBashExecutionModekeeps the system prompt consistent with the actual execution path.src/llm-coding-tools-bubblewrap/src/wrap/command.rs (1)
21-57: Nice allocation profile here.Borrowing the profile's program/argv/shell and using
Cowonly for a remapped CWD keeps the per-command wrapping path cheap.
The blocking-mode test for llm-coding-tools-bubblewrap was missing --no-default-features, causing both tokio (default) and blocking to be enabled simultaneously despite being mutually exclusive.
The FileOverlay validation (validate_file_overlays) only checks that the source exists and is absolute - it does not require a regular file. The trusted_maintenance preset intentionally uses /dev/null to mask /etc/shadow. Updated the doc comment to match.
The fallback re-added /usr/bin, /usr/lib, and /lib64 — all entries in PUBLIC_BOT_PREFIXES — after the loop confirmed none of those prefixes exist. This guaranteed a build-time validation failure. Returning an empty list is safe: all downstream consumers handle empty correctly.
Both RO and RW binds now come after overlays in the argument list, ensuring they always take precedence at overlapping paths per Linux mount semantics. This simplifies classify() logic and makes behavior more intuitive - explicit mounts always win over overlays. - Reorder build_static_args to emit --ro-bind after tmpfs overlays - Revert classify() to simple combined RO+RW check with clarifying comment
Previously resolve_shell_for_builder used path_visible() which only checked visibility but returned the host path. This failed when shells were remapped (e.g., workspace or synthetic_home bind mounts). Changed to use classify() to get the proper sandbox mapping: - For SamePath: use the original path - For Remap: compute the sandbox destination path Also simplified probe.rs by: - Introducing first_shell_candidate_with() to return mapped values - Removing first_shell_candidate_matching() wrapper - Removing redundant seen Vec since classify() is inexpensive - Removing unused path_visible() method All 453 tests pass.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/llm-coding-tools-bubblewrap/src/probe.rs (1)
109-137: Docstring claims deduplication that isn't implemented.Line 112 states "Duplicates (resolved to the same absolute path) are skipped" but the function doesn't track seen paths. If
/usr/bin/bashappears both on PATH and inSHELL_CANDIDATES, the classify closure will be called twice for the same normalized path.This is functionally correct (classify is deterministic, so the redundant call yields the same result), but the documentation is misleading. Either remove the deduplication claim or add a
HashSetto track seen paths.Option: Remove misleading claim
/// Returns the first shell binary for which `classify` returns [`Some`], /// checking `PATH` first then the hardcoded [`SHELL_CANDIDATES`]. /// -/// Duplicates (resolved to the same absolute path) are skipped. /// On success the host path and the classifier's return value are yielded /// together so the caller need not re-classify.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/llm-coding-tools-bubblewrap/src/probe.rs` around lines 109 - 137, The docstring for first_shell_candidate_with claims duplicates (resolved to the same absolute path) are skipped but the implementation doesn't track seen paths; either update the docstring to remove that claim or implement deduplication by tracking normalized paths: inside first_shell_candidate_with create a HashSet<PathBuf> (or HashSet<Box<Path>>/String) named e.g. seen, insert each normalized path before calling classify, and skip calling classify when the normalized path is already in seen; ensure you reference the normalize_path call and the two loops over find_binary_on_path and SHELL_CANDIDATES so the new seen-check covers both PATH candidates and SHELL_CANDIDATES.src/.cargo/verify.ps1 (1)
56-71: Mirror the new feature matrix inclippy.Lines 65-71 add feature-specific test coverage, but Lines 56-63 still lint only the default feature set. Warnings in
blocking/linux-bubblewrapcode can still bypass the-D warningscheck in this verifier. If you extend it here, please mirror the same matrix insrc/.cargo/verify.shso Line 3 stays accurate.Possible follow-up
Write-Host "Clippy..." if ($onLinux) { Invoke-LoggedCommand "cargo" @("clippy", "-p", "llm-coding-tools-bubblewrap", "--quiet", "--", "-D", "warnings") + Invoke-LoggedCommand "cargo" @("clippy", "-p", "llm-coding-tools-bubblewrap", "--no-default-features", "--features", "blocking", "--quiet", "--", "-D", "warnings") } Invoke-LoggedCommand "cargo" @("clippy", "-p", "llm-coding-tools-core", "--quiet", "--", "-D", "warnings") Invoke-LoggedCommand "cargo" @("clippy", "-p", "llm-coding-tools-agents", "--quiet", "--", "-D", "warnings") Invoke-LoggedCommand "cargo" @("clippy", "-p", "llm-coding-tools-serdesai", "--quiet", "--", "-D", "warnings") Invoke-LoggedCommand "cargo" @("clippy", "-p", "llm-coding-tools-models-dev", "--quiet", "--", "-D", "warnings") + if ($onLinux) { + Invoke-LoggedCommand "cargo" @("clippy", "-p", "llm-coding-tools-core", "--features", "linux-bubblewrap", "--quiet", "--", "-D", "warnings") + Invoke-LoggedCommand "cargo" @("clippy", "-p", "llm-coding-tools-core", "--no-default-features", "--features", "blocking,linux-bubblewrap", "--quiet", "--", "-D", "warnings") + Invoke-LoggedCommand "cargo" @("clippy", "-p", "llm-coding-tools-serdesai", "--features", "linux-bubblewrap", "--quiet", "--", "-D", "warnings") + }Based on learnings: Applies to src/**/Cargo.toml : Enable
tokiofeature (default) for async mode with tokio runtime, orblockingfeature for sync/blocking mode. Theasyncandblockingfeatures are mutually exclusive - enabling both causes a compile error.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/.cargo/verify.ps1` around lines 56 - 71, The Clippy step in verify.ps1 only lints defaults but tests added feature-specific matrices (tokio, blocking, linux-bubblewrap); update the Invoke-LoggedCommand calls in the Clippy section (where Invoke-LoggedCommand is invoked for packages llm-coding-tools-bubblewrap, -core, -agents, -serdesai, -models-dev) to mirror the same feature combinations you added to the test section (include clippy runs for bubblewrap with tokio, bubblewrap with no-default-features+blocking, core with linux-bubblewrap and with no-default-features+blocking, and serdesai with linux-bubblewrap) and then apply the same changes to src/.cargo/verify.sh so the shell verifier and the PowerShell verifier remain consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/.cargo/verify.ps1`:
- Around line 56-71: The Clippy step in verify.ps1 only lints defaults but tests
added feature-specific matrices (tokio, blocking, linux-bubblewrap); update the
Invoke-LoggedCommand calls in the Clippy section (where Invoke-LoggedCommand is
invoked for packages llm-coding-tools-bubblewrap, -core, -agents, -serdesai,
-models-dev) to mirror the same feature combinations you added to the test
section (include clippy runs for bubblewrap with tokio, bubblewrap with
no-default-features+blocking, core with linux-bubblewrap and with
no-default-features+blocking, and serdesai with linux-bubblewrap) and then apply
the same changes to src/.cargo/verify.sh so the shell verifier and the
PowerShell verifier remain consistent.
In `@src/llm-coding-tools-bubblewrap/src/probe.rs`:
- Around line 109-137: The docstring for first_shell_candidate_with claims
duplicates (resolved to the same absolute path) are skipped but the
implementation doesn't track seen paths; either update the docstring to remove
that claim or implement deduplication by tracking normalized paths: inside
first_shell_candidate_with create a HashSet<PathBuf> (or
HashSet<Box<Path>>/String) named e.g. seen, insert each normalized path before
calling classify, and skip calling classify when the normalized path is already
in seen; ensure you reference the normalize_path call and the two loops over
find_binary_on_path and SHELL_CANDIDATES so the new seen-check covers both PATH
candidates and SHELL_CANDIDATES.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b9b05441-b6a7-4982-ac66-03cbbf6e7427
📒 Files selected for processing (8)
src/.cargo/verify.ps1src/.cargo/verify.shsrc/llm-coding-tools-bubblewrap/src/probe.rssrc/llm-coding-tools-bubblewrap/src/profile/builder.rssrc/llm-coding-tools-bubblewrap/src/profile/layout.rssrc/llm-coding-tools-bubblewrap/src/profile/presets.rssrc/llm-coding-tools-bubblewrap/src/profile/types.rssrc/llm-coding-tools-core/src/context/tool_prompt/mod.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- src/.cargo/verify.sh
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Build and Test (Async/Tokio) (ubuntu-latest, x86_64-unknown-linux-gnu)
- GitHub Check: Build and Test (Async/Tokio) (macos-latest, aarch64-apple-darwin)
- GitHub Check: Build and Test (Blocking) (windows-latest, x86_64-pc-windows-msvc)
- GitHub Check: Build and Test (Async/Tokio) (windows-latest, x86_64-pc-windows-msvc)
- GitHub Check: Build and Test (Blocking) (macos-latest, aarch64-apple-darwin)
🧰 Additional context used
📓 Path-based instructions (1)
src/**/*.rs
📄 CodeRabbit inference engine (src/AGENTS.md)
src/**/*.rs: Preallocate collections when size is known or estimable usingString::with_capacity(estimated_len),Vec::with_capacity(count), orBufReader::with_capacity(size, reader)
Prefer&str/&[T]returns over owned types when lifetime allows
UseCow<'_, str>for conditional ownership (e.g.,String::from_utf8_lossy)
Use&'static strfor compile-time constant strings
Reuse buffers by using.clear()and reusingVec/Stringinstead of reallocating
Use const generics for compile-time branching (e.g.,<const LINE_NUMBERS: bool>)
Use#[inline]on small, hot-path functions
Prefercoreoverstdwhere possible (e.g.,core::memoverstd::mem)
Stream data instead of loading entire files when possible
Usememchrfor fast byte searching over manual iteration
Keep modules under 500 lines (excluding tests); split if larger
Placeusestatements inside functions only for#[cfg]conditional compilation
Document public items with///and add examples in docs where helpful
Use//!for module-level documentation
Focus comments on 'why' not 'what'
Use [TypeName] rustdoc links in documentation, not backticks
Files:
src/llm-coding-tools-core/src/context/tool_prompt/mod.rssrc/llm-coding-tools-bubblewrap/src/profile/layout.rssrc/llm-coding-tools-bubblewrap/src/probe.rssrc/llm-coding-tools-bubblewrap/src/profile/presets.rssrc/llm-coding-tools-bubblewrap/src/profile/builder.rssrc/llm-coding-tools-bubblewrap/src/profile/types.rs
🧠 Learnings (6)
📓 Common learnings
Learnt from: Sewer56
Repo: Sewer56/llm-coding-tools PR: 69
File: src/llm-coding-tools-bubblewrap/src/profile/validation.rs:57-67
Timestamp: 2026-03-28T02:14:00.864Z
Learning: In `src/llm-coding-tools-bubblewrap/src/profile/` (Rust, llm-coding-tools-bubblewrap crate), the `Builder` API paths (workspace, synthetic_home, cache_root, mount lists, overlays, etc.) are always set by trusted application/operator code — the library consumer is the trusted party. Path normalization and `..`-component hardening in validators like `validate_absolute_path` is therefore NOT required to defend against traversal attacks. Untrusted input (LLM-generated shell commands) only enters through `wrap_command`/`execute_command_with_mode`, not through the `Builder`.
📚 Learning: 2026-03-17T21:06:53.934Z
Learnt from: CR
Repo: Sewer56/llm-coding-tools PR: 0
File: src/AGENTS.md:0-0
Timestamp: 2026-03-17T21:06:53.934Z
Learning: Applies to src/**/*.rs : Document public items with `///` and add examples in docs where helpful
Applied to files:
src/llm-coding-tools-core/src/context/tool_prompt/mod.rs
📚 Learning: 2026-03-17T21:06:53.934Z
Learnt from: CR
Repo: Sewer56/llm-coding-tools PR: 0
File: src/AGENTS.md:0-0
Timestamp: 2026-03-17T21:06:53.934Z
Learning: After making changes to source code, always run `.cargo/verify.sh` (`.cargo/verify.ps1` on Windows) before returning to the user
Applied to files:
src/.cargo/verify.ps1
📚 Learning: 2026-03-17T21:06:53.934Z
Learnt from: CR
Repo: Sewer56/llm-coding-tools PR: 0
File: src/AGENTS.md:0-0
Timestamp: 2026-03-17T21:06:53.934Z
Learning: Applies to src/**/Cargo.toml : Enable `tokio` feature (default) for async mode with tokio runtime, or `blocking` feature for sync/blocking mode. The `async` and `blocking` features are mutually exclusive - enabling both causes a compile error.
Applied to files:
src/.cargo/verify.ps1
📚 Learning: 2026-03-28T02:14:00.864Z
Learnt from: Sewer56
Repo: Sewer56/llm-coding-tools PR: 69
File: src/llm-coding-tools-bubblewrap/src/profile/validation.rs:57-67
Timestamp: 2026-03-28T02:14:00.864Z
Learning: In the `src/llm-coding-tools-bubblewrap/src/profile/` module (Rust, `llm-coding-tools-bubblewrap` crate), reviewers should assume the `Builder` API’s path fields (`workspace`, `synthetic_home`, `cache_root`, mount lists, overlays, etc.) are set only by trusted application/operator code. Therefore, absence of traversal hardening/normalization in validator helpers (e.g., `validate_absolute_path`) should not be treated as a traversal vulnerability by itself.
However, this applies only if the call graph guarantees that untrusted/LLM-generated shell inputs never reach these `Builder` path fields. Untrusted input is expected to enter only via `wrap_command` / `execute_command_with_mode`. During review, verify the trust boundary: if any LLM/untrusted data can influence the `Builder` path fields or mount/overlay sources, then path normalization/`..`-component hardening and traversal-safe checks are required and should be flagged.
Applied to files:
src/llm-coding-tools-bubblewrap/src/profile/layout.rssrc/llm-coding-tools-bubblewrap/src/profile/presets.rssrc/llm-coding-tools-bubblewrap/src/profile/builder.rssrc/llm-coding-tools-bubblewrap/src/profile/types.rs
📚 Learning: 2026-03-28T02:14:00.864Z
Learnt from: Sewer56
Repo: Sewer56/llm-coding-tools PR: 69
File: src/llm-coding-tools-bubblewrap/src/profile/validation.rs:57-67
Timestamp: 2026-03-28T02:14:00.864Z
Learning: In `src/llm-coding-tools-bubblewrap/src/profile/` (Rust, llm-coding-tools-bubblewrap crate), the `Builder` API paths (workspace, synthetic_home, cache_root, mount lists, overlays, etc.) are always set by trusted application/operator code — the library consumer is the trusted party. Path normalization and `..`-component hardening in validators like `validate_absolute_path` is therefore NOT required to defend against traversal attacks. Untrusted input (LLM-generated shell commands) only enters through `wrap_command`/`execute_command_with_mode`, not through the `Builder`.
Applied to files:
src/llm-coding-tools-bubblewrap/src/probe.rs
🪛 PSScriptAnalyzer (1.25.0)
src/.cargo/verify.ps1
[warning] 38-38: File 'verify.ps1' uses Write-Host. Avoid using Write-Host because it might not work in all hosts, does not work when there is no host, and (prior to PS 5.0) cannot be suppressed, captured, or redirected. Instead, use Write-Output, Write-Verbose, or Write-Information.
(PSAvoidUsingWriteHost)
[warning] 47-47: File 'verify.ps1' uses Write-Host. Avoid using Write-Host because it might not work in all hosts, does not work when there is no host, and (prior to PS 5.0) cannot be suppressed, captured, or redirected. Instead, use Write-Output, Write-Verbose, or Write-Information.
(PSAvoidUsingWriteHost)
[warning] 56-56: File 'verify.ps1' uses Write-Host. Avoid using Write-Host because it might not work in all hosts, does not work when there is no host, and (prior to PS 5.0) cannot be suppressed, captured, or redirected. Instead, use Write-Output, Write-Verbose, or Write-Information.
(PSAvoidUsingWriteHost)
[warning] 65-65: File 'verify.ps1' uses Write-Host. Avoid using Write-Host because it might not work in all hosts, does not work when there is no host, and (prior to PS 5.0) cannot be suppressed, captured, or redirected. Instead, use Write-Output, Write-Verbose, or Write-Information.
(PSAvoidUsingWriteHost)
[warning] 73-73: File 'verify.ps1' uses Write-Host. Avoid using Write-Host because it might not work in all hosts, does not work when there is no host, and (prior to PS 5.0) cannot be suppressed, captured, or redirected. Instead, use Write-Output, Write-Verbose, or Write-Information.
(PSAvoidUsingWriteHost)
[warning] 76-76: File 'verify.ps1' uses Write-Host. Avoid using Write-Host because it might not work in all hosts, does not work when there is no host, and (prior to PS 5.0) cannot be suppressed, captured, or redirected. Instead, use Write-Output, Write-Verbose, or Write-Information.
(PSAvoidUsingWriteHost)
[warning] 80-80: File 'verify.ps1' uses Write-Host. Avoid using Write-Host because it might not work in all hosts, does not work when there is no host, and (prior to PS 5.0) cannot be suppressed, captured, or redirected. Instead, use Write-Output, Write-Verbose, or Write-Information.
(PSAvoidUsingWriteHost)
[warning] 94-94: File 'verify.ps1' uses Write-Host. Avoid using Write-Host because it might not work in all hosts, does not work when there is no host, and (prior to PS 5.0) cannot be suppressed, captured, or redirected. Instead, use Write-Output, Write-Verbose, or Write-Information.
(PSAvoidUsingWriteHost)
[warning] 97-97: File 'verify.ps1' uses Write-Host. Avoid using Write-Host because it might not work in all hosts, does not work when there is no host, and (prior to PS 5.0) cannot be suppressed, captured, or redirected. Instead, use Write-Output, Write-Verbose, or Write-Information.
(PSAvoidUsingWriteHost)
[warning] 108-108: File 'verify.ps1' uses Write-Host. Avoid using Write-Host because it might not work in all hosts, does not work when there is no host, and (prior to PS 5.0) cannot be suppressed, captured, or redirected. Instead, use Write-Output, Write-Verbose, or Write-Information.
(PSAvoidUsingWriteHost)
🔇 Additional comments (20)
src/llm-coding-tools-core/src/context/tool_prompt/mod.rs (2)
38-53: Good documentation coverage for new publicToolPrompt::Bashfields.The
network_disabledandsandboxeddocs clearly define semantics and interaction, which removes ambiguity for downstream users.As per coding guidelines, "Document public items with
///and add examples in docs where helpful".
132-132:ToolPromptFacts::recordupdate is correct for the struct-styleBashvariant.Using
ToolPrompt::Bash { .. }preserves existing fact tracking behavior cleanly after the enum shape change.src/llm-coding-tools-bubblewrap/src/probe.rs (3)
94-107: LGTM!Non-absolute PATH entries are now correctly filtered (line 98), preventing CWD-based resolution of untrusted binaries before sandbox activation.
144-164: LGTM!The PATH-keyed caching with
OnceLock<RwLock<...>>correctly invalidates on PATH changes and avoids redundant probing.
166-237: LGTM!The uncached probe logic correctly validates both the
bwrap --versioncheck and the minimal sandbox creation probe before reporting availability. The stderr extraction for failure reasons provides actionable diagnostics.src/llm-coding-tools-bubblewrap/src/profile/layout.rs (3)
49-102: LGTM!The
classifymethod correctly implements the mount precedence order. The comment at lines 79-80 accurately reflects that extra mounts appear after overlays in the bwrap command line (verified inbuild_static_args), so explicit mounts take precedence over overlays for overlapping paths.
113-141: LGTM!The overlay hiding logic correctly handles:
- Explicit tmpfs overlays (prefix match)
- File overlays (exact destination match)
/tmpspecial cases based onTmpBackingvariant
143-154: LGTM!
join_mapped_pathusesPathBuf::with_capacityfor preallocation and returnsCow::Borrowedwhen no join is needed, following the coding guidelines for efficient string/path handling.src/llm-coding-tools-bubblewrap/src/profile/presets.rs (4)
34-57: LGTM!The
public_botpreset correctly configures a restrictive sandbox: no network, cleared env with filtered PATH, minimal read-only mounts, and tmpfs-backed/tmp.
81-115: LGTM!The
trusted_maintenancepreset correctly enables broader access for trusted jobs while still masking/etc/shadowvia/dev/nullfile overlay and overlaying/homewith tmpfs.
189-201: LGTM!The
TrustedMaintenancebranch now explicitly checksentry.is_absolute()(line 195), preventing relative PATH entries like.orbinfrom bypassing the deny-list. ThePublicBotbranch implicitly rejects relative paths since none ofPUBLIC_BOT_PREFIXESmatch relative entries.
203-216: LGTM!
public_bot_read_only_mountsnow only includes paths that exist on the host, with no fallback that re-adds potentially nonexistent entries.src/llm-coding-tools-bubblewrap/src/profile/builder.rs (4)
397-415: LGTM!
resolve_shell_for_buildernow correctly returns the sandbox-side path by usinglayout.classify()and mapping the result:SamePathreturns the original,Remapjoins the destination prefix with the relative path. This ensuresprofile.shell()contains the path that's valid inside the sandbox.
434-491: LGTM!
build_static_argscorrectly orders bwrap arguments:
- Flags (
--die-with-parent,--new-session, network, clearenv)- Environment variables
- Read-only host rootfs (when enabled)
- Tmpfs overlays
- File overlays
- Read-only mounts (when not using host rootfs)
- Compat symlinks
/dev,/proc,/tmp- Synthetic home, cache root, workspace
- Read-write mounts, credential files
This ensures later mounts take precedence in bubblewrap, which is the correct behavior for overlays vs explicit mounts.
493-516: LGTM!The capacity calculation accounts for all argument components and matches the actual push count, as verified by the tests at lines 655-706.
743-758: LGTM!The test now uses
TempDirwith a known-nonexistentworkspace_does_not_existpath to reliably exercise the workspace validation branch.src/llm-coding-tools-bubblewrap/src/profile/types.rs (4)
217-255: LGTM!
FileOverlaydocumentation correctly specifies the validation rules: source must be an absolute path that exists (allowing/dev/null), destination must be absolute. This supports the/etc/shadowmasking use case intrusted_maintenance.
436-469: LGTM!
map_workdir_to_sandboxefficiently usesCow:
- Returns
Borrowedwhen the path appears at the same location in the sandbox- Returns
Ownedonly when prefix rewriting is requiredThis follows the coding guideline for using
Cowfor conditional ownership.
471-487: LGTM!
sandbox_layoutcorrectly constructs theSandboxLayoutwith all relevant profile fields for path classification.
523-543: LGTM!Tests correctly verify the
Cow::BorrowedvsCow::Ownedbehavior, ensuring allocations only occur when prefix rewriting is needed.
The `first_shell_candidate_with` function's docstring claimed that duplicates (resolved to the same absolute path) are skipped, but the implementation did not track seen paths to actually enforce this. Removed the incorrect claim to match the actual behavior.
- Refactor GitHub Actions workflow to use flat labeled matrix (Async/Blocking x Linux/Windows/macOS) for readability - Collapse Linux-only feature checks into single bottom branch in both verify.sh and verify.ps1 - Add blocking mode semver checks to cover all public API surfaces - Add explanatory comment: Linux is the only target with distinct exported APIs (linux-bubblewrap); Windows/macOS cfg usage is implementation-only today - Remove cargo publish dry-run and cargo package from verify scripts
Preallocate Vecs in bubblewrap preset functions to avoid reallocations: - `inherited_path`: capacity = PATH separator count + 1 - `public_bot_read_only_mounts`: capacity = PUBLIC_BOT_PREFIXES.len() - `public_bot_compat_symlinks`: capacity = 3 Aligns with AGENTS.md performance guidelines for memory efficiency.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/llm-coding-tools-bubblewrap/src/profile/builder.rs (1)
335-579: Split the validation/argv helpers out ofbuilder.rs.The non-test portion of this module is already over the repo’s 500-line limit and currently mixes the public builder API, validation, shell resolution, and argv planning in one place. Extracting the helper layer would make the builder surface easier to navigate.
As per coding guidelines, "Keep modules under 500 lines (excluding tests); split if larger".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/llm-coding-tools-bubblewrap/src/profile/builder.rs` around lines 335 - 579, Split the large builder.rs by extracting validation and argv helper logic into one or two new modules (e.g., profile/validate.rs and profile/argv.rs): move validate_builder, validate_credential_file_mounts, credential_dest_is_allowed, and any validate_* helper functions into validate.rs; move build_static_args, arg_capacity_for, push_bind, push_symlinks, push_env_args, push_same_path_bind, push_same_path_binds, push_tmpfs_mounts, push_file_overlay_mounts, push_tmp_mount, and push_file_mounts into argv.rs (keep builder_sandbox_layout and resolve_shell_for_builder in whichever module needs them or export a small SandboxLayout helper if needed); update visibility (pub(crate) or pub as required) and imports in builder.rs, add mod declarations, and run/adjust tests to import the new module paths so that public Builder API is unchanged while helper code is relocated.src/llm-coding-tools-bubblewrap/src/profile/presets.rs (1)
207-235: Preallocate these fixed-size preset lists.Both helpers know their upper bounds up front, so
Vec::new()misses the repo’s Rust preallocation rule here.As per coding guidelines, "Preallocate collections when size is known or estimable using `String::with_capacity(estimated_len)`, `Vec::with_capacity(count)`, or `BufReader::with_capacity(size, reader)`".♻️ Proposed fix
fn public_bot_read_only_mounts() -> Arc<[Box<Path>]> { - let mut mounts = Vec::new(); + let mut mounts = Vec::with_capacity(PUBLIC_BOT_PREFIXES.len()); for path in PUBLIC_BOT_PREFIXES { let path = PathBuf::from(path); if path.exists() { mounts.push(path.into_boxed_path()); } @@ fn public_bot_compat_symlinks() -> Arc<[Symlink]> { - let mut symlinks = Vec::new(); + let mut symlinks = Vec::with_capacity(3); for (target, link_path, required_path) in [ ("usr/bin", "/bin", "/usr/bin"), ("usr/lib", "/lib", "/usr/lib"), ("usr/sbin", "/sbin", "/usr/sbin"),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/llm-coding-tools-bubblewrap/src/profile/presets.rs` around lines 207 - 235, Change the two helpers to preallocate their Vecs: in public_bot_read_only_mounts() replace Vec::new() with Vec::with_capacity(PUBLIC_BOT_PREFIXES.len()) so the known upper bound is reserved up front, and in public_bot_compat_symlinks() replace Vec::new() with Vec::with_capacity(3) (or a constant for the three tuple entries) before pushing Symlink::new(...) entries; keep the existing iteration and logic unchanged and return symlinks.into() / mounts.into() as before.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/llm-coding-tools-bubblewrap/src/profile/presets.rs`:
- Around line 154-181: Refactor inherited_path by extracting a new function
inherited_path_from_os(preset: Preset, path: Option<&std::ffi::OsStr>) -> String
that contains the current PATH parsing and filtering logic so tests can pass a
synthetic path without mutating process environment; make inherited_path() a
thin wrapper that calls std::env::var_os("PATH") and forwards the result to
inherited_path_from_os. Update tests (e.g.,
public_bot_path_filters_user_and_temp_entries and
trusted_maintenance_path_filters_hidden_and_volatile_entries) to call
inherited_path_from_os with a supplied OsStr instead of using env::set_var.
While here, replace Vec::new() with Vec::with_capacity(N) in
public_bot_read_only_mounts and public_bot_compat_symlinks where N is the known
fixed collection size to avoid reallocations.
---
Nitpick comments:
In `@src/llm-coding-tools-bubblewrap/src/profile/builder.rs`:
- Around line 335-579: Split the large builder.rs by extracting validation and
argv helper logic into one or two new modules (e.g., profile/validate.rs and
profile/argv.rs): move validate_builder, validate_credential_file_mounts,
credential_dest_is_allowed, and any validate_* helper functions into
validate.rs; move build_static_args, arg_capacity_for, push_bind, push_symlinks,
push_env_args, push_same_path_bind, push_same_path_binds, push_tmpfs_mounts,
push_file_overlay_mounts, push_tmp_mount, and push_file_mounts into argv.rs
(keep builder_sandbox_layout and resolve_shell_for_builder in whichever module
needs them or export a small SandboxLayout helper if needed); update visibility
(pub(crate) or pub as required) and imports in builder.rs, add mod declarations,
and run/adjust tests to import the new module paths so that public Builder API
is unchanged while helper code is relocated.
In `@src/llm-coding-tools-bubblewrap/src/profile/presets.rs`:
- Around line 207-235: Change the two helpers to preallocate their Vecs: in
public_bot_read_only_mounts() replace Vec::new() with
Vec::with_capacity(PUBLIC_BOT_PREFIXES.len()) so the known upper bound is
reserved up front, and in public_bot_compat_symlinks() replace Vec::new() with
Vec::with_capacity(3) (or a constant for the three tuple entries) before pushing
Symlink::new(...) entries; keep the existing iteration and logic unchanged and
return symlinks.into() / mounts.into() as before.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: eeb0a441-bb2b-483d-8d74-05b94cb4046d
📒 Files selected for processing (9)
.github/workflows/rust.ymlsrc/.cargo/verify.ps1src/.cargo/verify.shsrc/llm-coding-tools-bubblewrap/src/probe.rssrc/llm-coding-tools-bubblewrap/src/profile/builder.rssrc/llm-coding-tools-bubblewrap/src/profile/layout.rssrc/llm-coding-tools-bubblewrap/src/profile/presets.rssrc/llm-coding-tools-bubblewrap/src/profile/types.rssrc/llm-coding-tools-core/src/context/tool_prompt/mod.rs
✅ Files skipped from review due to trivial changes (1)
- src/llm-coding-tools-bubblewrap/src/probe.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- src/llm-coding-tools-bubblewrap/src/profile/layout.rs
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: Blocking Windows
- GitHub Check: Async Linux
- GitHub Check: Blocking macOS
- GitHub Check: Async macOS
- GitHub Check: Async Windows
- GitHub Check: Blocking Linux
🧰 Additional context used
📓 Path-based instructions (1)
src/**/*.rs
📄 CodeRabbit inference engine (src/AGENTS.md)
src/**/*.rs: Preallocate collections when size is known or estimable usingString::with_capacity(estimated_len),Vec::with_capacity(count), orBufReader::with_capacity(size, reader)
Prefer&str/&[T]returns over owned types when lifetime allows
UseCow<'_, str>for conditional ownership (e.g.,String::from_utf8_lossy)
Use&'static strfor compile-time constant strings
Reuse buffers by using.clear()and reusingVec/Stringinstead of reallocating
Use const generics for compile-time branching (e.g.,<const LINE_NUMBERS: bool>)
Use#[inline]on small, hot-path functions
Prefercoreoverstdwhere possible (e.g.,core::memoverstd::mem)
Stream data instead of loading entire files when possible
Usememchrfor fast byte searching over manual iteration
Keep modules under 500 lines (excluding tests); split if larger
Placeusestatements inside functions only for#[cfg]conditional compilation
Document public items with///and add examples in docs where helpful
Use//!for module-level documentation
Focus comments on 'why' not 'what'
Use [TypeName] rustdoc links in documentation, not backticks
Files:
src/llm-coding-tools-core/src/context/tool_prompt/mod.rssrc/llm-coding-tools-bubblewrap/src/profile/builder.rssrc/llm-coding-tools-bubblewrap/src/profile/presets.rssrc/llm-coding-tools-bubblewrap/src/profile/types.rs
🧠 Learnings (7)
📓 Common learnings
Learnt from: Sewer56
Repo: Sewer56/llm-coding-tools PR: 69
File: src/llm-coding-tools-bubblewrap/src/profile/validation.rs:57-67
Timestamp: 2026-03-28T02:14:00.864Z
Learning: In `src/llm-coding-tools-bubblewrap/src/profile/` (Rust, llm-coding-tools-bubblewrap crate), the `Builder` API paths (workspace, synthetic_home, cache_root, mount lists, overlays, etc.) are always set by trusted application/operator code — the library consumer is the trusted party. Path normalization and `..`-component hardening in validators like `validate_absolute_path` is therefore NOT required to defend against traversal attacks. Untrusted input (LLM-generated shell commands) only enters through `wrap_command`/`execute_command_with_mode`, not through the `Builder`.
📚 Learning: 2026-03-17T21:06:53.934Z
Learnt from: CR
Repo: Sewer56/llm-coding-tools PR: 0
File: src/AGENTS.md:0-0
Timestamp: 2026-03-17T21:06:53.934Z
Learning: Applies to src/**/*.rs : Document public items with `///` and add examples in docs where helpful
Applied to files:
src/llm-coding-tools-core/src/context/tool_prompt/mod.rs
📚 Learning: 2026-03-17T21:06:53.934Z
Learnt from: CR
Repo: Sewer56/llm-coding-tools PR: 0
File: src/AGENTS.md:0-0
Timestamp: 2026-03-17T21:06:53.934Z
Learning: After making changes to source code, always run `.cargo/verify.sh` (`.cargo/verify.ps1` on Windows) before returning to the user
Applied to files:
src/.cargo/verify.shsrc/.cargo/verify.ps1
📚 Learning: 2026-03-17T21:06:53.934Z
Learnt from: CR
Repo: Sewer56/llm-coding-tools PR: 0
File: src/AGENTS.md:0-0
Timestamp: 2026-03-17T21:06:53.934Z
Learning: Applies to src/**/Cargo.toml : Enable `tokio` feature (default) for async mode with tokio runtime, or `blocking` feature for sync/blocking mode. The `async` and `blocking` features are mutually exclusive - enabling both causes a compile error.
Applied to files:
src/.cargo/verify.shsrc/.cargo/verify.ps1.github/workflows/rust.yml
📚 Learning: 2026-03-28T02:14:00.864Z
Learnt from: Sewer56
Repo: Sewer56/llm-coding-tools PR: 69
File: src/llm-coding-tools-bubblewrap/src/profile/validation.rs:57-67
Timestamp: 2026-03-28T02:14:00.864Z
Learning: In the `src/llm-coding-tools-bubblewrap/src/profile/` module (Rust, `llm-coding-tools-bubblewrap` crate), reviewers should assume the `Builder` API’s path fields (`workspace`, `synthetic_home`, `cache_root`, mount lists, overlays, etc.) are set only by trusted application/operator code. Therefore, absence of traversal hardening/normalization in validator helpers (e.g., `validate_absolute_path`) should not be treated as a traversal vulnerability by itself.
However, this applies only if the call graph guarantees that untrusted/LLM-generated shell inputs never reach these `Builder` path fields. Untrusted input is expected to enter only via `wrap_command` / `execute_command_with_mode`. During review, verify the trust boundary: if any LLM/untrusted data can influence the `Builder` path fields or mount/overlay sources, then path normalization/`..`-component hardening and traversal-safe checks are required and should be flagged.
Applied to files:
src/llm-coding-tools-bubblewrap/src/profile/builder.rssrc/llm-coding-tools-bubblewrap/src/profile/presets.rssrc/llm-coding-tools-bubblewrap/src/profile/types.rs
📚 Learning: 2026-03-28T02:14:00.864Z
Learnt from: Sewer56
Repo: Sewer56/llm-coding-tools PR: 69
File: src/llm-coding-tools-bubblewrap/src/profile/validation.rs:57-67
Timestamp: 2026-03-28T02:14:00.864Z
Learning: In `src/llm-coding-tools-bubblewrap/src/profile/` (Rust, llm-coding-tools-bubblewrap crate), the `Builder` API paths (workspace, synthetic_home, cache_root, mount lists, overlays, etc.) are always set by trusted application/operator code — the library consumer is the trusted party. Path normalization and `..`-component hardening in validators like `validate_absolute_path` is therefore NOT required to defend against traversal attacks. Untrusted input (LLM-generated shell commands) only enters through `wrap_command`/`execute_command_with_mode`, not through the `Builder`.
Applied to files:
.github/workflows/rust.yml
📚 Learning: 2026-03-17T21:06:53.934Z
Learnt from: CR
Repo: Sewer56/llm-coding-tools PR: 0
File: src/AGENTS.md:0-0
Timestamp: 2026-03-17T21:06:53.934Z
Learning: Applies to src/**/Cargo.toml : Keep dependency footprint minimal
Applied to files:
.github/workflows/rust.yml
🪛 PSScriptAnalyzer (1.25.0)
src/.cargo/verify.ps1
[warning] 38-38: File 'verify.ps1' uses Write-Host. Avoid using Write-Host because it might not work in all hosts, does not work when there is no host, and (prior to PS 5.0) cannot be suppressed, captured, or redirected. Instead, use Write-Output, Write-Verbose, or Write-Information.
(PSAvoidUsingWriteHost)
[warning] 44-44: File 'verify.ps1' uses Write-Host. Avoid using Write-Host because it might not work in all hosts, does not work when there is no host, and (prior to PS 5.0) cannot be suppressed, captured, or redirected. Instead, use Write-Output, Write-Verbose, or Write-Information.
(PSAvoidUsingWriteHost)
[warning] 50-50: File 'verify.ps1' uses Write-Host. Avoid using Write-Host because it might not work in all hosts, does not work when there is no host, and (prior to PS 5.0) cannot be suppressed, captured, or redirected. Instead, use Write-Output, Write-Verbose, or Write-Information.
(PSAvoidUsingWriteHost)
[warning] 56-56: File 'verify.ps1' uses Write-Host. Avoid using Write-Host because it might not work in all hosts, does not work when there is no host, and (prior to PS 5.0) cannot be suppressed, captured, or redirected. Instead, use Write-Output, Write-Verbose, or Write-Information.
(PSAvoidUsingWriteHost)
[warning] 60-60: File 'verify.ps1' uses Write-Host. Avoid using Write-Host because it might not work in all hosts, does not work when there is no host, and (prior to PS 5.0) cannot be suppressed, captured, or redirected. Instead, use Write-Output, Write-Verbose, or Write-Information.
(PSAvoidUsingWriteHost)
[warning] 64-64: File 'verify.ps1' uses Write-Host. Avoid using Write-Host because it might not work in all hosts, does not work when there is no host, and (prior to PS 5.0) cannot be suppressed, captured, or redirected. Instead, use Write-Output, Write-Verbose, or Write-Information.
(PSAvoidUsingWriteHost)
[warning] 68-68: File 'verify.ps1' uses Write-Host. Avoid using Write-Host because it might not work in all hosts, does not work when there is no host, and (prior to PS 5.0) cannot be suppressed, captured, or redirected. Instead, use Write-Output, Write-Verbose, or Write-Information.
(PSAvoidUsingWriteHost)
[warning] 78-78: File 'verify.ps1' uses Write-Host. Avoid using Write-Host because it might not work in all hosts, does not work when there is no host, and (prior to PS 5.0) cannot be suppressed, captured, or redirected. Instead, use Write-Output, Write-Verbose, or Write-Information.
(PSAvoidUsingWriteHost)
[warning] 81-81: File 'verify.ps1' uses Write-Host. Avoid using Write-Host because it might not work in all hosts, does not work when there is no host, and (prior to PS 5.0) cannot be suppressed, captured, or redirected. Instead, use Write-Output, Write-Verbose, or Write-Information.
(PSAvoidUsingWriteHost)
[warning] 83-83: File 'verify.ps1' uses Write-Host. Avoid using Write-Host because it might not work in all hosts, does not work when there is no host, and (prior to PS 5.0) cannot be suppressed, captured, or redirected. Instead, use Write-Output, Write-Verbose, or Write-Information.
(PSAvoidUsingWriteHost)
[warning] 88-88: File 'verify.ps1' uses Write-Host. Avoid using Write-Host because it might not work in all hosts, does not work when there is no host, and (prior to PS 5.0) cannot be suppressed, captured, or redirected. Instead, use Write-Output, Write-Verbose, or Write-Information.
(PSAvoidUsingWriteHost)
[warning] 93-93: File 'verify.ps1' uses Write-Host. Avoid using Write-Host because it might not work in all hosts, does not work when there is no host, and (prior to PS 5.0) cannot be suppressed, captured, or redirected. Instead, use Write-Output, Write-Verbose, or Write-Information.
(PSAvoidUsingWriteHost)
[warning] 98-98: File 'verify.ps1' uses Write-Host. Avoid using Write-Host because it might not work in all hosts, does not work when there is no host, and (prior to PS 5.0) cannot be suppressed, captured, or redirected. Instead, use Write-Output, Write-Verbose, or Write-Information.
(PSAvoidUsingWriteHost)
[warning] 102-102: File 'verify.ps1' uses Write-Host. Avoid using Write-Host because it might not work in all hosts, does not work when there is no host, and (prior to PS 5.0) cannot be suppressed, captured, or redirected. Instead, use Write-Output, Write-Verbose, or Write-Information.
(PSAvoidUsingWriteHost)
[warning] 106-106: File 'verify.ps1' uses Write-Host. Avoid using Write-Host because it might not work in all hosts, does not work when there is no host, and (prior to PS 5.0) cannot be suppressed, captured, or redirected. Instead, use Write-Output, Write-Verbose, or Write-Information.
(PSAvoidUsingWriteHost)
[warning] 110-110: File 'verify.ps1' uses Write-Host. Avoid using Write-Host because it might not work in all hosts, does not work when there is no host, and (prior to PS 5.0) cannot be suppressed, captured, or redirected. Instead, use Write-Output, Write-Verbose, or Write-Information.
(PSAvoidUsingWriteHost)
[warning] 119-119: File 'verify.ps1' uses Write-Host. Avoid using Write-Host because it might not work in all hosts, does not work when there is no host, and (prior to PS 5.0) cannot be suppressed, captured, or redirected. Instead, use Write-Output, Write-Verbose, or Write-Information.
(PSAvoidUsingWriteHost)
[warning] 122-122: File 'verify.ps1' uses Write-Host. Avoid using Write-Host because it might not work in all hosts, does not work when there is no host, and (prior to PS 5.0) cannot be suppressed, captured, or redirected. Instead, use Write-Output, Write-Verbose, or Write-Information.
(PSAvoidUsingWriteHost)
🔇 Additional comments (15)
src/llm-coding-tools-core/src/context/tool_prompt/mod.rs (2)
38-53: Nice improvement:ToolPrompt::Bashfield docs are now clear and actionable.The new docs explain behavior, defaults, and the
network_disabled/sandboxedinteraction well for downstream users.As per coding guidelines, "Document public items with
///and add examples in docs where helpful".
132-132: Struct-pattern match update is correct for the newBashvariant shape.Line 132 cleanly handles
ToolPrompt::Bash { .. }while preserving existingToolPromptFactsbehavior.src/.cargo/verify.ps1 (3)
35-36: LGTM! Previous shadowing issue resolved.The variable is now correctly named
$onLinuxinstead of$isLinux, avoiding the conflict with PowerShell 6.0+'s readonly automatic variable.
68-79: LGTM! Docs and formatting phases are well-structured.The array concatenation now uses parentheses to force expression mode evaluation, and
RUSTDOCFLAGSis properly saved/restored. The bubblewrap crate is correctly excluded from workspace docs on all platforms.
81-120: LGTM! Linux-only feature coverage properly gated.The bubblewrap-related steps are correctly conditional on
$onLinux, and blocking mode tests properly use--no-default-features --features blockingto avoid the mutually exclusive feature conflict.src/.cargo/verify.sh (3)
25-28: LGTM! Platform detection is correct.Using
uname -sto detect Linux is the standard approach for shell scripts.
48-58: LGTM! Blocking feature testing properly isolates features.All blocking mode invocations correctly use
--no-default-features --features blockingto prevent enabling bothtokioandblockingsimultaneously.
67-101: LGTM! Linux-only coverage is well-structured.The conditional block properly handles:
- Async bubblewrap tests (default features)
- Blocking bubblewrap tests with
--no-default-features --features blocking- Feature combinations for core and serdesai with
linux-bubblewrap- Dedicated docs generation for the Linux-only package
.github/workflows/rust.yml (6)
13-24: LGTM! Matrix consolidation is clean and well-organized.The unified matrix with
modeandlinux_bwrapparameters reduces duplication while maintaining all necessary test combinations. Usingfail-fast: falseensures all matrix entries run to completion.
47-61: LGTM! Blocking mode coverage correctly isolates features.The blocking step properly sets
no-default-features: truewithfeatures: "blocking"to avoid enabling bothtokioandblockingsimultaneously. The package list is also correctly scoped to onlyllm-coding-tools-coreandllm-coding-tools-models-dev(the crates that support blocking mode).
63-78: LGTM! Linux-only feature coverage properly isolated.Both async and blocking bubblewrap tests are correctly gated by
matrix.linux_bwrap. The blocking tests properly use--no-default-features --features blockingto prevent feature conflicts.
141-171: LGTM! Documentation build covers all mode/platform combinations.The case statement correctly handles all four combinations of
mode:linux_bwrap, ensuring:
- Async/Linux: bubblewrap + linux-bubblewrap features documented
- Async/non-Linux: standard crates without bubblewrap
- Blocking/Linux: bubblewrap + blocking features documented
- Blocking/non-Linux: core and models-dev only
216-233: LGTM! Publish job correctly includes the new bubblewrap crate.The
rust-cargo-project-pathsnow includessrc/llm-coding-tools-bubblewrapalongside existing crates, andneeds: [ci, format]ensures all checks pass before publishing.
113-121: Semver-checks feature isolation is correct—no changes needed.The use of
--only-explicit-featureswithout--no-default-featuresis correct.--only-explicit-featuresis the appropriate flag for cargo-semver-checks; it ignores default features and selects an empty feature set unless features are explicitly passed via--features. The three invocations correctly test semver compatibility for: no features, tokio feature, and blocking feature. This approach is correct and appropriate for the tool.src/llm-coding-tools-bubblewrap/src/profile/types.rs (1)
436-469: Nice zero-copy workdir mapping.
Cow<Path>keeps the unchanged-path case allocation-free, and the tests exercise both the borrowed and remapped branches.Also applies to: 523-543
Sewer56
force-pushed
the
bwrap-support-new
branch
2 times, most recently
from
f9254f5 to
2e9a6d3
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/llm-coding-tools-bubblewrap/src/profile/presets.rs (1)
88-91: Minor optimization: avoidcache_root.clone()by extracting XDG paths first.The clone allocates a new
Box<Path>. Extracting the XDG path strings before callingSelf::newwould eliminate this allocation.♻️ Suggested refactor
pub fn trusted_maintenance( workspace: impl Into<Box<Path>>, synthetic_home: impl Into<Box<Path>>, cache_root: impl Into<Box<Path>>, host_tmp: impl Into<Box<Path>>, ) -> Self { let cache_root = cache_root.into(); + let xdg_cache = cache_root.join("xdg-cache").to_string_lossy().into_owned(); + let xdg_state = cache_root.join("xdg-state").to_string_lossy().into_owned(); let tmp_backing = TmpBacking::BindHost(host_tmp.into()); - Self::new(workspace, synthetic_home, cache_root.clone(), tmp_backing) + Self::new(workspace, synthetic_home, cache_root, tmp_backing) .with_preset(Preset::TrustedMaintenance) // ... (unchanged) .with_default_env(Arc::from([ EnvVar::new("PATH", inherited_path(Preset::TrustedMaintenance)), EnvVar::new("HOME", SYNTHETIC_HOME_DEST), EnvVar::new("TMPDIR", "/tmp"), - EnvVar::new( - "XDG_CACHE_HOME", - cache_root.join("xdg-cache").to_string_lossy().into_owned(), - ), + EnvVar::new("XDG_CACHE_HOME", xdg_cache), EnvVar::new("XDG_CONFIG_HOME", SYNTHETIC_HOME_CONFIG), - EnvVar::new( - "XDG_STATE_HOME", - cache_root.join("xdg-state").to_string_lossy().into_owned(), - ), + EnvVar::new("XDG_STATE_HOME", xdg_state), ])) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/llm-coding-tools-bubblewrap/src/profile/presets.rs` around lines 88 - 91, The code currently clones cache_root when calling Self::new, causing an unnecessary Box<Path> allocation; instead, convert/extract the XDG/cache path (cache_root) into the concrete Path/Box value before creating tmp_backing and calling Self::new so you can pass the already-owned value (or a reference if Self::new accepts it) without calling cache_root.clone(); update the sequence around cache_root, TmpBacking::BindHost(host_tmp.into()), and the Self::new(workspace, synthetic_home, cache_root, tmp_backing) call so the owned cache_root is moved into Self::new rather than cloned.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/llm-coding-tools-bubblewrap/src/profile/presets.rs`:
- Around line 88-91: The code currently clones cache_root when calling
Self::new, causing an unnecessary Box<Path> allocation; instead, convert/extract
the XDG/cache path (cache_root) into the concrete Path/Box value before creating
tmp_backing and calling Self::new so you can pass the already-owned value (or a
reference if Self::new accepts it) without calling cache_root.clone(); update
the sequence around cache_root, TmpBacking::BindHost(host_tmp.into()), and the
Self::new(workspace, synthetic_home, cache_root, tmp_backing) call so the owned
cache_root is moved into Self::new rather than cloned.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9223b948-d047-4591-baa2-d763a915e4dd
📒 Files selected for processing (1)
src/llm-coding-tools-bubblewrap/src/profile/presets.rs
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Async Windows
- GitHub Check: Async macOS
- GitHub Check: Blocking macOS
- GitHub Check: Blocking Windows
🧰 Additional context used
📓 Path-based instructions (1)
src/**/*.rs
📄 CodeRabbit inference engine (src/AGENTS.md)
src/**/*.rs: Preallocate collections when size is known or estimable usingString::with_capacity(estimated_len),Vec::with_capacity(count), orBufReader::with_capacity(size, reader)
Prefer&str/&[T]returns over owned types when lifetime allows
UseCow<'_, str>for conditional ownership (e.g.,String::from_utf8_lossy)
Use&'static strfor compile-time constant strings
Reuse buffers by using.clear()and reusingVec/Stringinstead of reallocating
Use const generics for compile-time branching (e.g.,<const LINE_NUMBERS: bool>)
Use#[inline]on small, hot-path functions
Prefercoreoverstdwhere possible (e.g.,core::memoverstd::mem)
Stream data instead of loading entire files when possible
Usememchrfor fast byte searching over manual iteration
Keep modules under 500 lines (excluding tests); split if larger
Placeusestatements inside functions only for#[cfg]conditional compilation
Document public items with///and add examples in docs where helpful
Use//!for module-level documentation
Focus comments on 'why' not 'what'
Use [TypeName] rustdoc links in documentation, not backticks
Files:
src/llm-coding-tools-bubblewrap/src/profile/presets.rs
🧠 Learnings (2)
📓 Common learnings
Learnt from: Sewer56
Repo: Sewer56/llm-coding-tools PR: 69
File: src/llm-coding-tools-bubblewrap/src/profile/validation.rs:57-67
Timestamp: 2026-03-28T02:14:00.864Z
Learning: In `src/llm-coding-tools-bubblewrap/src/profile/` (Rust, llm-coding-tools-bubblewrap crate), the `Builder` API paths (workspace, synthetic_home, cache_root, mount lists, overlays, etc.) are always set by trusted application/operator code — the library consumer is the trusted party. Path normalization and `..`-component hardening in validators like `validate_absolute_path` is therefore NOT required to defend against traversal attacks. Untrusted input (LLM-generated shell commands) only enters through `wrap_command`/`execute_command_with_mode`, not through the `Builder`.
📚 Learning: 2026-03-28T02:14:00.864Z
Learnt from: Sewer56
Repo: Sewer56/llm-coding-tools PR: 69
File: src/llm-coding-tools-bubblewrap/src/profile/validation.rs:57-67
Timestamp: 2026-03-28T02:14:00.864Z
Learning: In the `src/llm-coding-tools-bubblewrap/src/profile/` module (Rust, `llm-coding-tools-bubblewrap` crate), reviewers should assume the `Builder` API’s path fields (`workspace`, `synthetic_home`, `cache_root`, mount lists, overlays, etc.) are set only by trusted application/operator code. Therefore, absence of traversal hardening/normalization in validator helpers (e.g., `validate_absolute_path`) should not be treated as a traversal vulnerability by itself.
However, this applies only if the call graph guarantees that untrusted/LLM-generated shell inputs never reach these `Builder` path fields. Untrusted input is expected to enter only via `wrap_command` / `execute_command_with_mode`. During review, verify the trust boundary: if any LLM/untrusted data can influence the `Builder` path fields or mount/overlay sources, then path normalization/`..`-component hardening and traversal-safe checks are required and should be flagged.
Applied to files:
src/llm-coding-tools-bubblewrap/src/profile/presets.rs
🔇 Additional comments (9)
src/llm-coding-tools-bubblewrap/src/profile/presets.rs (9)
96-99: Verify/dev/nullasFileOverlaysource aligns with documented contract.Using
/dev/null(a character device) works at runtime for masking/etc/shadow, but ifFileOverlaydocumentation specifies regular files as sources, this creates an API inconsistency. Ensure the documentation and validation intypes.rsare aligned with this usage.
1-13: LGTM!Module-level documentation and imports are well-organized. Proper use of
//!for module docs and rustdoc links for cross-references.
15-58: LGTM!The
public_botpreset establishes appropriate security defaults for untrusted input: network disabled, environment cleared, cache root unmounted, and minimal system paths exposed.
119-147: LGTM!Constants are well-organized with comprehensive coverage of system paths. The allowlist/denylist approach for PATH filtering is appropriate for the respective security postures.
149-195: LGTM!Excellent refactoring for testability. The separation of
inherited_path(thin wrapper) frominherited_path_from_os(core logic with injectable parameter) eliminates the need for unsafe environment mutation in tests. Proper use ofHashSetfor O(n) deduplication and capacity preallocation.
197-215: LGTM!The
is_absolute()check on line 209 properly addresses the relative PATH entry concern forTrustedMaintenance. ForPublicBot, the allowlist of absolute prefixes implicitly rejects relative paths.
217-230: LGTM!Proper preallocation and existence checking. The removal of the problematic fallback that re-added nonexistent paths is the correct fix.
232-250: LGTM!Good handling of merged
/usrlayout compatibility with proper existence checks for both link paths and targets.
252-370: LGTM!Comprehensive test coverage with proper use of the refactored
inherited_path_from_osfor injection testing. Tests verify security differentiation between presets and edge cases for PATH handling.
Added: Linux bubblewrap sandbox for shell command execution
1 participant
Summary
llm-coding-tools-bubblewrap, a Linux-only crate that builds bubblewrap sandbox profiles, probes the host for a workingbwrapbinary, and produces wrapped command lines that confine shell execution to an isolated filesystem and network namespace.llm-coding-tools-coreandllm-coding-tools-serdesaivia alinux-bubblewrapfeature flag, with aBashExecutionModeenum that selects host or sandboxed execution and aBashTool::with_linux_bwrap(profile)builder method.SANDBOX-PROFILES.md— operator guide, security model, and pre-deployment checklist for the two preset profiles.verify.sh/verify.ps1to build, test, lint, and docs-check the new crate on Linux; skipped on other platforms.Sandbox profiles
--unshare-net)/read-only/tmp/tmpPATH+HOMEPATH+ XDG/build varsPublic Bot is the default when sandboxing is enabled.
Key changes
llm-coding-tools-bubblewrap— profile builder, presets, validation, sandbox layout, path visibility, probe cache,wrap_command, and tokio/blocking adapters.BashExecutionModeenum,execute_command_with_mode, conditional system prompt lines for sandbox/network status,validate_workdirextracted for reuse.BashTool::host()/BashTool::with_linux_bwrap(profile), backward-compatibleBashTool::new()alias, re-exported profile types,serdesai-sandboxed-bashexample.SANDBOX-PROFILES.md, updatedREADME.mdand per-crate READMEs,ARCHITECTURE.mdfor the new crate.ubuntu-latest, semver-checks, publish dry-run.Testing
linux-bubblewrap.bwrap/bashscripts in temp dirs — no real bubblewrap installation required.Breaking changes
None.
BashTool::new()remains a backward-compatible alias forBashTool::host(). Thelinux-bubblewrapfeature is opt-in.