chore: simplify and clarify server, CLI, and integration tests#403
Conversation
…ules Signed-off-by: StreamKit Devin <devin@streamkit.dev> Co-Authored-By: Claudio Costa <cstcld91@gmail.com>
…ules Signed-off-by: StreamKit Devin <devin@streamkit.dev> Co-Authored-By: Claudio Costa <cstcld91@gmail.com>
Signed-off-by: StreamKit Devin <devin@streamkit.dev> Co-Authored-By: Claudio Costa <cstcld91@gmail.com>
Signed-off-by: StreamKit Devin <devin@streamkit.dev> Co-Authored-By: Claudio Costa <cstcld91@gmail.com>
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
![staging-devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/870844?s=60&v=4){kind=small}
| pub const fn is_enabled(&self) -> bool { | ||
| self.enabled | ||
| } | ||
|
|
||
| /// Get the revocation store (for checking revocation status). | ||
| pub fn revocation_store(&self) -> Option<&Arc<dyn RevocationStore>> { | ||
| self.revocation_store.as_ref() | ||
| } | ||
|
|
||
| /// Check if a token is revoked by its token hash. | ||
| /// | ||
| /// Returns false if auth is disabled or if the revocation store is not available. | ||
| #[allow(dead_code)] | ||
| pub fn is_revoked(&self, token_hash: &str) -> bool { | ||
| self.revocation_store.as_ref().is_some_and(|store| store.is_revoked(token_hash)) | ||
| } | ||
|
|
||
| /// Get the token metadata store. | ||
| pub fn token_metadata_store(&self) -> Option<&Arc<dyn TokenMetadataStore>> { | ||
| self.token_metadata_store.as_ref() | ||
| } | ||
|
|
||
| /// Get the key provider. | ||
| #[allow(dead_code)] | ||
| pub fn key_provider(&self) -> Option<&Arc<dyn KeyProvider>> { | ||
| self.key_provider.as_ref() |
There was a problem hiding this comment.
📝 Info: Doc comments removed from public accessor methods on AuthState
Four trivial accessor methods on pub struct AuthState lost their doc comments: is_enabled(), revocation_store(), token_metadata_store(), and key_provider(). While CONTRIBUTING.md says "Add doc comments for public APIs," these are self-explanatory getters (e.g., the removed doc for is_enabled() was just "Check if authentication is enabled."). The crate is publish = false and the latest commit in this PR already restored doc comments on non-trivial methods like is_revoked(). This is a judgment call — the removed docs were redundant, but some teams prefer all pub items to be documented regardless.
Was this helpful? React with 👍 or 👎 to provide feedback.
Debug
There was a problem hiding this comment.
Good catch — the is_revoked doc comment documented non-obvious fallback behavior (returns false when auth is disabled or revocation store unavailable). Restored it in f9b6059. The other four trivial getter doc comments remain removed since they're self-documenting.
| /// Check if a sample pipeline path is allowed. | ||
| /// | ||
| /// Empty list = nothing allowed (secure by default). Use `["*"]` to allow all. | ||
| pub fn is_sample_allowed(&self, path: &str) -> bool { | ||
| // Empty list means nothing is allowed (secure by default) | ||
| // Use ["*"] wildcard to allow everything | ||
| if self.allowed_samples.is_empty() { | ||
| return false; | ||
| } | ||
|
|
||
| // Check against glob patterns | ||
| self.allowed_samples | ||
| .iter() | ||
| .any(|pattern| Pattern::new(pattern).ok().is_some_and(|p| p.matches(path))) | ||
| } | ||
|
|
||
| /// Check if a node type is allowed | ||
| /// Check if a node type is allowed. | ||
| /// | ||
| /// Empty list = nothing allowed (secure by default). Use `["*"]` to allow all. | ||
| pub fn is_node_allowed(&self, node_type: &str) -> bool { | ||
| // Empty list means nothing is allowed (secure by default) | ||
| // Use ["*"] wildcard to allow everything | ||
| if self.allowed_nodes.is_empty() { | ||
| return false; | ||
| } | ||
|
|
||
| // Check against patterns (supports wildcards like "audio::*") | ||
| self.allowed_nodes | ||
| .iter() | ||
| .any(|pattern| Pattern::new(pattern).ok().is_some_and(|p| p.matches(node_type))) | ||
| } | ||
|
|
||
| /// Check if a plugin is allowed | ||
| /// Check if a plugin is allowed. | ||
| /// | ||
| /// Empty list = nothing allowed (secure by default). Use `["*"]` to allow all. | ||
| pub fn is_plugin_allowed(&self, plugin_name: &str) -> bool { | ||
| // Empty list means nothing is allowed (secure by default) | ||
| // Use ["*"] wildcard to allow everything | ||
| if self.allowed_plugins.is_empty() { | ||
| return false; | ||
| } | ||
|
|
||
| // Check against patterns (supports wildcards like "plugin::*") | ||
| self.allowed_plugins | ||
| .iter() | ||
| .any(|pattern| Pattern::new(pattern).ok().is_some_and(|p| p.matches(plugin_name))) | ||
| } | ||
|
|
||
| /// Check if an audio asset path is allowed | ||
| /// Check if an audio asset path is allowed. | ||
| /// | ||
| /// Empty list = nothing allowed (secure by default). Use `["*"]` to allow all. | ||
| pub fn is_asset_allowed(&self, path: &str) -> bool { | ||
| // Empty list means nothing is allowed (secure by default) | ||
| // Use ["*"] wildcard to allow everything | ||
| if self.allowed_assets.is_empty() { | ||
| return false; | ||
| } | ||
|
|
||
| // Check against glob patterns | ||
| self.allowed_assets | ||
| .iter() | ||
| .any(|pattern| Pattern::new(pattern).ok().is_some_and(|p| p.matches(path))) |
There was a problem hiding this comment.
📝 Info: Permissions doc comments improved by promoting inline comments
The inline comments (e.g. // Empty list means nothing is allowed (secure by default)) inside is_sample_allowed, is_node_allowed, is_plugin_allowed, and is_asset_allowed were moved into proper /// doc comments on the methods. This is a net positive — the information is now visible in rustdoc and IDE hover documentation rather than only when reading the implementation. The content is equivalent.
(Refers to lines 278-328)
Was this helpful? React with 👍 or 👎 to provide feedback.
Debug
There was a problem hiding this comment.
Agreed — the doc comment consolidation is intentional. The information is now more discoverable via rustdoc/IDE hover.
Signed-off-by: StreamKit Devin <devin@streamkit.dev> Co-Authored-By: Claudio Costa <cstcld91@gmail.com>
| let session_manager = app_state.session_manager.lock().await; | ||
| session_manager.get_session_by_name_or_id(&session_id) | ||
| }; // Session manager lock released here | ||
| }; |
There was a problem hiding this comment.
📝 Info: Lock-scope design comments removed from websocket handlers
Several instances of // Get session with SHORT lock hold to avoid blocking other operations and // Session manager lock released here were removed from handle_add_node, handle_remove_node, handle_connect, handle_disconnect, and handle_get_pipeline. While the code structure (block-scoped lock acquisition) makes the intent somewhat visible, these comments documented an important design decision about why the lock scope is intentionally kept minimal. Future developers modifying these handlers might not realize the significance of the block structure without the comments. Not a bug since the behavior is unchanged, but a maintainability consideration.
Was this helpful? React with 👍 or 👎 to provide feedback.
Debug
There was a problem hiding this comment.
Valid point. Restored a single comment on the first occurrence in handle_add_node documenting the lock-scope design pattern (fff0b77). This documents the intent once without repeating it 9 times across every handler.
Signed-off-by: StreamKit Devin <devin@streamkit.dev> Co-Authored-By: Claudio Costa <cstcld91@gmail.com>
2 participants
Summary
Comment-only cleanup pass across the backend server (
apps/skit/src/), CLI (apps/skit-cli/src/), and integration tests (apps/skit/tests/). Removes ~310 lines of redundant, stale, or overly verbose comments that restate what the code obviously does. No behavioral changes.26 files touched, net ~310 lines removed.
What was removed
// Check if emptybeforeif filename.is_empty()).// Require authand// Require adminthat appeared identically across many handlers.What was preserved
websocket_handlers.rs— documents why session_manager lock is block-scoped.is_revokeddoc comment — documents non-obvious fallback behavior.Refactoring in
permissions.rsConsolidated repeated inline comments (
// Empty list means nothing is allowed (secure by default) // Use ["*"] wildcard to allow everything) into concise doc comments on each method, making them visible in rustdoc/IDE hover.No bugs found
No functional changes or bug fixes — this is a pure comment cleanup.
Review & Testing Checklist for Human
permissions.rsdoc comment consolidation reads welljust testto confirm no regressions (CI will also verify)Notes
wrapper::ffi_guard_tests::handle_guard_catches_panic_between_create_and_okincrates/plugin-native/is outside this PR's scope and passes when run in isolation.Link to Devin session: https://staging.itsdev.in/sessions/10126ec337e348649790d60de8cd303f
Requested by: @streamer45
Devin Review
fff0b77