feat(plugins): directory-per-plugin layout and unified native loader by staging-devin-ai-integration[bot] · Pull Request #272 · streamer45/streamkit

staging-devin-ai-integration · 2026-04-08T12:03:13Z

Summary

Implements two outstanding items from #254:

Local plugin directory layout — just copy-plugins-native now outputs a directory-per-plugin structure (.plugins/native/<id>/lib*.so + plugin.yml) instead of a flat layout. The loader only accepts subdirectory-based plugins; bare .so files at the top level are warned and skipped. This is a breaking change for anyone with a manual flat layout.
Unified native loader — load_all_native_plugins() merges the old load_active_plugins_from_dir and load_native_plugins_from_dir into a single two-phase pass:
- Phase 1: active records (marketplace bundles from .plugins/bundles/)
- Phase 2: directory bundles (.plugins/native/<id>/)
Plugins loaded by an earlier phase are automatically skipped so marketplace versions always take precedence.

Additional fixes from review feedback

Shared conflict detection — extracted check_kind_conflict() used by both load_native_plugin and check_native_upload_conflict, eliminating duplicated logic.
Sentinel constants — ERR_ALREADY_LOADED / ERR_ALREADY_REGISTERED replace fragile inline string matching for dedup detection.
Temp file leak fix — load_from_bytes now cleans up the .tmp file on rename failure.
Empty subdir leak fix — try_remove_empty_plugin_dir called unconditionally on error (not only when file_placed is true).
Upload conflict probing — check_native_upload_conflict probes plugin kind from a temp file before final placement, preventing overwrite of existing plugins.
Upload stem validation — rejects edge-case filenames like lib..so (stem .) and lib.so (empty stem).
Deterministic ordering — dir_bundle_libs sorted before loading.
Native dir read failure logged — warn! on read_dir failure (was silent).
Dedup skips at debug level — "already loaded" skips log at debug! instead of misleading warn!.
gain-native uses copy_plugin — consistent with all other plugins in the justfile.
aac-encoder added to the copy loop.

Review & Testing Checklist for Human

Run just copy-plugins-native after a build and verify the new directory layout (.plugins/native/<id>/lib*.so + plugin.yml per plugin)
Start the server with just skit and confirm all native plugins load from the directory layout (check logs for "Loaded native plugin from directory bundle")
Verify that a bare .so file placed directly in .plugins/native/ produces a warning and is not loaded
Test HTTP plugin upload: upload a plugin .so and verify it creates the correct subdirectory structure
Test duplicate upload: upload a plugin whose kind is already loaded and verify the existing plugin is preserved (not overwritten)

Notes

This is a breaking change for v0.x: bare .so files in .plugins/native/ are no longer loaded. Users must migrate to the directory layout.
The load_from_bytes method is currently unused at runtime (HTTP uploads use load_from_temp_file) but is kept as a public API for non-streaming callers. It has full test coverage via test_duplicate_upload_preserves_existing_plugin.
check_native_upload_conflict sets 0o755 on the probe file for dlopen, and load_from_written_path sets it again on the final file after the move — these are intentionally on different files.

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

