Skip to content

fix(rivetkit): match agentOS database vfs directory prefixes literally - #5794

Open
breken-ai wants to merge 1 commit into
rivet-dev:mainfrom
breken-ai:fix/agent-os-vfs-prefix-match
Open

breken-ai wants to merge 1 commit into
rivet-dev:mainfrom
breken-ai:fix/agent-os-vfs-prefix-match

Conversation

@breken-ai

Copy link
Copy Markdown

Description

  • createDatabaseVfs found a directory's children and descendants with path LIKE '<dir>/%'. In SQLite LIKE, _ and % are wildcards and ASCII letters match without regard to case, so a directory named /my_dir also matched /myXdir/... and /MY_DIR/....
  • As a result readDir("/my_dir") listed files from those sibling directories, removeDir failed with ENOTEMPTY on an empty directory whose name has _ or %, and rename("/work_dir", "/moved") also moved the files of /workXdir and /Work_Dir into /moved.
  • Both queries now compare the path prefix literally with substr(path, 1, length(?)) = ?.
  • Added database-vfs.test.ts, which runs the VFS against a real in-memory SQLite database (node:sqlite, as in unstable/migrations.test.ts).

Note: the same LIKE prefix pattern appears in the Rust port in the draft #5309.

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How Has This Been Tested?

  • pnpm vitest run src/agent-os/fs/database-vfs.test.ts in rivetkit-typescript/packages/rivetkit. On main all 3 tests fail: readDir returns [ 'other.txt', 'upper.txt', 'own.txt' ], removeDir("/a_b") throws ENOTEMPTY, and the renamed directory contains sibling.txt and upper.txt. With this change all 3 pass.
  • biome check is clean on the changed files. tsc --noEmit for the package reports no errors in the changed files.
  • vitest run src for the package: the only failures are the same 3 that fail on main in my local setup (inline-websocket-adapter.test.ts and runtime.test.ts fail to load, and one workflow/driver.test.ts case), so this change adds no new failures.

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

@the-company-company the-company-company Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟠 1 medium-severity finding

Reviewed commit 803e626.

const rows = await db.execute<FsRow>(
"SELECT * FROM agent_os_fs_entries WHERE path LIKE ? AND path != ?",
`${prefix}%`,
"SELECT * FROM agent_os_fs_entries WHERE substr(path, 1, length(?)) = ? AND path != ?",

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟠 Medium · Literal prefix lookups still scan the entire filesystem table

Applying substr to path makes the path index unusable; EXPLAIN QUERY PLAN reports SCAN agent_os_fs_entries for this predicate. Because this helper backs every directory read and emptiness check, each operation examines every VFS entry, and the identical rename predicate has the same cost. Use an indexable binary range instead: since every normalized prefix ends in /, bind the lower bound to prefix and the exclusive upper bound to `${prefix.slice(0, -1)}0`, then apply the same predicate to the descendant query. This remains literal and case-sensitive while allowing the existing path index to bound the scan.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant