Repository navigation
Conversation
Tiny Sweeper reviewTiny Sweeper reviewed this change across 5 lane(s) and found 0 active actionable finding(s). Detailed lane evidence and any incomplete work are listed below. State: Incomplete Review snapshot
Completeness: Incomplete FeaturesNone identified with supported citations. TestsNo supported feature-to-test mapping was produced. Test execution is not inferred. FindingsNo active actionable findings. Could not review: crates/openhuman-core/src/sandbox/ops.rs Before merge
How this fits togetherflowchart LR
n0["approved_environment<br/>changed"]:::changed
n1["execute_owned<br/>changed"]:::changed
n2["jail_request<br/>changed"]:::changed
n3["...preserves_grants_and_requires_enforcement<br/>changed"]:::changed
n4["Result"]:::impacted
n5["SandboxPolicy"]:::impacted
n6["execute_in_sandbox_metered"]:::impacted
n7["execute_in_sandbox"]:::impacted
n8["resolve_command_policy"]:::impacted
n9["load_config"]:::impacted
n0 -->|uses| n4
n1 -->|calls| n0
n1 -->|calls| n2
n1 -->|uses| n4
n1 -->|uses| n5
n2 -->|uses| n4
n2 -->|uses| n5
n3 -->|uses| n4
n6 -->|uses| n4
n6 -->|uses| n5
n6 -->|calls| n9
n7 -->|uses| n4
n7 -->|uses| n5
n7 -->|calls| n6
n8 -->|uses| n4
n8 -->|uses| n5
n8 -->|calls| n9
n9 -->|uses| n4
classDef changed fill:#0d4429,stroke:#238636,color:#e6edf3
classDef impacted fill:#161b22,stroke:#6e7681,color:#c9d1d9
classDef flagged fill:#5a1e02,stroke:#d93f0b,color:#ffffff
classDef blocking fill:#67060c,stroke:#f85149,color:#ffffff
Agent review detailscritique
security
tests
commits
description
Evidence and run details
|
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Important Review skippedReview was skipped as selected files did not have any reviewable changes. ⚙️ Run configuration
You can disable this status message by setting the Use the checkbox below for a quick retry:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Walkthrough
Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 78.95% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 19 functions across 7 files. (1 skipped: … | Write docstrings for the functions missing them to satisfy the coverage threshold. |
| Linked Issues check |  |
Issue [#7819] requires macOS commands to run in the selected folder and requires writes outside that folder to fail. The PR adds jail_isolation, keeps Isolated for explicit Landlock and SaaS paths… |
Provide reviewable runtime or integration-test evidence that a macOS Seatbelt command cannot write outside policy.workspace_root. Keep the existing evidence for the decision, status, and refused-path requirements. |
-
Check name Status Explanation Out of Scope Changes check The Docker isolation: Nonedefault, publicSandboxIsolationtype, status schema changes, request mapping, network-policy alignment, and README updates support [#7819] by reporting the confinement …Description Check Check skipped - CodeRabbit’s high-level summary is enabled. Title check The title clearly describes the main change: enabling the macOS desktop sandbox to confine execution to the selected folder.
Full details: Linked Issues check
-
Explanation
Issue [
#7819] requires macOS commands to run in the selected folder and requires writes outside that folder to fail. The PR addsjail_isolation, keepsIsolatedfor explicit Landlock and SaaS paths, accepts Seatbelt only for trusted local runs, maps status toisolatedorfolder_confined, and tests accepted and refused decision paths. The request mapping passes the workspace and grants to TinyBox and disables the process-isolation requirement only for folder confinement. However, the available evidence does not establish the outside-folder write denial. The PR reports that this behavior was not verified live, and the tests do not execute an outside-folder write.
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 78.95% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 19 functions across 7 files. (1 skipped: 1 unsupported.)
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
A rabbit checks the sandbox gate,
Landlock guards the path we take.
Seatbelt keeps writes in their place,
Two postures show the jail’s embrace.
I thump: clear status, hop, and build!
Comment @coderabbitai help to get the list of available commands.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 08df2e24a0
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| } else if !untrusted && backend == SEATBELT { | ||
| Some(SandboxIsolation::FolderConfined) |
There was a problem hiding this comment.
On a macOS single-user run (untrusted == false), this accepts Seatbelt even though this posture leaves reads allow-by-default. security/policy/command_checks.rs:319-322 explicitly says the literal-path guard cannot protect arbitrary shell/interpreter code without a filesystem sandbox, so an agent can construct a path such as Path.home() / (chr(46) + "ssh") / "id_rsa" at runtime, evade the analyzed path words, and read it; the local policy also permits network access. Sandbox mode therefore newly exposes credential and system paths that are meant to remain unreachable; keep Seatbelt unavailable until the TinyBox profile enforces those read denials.
AGENTS.md reference: AGENTS.md:L262-L264
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed, this is a real gap, and it is not fixed in this PR. I am leaving this thread open.
Evidence that the host cannot close it with the TinyBox contract we pin:
tinybox-bus/src/jail.rsNativeJailConfighas onlyread_only,read_write,allow_net,allow_subprocess,enforcementandrequire_process_isolation. There is no deny list. That holds at this branch's pin, at main's pin (7ceaf5c3bd) and on tinybox main.tinybox-jail/src/macos.rsrender_profilestarts with(allow default)and adds only(deny network*), process denies and(deny file-write*)with write re-allows. It never emits afile-read*deny, andread_onlyis "informational on macOS". No field we send can deny reads.
Fix plan, with the capability in TinyBox first per the submodule-ownership rule:
- feat(jail): deny reads of host-chosen paths in native jails (contract 1.20) tinybox#87 (contract 1.20) adds
NativeJailConfig::deny_read.- Seatbelt ends its profile with
(deny file-read* file-write* (subpath …)). - The module canonicalizes denials and refuses one that covers the root or a writable grant, or that lies inside a grant on a backend that cannot carve it out.
- Seatbelt ends its profile with
- After its release and repin,
jail_request(.., FolderConfined)fillsdeny_readfrom the same floor asis_always_forbidden/SENSITIVE_COMPONENTS(security/policy/path_checks.rs, one list, not a fork), expanded against$HOME, plus Keychains and the single-file stores. A test will check that the request carries them.
Until then the sandbox README states this as a known limitation of folder_confined (1922647).
There was a problem hiding this comment.
Follow-up for this P1 is #7917. tinybox v0.1.35 shipped NativeJailConfig.deny_read (tinybox#87, contract 1.20). #7917 repins to it, and jail_request now sends the credential stores that is_always_forbidden protects as deny_read for FolderConfined, so Seatbelt denies them by absolute path. A path built at runtime, like the chr(46) + "ssh" example, still lands on that denial. Only stores that exist are sent; a store holding the jail root or a writable grant is dropped with a warning, since TinyBox refuses such a denial. Landlock (Isolated) is unchanged. I am leaving this thread for the reviewer to resolve.
![tinysweeper[bot]](https://avatars.githubusercontent.com/in/4526585?s=60&v=4){kind=small}
There was a problem hiding this comment.
tinysweeper found nothing blocking, but could not review everything, so this is not an approval: crates/openhuman-core/src/sandbox/native_jail.rs.
$0.0069 · 913,843 in / 90,232 out · 170,752 cached (19%) · ladder/vectors-oai3, deepseek-v4.1-flash · 546 embedded
critique: $0.0014 · 440,447 in / 16,916 out · 37,888 cached (9%) · deepseek-v4.1-flash
security: $0.0026 · 361,219 in / 33,844 out · 112,384 cached (31%) · deepseek-v4.1-flash
tests: $0.0014 · 64,337 in / 20,201 out · 16,384 cached (25%) · deepseek-v4.1-flash
description: $0.0009 · 41,532 in / 12,711 out · 4,096 cached (10%) · deepseek-v4.1-flash
tinysweeper
Bot
added
the
priority: p3
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
08df2e2 to
1922647
Compare
There was a problem hiding this comment.
tinysweeper found nothing blocking, but could not review everything, so this is not an approval: crates/openhuman-core/src/sandbox/README.md, crates/openhuman-core/src/sandbox/native_jail.rs, tinysweeper/tests.
$0.0008 · 71,316 in / 1,398 out · 0 cached (0%) · ladder/vectors-oai3, glm-5.3-flash · 654 embedded
critique: $0.0004 · 46,404 in / 484 out · 0 cached (0%) · glm-5.3-flash
security: $0.0001 · 13,482 in / 901 out · 0 cached (0%) · glm-5.3-flash
description: $0.0001 · 11,430 in / 13 out · 0 cached (0%) · glm-5.3-flash
…folder The native jail gated every command on TinyBox's untrusted-code floor (kernel isolation). Seatbelt reports no process isolation, so on macOS sandbox mode refused every shell command, even in the selected folder. Split the two requirements in a pure jail_isolation decision: kernel isolation stays required for an explicit Landlock request and for SaaS tenants; the local desktop sandbox accepts Seatbelt as folder confinement (writes only in the selected folder and the toolchain grants), asked of TinyBox as declared constraints without process isolation. Sandbox status reports which one applies, and the resource's network declaration now follows the policy the jail already enforces. Refs tinyhumansai#7819
Document the two postures jail_isolation picks, who each is accepted for, and the known limitation of folder confinement: Seatbelt reads are allow-default, so credential stores stay readable until TinyBox's deny_read (tinyhumansai/tinybox#87) is released and pinned.
1922647 to
39c890a
Compare
There was a problem hiding this comment.
tinysweeper found nothing blocking, but could not review everything, so this is not an approval: crates/openhuman-core/src/sandbox/ops.rs.
$0.0047 · 774,531 in / 47,127 out · 152,704 cached (20%) · ladder/vectors-oai3, deepseek-v4.1-flash, glm-5.3-flash, · 654 embedded
critique: $0.0029 · 514,732 in / 38,110 out · 138,240 cached (27%) · deepseek-v4.1-flash
security: $0.0013 · 221,836 in / 5,770 out · 14,464 cached (7%) · glm-5.3-flash, deepseek-v4.1-flash,
tests: $0.0002 · 18,081 in / 72 out · 0 cached (0%) · glm-5.3-flash
description: $0.0001 · 11,405 in / 942 out · 0 cached (0%) · glm-5.3-flash
…ial-stores fix(sandbox): deny reads of credential stores in the folder-confined jail
1 participant
Summary
jail_isolation. It keeps the Kernel floor for an explicitly requested Landlock jail and for SaaS tenants.isolation: "isolated"or"folder_confined".Problem
On macOS, sandbox mode refused every shell command with
TinyBox native jail cannot meet the required isolation floor, evenlsin the selected folder (#7819):native_jail.rsgatedstatus,require_backendandexecute_ownedonsuitable_for_untrusted_code, which means Kernel isolation.IsolationLevel::None(tinybox-jail/src/macos.rs).The gate conflated "confine to the selected folder" with "isolate untrusted code".
Solution
jail_isolation(backend, available, suitable_for_untrusted_code, untrusted)returns:Nonewhen the jail is unavailableIsolatedwhen it meets the Kernel floorFolderConfinedforseatbeltwhen the caller is not untrustedNonein every other case (noop, Landlock below the floor, Seatbelt for untrusted code)statuspassesuntrusted = is_saas().require_backend(explicit Landlock only) passesuntrusted = true.execute_ownedpassesuntrusted = explicitly_requested || is_saas().Isolatedis unchanged:Enforced,require_process_isolation: true.FolderConfinedasks forDeclaredandrequire_process_isolation: false. That is what Seatbelt can give: it declares Filesystem, Network and Subprocess as BestEffort, and TinyBox's admission refusesEnforcedor a process-isolation demand for it.Declaredstill refuses an Unsupported constraint.policy.workspace_root) is the jail root. The Seatbelt profile allows writes only under the root, the TinyBox-resolved read-write grants, the per-call scratch dir,/private/tmpand/dev/null. Reads are allow-default, so the toolchain works.SandboxBackendHandle.isolation: Option<SandboxIsolation>, skipped whenNone, so other handles serialize unchanged.Network decision
resolve_sandbox_policysetsallow_network = !is_remote_sessionfor Sandboxed mode. The local desktop therefore has network, and remote sessions do not.allow_netfollows that policy. It is what Seatbelt enforces:(deny network*)is added only when it is false.run.isolate_network = trueonly set the passthrough resource's declaredNetworkPolicy, which the passthrough backend never reads. It is now!policy.allow_network, so the declaration matches what is enforced.npm installandgit pushwork in the local sandbox, and remote sessions still deny network.Submission Checklist
native_jail_tests.rscovers each backend/isolation combination. Seatbelt is accepted for folder confinement and refused for untrusted code (explicit Landlock or SaaS). Kernel isolation is accepted for every caller. A missing launcher, the noop backend and Landlock below the floor are refused. The status mapping and bothjail_requestpostures are also tested.status/execute_ownedcall sites need a live TinyBox module.Impact
pwd,git,gh,curlandgit init.curlexercises the network decision.seatbeltwithisolation: folder_confined.folder_confineddoes not confine reads. Seatbelt is allow-default for reads, so credential stores stay readable and, with network on, could be sent out. The pinned TinyBox contract cannot deny reads. That capability is feat(jail): deny reads of host-chosen paths in native jails (contract 1.20) tinybox#87 (deny_read, contract 1.20). After its release and repin, a follow-up fillsdeny_readfrom theis_always_forbiddenset. The sandbox README states this limitation.Declaredwithrequire_process_isolation: false, as the vendored source at the gitlink does. The live run above shows that it does.Related
AI Authored PR Metadata (required for Codex/Linear PRs)
Linear Issue
Commit & Branch
fix/gh-7819-macos-sandbox-folder-confinedValidation Run
pnpm --filter openhuman-app format:check: no frontend filespnpm typecheck: no frontend filesrustfmt --checkon the changed filesValidation Blocked
command:N/Aerror:N/Aimpact:N/ABehavior Changes
Parity Contract
Duplicate / Superseded PR Handling
Summary by CodeRabbit