Update `just copy-plugins-native` to output a directory-per-plugin structure under `.plugins/native/<id>/` instead of the flat layout. Each plugin now gets its own subdirectory containing the library and `plugin.yml` manifest. Also adds aac-encoder to the copy loop. Merge `load_active_plugins_from_dir` and `load_native_plugins_from_dir` into a single `load_all_native_plugins` method that discovers plugins from three sources in priority order: 1. Active records (.plugins/active/*.json) — marketplace bundles 2. Directory bundles (.plugins/native/<id>/) — local directory layout 3. Bare .so files (.plugins/native/*.so) — legacy flat fallback Plugins already loaded by a higher-priority source are skipped. Update bundle convention docs to reflect implemented changes. Closes #254 Signed-off-by: StreamKit Devin <devin@streamkit.dev> Co-Authored-By: Claudio Costa <cstcld91@gmail.com>

staging-devin-ai-integration · 2026-04-08T12:03:17Z

🤖 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 and CI monitoring

staging-devin-ai-integration

✅ Devin Review: No Issues Found

Devin Review analyzed this PR and found no potential bugs to report.

View in Devin Review to see 3 additional findings.

Debug

Playground

- Use copy_plugin helper for gain-native instead of raw cp + glob, ensuring consistent handling of versioned .so.* and plugin.yml. - Log a warning when the native plugins directory cannot be read, matching the behavior of load_active_plugin_records. - Distinguish 'already loaded' skips (debug level) from genuine load failures (warn level) to avoid misleading log output. - Sort discovered library paths for deterministic load order. - Add comment explaining the source-label heuristic. Signed-off-by: StreamKit Devin <devin@streamkit.dev> Co-Authored-By: Claudio Costa <cstcld91@gmail.com>

Since we are pre-1.0 (v0.x), enforce the directory-per-plugin layout everywhere instead of maintaining a legacy flat-file fallback: - load_native_dir_plugins now only scans subdirectories of .plugins/native/, ignoring top-level .so files. - validate_plugin_upload_target places HTTP-uploaded native plugins into a subdirectory derived from the library stem (e.g. libgain.so → .plugins/native/gain/libgain.so). - unload_plugin cleans up the parent directory if empty after removing the .so file. - Updated docs and code comments to reflect the two-source model (active records + directory bundles). Signed-off-by: StreamKit Devin <devin@streamkit.dev> Co-Authored-By: Claudio Costa <cstcld91@gmail.com>

staging-devin-ai-integration

Devin Review found 2 new potential issues.

View 7 additional findings in Devin Review.

Debug

Playground

apps/skit/src/plugins.rs

staging-devin-ai-integration · 2026-04-08T13:40:43Z

apps/skit/src/plugins.rs

+    fn load_native_dir_plugins(&mut self) -> Vec<PluginSummary> {
+        let mut lib_paths: Vec<std::path::PathBuf> = Vec::new();

-            if path.is_dir() {
-                // Scan one level of subdirectories for plugin bundles.
-                if let Ok(sub_entries) = std::fs::read_dir(&path) {
-                    for sub_entry in sub_entries.flatten() {
-                        let sub_path = sub_entry.path();
-                        if Self::is_native_lib(&sub_path) {
-                            lib_paths.push(sub_path);
+        match std::fs::read_dir(&self.native_directory) {
+            Ok(entries) => {
+                for entry in entries.flatten() {
+                    let path = entry.path();
+                    if !path.is_dir() {
+                        continue;
+                    }
+                    // Scan one level of subdirectories for plugin libraries.
+                    if let Ok(sub_entries) = std::fs::read_dir(&path) {
+                        for sub_entry in sub_entries.flatten() {
+                            let sub_path = sub_entry.path();
+                            if Self::is_native_lib(&sub_path) {
+                                lib_paths.push(sub_path);
+                            }
                        }
                    }
                }
-                continue;
-            }
-
-            if Self::is_native_lib(&path) {
-                lib_paths.push(path);
-            }
+            },
+            Err(err) => {
+                warn!(
+                    error = %err,
+                    dir = %self.native_directory.display(),
+                    "Failed to read native plugins directory"
+                );
+            },
        }


🚩 Bare .so files in native directory are now warned and skipped (breaking change for existing flat layouts)

The old load_native_plugins_from_dir loaded bare .so files directly from .plugins/native/. The new load_native_dir_plugins (lines 290-299) explicitly skips bare files with a warning, requiring plugins to be in subdirectories. Users who had a flat layout (e.g., .plugins/native/libslint.so) will see their plugins stop loading after this change, with only a warning in logs. The doc update at plugin-bundle-convention.md documents this as the new convention. The justfile copy-plugins-native recipe is updated to use the directory layout, so just install-plugins will produce the correct structure. Manual installations would need migration.

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

Debug

Playground

This is intentional — the user explicitly requested dropping the bare .so fallback since we're at v0.x. However, good point about the silent behavior. Added a warn! log in c103a07 that fires when bare .so files are found in the native directory, guiding users to move them into subdirectories.

- Guard against lib.so/lib.dylib/lib.dll uploads where stripping the 'lib' prefix yields an empty stem, which would place the file flat in the native directory. Now falls back to the full filename. - Log a warning when bare .so files are found directly in the native directory, guiding users to move them into subdirectories. Signed-off-by: StreamKit Devin <devin@streamkit.dev> Co-Authored-By: Claudio Costa <cstcld91@gmail.com>

…upload - Update comment from 'Phase 2 & 3' to 'Phase 2' since bare .so files are no longer loaded. - Clean up the subdirectory when a native plugin upload fails to load, mirroring the cleanup logic in unload_plugin. Signed-off-by: StreamKit Devin <devin@streamkit.dev> Co-Authored-By: Claudio Costa <cstcld91@gmail.com>

Deduplicate the directory cleanup logic used by unload_plugin, load_from_bytes, and load_from_temp_file into a shared helper method. Also fixes load_from_bytes which was missing cleanup on failure. Signed-off-by: StreamKit Devin <devin@streamkit.dev> Co-Authored-By: Claudio Costa <cstcld91@gmail.com>

…ling - Reject stems that resolve to '.' or '..' (e.g. lib..so) which would create problematic directories. - Document the coupling between load_native_plugin's error messages and load_native_dir_plugins' string-matching control flow, at both the producer and consumer sites. Signed-off-by: StreamKit Devin <devin@streamkit.dev> Co-Authored-By: Claudio Costa <cstcld91@gmail.com>

Use closure-based error handling in load_from_bytes and load_from_temp_file so that try_remove_empty_plugin_dir runs on every error path after validate_plugin_upload_target creates the subdirectory, not just when load_from_written_path fails. Signed-off-by: StreamKit Devin <devin@streamkit.dev> Co-Authored-By: Claudio Costa <cstcld91@gmail.com>

When uploading a native plugin whose kind is already loaded, the old code would move the new file to the target path (overwriting the existing library), then discover the conflict and delete the file — destroying the original plugin's .so on disk. Now load_from_temp_file probes the plugin kind from the temp file (via check_native_upload_conflict) before touching the final location. If the kind conflicts, the upload is rejected immediately and the existing library file is never overwritten. Also adds an integration test (test_duplicate_upload_preserves_existing_plugin) that verifies: duplicate upload is rejected with 422, the original .so file is preserved on disk, and the plugin remains loaded. Signed-off-by: StreamKit Devin <devin@streamkit.dev> Co-Authored-By: Claudio Costa <cstcld91@gmail.com>

…here The error handler in load_from_temp_file was unconditionally deleting target_path on any error. When the conflict probe (check_native_upload_conflict) fails *before* the move/copy, target_path points to the pre-existing plugin's library — deleting it destroys the original. Track placement with a file_placed flag so we only remove files we created. Signed-off-by: StreamKit Devin <devin@streamkit.dev> Co-Authored-By: Claudio Costa <cstcld91@gmail.com>

…licts Mirrors the load_from_temp_file pattern: write bytes to a .tmp file, run check_native_upload_conflict on it, then rename to the final path. This prevents overwriting an existing plugin's library file when the uploaded kind is already loaded. Also adds the file_placed guard so the error handler only cleans up files that were actually placed at target_path. Signed-off-by: StreamKit Devin <devin@streamkit.dev> Co-Authored-By: Claudio Costa <cstcld91@gmail.com>

…cleanup - Extract shared check_kind_conflict() helper used by both load_native_plugin and check_native_upload_conflict, eliminating duplicated conflict-check logic. - Extract ERR_ALREADY_LOADED / ERR_ALREADY_REGISTERED as shared constants, replacing fragile string literals in both the producer (check_kind_conflict) and consumer (load_native_dir_plugins). - Fix temp file leak in load_from_bytes: clean up .tmp file on rename failure instead of letting it accumulate on disk. - Fix empty subdirectory leak: call try_remove_empty_plugin_dir unconditionally on error (not only when file_placed is true), so subdirectories created by validate_plugin_upload_target are cleaned up even when the conflict check fails before placement. - Fix stale 'Phase 2 & 3' comment (bare .so fallback was removed). - Clarify redundant-looking permissions comment (probe file vs final file are different). - Improve dead_code annotation rationale on load_from_bytes. Signed-off-by: StreamKit Devin <devin@streamkit.dev> Co-Authored-By: Claudio Costa <cstcld91@gmail.com>

Bump the dedup-skip log from debug to warn and include both the loaded (marketplace) version and the skipped (local) version read from plugin.yml manifests. This helps operators spot cases where a marketplace install is outdated relative to a locally patched plugin. Signed-off-by: StreamKit Devin <devin@streamkit.dev> Co-Authored-By: Claudio Costa <cstcld91@gmail.com>

Move the uploaded temp file into the plugin subdirectory (.tmp extension) before dlopen-probing it. The original temp file may reside on a noexec mount (e.g. /tmp on hardened hosts), causing dlopen to fail even though .plugins/native/ would work fine. The probe file is cleaned up on conflict or rename failure, and an existing plugin's library is never overwritten until the probe passes. Signed-off-by: StreamKit Devin <devin@streamkit.dev> Co-Authored-By: Claudio Costa <cstcld91@gmail.com>

staging-devin-ai-integration

Devin Review found 1 new potential issue.

View 13 additional findings in Devin Review.

Debug

Playground

staging-devin-ai-integration · 2026-04-08T20:16:48Z

apps/skit/src/plugins.rs

+            } else {
+                // WASM plugins don't need dlopen probing; move directly.
+                if let Err(e) = std::fs::rename(temp_path, &target_path) {
+                    debug!(
+                        error = %e,
+                        from = %temp_path.display(),
+                        to = %target_path.display(),
+                        "rename failed; falling back to copy+remove"
+                    );
+                    std::fs::copy(temp_path, &target_path).with_context(|| {
+                        format!(
+                            "failed to copy temp plugin file from {} to {}",
+                            temp_path.display(),
+                            target_path.display()
+                        )
+                    })?;
+                    let _ = std::fs::remove_file(temp_path);
+                }


🚩 WASM upload path still overwrites existing plugin files on duplicate upload

The PR adds a check_native_upload_conflict probe mechanism for native plugins to prevent overwriting an existing plugin's library on duplicate upload. However, the WASM path in both load_from_temp_file (plugins.rs:1105-1122) and load_from_bytes (plugins.rs:1017-1021) still moves/writes directly to target_path without conflict checking. If a WASM plugin with the same filename is already installed, the rename/copy overwrites the existing file. If load_wasm_plugin then rejects it (kind already loaded), the error handler deletes the new file — but the original is already gone. This is pre-existing behavior (identical to the old code), so it's not a regression, but it's an asymmetry worth noting since native uploads are now protected.

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

Debug

Playground

Correct — the WASM upload path has the same pre-existing overwrite behavior. This is not a regression from this PR. WASM plugins don't have an equivalent of dlopen probing, so a conflict check would need a different approach (e.g. parsing the WASM module to extract the kind before moving). Worth tracking as a follow-up if WASM plugin uploads become common.

can we validate?

Validated — the finding is accurate. Here's the trace:

load_from_temp_file WASM path (lines 1105-1122):

std::fs::rename(temp_path, &target_path) — overwrites any existing .wasm at target_path

file_placed = true

load_wasm_plugin rejects (kind already loaded, line 713)

Error handler deletes target_path — original file already gone at step 1

load_from_bytes WASM path (lines 1017-1021): Same — std::fs::write overwrites directly.

Practical risk: Low — requires same filename to collide (target path is derived from filename, not kind). A same-filename re-upload is the realistic trigger.

Fix approach if desired: For WASM, we can't cheaply probe the kind without compiling the module. But we can use the same .tmp staging pattern — move to target_path.with_extension("tmp"), attempt load_wasm_plugin from there, and only rename to final path on success. This mirrors the native flow. Alternatively, a simpler filename-existence check (target_path.exists()) would catch same-file re-uploads, though it wouldn't catch different-filename-same-kind conflicts.

Also noted: load_wasm_plugin (lines 713-731) has its own inline plugins.contains_key + registry.contains checks — a third copy of the conflict logic that doesn't use the shared check_kind_conflict helper. Not a regression, but worth consolidating in a follow-up.

staging-devin-ai-integration bot assigned streamer45 Apr 8, 2026

staging-devin-ai-integration bot requested a review from streamer45 April 8, 2026 12:03

staging-devin-ai-integration bot commented Apr 8, 2026

View reviewed changes

streamkit-devin and others added 2 commits April 8, 2026 13:03

staging-devin-ai-integration bot commented Apr 8, 2026

View reviewed changes

This comment was marked as resolved.

Sign in to view

streamkit-devin and others added 2 commits April 8, 2026 13:55

This comment was marked as resolved.

Sign in to view

streamkit-devin and others added 3 commits April 8, 2026 14:13

This comment was marked as resolved.

Sign in to view

streamkit-devin and others added 5 commits April 8, 2026 15:03

Merge branch 'main' into devin/1775648777-plugin-directory-convention

4df62fa

staging-devin-ai-integration bot commented Apr 8, 2026

View reviewed changes

Conversation

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

Uh oh!

Summary

Additional fixes from review feedback

Review & Testing Checklist for Human

Notes

Uh oh!

staging-devin-ai-integration bot commented Apr 8, 2026

🤖 Devin AI Engineer

Uh oh!

staging-devin-ai-integration bot left a comment

Choose a reason for hiding this comment

✅ Devin Review: No Issues Found

Uh oh!

staging-devin-ai-integration bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

staging-devin-ai-integration bot Apr 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

staging-devin-ai-integration bot Apr 8, 2026

Choose a reason for hiding this comment

Uh oh!

This comment was marked as resolved.

Uh oh!

This comment was marked as resolved.

Uh oh!

This comment was marked as resolved.

Uh oh!

This comment was marked as resolved.

Uh oh!

staging-devin-ai-integration bot left a comment

Choose a reason for hiding this comment

Uh oh!

staging-devin-ai-integration bot Apr 8, 2026

Choose a reason for hiding this comment

Uh oh!

staging-devin-ai-integration bot Apr 8, 2026

Choose a reason for hiding this comment

Uh oh!

streamer45 Apr 8, 2026

Choose a reason for hiding this comment

Uh oh!

staging-devin-ai-integration bot Apr 8, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

staging-devin-ai-integration bot commented Apr 8, 2026 •

edited

Loading

staging-devin-ai-integration bot Apr 8, 2026 •

edited

Loading