-
Notifications
You must be signed in to change notification settings - Fork 53
feat(elogin.lic): v2.0.0 for Lich 5.12+ yaml support #1982
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
Draft
OSXLich-Doug
wants to merge
5
commits into
master
Choose a base branch
from
5_12_elogin_variant_proposal
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
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
This proposal for a new elogin comes with some particulars. Please check 'other scripts' channel for details and helpful hints.
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 c08f480 in 2 minutes and 21 seconds. Click for details.
- Reviewed
742
lines of code in1
files - Skipped
0
files when reviewing. - Skipped posting
3
draft 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. scripts/elogin.lic:337
- Draft comment:
The argument normalization using 'args.shift' may be brittle. Add a comment explaining its intent (e.g. handling duplicate arguments) to aid future maintenance. - Reason this comment was not posted:
Comment looked like it was already resolved.
2. scripts/elogin.lic:480
- Draft comment:
The fallback using char_entry[:_requested_game_code] might be risky if that key isn’t set in all scenarios. Ensure that this key is consistently provided in all expected data formats. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%
<= threshold50%
The comment is asking the PR author to ensure that a key is consistently provided, which violates the rule against asking the author to ensure behavior. It doesn't provide a specific suggestion or ask for a test to be written.
3. scripts/elogin.lic:214
- Draft comment:
Storing passwords in plain text (e.g. in the entry data) poses a security risk. Consider using encryption or another secure storage method if feasible. - 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 security concern is valid, this appears to be a fundamental design choice of the login system. The passwords need to be stored in a way that allows them to be used for automated login. Changing this would require a major architectural change to the entire login system, not just this file. The comment doesn't provide specific actionable guidance on how to implement encryption while maintaining functionality. The security concern is legitimate and important. Storing passwords in plain text is generally considered bad practice. However, this is a local client-side script where the passwords are already stored by the game client. The scope for improving security is limited without breaking core functionality. The comment should be deleted as it raises a valid but non-actionable concern that would require fundamental changes to the login system architecture.
Workflow ID: wflow_zdTNRWmVTh3ra57C
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
ellipsis recommend Co-authored-by: ellipsis-dev[bot] <65095814+ellipsis-dev[bot]@users.noreply.github.com>
mrhoribu
reviewed
Jun 18, 2025
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.
This proposal for a new elogin comes with some particulars. Please check 'other scripts' channel for details and helpful hints.
Important
Introduces a new
elogin
script for Lich 5+ with enhanced login capabilities, supporting YAML and DAT backends, and refactored command handling.elogin
script for Lich 5+ with enhanced login capabilities usingLoginHelpers
.set realm
,add
,modify
,delete
,list
, andlogin
with optional scripts.lich_version_at_least?
,debug_log
,handle_error
,log_operation
,load_entry_data
,save_entry_data
, and others for internal operations.resolve_login_helpers_paths
andensure_login_helpers_loaded
manageLoginHelpers
loading.This description was created by
for c08f480. You can customize this summary. It will automatically update as commits are pushed.