refactor(assets): replace CWD-relative paths with configurable asset_root by staging-devin-ai-integration[bot] · Pull Request #520 · streamer45/streamkit

staging-devin-ai-integration · 2026-05-25T14:47:39Z

Summary

Add an optional asset_root field to Config / AppState that controls where samples/ paths are resolved. When unset (the default), the process working directory is used — preserving existing behavior.
All asset handlers (audio, images, fonts, plugin assets) now join paths against asset_root instead of hardcoding relative paths against CWD.
Asset tests no longer mutate CWD via figment::Jail — each handler test creates a TempDir and passes it as asset_root, eliminating the cross-module CWD race with config.rs tests.

Closes #478

Review & Validation

Verify the server still works when launched from repo root with default config (no asset_root set) — behavior should be identical to before.
Confirm just test passes without flakes — the CWD race between assets::tests and config::tests should no longer be possible.
Check that plugin asset handlers (list, upload, delete, serve, update) correctly resolve RegisteredAssetType paths against asset_root.

Notes

asset_root is excluded from the JSON schema (#[schemars(skip)]) since it's an internal implementation detail, not a user-facing config knob.
The figment dev-dependency is no longer used by assets.rs tests but remains in Cargo.toml because config.rs tests still use it.

Link to Devin session: https://staging.itsdev.in/sessions/946c01a5ae7042019d4b570cf76ea860
Requested by: @streamer45

Devin Review

Status	Commit
🟢 Reviewed	`90ab09f`

…root Add an optional `asset_root` field to Config / AppState. All asset handlers (audio, images, fonts, plugin assets) now resolve `samples/` paths against this root instead of the process working directory. Tests no longer mutate CWD via `figment::Jail` — each handler test creates a `TempDir` and passes it as `asset_root`, eliminating the cross-module CWD race with `config.rs` tests (issue #478). Closes #478 Signed-off-by: streamkit-devin <devin@streamkit.dev>

staging-devin-ai-integration · 2026-05-25T14:47:44Z

🤖 Devin AI Engineer

I'll be helping with this pull request! Here's what you should know:

✅ I will automatically:

Address comments on this PR. Add '(aside)' to your comment to have me ignore it.
Look at CI failures and help fix them

Note: I can only respond to comments from users who have write access to this repository.

⚙️ Control Options:

Disable automatic comment, CI, and merge conflict monitoring

staging-devin-ai-integration

Devin Review found 3 potential issues.

🐛 1 issue in files not directly in the diff

🐛 Custom asset_root stores assets where returned pipeline paths cannot resolve (`apps/skit/src/assets.rs:208`)

The asset handlers now read/write files under app_state.asset_root, but the API still returns paths like samples/audio/... (for example apps/skit/src/assets.rs:129-133 and apps/skit/src/assets.rs:320) and downstream file-reader validation resolves those relative paths against the process working directory, not asset_root (apps/skit/src/file_security.rs:29-44). When asset_root is configured to anything other than the current working directory (apps/skit/src/config.rs:994-999), uploads/listing succeed against <asset_root>/samples/..., but selecting the returned asset path in a pipeline either fails validation as missing/outside the allowlist or reads a different stale file from <cwd>/samples/.... The same path-space mismatch also affects the image/font/plugin asset changes that join asset_root for storage/serving while preserving samples/... asset paths.

Debug

Playground

staging-devin-ai-integration · 2026-05-25T14:50:53Z

+    /// Root directory for sample assets (`samples/audio`, `samples/images`,
+    /// `samples/fonts`, and plugin asset directories).  When `None` (the
+    /// default), the process working directory is used.
+    #[serde(default, skip_serializing_if = "Option::is_none")]
+    #[schemars(skip)]
+    pub asset_root: Option<std::path::PathBuf>,


🚩 asset_root is runtime-only and intentionally hidden from generated config schema

asset_root is added with #[schemars(skip)] and is not present in the sample or Docker configs. That can be reasonable for test/runtime injection, but if this is intended as a supported operator-facing setting, reviewers should confirm how users are expected to discover and set it, and whether generated docs should mention it. This is not a correctness bug by itself because serde/figment can still deserialize the field from TOML/env even when schemars skips it.

Was this helpful? React with 👍 or 👎 to provide feedback.

Debug

Playground

Correct — asset_root is intentionally not user-facing. It exists purely for test isolation (so handler tests can use a TempDir instead of mutating process CWD). The #[schemars(skip)] ensures it stays out of generated docs/schema. In production the field is always None, which falls back to current_dir() — identical to the pre-refactor behavior.

staging-devin-ai-integration · 2026-05-25T14:50:54Z

+    let system_dir = app_state.asset_root.join(&asset_type.system_dir);
+    let user_dir = app_state.asset_root.join(&asset_type.user_dir);


📝 Info: Plugin asset permission patterns remain logical paths, not filesystem paths

The plugin asset handlers now scan under asset_root, but process_entry still builds permission checks from the registered logical directory (asset_type.system_dir.parent()), and role_extractor augments permissions from the same registered system_dir/user_dir values. That means roles continue matching samples/...-style API paths rather than absolute filesystem paths under asset_root. I did not flag this as a separate bug because the core asset handlers follow the same logical-path permission model, and changing permissions to filesystem paths would break existing allowlists.

Was this helpful? React with 👍 or 👎 to provide feedback.

Debug

Playground

Correct — permissions continue to use logical samples/... paths, which is the intended behavior. The asset_root join only affects filesystem I/O (where to read/write files), not the API-level asset path strings used for permission checks and pipeline references.

staging-devin-ai-integration · 2026-05-25T14:51:24Z

Re the path-space mismatch concern: this is correct in theory, but in practice asset_root is only used for test isolation — it's hidden from the config schema (#[schemars(skip)]) and defaults to current_dir() when unset. In production the field is always None, so asset_root == CWD and the returned samples/... paths resolve correctly against the working directory, exactly as before this refactor.

If asset_root were ever promoted to a user-facing config option, the downstream path resolution in file_security.rs would indeed need to be updated — but that's out of scope for this PR, which is strictly about eliminating the CWD race in tests.

staging-devin-ai-integration · 2026-05-25T14:51:37Z

Filed #521 to track the path-space mismatch if asset_root is ever promoted to a user-facing config option. Currently no production impact since the field defaults to CWD and is hidden from the schema.

codecov · 2026-05-25T15:03:08Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 79.41%. Comparing base (b2d01c2) to head (90ab09f).

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #520      +/-   ##
==========================================
+ Coverage   79.39%   79.41%   +0.02%     
==========================================
  Files         232      232              
  Lines       66904    66912       +8     
  Branches     1909     1909              
==========================================
+ Hits        53117    53137      +20     
+ Misses      13781    13769      -12     
  Partials        6        6

Flag	Coverage Δ
backend	`79.14% <100.00%> (+0.02%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Components	Coverage Δ
core	`85.27% <ø> (ø)`
engine	`83.68% <ø> (ø)`
api	`89.98% <ø> (ø)`
nodes	`75.38% <ø> (+0.04%)`	⬆️
server	`80.26% <100.00%> (+<0.01%)`	⬆️
plugin-native	`83.47% <ø> (ø)`
plugin-wasm	`91.90% <ø> (ø)`
ui-services	`84.67% <ø> (ø)`
ui-components	`60.49% <ø> (ø)`

Files with missing lines	Coverage Δ
apps/skit/src/assets.rs	`93.41% <100.00%> (-0.02%)`	⬇️
apps/skit/src/config.rs	`97.53% <ø> (ø)`
apps/skit/src/plugin_assets.rs	`97.83% <100.00%> (+<0.01%)`	⬆️
apps/skit/src/server/mod.rs	`80.79% <100.00%> (+0.05%)`	⬆️
apps/skit/src/state.rs	`95.23% <ø> (ø)`

... and 1 file with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

staging-devin-ai-integration Bot assigned streamer45 May 25, 2026

staging-devin-ai-integration Bot requested a review from streamer45 May 25, 2026 14:47

staging-devin-ai-integration Bot commented May 25, 2026

View reviewed changes

staging-devin-ai-integration Bot mentioned this pull request May 25, 2026

file_security.rs resolves asset paths against CWD, not asset_root #521

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(assets): replace CWD-relative paths with configurable asset_root#520

refactor(assets): replace CWD-relative paths with configurable asset_root#520
staging-devin-ai-integration[bot] wants to merge 1 commit into
mainfrom
devin/1779719236-configurable-asset-root

staging-devin-ai-integration Bot commented May 25, 2026 •

edited

Loading

Uh oh!

staging-devin-ai-integration Bot commented May 25, 2026

Uh oh!

staging-devin-ai-integration Bot left a comment

Uh oh!

staging-devin-ai-integration Bot May 25, 2026

Uh oh!

staging-devin-ai-integration Bot May 25, 2026

Uh oh!

staging-devin-ai-integration Bot May 25, 2026

Uh oh!

staging-devin-ai-integration Bot May 25, 2026

Uh oh!

staging-devin-ai-integration Bot commented May 25, 2026

Uh oh!

staging-devin-ai-integration Bot commented May 25, 2026

Uh oh!

codecov Bot commented May 25, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

		let system_dir = app_state.asset_root.join(&asset_type.system_dir);
		let user_dir = app_state.asset_root.join(&asset_type.user_dir);

Conversation

staging-devin-ai-integration Bot commented May 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Review & Validation

Notes

Devin Review

Uh oh!

staging-devin-ai-integration Bot commented May 25, 2026

🤖 Devin AI Engineer

Uh oh!

staging-devin-ai-integration Bot left a comment

Choose a reason for hiding this comment

🐛 Custom asset_root stores assets where returned pipeline paths cannot resolve (apps/skit/src/assets.rs:208)

Uh oh!

staging-devin-ai-integration Bot May 25, 2026

Choose a reason for hiding this comment

Uh oh!

staging-devin-ai-integration Bot May 25, 2026

Choose a reason for hiding this comment

Uh oh!

staging-devin-ai-integration Bot May 25, 2026

Choose a reason for hiding this comment

Uh oh!

staging-devin-ai-integration Bot May 25, 2026

Choose a reason for hiding this comment

Uh oh!

staging-devin-ai-integration Bot commented May 25, 2026

Uh oh!

staging-devin-ai-integration Bot commented May 25, 2026

Uh oh!

codecov Bot commented May 25, 2026

Codecov Report

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

staging-devin-ai-integration Bot commented May 25, 2026 •

edited

Loading

🐛 Custom asset_root stores assets where returned pipeline paths cannot resolve (`apps/skit/src/assets.rs:208`)