-
Notifications
You must be signed in to change notification settings - Fork 16
feat: git hooks for commit signature validation for studio #1257
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: sujitaw <[email protected]>
Signed-off-by: sujitaw <[email protected]>
Signed-off-by: sujitaw <[email protected]>
Signed-off-by: sujitaw <[email protected]>
Signed-off-by: sujitaw <[email protected]>
Signed-off-by: sujitaw <[email protected]>
Signed-off-by: sujitaw <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Caution
Changes requested ❌
Reviewed everything up to 65229c0 in 2 minutes and 20 seconds. Click for details.
- Reviewed
261lines of code in5files - 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. .husky/commit-msg:25
- Draft comment:
Consider adding a trailing newline at the end of the file for POSIX compliance. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%None
2. .husky/post-commit:10
- Draft comment:
If environment setup from husky.sh is required for consistency, consider uncommenting the sourcing line. - Reason this comment was not posted:
Confidence changes required:20%<= threshold50%None
3. .husky/pre-push:48
- Draft comment:
'default_branch' is used without an explicit definition. Consider defining it to avoid ambiguity when computing the base branch. - 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 code pattern${default_branch:-...}is a common bash idiom for optional configuration via environment variables. This allows users to optionally setdefault_branchas an environment variable if they want to override the default behavior. If it's not set, the code gracefully falls back to detecting origin/HEAD, and then to "main". This seems intentional, not a bug. The comment is suggesting to "define it to avoid ambiguity", but this would remove the flexibility of the optional environment variable pattern. The comment doesn't identify an actual bug - the code works correctly whetherdefault_branchis set or not. This appears to be a misunderstanding of the intentional design pattern. Could the author have intended to document thatdefault_branchcan be set as an environment variable? Maybe the comment is suggesting adding a comment in the code to clarify this optional configuration? However, the comment says "define it" which suggests setting a value, not documenting it. Even if documentation would be helpful, the rules state not to make purely informative comments. The comment as written suggests a code change ("define it") but the current code pattern is a standard bash idiom that works correctly. There's no bug here, just a potential misunderstanding of the intentional design. This comment should be deleted. The code is using a standard bash pattern for optional environment variable configuration (${var:-default}), which is intentional and correct. The comment misinterprets this as an undefined variable issue when it's actually a feature that allows flexible configuration.
4. .husky/pre-push:107
- Draft comment:
Verify that the nested command substitution and escaping within the '--exec' option works as intended across environments. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%This comment is asking the PR author to verify the behavior of a specific code change, which violates the rule against asking for confirmation or verification of intended behavior. It does not provide a specific suggestion or point out a clear issue.
5. package.json:18
- Draft comment:
Confirm that switching the 'prepare' script from 'husky install' to 'husky' is intentional and that hook installation works as expected. - 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 is asking the PR author to "confirm" something, which directly violates the rule: "Do NOT ask the PR author to confirm their intention, to explain, to double-check things, to ensure the behavior is intended". The comment is not providing actionable feedback or pointing out a clear bug - it's just asking for confirmation. The change fromhusky installtohuskyis likely related to a husky version upgrade (newer versions of husky use justhuskyinstead ofhusky install), and this would be caught during testing if it doesn't work. This is not a clear code issue that needs to be fixed. Could this be a legitimate concern about a breaking change that might not be caught by automated tests? Perhaps the hook installation could silently fail and not be noticed until later. Even if hook installation could fail, the comment is still asking for confirmation rather than pointing out a specific issue. If the hooks don't work, it would be noticed immediately when committing. The rule explicitly states not to ask authors to confirm intentions or ensure behavior works as expected. This comment should be deleted because it explicitly asks the PR author to "confirm" their intention and "ensure" something works as expected, which directly violates the stated rules. It's not providing actionable feedback about a clear bug.
Workflow ID: wflow_5oe9uLKscpOCvAeh
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
Signed-off-by: sujitaw <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Important
Looks good to me! 👍
Reviewed c9e7829 in 1 minute and 11 seconds. Click for details.
- Reviewed
21lines 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. .husky/pre-push:19
- Draft comment:
Fix grammar by adding the missing apostrophe in "won't". - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%This comment is purely informative and focuses on a minor grammatical issue. It doesn't provide any substantial feedback or suggestions related to the code functionality or logic.
2. .husky/pre-push:85
- Draft comment:
Remove extraneous 'exit 1' that interrupted unsigned commit handling. - Reason this comment was not posted:
Comment looked like it was already resolved.
Workflow ID: wflow_OXb99FMaLX92WeCO
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
Signed-off-by: sujitaw <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Important
Looks good to me! 👍
Reviewed baae980 in 2 minutes and 21 seconds. Click for details.
- Reviewed
631lines of code in12files - Skipped
0files when reviewing. - Skipped posting
18draft 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. .env.demo:33
- Draft comment:
New env variable 'NEXT_PUBLIC_LOGO_BASE_URL' added. Ensure its purpose is clearly documented and that test/deployment environments have the correct value. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
2. package.json:3
- Draft comment:
Version bumped to 2.1.2. Confirm that release notes and changelog reflect any breaking changes associated with this update. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
3. src/app/api/Auth.ts:77
- Draft comment:
The forgotPassword function now accepts an optional 'clientAlias'. Ensure backend validation correctly handles null or undefined values. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
4. src/app/reset-password/ResetPassword.tsx:86
- Draft comment:
Refactored to use DynamicApplicationLogo instead of manual theme-based logic. Confirm that the new component covers all theme variations. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
5. src/features/components/DynamicLogo.tsx:20
- Draft comment:
Dynamic logo computation relies on NEXT_PUBLIC_ACTIVE_THEME and NEXT_PUBLIC_LOGO_BASE_URL. Ensure these env vars are correctly set in all deployment environments and consider more robust error handling in onError. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
6. src/features/components/user-auth-form.tsx:259
- Draft comment:
When calling forgotPassword, the 'clientAlias' is passed from searchParams. Verify backend behavior when clientAlias is null or undefined. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
7. src/features/organization/bulkIssuance/components/BulkIssuance.tsx:64
- Draft comment:
Review the use of the 'clear' state which is defined as constant false and never updated. Also, consider removing commented-out filter change code if it’s not planned for immediate future use. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%None
8. src/features/organization/connectionIssuance/components/Issuance.tsx:327
- Draft comment:
There is commented-out filter handling code. If it's not needed, consider removing it to keep the codebase clean, or add a clear TODO for future work. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
9. src/features/organization/emailIssuance/components/EmaiIssuanceCard.tsx:35
- Draft comment:
Commented-out code related to SchemaSelect is present. Remove dead code if it is not expected to be used soon. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
10. src/features/schemas/components/SchemaList.tsx:294
- Draft comment:
There is commented-out code for schema filtering using a Select component. Consider removing it if it's not planned for immediate use to reduce clutter. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
11. src/features/organization/bulkIssuance/components/BulkIssuance.tsx:239
- Draft comment:
There's an extra trailing space in the string literal on line 239 ('error-in-bulk-issuance-process-initiated '). It may be unintentional and should be removed. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 15% vs. threshold = 50% This comment is about a very minor issue - a trailing space in a console.log statement. While technically correct, I need to consider if this violates the rule about "Do NOT make comments that are obvious or unimportant." A trailing space in a debug log message is extremely minor and has no functional impact. The rules state I should not comment on things that are "obvious or unimportant." This seems to fall into the "unimportant" category. Additionally, the rules say "Do NOT comment unless there is clearly a code change required" - a trailing space in a console.log doesn't clearly require a code change. This is the kind of nitpicky comment that would likely be considered not useful. However, one could argue that maintaining clean code without unnecessary whitespace is a valid concern, and this is a clear, actionable suggestion. The comment is technically correct and the fix is trivial. Some teams have strict linting rules about trailing spaces. While some teams care about trailing spaces, this is in a console.log statement that appears to be for debugging purposes (note the eslint-disable comment above it). The functional impact is zero, and this falls squarely into the "unimportant" category. If this were caught by a linter, it would be caught by the build (which the rules say to ignore). The rules explicitly state to avoid obvious or unimportant comments, and a trailing space in a debug log is definitely unimportant. This comment should be deleted. While technically correct, it's pointing out an unimportant issue (a trailing space in a debug console.log) that has no functional impact and violates the rule against making obvious or unimportant comments.
12. src/features/organization/bulkIssuance/components/BulkIssuance.tsx:305
- Draft comment:
Typographical error: The comment on line 305 contains an unnecessary '/' at the end. It would be clearer to remove the trailing '/' or adjust the comment format. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 35% vs. threshold = 50% This is a legitimate issue - there's a mismatched comment syntax where//is used to start the comment but*/appears at the end. However, I need to consider the rules: 1) This is a very minor issue that doesn't affect functionality at all. 2) The rule states "Do NOT comment unless there is clearly a code change required" and "Do NOT make comments that are obvious or unimportant." 3) While technically incorrect, this typo doesn't break anything - it's just poor style. 4) The comment does point to actual changed code (the comment was added in this PR). 5) This is arguably "obvious" - any developer reviewing their own code should catch this. The question is whether fixing comment formatting rises to the level of "clearly a code change required." While this is a real formatting error, it's extremely minor and doesn't affect code execution. The rules emphasize not making "obvious or unimportant" comments, and a typo in a comment's syntax that has no functional impact could be considered unimportant. It's also the kind of thing that would be caught in a basic code review. However, the comment does identify a clear, objective error in the code that was introduced in this PR. It's actionable (just remove the*/), specific, and correct. While minor, maintaining clean code includes proper comment syntax. The suggestion is clear and easy to implement. This comment identifies a legitimate but very minor formatting issue. Given the emphasis on not making "obvious or unimportant" comments, and that this is purely cosmetic with no functional impact, I lean toward removing it. However, since it's objectively correct and actionable, I'll give it a low-to-moderate score.
13. src/features/organization/bulkIssuance/components/BulkIssuance.tsx:392
- Draft comment:
There's a typographical issue in the comment. It reads "Temporarily commented will be worked on later" which might be rephrased as "Temporarily commented out; will be worked on later" for clarity. - 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% This is a very minor stylistic/grammatical suggestion about a code comment. While the suggested change would make the comment slightly clearer, this falls under "obvious or unimportant" changes. The rules explicitly state "Do NOT make comments that are obvious or unimportant." A minor grammatical improvement to a comment that is already understandable doesn't constitute a clearly required code change. The comment is perfectly understandable as-is, even if it could be slightly more polished. This type of nitpicky grammar correction doesn't add meaningful value to the code review. Could this be considered a quality improvement that makes the codebase more professional? Some teams do care about comment quality and consistency. However, the comment is already clear enough to understand the intent, so this is really just polish rather than fixing something broken or unclear. While some teams value polished comments, the rules explicitly prioritize avoiding "obvious or unimportant" comments. A minor grammatical tweak to an already-understandable comment falls squarely into the "unimportant" category. The comment serves its purpose as written, and this suggestion doesn't meet the bar of "clearly a code change required." This comment should be deleted. It's a minor grammatical suggestion on a code comment that is already clear and understandable. This falls under the category of "obvious or unimportant" changes that the rules explicitly say should not be commented on.
14. src/features/organization/connectionIssuance/components/Issuance.tsx:362
- Draft comment:
Typo: The comment 'Temporarily commented will be worked on later' could be reworded for clarity. Consider updating to something like 'Temporarily commented out; will be worked on later'. - 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% This comment is suggesting a minor grammatical improvement to a code comment. According to the rules, I should NOT comment unless there is clearly a code change required. This is not a code change - it's just a suggestion to improve the wording of a comment. The rules also state "Do NOT make comments that are obvious or unimportant." A minor grammatical improvement to a comment is not important. The original comment is perfectly understandable even if it's not grammatically perfect. This type of nitpicky comment about comment wording doesn't provide meaningful value and would likely be considered obvious/unimportant. The rules emphasize only keeping comments where there is STRONG EVIDENCE the comment is correct and actionable. While the grammar suggestion is technically correct, it's not an actionable code change that matters. Could this be considered a code quality improvement? Some teams do care about comment quality and consistency. However, the rules specifically say to only comment if there's clearly a code change required, and improving comment grammar doesn't qualify as a necessary code change. While some teams might care about comment quality, the rules are clear that comments should only be made when there's a clear code change required. A minor grammatical improvement to a comment is not a necessary code change - the original comment is understandable. This falls under "obvious or unimportant" comments that should be avoided. This comment should be deleted. It's suggesting a minor grammatical improvement to a code comment, which is not a necessary code change. The original comment is perfectly understandable, and this type of nitpicky suggestion about comment wording violates the rule against making obvious or unimportant comments.
15. src/features/organization/emailIssuance/components/EmaiIssuanceCard.tsx:35
- Draft comment:
Typo/clarity suggestion: The comment reads "Temporarily commented will be worked on later". Consider revising it to "Temporarily commented-out code will be worked on later" for improved clarity. - 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% This is a very minor grammar/clarity suggestion about a code comment. While the suggested wording is slightly clearer, this falls into the category of "obvious or unimportant" comments. The original comment is perfectly understandable - it's clear that code was temporarily commented out and will be worked on later. The suggestion doesn't point out any code issue, bug, or meaningful improvement to the logic. It's purely about the exact wording of an explanatory comment, which is not a code change that needs to be made. According to the rules, I should not comment on things that are obvious or unimportant, and this is a very minor stylistic preference. Could this be considered a helpful code quality improvement? Some teams do value clear and grammatically correct comments. The suggestion does make the comment slightly more readable and professional. While clear comments are valuable, this is such a minor improvement that it falls below the threshold of what should be flagged in a PR review. The original comment is already clear enough to understand the intent. This is the type of nitpick that doesn't warrant a review comment, especially from an automated tool. This comment should be deleted. It's a minor stylistic suggestion about comment wording that doesn't address any code logic, bugs, or meaningful improvements. The original comment is already clear and understandable.
16. src/features/schemas/components/SchemaList.tsx:294
- Draft comment:
There is a slight grammatical issue in the comment on line 294. Consider revising it to something like "Temporarily commented out; will work on this later" or "Temporarily commented code to be addressed later". - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =20%<= threshold50%The comment is suggesting a change to a comment within the code for grammatical reasons. This is not related to the functionality or correctness of the code itself, and it doesn't provide a specific code suggestion or ask for tests. Therefore, it seems to be purely informative and not particularly useful for the purpose of a code review.
17. src/features/schemas/components/SchemaList.tsx:408
- Draft comment:
Typographical error: The comment 'Commented temporarily will we worked on later' appears to have a mistake. Consider revising it to something like 'Temporarily commented out; work will be done later.' - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 15% vs. threshold = 50% This is a comment about fixing a typo in a code comment. While technically correct, the rules state that I should NOT comment on things that are "obvious or unimportant." A typo in a comment is relatively minor - it doesn't affect functionality. The rules also say "Do NOT comment unless there is clearly a code change required" - while this is technically a code change, it's a very minor one. The original comment is still understandable despite the typo. This seems like the kind of nitpicky, low-value comment that should be filtered out. The rules emphasize keeping only comments with "STRONG EVIDENCE" that they are correct and important. However, code quality does matter, and fixing typos in comments improves readability and professionalism. The automated tool is providing a clear, actionable suggestion. This could be considered a legitimate code quality improvement, even if minor. While code quality matters, the rules explicitly state to avoid "obvious or unimportant" comments. A typo in a temporary comment (which even says it will be worked on later) is not important enough to warrant a review comment. The comment is understandable as-is, and this is the kind of minor nitpick that clutters reviews without adding real value. This comment should be deleted. It's pointing out a minor typo in a temporary code comment that doesn't affect functionality. According to the rules, this is too obvious and unimportant to warrant a review comment.
18. src/features/schemas/components/SchemaList.tsx:425
- Draft comment:
Typographical error: the line ends with a stray '*/}' sequence. Confirm if this is an unintended comment closure and remove it if not needed. - Reason this comment was not posted:
Comment looked like it was already resolved.
Workflow ID: wflow_THmZqBavY3CQwRAp
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
Signed-off-by: sujitaw <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Important
Looks good to me! 👍
Reviewed 1877a26 in 1 minute and 6 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. .husky/pre-push:18
- Draft comment:
The updated message is clearer now. Consider adding an explicit 'exit 0' after the echo to immediately end the script (avoiding the later 'No unsigned commits found' output) when no refs are pushed. - Reason this comment was not posted:
Comment was on unchanged code.
Workflow ID: wflow_kp8SD7ga55ivuscg
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
Signed-off-by: sujitaw <[email protected]>
GHkrishna
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
We've reviewed a similar changes PR on: credebl/platform#1487
What
Important
Add git hooks for commit signature validation, including commit message checks, post-commit verification, and pre-push validation.
.husky/commit-msgto ensure commit messages include aSigned-off-byline..husky/post-committo verify the last commit's signature, skipping merge commits..husky/pre-pushto validate commit signatures before pushing, handling local branches and remote branches.package.jsonto usehuskyfor managing git hooks..husky/pre-committo include Husky script initialization.This description was created by
for 22cbdda. You can customize this summary. It will automatically update as commits are pushed.