feat(storage): joined filesystem, in-store authorization, task-file identity, and rocketlib.getTask - #1686
Conversation
github-actions
Bot
added
docs
🤖 Internal: Discord sync markerAuto-managed by the Discord notification workflow. Stores the linked Discord message ID and forum thread ID. Do not edit or delete. |
|
Paired saas PR: rocketride-ai/rocketride-saas#395 (alembic Phase 0 + submodule pointer). Merge this PR first, then re-stamp the saas pointer to the squash-merged develop commit. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR replaces environment-based filesystem identity with engine-published task metadata, adds context-bound storage authorization and shared handle management, applies team-scoped task permissions, updates filesystem tooling and Explorer integration, and expands related tests. ChangesTask identity and storage authorization
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant TaskEngine
participant EngineBinding
participant FilesystemNode
participant Store
participant FileStore
TaskEngine->>EngineBinding: publish TASK FILE JSON
EngineBinding->>FilesystemNode: expose rocketlib.getTask()
FilesystemNode->>Store: request engine_file_store()
Store->>FileStore: create anchored context-bound store
FileStore->>FileStore: authorize and resolve path
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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: 14
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/ai/tests/ai/account/test_store.py (1)
563-586: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
monkeypatchforRR_STORE_URLinstead of mutatingos.environdirectly.
test_create_with_default_urlpops the var and never restores it, andtest_create_with_env_varonly cleans up on the success path — a failed assertion at Lines 581-583 leavesRR_STORE_URLset for every later test in the session (Store.create()reads it).monkeypatch.delenv/setenvrestores automatically.♻️ Proposed fix
- def test_create_with_default_url(self): + def test_create_with_default_url(self, monkeypatch): """Test creating store with default URL.""" # Clear environment - os.environ.pop('RR_STORE_URL', None) + monkeypatch.delenv('RR_STORE_URL', raising=False) @@ - def test_create_with_env_var(self, tmp_path): - """Test creating store from STORE_URL environment variable.""" - os.environ['RR_STORE_URL'] = f'filesystem://{tmp_path}' + def test_create_with_env_var(self, tmp_path, monkeypatch): + """Test creating store from RR_STORE_URL environment variable.""" + monkeypatch.setenv('RR_STORE_URL', f'filesystem://{tmp_path}') @@ assert store._store._root_path == tmp_path - - # Cleanup - os.environ.pop('RR_STORE_URL', None)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/ai/tests/ai/account/test_store.py` around lines 563 - 586, Update test_create_with_default_url and test_create_with_env_var to accept pytest’s monkeypatch fixture and use monkeypatch.delenv/setenv for RR_STORE_URL instead of directly mutating os.environ. Remove the manual cleanup so the environment is restored automatically even when assertions fail.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/explorer-ui/src/ExplorerSidebar.tsx`:
- Line 117: Update handleMove and handleUpload so an empty targetDir or
dragsTargetDir uses the “@” mount before appending file.name, matching the root
prefix returned by listRecursive(client, '@'). Preserve existing path behavior
for non-root directories and ensure root writes target “@/…” rather than the
bare filename.
In `@nodes/src/nodes/tool_filesystem/IGlobal.py`:
- Around line 27-29: The task-storage documentation still claims all operations
use a universal user-root path. Update
nodes/src/nodes/tool_filesystem/IGlobal.py lines 27-29 to document the
engine-provided task storage anchor and both development and deployed roots;
update nodes/src/nodes/tool_filesystem/README.md lines 8-17 to make the
whitelist and storage-location sections anchor-relative; and update the
filesystem.pathWhitelist source description in
nodes/src/nodes/tool_filesystem/services.json line 12 so generated documentation
reflects anchor-relative paths, without editing the generated README block.
In `@packages/ai/src/ai/account/file_store.py`:
- Around line 18-23: Update the usage examples in the module and the class
docstring around Store.file_store to call it with ctx first and the client
identifier via client_id. Remove the obsolete connection_id argument from the
write and read examples, while preserving the existing file paths and
operations.
- Around line 840-844: Update the scope-root guard in the rename flow to reject
empty or normalized-root old_path and new_path regardless of whether _resolve
returns kind 'own'. Preserve valid non-root renames while preventing rename('')
from enumerating, copying, and deleting the entire account.
- Around line 929-941: Update _get_handle to fail closed when the caller’s
conn_id is empty: reject the handle before returning it, rather than
conditionally skipping the ownership check. Preserve the existing rejection for
mismatched non-empty connection IDs and allow access only when both the caller
and handle have the same valid connection ID.
In `@packages/ai/src/ai/account/store.py`:
- Around line 691-724: Extract the per-handle teardown currently duplicated in
Store.close_all_handles and FileStore._force_close_handle/_release_handle into a
shared Store helper, preserving the existing close/commit, error logging,
closed-state, registry removal, and write-lock release sequence. Update both
Store.close_all_handles and FileStore’s force-close path to delegate to that
helper, keeping behavior consistent across callers.
- Around line 676-687: Update the client_id resolution around is_session so
non-internal authenticated sessions are handled as sessions even when
AccountInfo.userId is empty. Reject such sessions before accepting any explicit
client_id, and only allow a client_id that matches the authenticated userId;
otherwise derive it from that identity. Preserve the existing explicit-client
requirement for internal/engine contexts.
In `@packages/ai/src/ai/modules/task/commands/cmd_debug.py`:
- Around line 228-231: The org_id fallback in the task handlers must not default
to an empty string for authorized callers without caller-side team membership.
In packages/ai/src/ai/modules/task/commands/cmd_debug.py lines 228-231, update
the launch resolution to obtain the target team’s real organization or reject
the request, and correct the comment to cover all applicable bypasses; apply the
same resolution in packages/ai/src/ai/modules/task/commands/cmd_task.py lines
144-151 so on_execute and on_launch consistently anchor tasks to the target
organization.
In `@packages/ai/src/ai/modules/task/task_engine.py`:
- Around line 477-483: Update _storage_root to validate the derived storage
anchor before returning it, using the existing validate_storage_root helper for
both deploy anchors containing team_id and non-deploy anchors containing
client_id. Ensure invalid project_id or team_id values, including path traversal
or disallowed leading characters, raise a clear launch-time error while
preserving the current deploy team_id requirement and valid anchor formats.
In `@packages/ai/src/ai/modules/task/task_server.py`:
- Around line 662-672: Remove the outer sys.admin permission check in the task
authorization block and always call resolve_task_permissions when account_info
is present and require is set. Preserve the existing no-permission and
missing-required-permission errors, relying on resolve_task_permissions to
provide the admin/internal bypass.
In `@packages/ai/tests/ai/account/test_file_store.py`:
- Around line 639-648: Remove the first, unused unowned FileStore construction
in test_handle_cap_not_enforced_for_unowned_context, including its
unauthenticated RequestContext argument. Keep the later internal context setup
with an empty conn_id and the resulting FileStore construction as the sole test
fixture.
In `@packages/ai/tests/ai/modules/task/commands/test_cmd_task.py`:
- Around line 129-139: Update
test_on_execute_checks_permission_on_the_target_team to patch or mock
account.get_merged_env, matching the sibling permission test, so execution stops
before the real secret-merge backend lookup while preserving the assertion that
team-target is checked with task.control.
In `@packages/ai/tests/ai/modules/task/test_log_stream.py`:
- Around line 154-161: The inline FileStore construction is duplicated across
the run-log tests instead of using one shared helper. In
packages/ai/tests/ai/modules/task/test_run_log.py#L420-L427, define the helper
beside open_writer, ensuring each call creates its own Store wrapper, and
replace the listed constructions there; in
packages/ai/tests/ai/modules/task/test_log_stream.py#L154-L161 and its four
sibling call sites, use that helper; in
packages/ai/tests/ai/modules/task/test_run_log_reader.py#L93-L115 and its three
sibling call sites, import and use the helper.
In `@packages/server/engine-lib/engLib/task/headers/task.hpp`:
- Around line 121-130: Update the public API contract for task::currentTask() to
explicitly state that it is main-task-thread-only and unsafe for concurrent
worker-thread or Python-binding access, unless implementing a thread-safe
publication mechanism for g_currentTask. Ensure the documentation and
implementation consistently enforce the chosen access model, including
rocketlib.getTask() callers.
---
Outside diff comments:
In `@packages/ai/tests/ai/account/test_store.py`:
- Around line 563-586: Update test_create_with_default_url and
test_create_with_env_var to accept pytest’s monkeypatch fixture and use
monkeypatch.delenv/setenv for RR_STORE_URL instead of directly mutating
os.environ. Remove the manual cleanup so the environment is restored
automatically even when assertions fail.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 60da8b6b-cb15-4182-aed0-c549d2844850
📒 Files selected for processing (39)
apps/explorer-ui/src/ExplorerSidebar.tsxnodes/src/nodes/tool_filesystem/IGlobal.pynodes/src/nodes/tool_filesystem/README.mdnodes/src/nodes/tool_filesystem/services.jsonpackages/ai/src/ai/account/__init__.pypackages/ai/src/ai/account/file_store.pypackages/ai/src/ai/account/models.pypackages/ai/src/ai/account/store.pypackages/ai/src/ai/account/store_providers/filesystem/__init__.pypackages/ai/src/ai/account/store_providers/filesystem/filesystem.pypackages/ai/src/ai/account/store_providers/memory/__init__.pypackages/ai/src/ai/account/store_providers/memory/memory.pypackages/ai/src/ai/modules/task/commands/cmd_cprofile.pypackages/ai/src/ai/modules/task/commands/cmd_debug.pypackages/ai/src/ai/modules/task/commands/cmd_log.pypackages/ai/src/ai/modules/task/commands/cmd_store.pypackages/ai/src/ai/modules/task/commands/cmd_task.pypackages/ai/src/ai/modules/task/fetch.pypackages/ai/src/ai/modules/task/task_conn.pypackages/ai/src/ai/modules/task/task_engine.pypackages/ai/src/ai/modules/task/task_server.pypackages/ai/tests/ai/account/test_file_store.pypackages/ai/tests/ai/account/test_store.pypackages/ai/tests/ai/account/test_store_auth.pypackages/ai/tests/ai/modules/task/commands/test_cmd_cprofile.pypackages/ai/tests/ai/modules/task/commands/test_cmd_debug.pypackages/ai/tests/ai/modules/task/commands/test_cmd_task.pypackages/ai/tests/ai/modules/task/test_log_stream.pypackages/ai/tests/ai/modules/task/test_run_log.pypackages/ai/tests/ai/modules/task/test_run_log_reader.pypackages/ai/tests/ai/modules/task/test_task_conn.pypackages/ai/tests/ai/modules/task/test_task_engine.pypackages/ai/tests/ai/modules/task/test_task_server.pypackages/server/engine-lib/engLib/store/python/bindings.cpppackages/server/engine-lib/engLib/task/core/task.cpppackages/server/engine-lib/engLib/task/headers/task.hpppackages/server/engine-lib/rocketlib-python/lib/rocketlib/__init__.pypackages/server/engine-lib/rocketlib-python/lib/rocketlib/__init__.pyipackages/server/engine-lib/rocketlib-python/lib/rocketlib/engine.py
… universal root guards, real org resolution
Addresses the 14 CodeRabbit review threads:
Security/correctness:
- file_store: _get_handle requires an EXACT conn_id match. The handle
registry became Store-wide in this PR, so the lenient empty-conn_id
check would have let any default RequestContext reach another
connection's live handle. Fail closed.
- file_store: the rename/delete root guards no longer exempt the caller's
own namespace — rename('') resolved to the account root and turned the
copy+delete loop into a whole-account move. delete/rename/rmdir now
reject every root uniformly.
- store: a session-shaped ctx (account present, not internal/engine) with
an empty userId can no longer anchor a FileStore via explicit client_id;
that combination (e.g. a task-scoped AccountInfo built from an empty
control.userId) would unlock an arbitrary user's namespace. Account-LESS
contexts stay constructible because resolve_scope already denies their
every operation with 'Not authenticated'.
- task_conn/cmd_task/cmd_debug: new resolve_org_for_team() — members
resolve via their own membership; callers that pass the team check
without membership (sys.admin, internal) resolve the target team's REAL
org through the account backend, and an unresolvable team is denied with
the uniform message. Previously such callers registered the task with
orgId='' travelling in the task file as trusted identity.
- task_engine: _storage_root() validates through validate_storage_root at
launch, so a malformed client-supplied project_id fails with a clear
launch-time error instead of tool_filesystem disabling itself mid-run
inside the subprocess.
Minor:
- explorer-ui: handleMove/handleUpload carry the '@' mount through an
empty targetDir — the tree is rooted at '@', so bare names silently
targeted the plain user store.
- tool_filesystem: IGlobal docstring, README handwritten sections, and the
services.json pathWhitelist description describe the task storage anchor
(dev = user tree, deploy = team task subtree) instead of the universal
user-root claim. The generated README block regenerates on develop
(nodes:docs-generate is branch-gated by design).
- file_store: stale usage examples fixed (Store.file_store(ctx), no
connection_id params); handle-cap comment updated to string conn_ids.
- task.hpp/task.cpp: currentTask()'s main-task-thread-only access model is
stated on the public API — publish, reads, and clear are strictly
sequential on the single main task thread, so the value is
unsynchronized by design.
- store/file_store: per-handle force-close teardown deduped into
Store._teardown_handle(); FileStore._force_close_handle and
close_all_handles delegate so the sequences cannot drift.
- task_server: dropped the outer sys.admin short-circuit duplicated inside
resolve_task_permissions.
Tests: real resolve_org_for_team bound into the cmd_task/cmd_debug stubs,
get_merged_env patched where execution reaches the secret merge, dead
unowned FileStore construction removed, 16 inline FileStore(Store(istore),
...) constructions replaced with a shared make_file_store() helper, and
the sys.admin bypass test now asserts the resolver-side grant.
1572 passed / 122 skipped; ruff clean; explorer-ui rebuilt.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/ai/src/ai/modules/task/task_conn.py`:
- Around line 437-475: Update the membership fast path in resolve_org_for_team
so it validates the organization ID before returning it. When the matching team
has a missing or empty org.get('id', ''), raise the same PermissionError used by
the fallback path; return the ID only when non-empty, preserving the existing
backend resolution behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 62f74d1c-be8f-4b2f-985d-2a1c6eb3db8e
📒 Files selected for processing (20)
apps/explorer-ui/src/ExplorerSidebar.tsxnodes/src/nodes/tool_filesystem/IGlobal.pynodes/src/nodes/tool_filesystem/README.mdnodes/src/nodes/tool_filesystem/services.jsonpackages/ai/src/ai/account/file_store.pypackages/ai/src/ai/account/store.pypackages/ai/src/ai/modules/task/commands/cmd_debug.pypackages/ai/src/ai/modules/task/commands/cmd_task.pypackages/ai/src/ai/modules/task/task_conn.pypackages/ai/src/ai/modules/task/task_engine.pypackages/ai/src/ai/modules/task/task_server.pypackages/ai/tests/ai/account/test_file_store.pypackages/ai/tests/ai/modules/task/commands/test_cmd_debug.pypackages/ai/tests/ai/modules/task/commands/test_cmd_task.pypackages/ai/tests/ai/modules/task/test_log_stream.pypackages/ai/tests/ai/modules/task/test_run_log.pypackages/ai/tests/ai/modules/task/test_run_log_reader.pypackages/ai/tests/ai/modules/task/test_task_server.pypackages/server/engine-lib/engLib/task/core/task.cpppackages/server/engine-lib/engLib/task/headers/task.hpp
💤 Files with no reviewable changes (1)
- packages/ai/tests/ai/account/test_file_store.py
…dentity, and rocketlib.getTask
Foundation for team-scoped deployments: deployments will belong to teams,
so storage, permissions, and subprocess identity all had to stop assuming
"everything belongs to the calling user". This lands that groundwork;
deployment records and team run logs build on it next.
WHY each piece exists:
* In-store authorization — file_store.py IS the security boundary.
Storage I/O happens from many places (rrext_store, rrext_log, fetch,
the run-log writer, deployments). Checking permissions in command
handlers meant every new caller could forget one — the "forgot to
check" bug class. Now every path of every operation authorizes INSIDE
the store, at the single point where paths resolve, against the
RequestContext bound at construction. Command handlers do zero storage
permission checks.
* RequestContext identity (copied verbatim from feat/alb, with MERGE
NOTEs). feat/alb generalizes per-request identity for every handler;
adopting the identical shape now makes that merge a dedupe instead of
a rework. The internal()/engine() factories make the entire trusted
surface greppable: every privileged caller is a
RequestContext.internal(...) call site.
* Store singleton + RR_STORE_URL (STORE_URL hard-fails). A server has
exactly one store, so Store.instance()/Store.file_store(ctx) replaces
the multi-step create()/get_file_store dance that invited divergent
configuration. The legacy env name fails loudly instead of falling
back, because a silent fallback would never get migrated. Providers
became packages (store_providers/filesystem/, memory/). Write locks
and open handles moved to the shared Store so per-connection FileStore
instances still exclude each other on one physical path.
* Joined filesystem, grammar v3: '@' is a virtual root listing User/,
Team/, Org/ so a file browser can walk user, team, and org storage as
ONE tree with ordinary path joining — while plain paths stay "my
files" for nodes and SDK code (simple mode). Bare segments are display
NAMES that resolve ONLY through the caller's own session membership
(a foreign org's names are inexpressible by construction) and
round-trip byte-exact: the path you browse to is the path you open.
=<id> is the only cross-boundary spelling (sys.admin support access).
Unknown, foreign, permission-less, and ambiguous references all deny
with ONE identical message so nothing leaks existence. Names starting
with '=' are uncreatable at scope tops so id references can never be
shadowed. Case-exact and alias-free: one location, one path string.
Normalization runs BEFORE parsing, killing the \@\Team\... bypass
family (fuzz-tested).
* System-owned trees: .logs and .deployments are engine-written and
served by their domain APIs (rrext_log today, rrext_deploy next).
Raw file access could corrupt them or expose data the user must not
see, so through the file API they are invisible in listings and denied
on access for every session identity except sys.admin (who may do
anything). Domain APIs act through the internal identity and keep
their own task.monitor/task.control gates at the command layer.
* Three live cross-team permission holes closed: on_execute accepted an
arbitrary teamId and merged that team's secrets into the task env
BEFORE any membership check (secret exfiltration); on_restart
restarted token-addressed tasks with only a defaultTeam check; the
cprofile proxy could drive another team's engine subprocess. All three
now resolve permissions against the ADDRESSED team/task first
(verify_team_permission / get_task), with uniform denials.
* Task-file identity + storage anchor: subprocess identity rides the
0600 task file ('identity' block) — never the environment, whose
ROCKETRIDE_* namespace is caller-influenced by design; the
ROCKETRIDE_CLIENT_ID env var is gone. The new 'storage' block gives
nodes chroot semantics: dev runs anchor at the owner's whole tree
(unchanged behavior), deploy runs at teams/<teamId>/files/tasks/
<projectId>, so node-level paths are IDENTICAL in both modes, a
deployed task needs no user, and concurrent deployments cannot fight
over working storage. The engine context rejects every '@' spelling
and system tree regardless of anchor — LLM-generated paths cannot
leave the sandbox.
* rocketlib.getTask() C++ binding: the engine already parses the task
file, so python re-reading argv would duplicate parsing and couple to
the launch contract. ITask::execute() publishes the task JSON around
the begin/end window — before beginTask, because endpoint (and node
global) construction happens inside it — and clears it on every exit
path. The store layer resolves identity+anchor itself
(Store.engine_file_store()), so tool_filesystem carries zero jobConfig
plumbing and future consumers get task data for free. Single-threaded
by contract (main task thread only), hence no lock.
* explorer-ui roots its listing at '@' to present the joined tree.
1572 server tests green (suite grew ~90 tests: authorization matrix,
grammar fuzz, system-tree policy, anchor chroot, cprofile proxy).
Engine rebuilt on both dists; rocketlib.getTask verified live.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… universal root guards, real org resolution
Addresses the 14 CodeRabbit review threads:
Security/correctness:
- file_store: _get_handle requires an EXACT conn_id match. The handle
registry became Store-wide in this PR, so the lenient empty-conn_id
check would have let any default RequestContext reach another
connection's live handle. Fail closed.
- file_store: the rename/delete root guards no longer exempt the caller's
own namespace — rename('') resolved to the account root and turned the
copy+delete loop into a whole-account move. delete/rename/rmdir now
reject every root uniformly.
- store: a session-shaped ctx (account present, not internal/engine) with
an empty userId can no longer anchor a FileStore via explicit client_id;
that combination (e.g. a task-scoped AccountInfo built from an empty
control.userId) would unlock an arbitrary user's namespace. Account-LESS
contexts stay constructible because resolve_scope already denies their
every operation with 'Not authenticated'.
- task_conn/cmd_task/cmd_debug: new resolve_org_for_team() — members
resolve via their own membership; callers that pass the team check
without membership (sys.admin, internal) resolve the target team's REAL
org through the account backend, and an unresolvable team is denied with
the uniform message. Previously such callers registered the task with
orgId='' travelling in the task file as trusted identity.
- task_engine: _storage_root() validates through validate_storage_root at
launch, so a malformed client-supplied project_id fails with a clear
launch-time error instead of tool_filesystem disabling itself mid-run
inside the subprocess.
Minor:
- explorer-ui: handleMove/handleUpload carry the '@' mount through an
empty targetDir — the tree is rooted at '@', so bare names silently
targeted the plain user store.
- tool_filesystem: IGlobal docstring, README handwritten sections, and the
services.json pathWhitelist description describe the task storage anchor
(dev = user tree, deploy = team task subtree) instead of the universal
user-root claim. The generated README block regenerates on develop
(nodes:docs-generate is branch-gated by design).
- file_store: stale usage examples fixed (Store.file_store(ctx), no
connection_id params); handle-cap comment updated to string conn_ids.
- task.hpp/task.cpp: currentTask()'s main-task-thread-only access model is
stated on the public API — publish, reads, and clear are strictly
sequential on the single main task thread, so the value is
unsynchronized by design.
- store/file_store: per-handle force-close teardown deduped into
Store._teardown_handle(); FileStore._force_close_handle and
close_all_handles delegate so the sequences cannot drift.
- task_server: dropped the outer sys.admin short-circuit duplicated inside
resolve_task_permissions.
Tests: real resolve_org_for_team bound into the cmd_task/cmd_debug stubs,
get_merged_env patched where execution reaches the secret merge, dead
unowned FileStore construction removed, 16 inline FileStore(Store(istore),
...) constructions replaced with a shared make_file_store() helper, and
the sys.admin bypass test now asserts the resolver-side grant.
1572 passed / 122 skipped; ruff clean; explorer-ui rebuilt.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…s.json description
resolve_org_for_team returned org.get('id', '') straight from the
membership loop, so a membership record whose org carries no id skipped
the empty-org guard the function exists to enforce. Both paths now feed
one guard: membership resolves the id (falling through to the account
backend when it is empty), and an unresolvable org denies with the
uniform no-permissions message.
tool_filesystem/services.json keeps develop's (#1651) description text
verbatim as the conflict resolution for the rebase onto develop; the
sink code's storage wording will be reconciled with the task-anchor
model when this PR merges.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
44cd058 to
33e576a
Compare
There was a problem hiding this comment.
Actionable comments posted: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
packages/ai/tests/ai/account/test_store.py (1)
1336-1393: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winAdd a regression test for cross-connection handle rejection.
The strict
_get_handleownership check (file_store.pyLines 937-943) is the security-critical change in this area — the registry is now Store-wide, so a handle leak crosses identities, not just instances. Nothing here or intest_store_auth.pyasserts that a handle opened by oneconn_idis rejected for another.💚 Suggested test
`@pytest.mark.asyncio` async def test_handle_not_usable_from_another_connection(self, temp_dir): """A handle is bound to the conn_id that opened it.""" store = Store.create(url=f'filesystem://{temp_dir}') fs_a = store._file_store(RequestContext.internal('conn-a'), client_id='test-user') fs_b = store._file_store(RequestContext.internal('conn-b'), client_id='test-user') handle_id = await fs_a.open_write('owned.bin') with pytest.raises(StorageError, match='another connection'): await fs_b.write_chunk(handle_id, b'intruder') await fs_a.close_write(handle_id)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/ai/tests/ai/account/test_store.py` around lines 1336 - 1393, Add a regression test alongside the handle tests that creates two file-store instances with distinct connection IDs, opens a write handle through the first, and verifies the second cannot use it via write_chunk, raising StorageError with the expected cross-connection message. Close the handle through the owning instance afterward.packages/ai/src/ai/modules/task/task_engine.py (1)
1254-1283: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExtract the token substitution — the same six lines run twice.
The
{token}/{public_auth}/{project_id}/{source}replacement block is duplicated verbatim in thestranddictbranches, so a future token has to be added in two places. Also note thegetattrguards exist only for test stubs —project_id/sourceare assigned unconditionally in__init__(Lines 242-243), so a realTaskalways carries them.♻️ Proposed refactor
+ def _substitute_note_tokens(self, text: str) -> str: + """Replace the note placeholders with this task's identity values.""" + text = text.replace('{token}', self.token) + text = text.replace('{public_auth}', self.public_auth) + # getattr: test stubs may not carry the identity attributes. + for name in ('project_id', 'source'): + value = getattr(self, name, None) + if value is not None: + text = text.replace('{' + name + '}', str(value)) + return textThen both branches become
note = self._substitute_note_tokens(note)/value = self._substitute_note_tokens(value).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/ai/src/ai/modules/task/task_engine.py` around lines 1254 - 1283, Extract the duplicated token-replacement logic from the note-processing loop into a helper method on the task class, such as _substitute_note_tokens, covering token, public_auth, project_id, and source substitutions. Use this helper for string notes and every string value in dict notes, while preserving non-string values and the existing note-appending behavior; use the task’s initialized project_id and source attributes rather than repeating getattr guards.apps/explorer-ui/src/ExplorerSidebar.tsx (1)
114-122: 🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy liftRooting at
@turns refresh into a full multi-scope recursive crawl.
listRecursive(Line 361) awaits onefsListDirper directory, sequentially. Previously that walked only the caller's own tree; from@it now descends@/User, every team under@/Team, and@/Org— andrefresh()runs on mount and after every rename/delete/move/upload. For a user in several teams with deep trees this is a large serial request cascade before the sidebar paints.Consider lazy expansion (list a directory when its node is expanded) rather than eager full-depth recursion from the virtual root.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/explorer-ui/src/ExplorerSidebar.tsx` around lines 114 - 122, Replace the eager full-tree crawl in refresh with lazy directory loading: initialize the sidebar from the virtual root without recursively traversing all descendants, and invoke listRecursive or equivalent directory listing only when a node is expanded. Update the relevant expansion handler and refresh flow while preserving entry updates after rename, delete, move, and upload operations.nodes/src/nodes/tool_filesystem/services.json (1)
12-12: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winNode description still documents the removed
ROCKETRIDE_CLIENT_IDenv var.Line 118 was updated to the new task storage anchor, but this description still claims operations are scoped to
users/<client_id>/files/"resolved from the ROCKETRIDE_CLIENT_ID env var injected by the task engine" — the exact mechanism this PR replaces with task-file identity, and wrong for deploy runs that anchor under the team subtree. This string renders in the node UI.📝 Proposed wording
- "description": ["File system node. As an agent tool it reads, writes, deletes, lists, and creates directories within the account-scoped RocketRide file store (the same store exposed via the client SDK's fs_* methods); as a pipeline sink it persists incoming lane data (documents, text, tables, and streamed image/audio/video) to the same store.", "All operations are scoped to users/<client_id>/files/, resolved from the ROCKETRIDE_CLIENT_ID env var injected by the task engine.", "Per-operation toggles (allowRead, allowWrite, allowDelete, allowList, allowMkdir, allowStat) gate which methods the agent can call."], + "description": ["File system node. As an agent tool it reads, writes, deletes, lists, and creates directories within the account-scoped RocketRide file store (the same store exposed via the client SDK's fs_* methods); as a pipeline sink it persists incoming lane data (documents, text, tables, and streamed image/audio/video) to the same store.", "All operations are scoped to the task's storage anchor published in the task file: the owning user's file tree for development runs, the task's team subtree for deployed runs.", "Per-operation toggles (allowRead, allowWrite, allowDelete, allowList, allowMkdir, allowStat) gate which methods the agent can call."],🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@nodes/src/nodes/tool_filesystem/services.json` at line 12, Update the description metadata for the filesystem node to remove the obsolete ROCKETRIDE_CLIENT_ID reference and users/<client_id>/files/ scope. Describe the current task-file identity/storage anchor instead, including team-subtree behavior where applicable, while preserving the existing operation and toggle descriptions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/ai/src/ai/account/file_store.py`:
- Around line 785-790: Update the root guard in the rmdir implementation around
_resolve to reject every scope root with an unconditional not rest check,
matching the guards used by delete and rename; remove the kind != 'own'
exception while preserving the existing empty-path validation.
- Around line 166-172: The scope-entry validation in the shown parsing function
only rejects names whose first component starts with “=”. Extend the
first-component check to also reject “@”, preserving the existing ValueError
behavior and message style so scoped entries such as “@weird” remain
unavailable.
- Around line 307-314: Update the own-namespace branch in the resolver around
kind == 'own' and kind == 'user' with ref is None so non-admin users are granted
access to users/{client_id}/files without checking account_info.defaultTeam or
resolve_task_permissions. Preserve the existing admin behavior and path
resolution.
In `@packages/ai/src/ai/account/models.py`:
- Around line 169-177: Update the `account_info` attribute documentation in the
request context model to state that `None` is valid for both pre-auth commands
handled by `on_auth` and unauthenticated engine subprocess contexts created by
`RequestContext.engine()`. Keep the remaining `conn_id` and `source`
documentation unchanged.
In `@packages/ai/src/ai/account/store.py`:
- Around line 450-494: Make Store.instance() thread-safe using double-checked
locking so concurrent callers initialize exactly one Store and all callers
receive the same instance. Add a class-level synchronization primitive, guard
creation after rechecking cls._instance inside the lock, and preserve the
existing lazy Store.create() behavior and shared lock registries.
In `@packages/ai/src/ai/modules/task/commands/cmd_store.py`:
- Around line 329-353: In _store_fs_list_dir, normalize the requested path by
collapsing empty segments, then pass that same normalized value to
_get_file_store().list_dir so resolution and _is_scope_root filtering stay
aligned; update packages/ai/tests/ai/modules/task/commands/test_cmd_task.py
lines 597-611 to include bypassing spellings such as '`@//User`' in the scope-root
cases and verify system trees remain hidden for non-sys.admin callers.
In `@packages/ai/src/ai/modules/task/task_engine.py`:
- Around line 477-491: Update Task construction/startup around start_task(),
_build_task(), and _storage_root() so launches with the default empty client_id
do not produce users//files and fail unexpectedly: either reject the missing
client_id immediately with a clear user-facing message or preserve the existing
anonymous-run behavior by handling the empty value without calling
validate_storage_root on an invalid path.
In `@packages/ai/tests/ai/account/test_store_auth.py`:
- Around line 503-531: Add coverage to TestScopeRootGuards for the caller’s own
root, exercising delete and both rename directions with the spellings '', '.',
and '/'. Assert each operation raises StorageError matching 'root', so the tests
pin the universal root guard rather than only scoped and joined mount roots.
In `@packages/ai/tests/ai/account/test_store.py`:
- Around line 563-568: Update test_create_with_default_url to use the pytest
monkeypatch fixture and remove both RR_STORE_URL and legacy STORE_URL via
monkeypatch.delenv, allowing missing variables. Keep the existing Store.create()
default-URL assertion unchanged and rely on monkeypatch for automatic
environment cleanup.
In `@packages/ai/tests/ai/modules/task/commands/test_cmd_task.py`:
- Around line 597-611: Extend
test_listing_hides_system_trees_from_ordinary_sessions to include the malformed
scope-root spelling '`@/User`' with an extra slash, ensuring it still hides .logs
and .deployments while exposing docs. Preserve the existing path matrix and
assertions so the normalization behavior is pinned.
- Around line 27-35: Update the Store.file_store monkeypatch in
_patch_store_file_store to accept the production signature, including the
optional root parameter after client_id, while continuing to return the mocked
fs for all arguments.
In `@packages/ai/tests/ai/modules/task/test_task_engine.py`:
- Around line 217-226: Add a mirror test beside
test_build_task_deploy_without_team_refuses covering a dev run with an empty
client_id. Configure the task and filesystem/executable stubs consistently, call
Task._build_task, and assert it raises ValueError matching client_id.
---
Outside diff comments:
In `@apps/explorer-ui/src/ExplorerSidebar.tsx`:
- Around line 114-122: Replace the eager full-tree crawl in refresh with lazy
directory loading: initialize the sidebar from the virtual root without
recursively traversing all descendants, and invoke listRecursive or equivalent
directory listing only when a node is expanded. Update the relevant expansion
handler and refresh flow while preserving entry updates after rename, delete,
move, and upload operations.
In `@nodes/src/nodes/tool_filesystem/services.json`:
- Line 12: Update the description metadata for the filesystem node to remove the
obsolete ROCKETRIDE_CLIENT_ID reference and users/<client_id>/files/ scope.
Describe the current task-file identity/storage anchor instead, including
team-subtree behavior where applicable, while preserving the existing operation
and toggle descriptions.
In `@packages/ai/src/ai/modules/task/task_engine.py`:
- Around line 1254-1283: Extract the duplicated token-replacement logic from the
note-processing loop into a helper method on the task class, such as
_substitute_note_tokens, covering token, public_auth, project_id, and source
substitutions. Use this helper for string notes and every string value in dict
notes, while preserving non-string values and the existing note-appending
behavior; use the task’s initialized project_id and source attributes rather
than repeating getattr guards.
In `@packages/ai/tests/ai/account/test_store.py`:
- Around line 1336-1393: Add a regression test alongside the handle tests that
creates two file-store instances with distinct connection IDs, opens a write
handle through the first, and verifies the second cannot use it via write_chunk,
raising StorageError with the expected cross-connection message. Close the
handle through the owning instance afterward.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 2f0eb75c-f72f-443a-91e5-0ad89f424599
📒 Files selected for processing (39)
apps/explorer-ui/src/ExplorerSidebar.tsxnodes/src/nodes/tool_filesystem/IGlobal.pynodes/src/nodes/tool_filesystem/README.mdnodes/src/nodes/tool_filesystem/services.jsonpackages/ai/src/ai/account/__init__.pypackages/ai/src/ai/account/file_store.pypackages/ai/src/ai/account/models.pypackages/ai/src/ai/account/store.pypackages/ai/src/ai/account/store_providers/filesystem/__init__.pypackages/ai/src/ai/account/store_providers/filesystem/filesystem.pypackages/ai/src/ai/account/store_providers/memory/__init__.pypackages/ai/src/ai/account/store_providers/memory/memory.pypackages/ai/src/ai/modules/task/commands/cmd_cprofile.pypackages/ai/src/ai/modules/task/commands/cmd_debug.pypackages/ai/src/ai/modules/task/commands/cmd_log.pypackages/ai/src/ai/modules/task/commands/cmd_store.pypackages/ai/src/ai/modules/task/commands/cmd_task.pypackages/ai/src/ai/modules/task/fetch.pypackages/ai/src/ai/modules/task/task_conn.pypackages/ai/src/ai/modules/task/task_engine.pypackages/ai/src/ai/modules/task/task_server.pypackages/ai/tests/ai/account/test_file_store.pypackages/ai/tests/ai/account/test_store.pypackages/ai/tests/ai/account/test_store_auth.pypackages/ai/tests/ai/modules/task/commands/test_cmd_cprofile.pypackages/ai/tests/ai/modules/task/commands/test_cmd_debug.pypackages/ai/tests/ai/modules/task/commands/test_cmd_task.pypackages/ai/tests/ai/modules/task/test_log_stream.pypackages/ai/tests/ai/modules/task/test_run_log.pypackages/ai/tests/ai/modules/task/test_run_log_reader.pypackages/ai/tests/ai/modules/task/test_task_conn.pypackages/ai/tests/ai/modules/task/test_task_engine.pypackages/ai/tests/ai/modules/task/test_task_server.pypackages/server/engine-lib/engLib/store/python/bindings.cpppackages/server/engine-lib/engLib/task/core/task.cpppackages/server/engine-lib/engLib/task/headers/task.hpppackages/server/engine-lib/rocketlib-python/lib/rocketlib/__init__.pypackages/server/engine-lib/rocketlib-python/lib/rocketlib/__init__.pyipackages/server/engine-lib/rocketlib-python/lib/rocketlib/engine.py
…ation, sigil reservation, membership-based own-tree access
- file_store: normalize_path extracted to module level as THE wire-path
normalization; FileStore._validate_path delegates to it. cmd_store's
system-tree filter now normalizes with this same routine AND resolves the
listing from the same normalized path — deciding on the raw spelling while
resolving the normalized one let '@//User'-family spellings reach the user
root with .logs/.deployments unfiltered.
- file_store: enforce the documented '@' reservation at scope tops alongside
'=' — a scoped rest could smuggle in an '@'-leading name inside the
grammar room the docs already reserved.
- file_store: the caller's OWN tree no longer hinges on the defaultTeam
pointer (an unset or stale defaultTeam denied a user their own storage);
the file-storage permission grants when ANY membership carries it.
- file_store: rmdir root guard made universal (plain `not rest`) to match
delete/rename — the kind-conditional form was exactly the guard a future
normalization change could quietly defeat.
- store: double-checked locking on the Store singleton — two threads racing
instance() could each build a Store with its own _shared_handles /
_shared_write_locks registry, silently defeating the cross-instance write
exclusion this PR introduces (engine-subprocess node init is the realistic
race).
- task_engine: anonymous dev runs (client_id='' — OSS/standalone) carry NO
storage anchor instead of failing the launch: identity.userId rides empty
too, so engine_file_store() yields None and the storage tools disable
themselves — the same degradable posture as the run-log writer; the shared
'users//files' prefix is never composed.
- models: RequestContext.account_info docs now name engine-subprocess
contexts as the second legitimate None case.
- tests: pin the universal own-root guard (delete/rename of ''), the
normalization-bypass listing matrix ('@//User', '@/./User', '\@\User',
'/'), the dev-run no-anchor posture, STORE_URL/RR_STORE_URL isolation via
monkeypatch.delenv, and mirror Store.file_store's real signature
(root=None) in the cmd_task stub.
1636 server tests green (./builder ai:run-pytest --verbose).
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
kwit75
left a comment
kwit75
left a comment
There was a problem hiding this comment.
Read this one as an authorization boundary rather than a storage refactor, since that's the part that's expensive to get wrong later. The enforcement design is genuinely tight — but I found one thing I'd want resolved before it ships, and I was able to size it against prod.
What I went looking for and didn't find
No traversal escape. parse_scope runs on an already-normalized path, _split_ref rejects ./.., validate_storage_root re-validates the anchor even though _build_task produced it (right call — the subprocess hands it back, so it's untrusted on the return leg), and the filesystem provider still does resolve() + relative_to(root) as a backstop. @/Team/<name> can't reach a foreign org because the only dictionary consulted is the caller's own membership list, and unknown/foreign/ambiguous all collapse to one _DENIED string so there's no existence oracle. Reserving = at the top of every scope so id references can't be shadowed by a real file is a nice touch I wouldn't have thought of.
The engine-context restriction is the part I like most: ctx.source == 'engine' rejects every @ spelling and every system tree, so an LLM-authored path is confined to the task anchor by construction rather than by filtering. And checking _system_tree(rest) before scope resolution keeps that denial uniform and early.
The one thing: this inherits the singular-organization assumption, and prod already violates it
AccountInfo.organization is a single Optional[OrgInfo] — models.py:56 says so explicitly ("single org per user"). Both authorization dictionaries in the new code derive from it: _team_ids() and _resolve_team_name() walk organization['teams'], and resolve_scope's @/Org branch treats org['id'] as "my org, implicitly".
Prod has 9 users in two organizations. All 9 are members of a second org beyond the one holding their default_team_id, and 6 of them have actual team memberships in both. I checked directly:
orgs per user: 1 -> 288 users
2 -> 9 users
users with team memberships in >1 org 6
multi-org users whose default_team resolves 9 (always to a team in one of their orgs)
multi-org users also in a different org 9
Two consequences, and the second is the one that worries me:
Unconditionally: for those 9, the teams in their other org are inexpressible. _resolve_team_name won't find the name, and @/Org/=<other-org-id> is denied unless the caller is sys.admin. The docstring frames inexpressibility as the security property — which it is for a genuinely foreign org, but for these users it denies a team they are really a member of.
Conditionally, and worse: the ordinary-session branch for plain paths calls resolve_task_permissions(account_info, account_info.defaultTeam), which walks organization['teams'] and returns [] when the team isn't there — and resolve_scope turns [] into PermissionError("Permission 'task.store' denied"). So if the session materialises the org that doesn't contain the user's default_team_id, they lose every plain path, not just team scopes. That's total storage failure for that user, on the most ordinary code path. Which of the two orgs gets materialised is saas-side, so I can't tell you from here whether it currently trips — only that all 9 users have the ingredients.
Why I'd rather flag it now than let it land: this is the same root cause as saas #373/#374, the dashboard blanket-deny we fixed in June — one AccountInfo.organization where the user has several. That fix lives in the saas layer; this is the engine re-deriving the same assumption for storage. I recognise the # identical lines exist on feat/alb; dedupe on merge note, so the duplication is already on your radar — my point is narrower: the dedupe target itself is the shape that was wrong.
And the failure will be painful to diagnose, precisely because of a decision that is otherwise correct. The uniform _DENIED means a legitimately-denied member and a blocked cross-tenant attempt produce identical output. A user reporting "I can't open my team's files" will look exactly like an attack being correctly stopped. If the multi-org case isn't fixed in this PR, I'd at least want the denial to log a distinguishable internal reason code — visible in logs, never in the response.
On tests: test_store_auth.py:78 builds a single organization={'id': 'org-1', ...}, so the fixture can't express a two-org caller at all. That's why the matrix looks complete and still misses this. A fixture with a second org, and a case asserting that a team in the second org resolves, would pin it either way.
Smaller notes, none blocking
sys.admin =id crossing is the right call, but it's the one path with no membership check — @/Org/=<any-id> and @/Team/=<any-id> resolve mechanically for a sys.admin session. That is deliberate ("the platform support capability") and I agree with it; I'd just want those resolutions audited, since it's the only place in the function where a boundary is crossed rather than enforced. We already have AccountService.audit() (saas#366) — this seems like exactly its use case.
internal returns before the system-tree check. Reading the order, an internal identity gets users|teams|orgs/<id>/files without passing _system_tree. That's consistent with the docstring ("system trees included") so I read it as intended — flagging only to confirm it's intent and not fall-through, because it's the one branch where the early uniform denial doesn't apply.
resolve_task_permissions returns [] while resolve_team_permissions raises, for the same "no membership" condition. Both are correct in isolation and the docstrings call out the difference, but two functions one line apart with opposite failure modes is the kind of thing that gets picked wrong under time pressure. Worth a name that carries the difference.
Nothing here is a traversal or cross-tenant hole — I looked for that first and the boundary holds. The multi-org item is a fail-closed correctness bug, which is the right direction to fail, but it has 9 real users behind it today. Happy to take the fixture-plus-second-org test myself if that's useful, and I can pull the exact user ids for you privately if you want to confirm the behaviour against a real session before deciding.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
packages/ai/tests/ai/account/test_store.py (1)
579-590: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winUse
monkeypatchfor theRR_STORE_URLoverride.If
Store.create()or an assertion fails, Line 590 never runs and leaks storage configuration into later tests.Proposed fix
- def test_create_with_env_var(self, tmp_path): + def test_create_with_env_var(self, tmp_path, monkeypatch): """Test creating store from STORE_URL environment variable.""" - os.environ['RR_STORE_URL'] = f'filesystem://{tmp_path}' + monkeypatch.setenv('RR_STORE_URL', f'filesystem://{tmp_path}') store = Store.create() assert isinstance(store, Store) assert isinstance(store._store, FilesystemStore) assert store._store._root_path == tmp_path - - # Cleanup - os.environ.pop('RR_STORE_URL', None)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/ai/tests/ai/account/test_store.py` around lines 579 - 590, Update test_create_with_env_var to use pytest’s monkeypatch fixture for the RR_STORE_URL override instead of directly modifying os.environ. Set the variable through monkeypatch so cleanup occurs automatically even when Store.create or an assertion fails, and remove the manual os.environ cleanup.packages/ai/src/ai/modules/task/commands/cmd_store.py (2)
407-414: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
_store_fs_statdoesn't share_store_fs_list_dir's normalization fix.
path = (args.get('path') or '').strip('/')only strips slashes, and the fallthrough call still uses the originalargs.get('path')rather thanpath. Bypass spellings such as'@//User','@/./User','\@\User'won't match the virtual-mount tuple and instead fall through to a real.stat()call — not a security bypass (the store's ownresolve_scopestill authorizes correctly), but it's the same normalization-consistency gap that_store_fs_list_diralready fixed elsewhere in this file, so the "virtual" quick-path is inconsistently applied.♻️ Proposed fix
- path = (args.get('path') or '').strip('/') + path = normalize_path(args.get('path', '') or '') if path in ('@', '`@/User`', '`@/Team`', '`@/Org`'): return self.build_response(request, body={'exists': True, 'type': 'dir', 'virtual': True}) - result = await self._get_file_store().stat(args.get('path')) + result = await self._get_file_store().stat(path)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/ai/src/ai/modules/task/commands/cmd_store.py` around lines 407 - 414, Update _store_fs_stat to use the same path normalization as _store_fs_list_dir, including separator and dot-segment handling, before checking virtual mount roots. Pass the normalized path to the subsequent filesystem-stat fallthrough instead of the original args.get('path') value, while preserving the existing virtual response for @, `@/User`, `@/Team`, and `@/Org`.
48-55: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winStale class docstring — no longer matches
_get_file_store's identity resolution.Says access "Requires
self._server.store...self._account_info.userIdfor user-scoped file access", but_get_file_store(lines 125-136) now goes throughStore.file_store(self.request_context()), with scoping/identity resolved entirely inside the identity-boundFileStore.📝 Proposed fix
- Provides ``on_rrext_store`` which dispatches to ``fs_*`` subcommands. - Requires ``self._server.store`` for the Store instance and - ``self._account_info.userId`` for user-scoped file access. + Provides ``on_rrext_store`` which dispatches to ``fs_*`` subcommands. + Every subcommand routes through ``Store.file_store(self.request_context())``, + which resolves and authorizes identity/scoping internally.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/ai/src/ai/modules/task/commands/cmd_store.py` around lines 48 - 55, Update the StoreCommands class docstring to remove the outdated requirements for self._server.store and self._account_info.userId, and describe that user-scoped access is resolved through Store.file_store using the identity-bound FileStore obtained from request_context().packages/ai/src/ai/modules/task/task_engine.py (1)
456-475: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winRetain or reject
userIdfor deploy tasks to keep the deploy contract consistentDeploy runs currently require only
team_id, but the published task identity still writesidentity.userIdasclient_id. SinceStore.engine_file_store()returnsNonewheneveridentity.userIdis empty, a deploy task launched with an empty client/session id gets a valid team storage anchor with engine file-store disabled; deployment records also write underusers/<client_id>/deployments/.... Require a client identity for deploy launches, or use a deploy-only internal identity and decouple engine file-store availability fromidentity.userId.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/ai/src/ai/modules/task/task_engine.py` around lines 456 - 475, Update the deploy launch path and the identity/storage setup around _storage_root and engine_file_store so deploy tasks never publish an empty identity.userId while still claiming a valid team storage anchor. Either require a non-empty client_id for deploy launches, or assign a documented deploy-only internal identity and adjust Store.engine_file_store and deployment record paths to use it consistently; preserve the existing team-scoped storage behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@packages/ai/src/ai/modules/task/commands/cmd_store.py`:
- Around line 407-414: Update _store_fs_stat to use the same path normalization
as _store_fs_list_dir, including separator and dot-segment handling, before
checking virtual mount roots. Pass the normalized path to the subsequent
filesystem-stat fallthrough instead of the original args.get('path') value,
while preserving the existing virtual response for @, `@/User`, `@/Team`, and `@/Org`.
- Around line 48-55: Update the StoreCommands class docstring to remove the
outdated requirements for self._server.store and self._account_info.userId, and
describe that user-scoped access is resolved through Store.file_store using the
identity-bound FileStore obtained from request_context().
In `@packages/ai/src/ai/modules/task/task_engine.py`:
- Around line 456-475: Update the deploy launch path and the identity/storage
setup around _storage_root and engine_file_store so deploy tasks never publish
an empty identity.userId while still claiming a valid team storage anchor.
Either require a non-empty client_id for deploy launches, or assign a documented
deploy-only internal identity and adjust Store.engine_file_store and deployment
record paths to use it consistently; preserve the existing team-scoped storage
behavior.
In `@packages/ai/tests/ai/account/test_store.py`:
- Around line 579-590: Update test_create_with_env_var to use pytest’s
monkeypatch fixture for the RR_STORE_URL override instead of directly modifying
os.environ. Set the variable through monkeypatch so cleanup occurs automatically
even when Store.create or an assertion fails, and remove the manual os.environ
cleanup.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: feeb99fe-856f-448b-aa24-7b7c01c3b5ca
📒 Files selected for processing (9)
packages/ai/src/ai/account/file_store.pypackages/ai/src/ai/account/models.pypackages/ai/src/ai/account/store.pypackages/ai/src/ai/modules/task/commands/cmd_store.pypackages/ai/src/ai/modules/task/task_engine.pypackages/ai/tests/ai/account/test_store.pypackages/ai/tests/ai/account/test_store_auth.pypackages/ai/tests/ai/modules/task/commands/test_cmd_task.pypackages/ai/tests/ai/modules/task/test_task_engine.py
…ocument resolver contract split (PR #1686) Addresses the three non-blocking notes from the authorization review: - file_store: sys.admin '=id' resolutions are the ONE place resolve_scope crosses the storage boundary instead of enforcing it (the platform support capability). Each crossing — foreign team/org/user id — now emits a server-side audit trace (actor, conn, target) via rocketlib.debug. Member '=id' resolutions cross nothing and are NOT audited. Trace-only: the wire response still cannot distinguish a crossing from an ordinary resolve, so the uniform-denial / no-oracle posture is preserved. - file_store: comment the internal-identity branch as deliberately ordered BEFORE the system-tree gate (intent, not fall-through) — the run-log writer and domain APIs write .logs/.deployments through that identity; test_internal_identity_writes_logs is now also the ordering pin. - models: banner both resolve_task_permissions (RETURNS []) and resolve_team_permissions (RAISES) with their opposite no-membership contracts. The rename to carry the difference in the name (get_* vs require_*) is deferred to the feat/alb dedupe, where both branches touch these functions — renaming here would fight that dedupe. - tests: TestSysAdminCrossingAudit pins that foreign-id crossings audit and member resolutions don't (monkeypatches the hook, since the sink is native rocketlib.debug). 757 account+task tests green (./builder ai:run-pytest path). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
@kwit75 — thank you, this was exactly the review this PR needed. Reading it as an authorization boundary rather than a storage refactor is the right lens, and the confirmations (no traversal escape, no existence oracle, engine confinement by construction, The multi-org finding — resolved by policy, not code. One org per user is the product invariant: The three smaller notes — all addressed in 10a399b:
Appreciate the offer on the two-org fixture and the user ids — the ids would help us confirm which org materializes for the 3 users without second-org team memberships before the saas cleanup. |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/ai/src/ai/account/models.py (1)
177-179: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winDocument the
enginesource value.
RequestContext.engine()setssource='engine'on Line 226, but this contract only documentslocalandorchestrator. It also describeslocalas OSS-only althoughRequestContext.internal()uses it.Proposed fix
- source: ``'local'`` for OSS standalone connections or - ``'orchestrator'`` for commands forwarded by the - Orchestrator via an internal pod connection. + source: ``'local'`` for local connections and trusted internal + callers, ``'orchestrator'`` for commands forwarded via + an internal pod connection, or ``'engine'`` for engine + subprocess tool nodes.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/ai/src/ai/account/models.py` around lines 177 - 179, Update the source-value documentation near RequestContext.engine() and RequestContext.internal() to include the 'engine' value and accurately describe when 'local' is used, including internal connections beyond OSS standalone connections. Keep the documented values aligned with the source assignments in these methods.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@packages/ai/src/ai/account/models.py`:
- Around line 177-179: Update the source-value documentation near
RequestContext.engine() and RequestContext.internal() to include the 'engine'
value and accurately describe when 'local' is used, including internal
connections beyond OSS standalone connections. Keep the documented values
aligned with the source assignments in these methods.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 046e4a98-0887-47f1-bfb4-a8cbcbe044e5
📒 Files selected for processing (3)
packages/ai/src/ai/account/file_store.pypackages/ai/src/ai/account/models.pypackages/ai/tests/ai/account/test_store_auth.py
asclearuc
left a comment
asclearuc
left a comment
There was a problem hiding this comment.
Thanks @Rod-Christensen — the architecture here is the right call. Moving authorization into file_store.py at the single point where paths resolve makes "every caller is checked" structural instead of a convention, the internal()/engine() factories make the trusted surface greppable, and the three cross-team holes closed on the way (on_execute's unchecked teamId feeding the secret merge, on_restart's defaultTeam-only check, the cprofile proxy) are real. test_store_auth.py is a genuinely strong matrix — the normalization-bypass fuzzing and the sys.admin crossing audit are exactly the tests I would have asked for.
Requesting changes on one point, plus three smaller ones:
- Signed download URLs 500 for every team- and org-scoped path.
get_url()signs the wire path into the JWT;/task/fetchre-resolves it underRequestContext.internal('fetch'), which accepts=<id>references only. I ran the resolver from this branch to confirm — table and detail inline onfetch.py. Filesystem backend only, which is the live ALB dev config (RR_STORE_URLunset, per the comment rocketride-ai/rocketride-saas#395 renames), andexplorer-uinow roots its tree at@, so team paths are the normal spelling the UI holds. rocketlib.getTask()returns the whole task file, which includes the resolved pipeline with substituted secrets, whereStore.engine_file_store()needs two keys. Parity with existingjobConfigaccess rather than a regression, but cheap to narrow while the API is new. Inline onbindings.cpp.Store.reset()has zero call sites — the singleton is never reset between tests, so the firstStore.instance()pins the backend and its shared registries for the whole session. Inline onstore.py, together with twoSTORE_URLdocstrings the rename missed.- Two consistency nits, inline on
file_store.pyandcmd_store.py.
Coordination note: rocketride-ai/rocketride-saas#395's submodule pointer (33e576a3) is not an ancestor of this branch's head (10a399b5) — the review-fix commit landed after that pointer was stamped, so that PR's CI is not currently exercising the code under review here. Worth re-stamping before either merges.
Non-blocking: nothing in the repo covers get_url or /task/fetch, which is why finding 1 got past a 613-line auth suite. One round-trip test — issue a URL for a team path as an entitled session, drive handle_fetch with the token, assert 200 — would have caught it and would guard the fix.
…aths, live Store.reset, single-parse resolver
- get_url now signs the RESOLVED physical store path instead of the wire
spelling. The fetch handler re-resolved the claim under an internal
identity, which has no name dictionary and no org context — so signed
URLs for '@/Team/<name>' and '@/Org' paths died with an unhandled
PermissionError (HTTP 500). Authorization already ran at issuance under
the session identity; the signed JWT is the capability, so the handler
now serves the claim verbatim through backend._get_full_path (which
guards traversal) with no scope re-resolution at all.
FileStore.resolve() existed solely for that handler re-resolution and
lost its only caller — removed rather than left as dead public surface.
New TestSignedFetchUrls pins the reviewer's failure matrix (plain,
@/User, @/Team by name, @/Team by id, @/Org), the resolve-free serve
path, and that issuance still gates (@/Org without org.admin cannot
mint a URL).
- Store.reset() is now actually wired: an autouse fixture in the ai tests
conftest drops the process singleton around EVERY test, so no test can
pin the RR_STORE_URL (plus _shared_handles/_shared_write_locks) that
happened to be live when the first singleton-reaching test ran —
singleton state was order-dependent and stateful before this.
Also corrected the two stale STORE_URL docstrings in store.py to
RR_STORE_URL (the legacy name hard-fails at create()).
- resolve_scope now returns (kind, scope_root, rest): it already had the
kind in hand from its own parse_scope call, so _resolve no longer
re-parses every path just to recover it — one parse per operation, and
no "cannot fail" reasoning left for readers to re-verify.
- _store_fs_stat's virtual-mount check now runs normalize_path — the same
ONE-normalization rule _store_fs_list_dir documents and follows —
instead of .strip('/'), so mount detection and store resolution can
never disagree about a spelling, and traversal raises instead of being
stripped.
Full ai suite: 1646 passed, 122 skipped.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The root package.json pinned EXACT @types/react 18.2.79 / react-dom 18.2.25 while shared-ui had moved to ~18.3.29, so pnpm materialized TWO peer-variant instances of every @types/react-peer package (react-markdown and friends) — duplicated store copies and forked type-checking between apps. One types version ends that: root, shell-ui, chat-ui, and dropper-ui now declare the same ~18.3.29 / ~18.3.7 ranges shared-ui already uses. Types-only change; chat-ui and dropper-ui type-check clean on the new majors, and the pre-existing shell-ui contract-check delta is unchanged (verified against a stashed baseline). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
4b12d76 was meant to carry these pins but landed only the lockfile reshuffle (a stash-pop failure dropped the package.json edits before the commit — message and content disagreed). This commit lands the actual alignment: root, shell-ui, chat-ui, dropper-ui at ~18.3.29/~18.3.7; vscode's floor raised to the resolved ^18.3.31; shared-ui's tilde floor follows to ~18.3.31. One @types/react instance in the lock (zero 18.2 refs), so @types/react-peer packages (react-markdown et al.) no longer fork into duplicate peer-variant copies. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… PyPI (#1725) The -gpu build of 1.20.1 is absent from the index (not yanked; the release key itself is gone), so every dependency resolve that misses uv's cache dies with "no version of onnxruntime-gpu==1.20.1 ... requirements are unsatisfiable". The CPU build of the same version survives, which is what let the split sit broken unnoticed. Move all five consumers to 1.22.0 together. It has wheels for -gpu on win/linux and for the CPU build on macOS-arm, which 1.20.1 no longer does. Note that the obvious one-patch bump is a trap: 1.20.2 exists only for -gpu, so pinning it would break macOS. Also correct the comment that justified the pin. It claimed rtmlib needs exactly this version; rtmlib declares onnxruntime unpinned, as does gliner, and faster-whisper asks for <2,>=1.14. The pin exists so the ~200 MB runtime installs once across consumers — the number is ours to choose, provided all five files agree. Verified with the four consuming nodes, opted in via ROCKETRIDE_INCLUDE_SKIP (real installs and model downloads, not just resolution): pose_estimation (rtmlib), anonymize, ner (gliner), audio_transcribe (faster-whisper) — all pass. Fixes #1723
asclearuc
left a comment
asclearuc
left a comment
There was a problem hiding this comment.
Round 4. Four of the five went in cleanly, and the round-3 fixes are better than what I asked for — resolve_scope returning (kind, scope_root, rest) reads well, dropping FileStore.resolve() once its only caller went away was the right instinct rather than leaving dead public surface, the autouse Store.reset() fixture is exactly the wiring that was missing, and TestSignedFetchUrls pins the failure matrix from my last comment including the issuance gate. Worth noting the fetch fix also repaired a second latent bug nobody had reported: a deploy-run engine context signs an anchored path (teams/<id>/files/tasks/<project>/x), and the old handler would have re-resolved that under users/<client_id>/files/ and served the wrong file or 404'd.
One MUST fix, and one carry-over:
- The claim changed meaning but did not change shape, so pre-deploy tokens are reinterpreted as physical paths. The old payload signed the wire path, the new one signs the resolved path, and neither carries a version marker — so
handle_fetchcannot tell them apart and serves an old token's wire path straight through_get_full_path. I ran it against the realFilesystemStore: an attacker-minted pre-deploy token reads another user's file, a foreign team's file, and system-tree content. Bounded to the token TTL (≤1h) after the new code goes live, and the fix is the'v': 2claim from my last comment. Detail inline onfetch.py. rocketlib.getTask()still returns the whole task file — including the resolved pipeline with substituted secrets. Not mentioned indde31f47, so flagging it as still open rather than assuming it was declined. Inline onbindings.cpp(unchanged from round 3); if the call is that you would rather keep the binding general and narrow it when a second consumer appears, say so and I will drop it.
Resolved and verified, no action needed:
get_url//task/fetch500 — fixed at the root by signing the resolved path and removing re-resolution entirely, rather than by patching the internal identity.TestSignedFetchUrlscovers plain,@/User,@/Teamby name,@/Teamby id, and@/Org.Store.reset()dead code — now an autouse fixture inpackages/ai/tests/conftest.pydropping the singleton around every test; both staleSTORE_URLdocstrings corrected.- Double parse in
_resolve—resolve_scopenow returns the kind it already had. fs_statnormalization — nownormalize_path, and it passes the normalized path down tostat()as well, which is more than I asked for.
Housekeeping: the file count went 39 → 52 because develop merges brought @types/react and the onnxruntime pin along. I checked the content — those 13 files are byte-identical between origin/develop and this branch, so it is a merge-base artifact in GitHub's view, not real divergence. Nothing to do; a rebase before merge would clear the noise. The saas pointer is also now correct: rocketride-ai/rocketride-saas#395 points at 4e7fecb4, which is this branch's head.
|
…-nodes Adapts to #1686's task-file identity model: the task engine no longer sets ROCKETRIDE_CLIENT_ID in node subprocesses (identity rides the 0600 task file; the ROCKETRIDE_* env namespace is caller-influenced by design), so - _build_subprocess_env keeps the credential scrubs + DSN injection but no longer sets the identity env var (env test updated to assert absence) - the vector store's connection subKey now keys on the DSN's tenant database (the actual isolation unit) instead of env identity - rocketride_db docs updated: current_client_id remains only for the account fallback used by server-process callers and tests Local: 106 passed (45 translator + 35 seam + 26 integration in RR_REQUIRE_DB_TESTS mode). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
5 participants
Summary
Storage/identity foundation for team-scoped deployments: deployments will belong to teams, so storage, permissions, and subprocess identity all had to stop assuming "everything belongs to the calling user". Deployment records and team run logs build on this next.
What & why
In-store authorization —
file_store.pyis the security boundary. Storage I/O happens from many places (rrext_store, rrext_log, fetch, the run-log writer, deployments). Checking permissions in command handlers meant every new caller could forget one. Now every path of every operation authorizes inside the store, at the single point where paths resolve, against theRequestContextbound at construction. Command handlers do zero storage permission checks.RequestContext identity (copied verbatim from feat/alb, with MERGE NOTEs) so that merge becomes a dedupe instead of a rework. The
internal()/engine()factories make the entire trusted surface greppable.Store singleton +
RR_STORE_URL(legacySTORE_URLhard-fails — a silent fallback would never get migrated). Providers restructured into packages; write locks and open handles moved to the shared Store so per-connection FileStore instances still exclude each other on one physical path.Joined filesystem, grammar v3.
@is a virtual root listingUser/,Team/,Org/so a file browser walks user, team, and org storage as one tree with ordinary path joining — while plain paths stay "my files" for nodes and SDK code. Bare segments are display names resolved only through the caller's own session membership (a foreign org's names are inexpressible by construction) and round-trip byte-exact.=<id>is the only cross-boundary spelling (sys.admin support access). Unknown / foreign / permission-less / ambiguous references all deny with one identical message (no existence leak).=-prefixed names are uncreatable at scope tops so id references can never be shadowed. Case-exact, alias-free; normalization runs before parsing (the\@\Team\...bypass family is fuzz-tested).System-owned trees.
.logsand.deploymentsare engine-written and served by their domain APIs (rrext_log today, rrext_deploy next). Through the file API they are invisible in listings and denied on access for every session identity except sys.admin. Domain APIs act through the internal identity and keep their owntask.monitor/task.controlgates.Three live cross-team permission holes closed:
on_executeaccepted an arbitraryteamIdand merged that team's secrets into the task env before any membership check (secret exfiltration);on_restartrestarted token-addressed tasks with only a defaultTeam check; the cprofile proxy could drive another team's engine subprocess. All three now resolve permissions against the addressed team/task first, with uniform denials.Task-file identity + storage anchor. Subprocess identity rides the 0600 task file (
identityblock) — never the environment (ROCKETRIDE_CLIENT_IDis gone). The newstorageblock gives nodes chroot semantics: dev runs anchor at the owner's tree (unchanged behavior), deploy runs atteams/<teamId>/files/tasks/<projectId>— node-level paths are identical in both modes, a deployed task needs no user, and concurrent deployments cannot fight over working storage. The engine context rejects every@spelling and system tree regardless of anchor.rocketlib.getTask()C++ binding. The engine already parses the task file, so Python re-reading argv would duplicate parsing and couple to the launch contract.ITask::execute()publishes the task JSON around the begin/end window (beforebeginTask, because endpoint and node-global construction happens inside it) and clears it on every exit path. The store layer resolves identity + anchor itself (Store.engine_file_store()), so tool_filesystem carries zero jobConfig plumbing.Also: explorer-ui roots its listing at
@to present the joined tree.Testing
rocketlib.getTaskverified live (returnsNonewhen idle).Pairs with the saas
feat/deploy-1PR (alembic migration foundation + submodule pointer). Merge this one first, then re-stamp the saas pointer.🤖 Generated with Claude Code
Summary by CodeRabbit