feat: granular token scopes #6093
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Deploying windmill with
|
| Latest commit: |
404e062
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://4f48d379.windmill.pages.dev |
| Branch Preview URL: | https://dieri-refactor-jwt-token-sco.windmill.pages.dev |
HugoCasa
reviewed
Jul 2, 2025
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Changes requested ❌
Reviewed everything up to 8a13edd in 3 minutes and 4 seconds. Click for details.
- Reviewed
4169lines of code in30files - Skipped
0files when reviewing. - Skipped posting
7draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. frontend/src/lib/components/triggers/TriggersWrapper.svelte:71
- Draft comment:
New granular token scopes are used consistently – for example, passing scopes asjobs:run:flows:${currentPath}orjobs:run:scripts:${currentPath}. Ensure that 'currentPath' is validated and correctly computed to match backend expectations. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%The comment is asking the author to ensure that 'currentPath' is validated and correctly computed. This falls under the category of asking the author to double-check or ensure something, which is against the rules. Therefore, this comment should be removed.
2. frontend/src/lib/components/settings/CreateToken.svelte:70
- Draft comment:
The logic for selecting token scopes (using MCP mode or custom scopes) looks appropriate. Verify that custom scopes are validated against the expected granular format before submission. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
3. frontend/src/routes/(root)/(logged)/svix/create-webhook/+page@(root).svelte:69
- Draft comment:
The updateTokenAndScope function assigns a scope using the pattern${itemKind}s:run:${scriptPath}. Confirm that this format is intended and aligns with backend granular scope checking. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% The comment violates several rules. It's asking for confirmation ("Confirm that...") which is explicitly against our rules. It's speculative about backend alignment without clear evidence. If there was a real issue with the scope format, it would likely cause immediate failures. The author likely changed this format intentionally. The scope format change could be a critical security issue if incorrect. Backend compatibility is important for authorization. While security is important, asking for confirmation is not helpful. If there was a real concern about the format being wrong, it should be stated directly with evidence. The author clearly made this change intentionally. Delete this comment as it violates our rules against asking for confirmation and making speculative comments without clear evidence of issues.
4. frontend/src/lib/components/triggers/http/OpenAPISpecGenerator.svelte:190
- Draft comment:
When creating a token for OpenAPI generation with scope ['openapi:write'], ensure that the generated request body (serialized JSON object) properly matches the backend API expectation. Consider adding error handling for conversion issues between YAML and JSON. - Reason this comment was not posted:
Comment looked like it was already resolved.
5. backend/windmill-api/src/auth.rs:494
- Draft comment:
Typographical suggestion: In the comment "appending a 's' as runnable kind is singular while new scope format expect it to be plural", consider rephrasing it to "appending an 's' to the runnable kind (which is singular) while the new scope format expects it to be plural". - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% While the suggested rewording is technically more correct grammatically, the original comment is still clear and understandable. The change is purely cosmetic and doesn't improve understanding of the code's functionality. According to the rules, we should not make purely informative comments or comments that are obvious/unimportant. The comment does point out actual grammatical issues, and better documentation is generally good. Maybe clearer documentation could help future maintainers. While better documentation is good, this change is too minor to justify a PR comment. The original comment's meaning is clear despite its imperfect grammar. Delete this comment as it suggests only a minor grammatical improvement to documentation without meaningfully improving code quality or understanding.
6. backend/windmill-api/src/flows.rs:1118
- Draft comment:
Typographical error detected in the SQL query: it appears thatflow,descriptionshould likely beflow.description. Please verify and correct if unintended. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
7. frontend/src/lib/components/settings/ScopeSelector.svelte:439
- Draft comment:
Typographical Issue: The closing tag for the 'Administrator scope grants full access to all resources.' message appears to be split across lines (lines 440-441). Please ensure the tag is correctly closed on a single line. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 0% vs. threshold = 50% While the observation is technically correct, this is a very minor formatting issue that doesn't affect functionality. The code will work perfectly fine either way. This seems like the kind of nitpicky comment that adds noise without providing real value. The rules specifically say not to make comments that are obvious or unimportant. The split tag could theoretically make the code slightly harder to read or maintain. Some style guides do recommend keeping tags on single lines. While code formatting consistency is good, this is too minor to warrant a comment. Modern editors handle HTML formatting well regardless of tag placement. This comment should be deleted as it addresses a trivial formatting issue that doesn't meaningfully impact code quality or functionality.
Workflow ID: wflow_2kTwYlLMs4fDCykB
You can customize 
|
Claude finished @rubenfiszel's task —— View job PR Review: Granular Token ScopesTodo List:
OverviewThis PR introduces a comprehensive granular token scopes system for enhanced security. The implementation spans 30 files with significant changes to both backend (Rust) and frontend (Svelte) components. Security Analysis✅ Strengths
|
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed cfa271c in 2 minutes and 7 seconds. Click for details.
- Reviewed
59lines of code in3files - Skipped
0files when reviewing. - Skipped posting
4draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. backend/windmill-api/src/flows.rs:1122
- Draft comment:
Review SQL join: using array_upper(flow.versions, 1) requires that flow.versions is non-empty. Ensure that this column always has at least one element or add a safeguard. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
2. backend/windmill-api/src/utils.rs:116
- Draft comment:
Username generation loop: The conflict check across multiple tables may face race conditions under concurrent inserts. Consider enforcing uniqueness via database constraints or proper locking. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
3. frontend/src/lib/components/settings/ScopeSelector.svelte:365
- Draft comment:
Casting to ScopeDefinition: Using a literal ({ value: scopeValue } as ScopeDefinition) may omit required properties. Ensure the full expected interface is provided. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50% While technically correct that the cast may omit required properties, looking at the usage in updateDomainCheckboxState(), only the value property is actually used. The function works correctly with just this property. The type cast is being used intentionally to create a minimal object with just the needed property. This is a common TypeScript pattern when you only need a subset of properties. The comment correctly identifies a potential type safety issue. If ScopeDefinition changes in the future to require new properties that updateDomainCheckboxState needs, this could cause runtime errors. However, the current implementation is intentional and works correctly. The function only needs the value property, and creating a minimal object is a valid pattern. Making the code more verbose by including unused properties would not improve anything. The comment should be deleted. While technically correct about type safety, the current implementation is intentional and works correctly. The comment does not suggest a meaningful improvement.
4. backend/windmill-api/src/flows.rs:1118
- Draft comment:
Typographical error fixed: the query originally had "flow,description". It is now corrected to "flow.description". Please ensure that this change is intentional. - Reason this comment was not posted:
Comment looked like it was already resolved.
Workflow ID: wflow_6S6EcFMfPzPAubZY
You can customize
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed 5597964 in 43 seconds. Click for details.
- Reviewed
19lines of code in1files - Skipped
0files when reviewing. - Skipped posting
2draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. backend/windmill-api/src/token.rs:39
- Draft comment:
Updated 'Read permission' to 'Read'. Verify that this concise label meets UI expectations and aligns with labels used elsewhere. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
2. backend/windmill-api/src/token.rs:44
- Draft comment:
Updated 'Write permission' to 'Write'. Ensure consistency with overall scope labeling in the UI. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
Workflow ID: wflow_CXFPi7zrq140fLbs
You can customize
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed 3f23e47 in 1 minute and 41 seconds. Click for details.
- Reviewed
12lines of code in1files - Skipped
0files when reviewing. - Skipped posting
5draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. backend/windmill-api/src/jobs.rs:5780
- Draft comment:
Avoid manual SQL string concatenation for label filtering. Instead of using replace to escape quotes, use parameter binding to prevent SQL injection risks. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
2. backend/windmill-api/src/jobs.rs:3212
- Draft comment:
There is duplicated Deno core runtime logic in the JS expression evaluator; consider refactoring shared code to reduce maintenance overhead. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
3. backend/windmill-api/src/jobs.rs:3920
- Draft comment:
The Drop implementation for Guard spawns an asynchronous cancellation task. Consider handling potential errors or logging failures, as these errors currently don’t propagate. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
4. backend/windmill-api/src/jobs.rs:6000
- Draft comment:
Several SQL queries use unwraps or manual handling of Option values; ensure robust error handling to avoid runtime panics in edge cases. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%None
5. backend/windmill-api/src/jobs.rs:5765
- Draft comment:
The file is very large and handles many endpoints and SQL queries. Consider splitting it into smaller modules to improve maintainability and readability. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
Workflow ID: wflow_thUjkD0womvnowtw
You can customize
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed a4d33c8 in 1 minute and 41 seconds. Click for details.
- Reviewed
34lines of code in1files - Skipped
0files when reviewing. - Skipped posting
2draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. backend/windmill-api/src/apps.rs:1748
- Draft comment:
When using the token fallback you slice the token as t[0..TOKEN_PREFIX_LEN]. Ensure that the token is at least TOKEN_PREFIX_LEN long to avoid panics. Consider adding an explicit length check or documenting this invariant. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
2. backend/windmill-api/src/apps.rs:1663
- Draft comment:
The PERMIT_CACHE block builds a composite key (email, path, workspace) and caches a DB check. Consider whether this cache might become stale if app permissions change and also simplify the check using row.flatten().is_some() instead of is_some_and(|x| x.is_some()). - Reason this comment was not posted:
Comment was on unchanged code.
Workflow ID: wflow_wGAbBEG8Mpqkv10A
You can customize
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed 3fc92d4 in 53 seconds. Click for details.
- Reviewed
13lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. backend/windmill-api/src/http_triggers.rs:527
- Draft comment:
New admin check added via require_admin enforces that only admin users can create triggers. Verify that this aligns with the intended granular token scopes design (e.g., create_many and delete also require admin) and that non-admin users are not expected to create triggers. Consider documenting the reasoning if creation is admin-only. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =30%<= threshold50%The comment is asking the author to verify if the new admin check aligns with the intended design and to consider documenting the reasoning. This violates the rule against asking the author to confirm their intention or to ensure behavior is intended. However, it does suggest considering documentation, which is a specific suggestion. Overall, the comment leans more towards asking for confirmation rather than providing a specific code suggestion or improvement.
Workflow ID: wflow_gBTwyyA8cNO9PdKT
You can customize
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed 978ad0a in 1 minute and 50 seconds. Click for details.
- Reviewed
84lines of code in1files - Skipped
0files when reviewing. - Skipped posting
4draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. backend/windmill-api/src/scopes.rs:19
- Draft comment:
Removed RUN_KEYWORD constant. Ensure that the removal of explicit '/cancel/' handling is intentional so that cancellation endpoints aren’t inadvertently affected. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% The old code had RUN_KEYWORD which included "/cancel/" as a run action path. The new code has RUN_PATH_ACTIONS which doesn't explicitly mention cancel. However, looking at the full context, cancellation appears to be handled through other means - the comment seems to be raising a speculative concern rather than pointing out a definite issue. The comment is asking for verification rather than identifying a clear problem. I could be wrong about how cancellation is handled in the new code - there might be implications I'm missing. The removal of explicit cancel handling could potentially be a real issue. While cancellation handling is worth considering, the comment violates the rule about not asking for verification or confirmation. If there was a real issue with cancellation handling, it should be stated directly rather than asking to "ensure" something. The comment should be deleted because it asks for verification rather than identifying a clear issue, and it makes a speculative suggestion about potential problems rather than pointing out a definite issue.
2. backend/windmill-api/src/scopes.rs:468
- Draft comment:
The original map_http_method_to_action explicitly checked for "/cancel/" along with "/resume/". Verify that dropping the '/cancel/' check in the new logic is intended. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 0% vs. threshold = 50% The comment is asking for verification about an intentional change, which violates our rules. We should not ask authors to verify their intentions. Additionally, looking at the new code structure, it seems the change to remove "/cancel/" was likely intentional since the code was completely refactored to use a more specific path-based approach with SCRIPT_JOBS and FLOW_JOBS constants. The old approach was more generic with RUN_KEYWORD. The comment does identify a real change in behavior that could potentially impact functionality. Maybe there was a good reason "/cancel/" was included before. While the behavior change is real, asking for verification is not actionable. If the removal of "/cancel/" is a bug, it should be clearly stated as such with evidence. The comment should be deleted because it asks for verification rather than making a clear statement about what needs to be changed. If there is evidence that "/cancel/" needs to be included, that should be stated directly.
3. backend/windmill-api/src/scopes.rs:484
- Draft comment:
The introduction of a lazy_static RUN_PATH_ACTIONS vector is a neat consolidation of run-paths. Consider adding documentation to clarify which endpoints are included and to ensure no required paths are missed. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
4. backend/windmill-api/src/scopes.rs:497
- Draft comment:
In the new map_http_method_to_action, using route_path.contains may be overly broad and could lead to false positives. Consider if a more precise matching (e.g. starts_with or regex) might be more appropriate for identifying run endpoints. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
Workflow ID: wflow_zISqhvuo68mFvLmk
You can customize
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed f5edb18 in 1 minute and 36 seconds. Click for details.
- Reviewed
19lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. backend/windmill-api/src/scopes.rs:483
- Draft comment:
Removed 'jobs/flow/user_states' from FLOW_JOBS array. Confirm if this removal is intentional, and that corresponding routes and permissions are updated accordingly. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 0% vs. threshold = 50% This comment directly violates the rule "Do NOT ask the PR author to confirm their intention". The comment is asking for confirmation that the removal was intentional. Additionally, it's speculative in asking about "corresponding routes and permissions" without clear evidence that there are actual issues. The removal appears to be a deliberate change as evidenced by the array size update from 6 to 5. Could I be wrong about whether this is a significant change that needs attention? Maybe there are important permission implications I'm missing? No - even if there were permission implications, the rules state we should not make speculative comments or ask for confirmation. If there was a clear issue with permissions, it should be stated directly with evidence. This comment should be deleted as it violates multiple review rules by asking for confirmation of intent and making speculative suggestions about permissions without clear evidence of issues.
Workflow ID: wflow_WKFXOWhucOceIHlj
You can customize
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed 1226f7e in 2 minutes and 32 seconds. Click for details.
- Reviewed
418lines of code in2files - Skipped
0files when reviewing. - Skipped posting
6draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. backend/windmill-api/src/scopes.rs:59
- Draft comment:
In from_scope_string(), the match on parts.len() distinguishes between 'jobs:run' scopes and others. This logic is a bit brittle – if the format ever changes the interpretation of a 3-part scope, it may lead to unexpected results. Consider adding comments clarifying the intended format and perhaps extra validation. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
2. backend/windmill-api/src/scopes.rs:125
- Draft comment:
In the includes() method for jobs:run scopes, when self.kind is Some and other.kind is None the function returns false. Document this behavior explicitly so that it’s clear that a token with a specific kind does not grant access to requests that don’t specify one. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
3. backend/windmill-api/src/scopes.rs:496
- Draft comment:
map_http_method_to_action() uses route_path.contains(run_path) to decide if the action is Run. Using contains() might catch unintended substrings. Consider using starts_with() or a more precise matching logic to avoid false positives. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
4. backend/windmill-api/src/scopes.rs:206
- Draft comment:
The resource_matches_pattern() helper only supports wildcard patterns ending in '/*'. This works for simple cases, but consider documenting its limitations or extending it if more complex patterns are needed. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
5. backend/windmill-api/src/scopes.rs:48
- Draft comment:
The closure into_owned_vec() for splitting resource strings works correctly but could be simplified using direct iterator methods (e.g.resources.split(",").map(String::from).collect()). This might improve clarity. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
6. backend/windmill-api/src/scopes.rs:488
- Draft comment:
There's a minor typographical issue: there is no space after the comma between "jobs/run/dependencies" and "jobs/run/flow_dependencies". Consider adding a space for better readability. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 0% vs. threshold = 50% While consistent formatting is good, this is an extremely minor stylistic issue. The code is perfectly readable as is. The Rust compiler and formatter would catch any serious formatting issues. This kind of nitpicky comment about spaces doesn't add meaningful value and could be distracting from more important review feedback. The comment is technically correct - having spaces after commas is a common style convention. Perhaps consistent formatting helps with long-term maintainability. While consistent formatting is good, this level of nitpicking about spaces is better handled by automated formatters than PR comments. The code is perfectly readable and functional as is. Delete this comment as it's an extremely minor stylistic issue that doesn't meaningfully impact code quality or functionality.
Workflow ID: wflow_MC4mA43ZAhJCCVbS
You can customize
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
Comment on lines
+304
to
+307
| if (parts[2] === '*') return null | ||
|
|
||
| if (parts[2].includes('*')) { | ||
| return 'Wildcards can only be used as the full resource name (*)' |
There was a problem hiding this comment.
The validation logic for wildcards in resource paths is inconsistent. Currently, it allows parts[2] === '*' but then rejects any path where parts[2].includes('*'). This means paths like u/user/* are allowed, but u/user/prefix* would be rejected.
This implementation contradicts the error message which states "Wildcards can only be used as the full resource name (*)". For clarity and consistency, consider modifying the condition to specifically check if the wildcard is used anywhere except as the full resource name, or update the error message to match the current implementation.
Spotted by Diamond
Is this helpful? React 👍 or 👎 to let us know.
| if let Some(scopes) = scopes { | ||
| for scope in scopes.iter_mut() { | ||
| if scope.starts_with("run:") { | ||
| let (_, part_scope) = scope.split_once(":").unwrap(); |
There was a problem hiding this comment.
The unwrap() call on split_once(":") could cause a panic if the scope string doesn't contain a colon. Since this function processes user-provided token scopes, it would be safer to handle malformed inputs gracefully. Consider using a pattern match or split_once(":").unwrap_or(("", scope)) to prevent runtime panics when encountering unexpected scope formats.
Suggested change
| let (_, part_scope) = scope.split_once(":").unwrap(); | |
| let (_, part_scope) = scope.split_once(":").unwrap_or(("", scope)); |
Spotted by Diamond
Is this helpful? React 👍 or 👎 to let us know.
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed 9eb996f in 1 minute and 19 seconds. Click for details.
- Reviewed
576lines of code in4files - Skipped
0files when reviewing. - Skipped posting
6draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. backend/windmill-api/src/auth.rs:492
- Draft comment:
The function 'transform_old_scope_to_new_scope' is now public. Consider adding doc comments to explain its purpose and usage for scope migration. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%None
2. frontend/src/lib/components/settings/CreateToken.svelte:183
- Draft comment:
The custom scopes block now renders only when 'scopes' is undefined or empty. Confirm that this conditional behavior is intentional for token creation. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%None
3. frontend/src/lib/components/settings/ScopeSelector.svelte:69
- Draft comment:
The new 'isScopeDisabled' function disables ':read' scopes when full access is enabled or the corresponding write scope is selected. Verify that this logic covers all intended cases. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%None
4. frontend/src/lib/components/settings/ScopeSelector.svelte:389
- Draft comment:
In 'updateSelectedScopesForResourcePaths', ensure the state syncing between 'scopeState' and the 'selectedScopes' list remains consistent, especially when paths are added or removed. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%None
5. frontend/src/lib/components/settings/ScopeSelector.svelte:492
- Draft comment:
Styling for scope badges changed from 'rounded-full' to 'rounded'. Confirm that the visual update meets design expectations. - Reason this comment was not posted:
Confidence changes required:30%<= threshold50%None
6. frontend/src/lib/components/settings/ScopeSelector.svelte:596
- Draft comment:
The popover trigger now uses the 'Plus' icon instead of the old 'Pen' icon. Ensure that this icon change correctly reflects the action of adding a new resource path. - Reason this comment was not posted:
Confidence changes required:30%<= threshold50%None
Workflow ID: wflow_oHrNscLZBCkD54fr
You can customize
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed a259a44 in 1 minute and 5 seconds. Click for details.
- Reviewed
75lines of code in1files - Skipped
0files when reviewing. - Skipped posting
2draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. backend/windmill-api/src/auth.rs:221
- Draft comment:
Token prefix assignment has been reformatted to multi-line (e.g. token_prefix: Some(token[0..TOKEN_PREFIX_LEN].to_string())). This change is stylistic; please ensure it aligns with our overall code style consistency. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
2. backend/windmill-api/src/auth.rs:492
- Draft comment:
The function transform_old_scope_to_new_scope now returns () instead of WindmillResult<()> and the call site no longer uses the '?' operator. Confirm that dropping error propagation is acceptable and that potential errors (e.g. from unwraps in split_once) are properly handled. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%None
Workflow ID: wflow_o7LY2CDY7rSjpO5T
You can customize
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed c82b376 in 47 seconds. Click for details.
- Reviewed
13lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. backend/windmill-api/src/auth.rs:24
- Draft comment:
Removed unused alias 'Result as WindmillResult'. Confirm that its removal doesn't break any existing usages. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
Workflow ID: wflow_9NuGt7afR8lc7zxO
You can customize
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed 404e062 in 2 minutes and 7 seconds. Click for details.
- Reviewed
109lines of code in1files - Skipped
0files when reviewing. - Skipped posting
4draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. frontend/src/lib/components/settings/ScopeSelector.svelte:190
- Draft comment:
Removing the ':' wildcard for write and run scopes changes behavior. Confirm that appending just the scope value (without ':') is intentional for granular scopes. - Reason this comment was not posted:
Comment looked like it was already resolved.
2. frontend/src/lib/components/settings/ScopeSelector.svelte:231
- Draft comment:
The default initialization of a '*' resource path when a resource-based scope is checked has been removed. Ensure that not auto-adding a default wildcard is intended. - Reason this comment was not posted:
Comment looked like it was already resolved.
3. frontend/src/lib/components/settings/ScopeSelector.svelte:377
- Draft comment:
A new 'removeScope' flag has been added to updateSelectedScopesForResourcePaths. Please add inline documentation to clarify the intended behavior differences when removeScope is true vs false. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50% The parameter name is fairly self-explanatory. The logic in the function is straightforward - it determines whether to remove the base scope when no paths exist. The code is readable without additional documentation. Adding a comment would be nice-to-have but not essential. The parameter does control important behavior that affects the scope selection state. Future maintainers may benefit from explicit documentation of the intended behavior. While documentation could help, the parameter name and logic are clear enough that a developer can understand the behavior by reading the code. This is a request for nice-to-have documentation rather than identifying a real issue. The comment should be removed as it requests optional documentation for code that is already clear and readable.
4. frontend/src/lib/components/settings/ScopeSelector.svelte:374
- Draft comment:
updateSelectedScopesForResourcePaths is called with removeScope=false when removing a resource path, causing a bare scope to be retained even if no paths remain. Verify if this behavior is desired. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 0% vs. threshold = 50% The comment is asking for verification of intended behavior, which violates our rules. The code appears to have a clear purpose - when removing a path, keep the scope selected but just remove that path. The comment is not identifying a clear bug or issue, just questioning if the behavior is intended. This kind of "please verify" comment is explicitly called out as not useful in our rules. Maybe there is a real issue here that I'm missing? The behavior does seem a bit unusual. While the behavior may be unusual, the code is clear and intentional. If there was a real issue, the comment should state it directly rather than asking for verification. The current comment provides no value since it just asks a question without identifying a specific problem. Delete this comment. It violates our rule against asking authors to verify/confirm behavior. If there is a real issue, it should be stated directly rather than posed as a question.
Workflow ID: wflow_I1wUqP8SSDfDCSJj
You can customize
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Important
This pull request introduces granular token scopes, enhancing security by allowing specific permissions for tokens, with backend enforcement and frontend management capabilities.
auth.rs,scopes.rs, andtoken.rs.check_scopes()function inutils.rsto validate token scopes.apps.rs,flows.rs,jobs.rs) to check for specific scopes before allowing access./tokens/list/scopesinopenapi.yamlto list available scopes.ScopeSelector.sveltefor selecting scopes when creating tokens.CreateToken.svelteandTokensTable.svelteto support scope selection and display.UserSettings.svelteto integrate new token creation flow with scopes.OpenAPISpecGenerator.svelteto use new token scopes for OpenAPI generation.This description was created by
for 404e062. You can customize this summary. It will automatically update as commits are pushed.