feat(go2.lic): smart locker and guild targets based on CHE and profession data

[AndreasWelch]

Important

Enhances go2.lic with smart locker and guild navigation based on character data and adds Copilot instructions.

• Behavior:
  • go2.lic now supports ;go2 locker to navigate to public or CHE lockers based on Char.che data if Lich version is 5.12.2 or later.
  • ;go2 guild and ;go2 guild shop commands now automatically navigate to the appropriate guild or shop based on the character's profession.
• Functions:
  • Adds debug_unreachable_target() to go2.lic for debugging unreachable targets.
• Misc:
  • Adds .github/instructions/copilot-instructions.md with guidelines for script development and review.
  • Updates version to 2.2.13 in go2.lic.
  • Adds Dissonance to contributors in go2.lic.

^{This description was created by for 3927b43. You can customize this summary. It will automatically update as commits are pushed.}

[Copilot]

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated 9 comments.

Comments suppressed due to low confidence (1)

scripts/go2.lic:2040

• [nitpick] The guild/guild shop, locker, and generic tag search blocks (lines 1825-1866, 1867-2012, and 2013-2040) share nearly identical logic for finding the nearest target, calculating distances, and setting the confirm flag. Consider extracting this common logic into a helper method to reduce code duplication and improve maintainability.

For example, a method like:

def self.find_nearest_target(target_list, start_room)
  # Common logic for distance calculation and target selection
end

  elsif (XMLData.game =~ /^GS/ && (target_search_string.downcase.strip == "guild" || target_search_string.downcase.strip == "guild shop"))
    target_guild_tag = ("#{Stats.profession.downcase} #{target_search_string.downcase.strip}")

    target_list = Map.list.find_all { |iroom| iroom.tags.include?(target_guild_tag) }.collect { |iroom| iroom.id }
    if target_list.empty?
      echo 'fixme (1): no target found'
      exit
    end
    if target_list.include?(start_room.id)
      echo "you're already here..."
      exit
    end
    previous, shortest_distances = start_room.dijkstra(target_list)
    target_list.delete_if { |room_id| shortest_distances[room_id].nil? }
    if target_list.empty?
      echo 'fixme (2): no reachable target found'
      exit
    end
    target_id = target_list.sort { |a, b| shortest_distances[a] <=> shortest_distances[b] }.first

    unless target_id and (destination = Map[target_id])
      echo "DEBUGGING fixme (3):"
      echo "  target_id: #{target_id.inspect}"
      echo "  target_id.class: #{target_id.class}"
      echo "  Map[target_id]: #{Map[target_id].inspect}"
      echo "  target_list: #{target_list.inspect}"
      echo "  target_list.size: #{target_list.size}"
      echo "  Map.list.size: #{Map.list.size}"
      echo "  Room.current: #{Room.current.id}"
      if defined?(target_list) && target_list
        echo "  Valid rooms in target_list: #{target_list.select { |id| Map[id] }}"
        echo "  Invalid rooms in target_list: #{target_list.reject { |id| Map[id] }}"
      end
      echo "Please report this to the Elanthia Online team in the GS Discord #Scripting Channel."
      exit
    end

    if shortest_distances[destination.id] < 20
      confirm = false
    else
      confirm = true
    end
  elsif (target_search_string.downcase.strip == "locker" && XMLData.game =~ /^GS/)
    unless Gem::Version.new(LICH_VERSION) >= Gem::Version.new('5.12.2') # CHE membership detection added in 5.12.0, but requires 5.12.2+ for reliable population of Char.che data
      echo "Please update to Lich Version 5.12.2 or newer to use the locker target."
      echo "You can try \"public locker\" instead, but CHE locker targets are not supported in your current version."
      exit
    end

    # Determine the target locker based on CHE membership
    target_locker = []
    target_locker.push("public locker")                        if Char.che.eql?('none')
    target_locker.push("meta:che:#{Char.che}:locker")          if Map.list.any? { |iroom| iroom.tags.include?("meta:che:#{Char.che}:locker") }
    target_locker.push("meta:che:#{Char.che}:entrance_locker") if Map.list.any? { |iroom| iroom.tags.include?("meta:che:#{Char.che}:entrance_locker") }
    target_locker.push("meta:che:#{Char.che}:entrance_annex")  if Map.list.any? { |iroom| iroom.tags.include?("meta:che:#{Char.che}:entrance_annex") }
    target_locker.push("meta:che:#{Char.che}:entrance_che")    if Map.list.any? { |iroom| iroom.tags.include?("meta:che:#{Char.che}:entrance_che") }
    if target_locker.empty?
      Lich::Messaging.msg("error", "The Locker/Building Entrance for CHE: #{Char.che || 'unknown'} isn't mapped.  Cannot determine target.")
      exit
    end

    target_list = []
    target_locker.each { |locker|
      target_list += Map.list.find_all { |iroom| iroom.tags.include?(locker) }.collect { |iroom| iroom.id }
    }

    # if no target was found, exit
    if target_list.empty?
      echo 'fixme (1): no target found'
      exit
    end

    if target_list.include?(start_room.id)
      echo "you're already here..."
      exit
    end

### Distance Based Code
# This code is being left in place for now (5/2025) in case it is needed in the future.
# The location based code below is the preferred method of removing redundant CHE targets, but
# due to a lack of complete data at this time, it is not 100% sure that it will work in all cases.
=begin
    # Remove all entrance_lockers within 1 step of any locker,
    # and all entrance_annex within 15 steps of any locker or entrance_locker,
    # and all entrance_che or entrance_annex within 20 steps of any entrance_locker or locker
    locker_ids = target_list.select { |id| Map[id].tags.include?("meta:che:#{Char.che}:locker") }
    entrance_locker_ids = target_list.select { |id| Map[id].tags.include?("meta:che:#{Char.che}:entrance_locker") }

    if locker_ids.any? || entrance_locker_ids.any?
      target_list.delete_if do |room_id|
        ( # Remove entrance lockers within 1 step of any locker
          Map[room_id].tags.include?("meta:che:#{Char.che}:entrance_locker") &&
          locker_ids.any? do |locker_id|
            shortest_distances[room_id] && shortest_distances[locker_id] &&
            (shortest_distances[room_id] - shortest_distances[locker_id]).abs <= 1
          end
        ) ||
          (
            Map[room_id].tags.include?("meta:che:#{Char.che}:entrance_annex") &&
            ( # Remove entrance_annex within 15 steps of any locker
              locker_ids.any? do |locker_id|
                shortest_distances[room_id] && shortest_distances[locker_id] &&
                (shortest_distances[room_id] - shortest_distances[locker_id]).abs <= 15
              end ||
              entrance_locker_ids.any? do |entrance_id|
                shortest_distances[room_id] && shortest_distances[entrance_id] &&
                (shortest_distances[room_id] - shortest_distances[entrance_id]).abs <= 15
              end
            )
          ) ||
          (
              Map[room_id].tags.include?("meta:che:#{Char.che}:entrance_che") &&
            ( # Remove entrance_che within 20 steps of any locker or entrance_locker
              locker_ids.any? do |locker_id|
                shortest_distances[room_id] && shortest_distances[locker_id] &&
                (shortest_distances[room_id] - shortest_distances[locker_id]).abs <= 20
              end ||
              entrance_locker_ids.any? do |entrance_id|
                shortest_distances[room_id] && shortest_distances[entrance_id] &&
                (shortest_distances[room_id] - shortest_distances[entrance_id]).abs <= 20
              end
            )
            )
      end
    end
=end
    ### END Distance Based Code
    ### Location Based Code
    # Remove redundant CHE targets by location instead of distance
    locker_ids = target_list.select { |id| Map[id].tags.include?("meta:che:#{Char.che}:locker") }
    entrance_locker_ids = target_list.select { |id| Map[id].tags.include?("meta:che:#{Char.che}:entrance_locker") }

    locker_locations = locker_ids.map { |id| Map[id].location }.compact
    entrance_locker_locations = entrance_locker_ids.map { |id| Map[id].location }.compact

    # Remove entrance_che if its location matches a locker or entrance_locker
    target_list.delete_if do |room_id|
      Map[room_id].tags.include?("meta:che:#{Char.che}:entrance_che") &&
        (locker_locations.include?(Map[room_id].location) || entrance_locker_locations.include?(Map[room_id].location))
    end

    # Remove entrance_annex if its location matches a locker or entrance_locker
    target_list.delete_if do |room_id|
      Map[room_id].tags.include?("meta:che:#{Char.che}:entrance_annex") &&
        (locker_locations.include?(Map[room_id].location) || entrance_locker_locations.include?(Map[room_id].location))
    end

    # Remove entrance_locker if its location matches a locker
    target_list.delete_if do |room_id|
      Map[room_id].tags.include?("meta:che:#{Char.che}:entrance_locker") &&
        locker_locations.include?(Map[room_id].location)
    end
    ### End Location Based Code

    previous, shortest_distances = start_room.dijkstra(target_list)
    target_list.delete_if { |room_id| shortest_distances[room_id].nil? }
    if target_list.empty?
      echo 'fixme (2): no reachable target found'
      exit
    end

    # Sort the target list by distance to the start room
    target_id = target_list.sort { |a, b| shortest_distances[a] <=> shortest_distances[b] }.first

    Lich::Messaging.msg("debug", "Going to locker: #{target_id} based on CHE membership: #{Char.che}")

    unless target_id and (destination = Map[target_id])
      echo "DEBUGGING fixme (3):"
      echo "  target_id: #{target_id.inspect}"
      echo "  target_id.class: #{target_id.class}"
      echo "  Map[target_id]: #{Map[target_id].inspect}"
      echo "  target_list: #{target_list.inspect}"
      echo "  target_list.size: #{target_list.size}"
      echo "  Map.list.size: #{Map.list.size}"
      echo "  Room.current: #{Room.current.id}"
      if defined?(target_list) && target_list
        echo "  Valid rooms in target_list: #{target_list.select { |id| Map[id] }}"
        echo "  Invalid rooms in target_list: #{target_list.reject { |id| Map[id] }}"
      end
      echo "Please report this to the Elanthia Online team in the GS Discord #Scripting Channel."
      exit
    end

    if shortest_distances[destination.id] < 20
      confirm = false
    else
      confirm = true
    end
  elsif Map.list.any? { |iroom| iroom.tags.include?(target_search_string) }
    target_list = Map.list.find_all { |iroom| iroom.tags.include?(target_search_string) }.collect { |iroom| iroom.id }
    if target_list.empty?
      echo 'fixme (1): no target found'
      exit
    end
    if target_list.include?(start_room.id)
      echo "you're already here..."
      exit
    end
    previous, shortest_distances = start_room.dijkstra(target_list)
    target_list.delete_if { |room_id| shortest_distances[room_id].nil? }
    if target_list.empty?
      echo 'fixme (2): no reachable target found'
      exit
    end
    target_id = target_list.sort { |a, b| shortest_distances[a] <=> shortest_distances[b] }.first

    unless target_id and (destination = Map[target_id])
      self.debug_unreachable_target(target_id, target_list)
      exit
    end

    if shortest_distances[destination.id] < 20
      confirm = false
    else
      confirm = true
    end

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 14 comments.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 8 comments.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

Comments suppressed due to low confidence (1)

scripts/go2.lic:2032

• [nitpick] There's significant code duplication between the guild targeting logic (lines 1844-1870), locker targeting logic (lines 1890-2004), and the general tag-based targeting logic (lines 2006-2032). The pattern of finding targets, checking if already at destination, running dijkstra, filtering unreachable rooms, and selecting the nearest is repeated. Consider extracting this into a shared helper method to improve maintainability.

    target_list = Map.list.find_all { |iroom| iroom.tags.include?(target_guild_tag) }.collect { |iroom| iroom.id }
    if target_list.empty?
      echo 'fixme (1): no target found'
      exit
    end
    if target_list.include?(start_room.id)
      echo "you're already here..."
      exit
    end
    previous, shortest_distances = start_room.dijkstra(target_list)
    target_list.delete_if { |room_id| shortest_distances[room_id].nil? }
    if target_list.empty?
      echo 'fixme (2): no reachable target found'
      exit
    end
    target_id = target_list.sort { |a, b| shortest_distances[a] <=> shortest_distances[b] }.first

    unless target_id and (destination = Map[target_id])
      self.debug_unreachable_target(target_id, target_list)
      exit
    end

    if shortest_distances[destination.id] < 20
      confirm = false
    else
      confirm = true
    end
  elsif (target_search_string.downcase.strip == "locker" && XMLData.game =~ /^GS/)
    unless Gem::Version.new(LICH_VERSION) >= Gem::Version.new('5.12.2') # CHE membership detection added in 5.12.0, but requires 5.12.2+ for reliable population of Char.che data
      echo "Please update to Lich Version 5.12.2 or newer to use the locker target."
      echo "You can try \"public locker\" instead, but CHE locker targets are not supported in your current version."
      exit
    end

    # Determine the target locker based on CHE membership
    target_locker = []
    target_locker.push("public locker")                        if Char.che.eql?('none')
    target_locker.push("meta:che:#{Char.che}:locker")          if Map.list.any? { |iroom| iroom.tags.include?("meta:che:#{Char.che}:locker") }
    target_locker.push("meta:che:#{Char.che}:entrance_locker") if Map.list.any? { |iroom| iroom.tags.include?("meta:che:#{Char.che}:entrance_locker") }
    target_locker.push("meta:che:#{Char.che}:entrance_annex")  if Map.list.any? { |iroom| iroom.tags.include?("meta:che:#{Char.che}:entrance_annex") }
    target_locker.push("meta:che:#{Char.che}:entrance_che")    if Map.list.any? { |iroom| iroom.tags.include?("meta:che:#{Char.che}:entrance_che") }
    if target_locker.empty?
      Lich::Messaging.msg("error", "The Locker/Building Entrance for CHE: #{Char.che || 'unknown'} isn't mapped.  Cannot determine target.")
      exit
    end

    target_list = []
    target_locker.each { |locker|
      target_list += Map.list.find_all { |iroom| iroom.tags.include?(locker) }.collect { |iroom| iroom.id }
    }

    # if no target was found, exit
    if target_list.empty?
      echo 'fixme (1): no target found'
      exit
    end

    if target_list.include?(start_room.id)
      echo "you're already here..."
      exit
    end

### Distance Based Code
# This code is being left in place for now (5/2025) in case it is needed in the future.
# The location based code below is the preferred method of removing redundant CHE targets, but
# due to a lack of complete data at this time, it is not 100% sure that it will work in all cases.
=begin
    # Remove all entrance_lockers within 1 step of any locker,
    # and all entrance_annex within 15 steps of any locker or entrance_locker,
    # and all entrance_che or entrance_annex within 20 steps of any entrance_locker or locker
    locker_ids = target_list.select { |id| Map[id].tags.include?("meta:che:#{Char.che}:locker") }
    entrance_locker_ids = target_list.select { |id| Map[id].tags.include?("meta:che:#{Char.che}:entrance_locker") }

    if locker_ids.any? || entrance_locker_ids.any?
      target_list.delete_if do |room_id|
        ( # Remove entrance lockers within 1 step of any locker
          Map[room_id].tags.include?("meta:che:#{Char.che}:entrance_locker") &&
          locker_ids.any? do |locker_id|
            shortest_distances[room_id] && shortest_distances[locker_id] &&
            (shortest_distances[room_id] - shortest_distances[locker_id]).abs <= 1
          end
        ) ||
          (
            Map[room_id].tags.include?("meta:che:#{Char.che}:entrance_annex") &&
            ( # Remove entrance_annex within 15 steps of any locker
              locker_ids.any? do |locker_id|
                shortest_distances[room_id] && shortest_distances[locker_id] &&
                (shortest_distances[room_id] - shortest_distances[locker_id]).abs <= 15
              end ||
              entrance_locker_ids.any? do |entrance_id|
                shortest_distances[room_id] && shortest_distances[entrance_id] &&
                (shortest_distances[room_id] - shortest_distances[entrance_id]).abs <= 15
              end
            )
          ) ||
          (
              Map[room_id].tags.include?("meta:che:#{Char.che}:entrance_che") &&
            ( # Remove entrance_che within 20 steps of any locker or entrance_locker
              locker_ids.any? do |locker_id|
                shortest_distances[room_id] && shortest_distances[locker_id] &&
                (shortest_distances[room_id] - shortest_distances[locker_id]).abs <= 20
              end ||
              entrance_locker_ids.any? do |entrance_id|
                shortest_distances[room_id] && shortest_distances[entrance_id] &&
                (shortest_distances[room_id] - shortest_distances[entrance_id]).abs <= 20
              end
            )
            )
      end
    end
=end
    ### END Distance Based Code
    ### Location Based Code
    # Remove redundant CHE targets by location instead of distance
    locker_ids = target_list.select { |id| Map[id].tags.include?("meta:che:#{Char.che}:locker") }
    entrance_locker_ids = target_list.select { |id| Map[id].tags.include?("meta:che:#{Char.che}:entrance_locker") }

    locker_locations = locker_ids.map { |id| Map[id].location }.compact
    entrance_locker_locations = entrance_locker_ids.map { |id| Map[id].location }.compact

    # Remove entrance_che if its location matches a locker or entrance_locker
    target_list.delete_if do |room_id|
      Map[room_id].tags.include?("meta:che:#{Char.che}:entrance_che") &&
        (locker_locations.include?(Map[room_id].location) || entrance_locker_locations.include?(Map[room_id].location))
    end

    # Remove entrance_annex if its location matches a locker or entrance_locker
    target_list.delete_if do |room_id|
      Map[room_id].tags.include?("meta:che:#{Char.che}:entrance_annex") &&
        (locker_locations.include?(Map[room_id].location) || entrance_locker_locations.include?(Map[room_id].location))
    end

    # Remove entrance_locker if its location matches a locker
    target_list.delete_if do |room_id|
      Map[room_id].tags.include?("meta:che:#{Char.che}:entrance_locker") &&
        locker_locations.include?(Map[room_id].location)
    end
    ### End Location Based Code

    previous, shortest_distances = start_room.dijkstra(target_list)
    target_list.delete_if { |room_id| shortest_distances[room_id].nil? }
    if target_list.empty?
      echo 'fixme (2): no reachable target found'
      exit
    end

    # Sort the target list by distance to the start room
    target_id = target_list.sort { |a, b| shortest_distances[a] <=> shortest_distances[b] }.first

    Lich::Messaging.msg("debug", "Going to locker: #{target_id} based on CHE membership: #{Char.che}")

    unless target_id and (destination = Map[target_id])
      self.debug_unreachable_target(target_id, target_list)
      exit
    end

    if shortest_distances[destination.id] < 20
      confirm = false
    else
      confirm = true
    end
  elsif Map.list.any? { |iroom| iroom.tags.include?(target_search_string) }
    target_list = Map.list.find_all { |iroom| iroom.tags.include?(target_search_string) }.collect { |iroom| iroom.id }
    if target_list.empty?
      echo 'fixme (1): no target found'
      exit
    end
    if target_list.include?(start_room.id)
      echo "you're already here..."
      exit
    end
    previous, shortest_distances = start_room.dijkstra(target_list)
    target_list.delete_if { |room_id| shortest_distances[room_id].nil? }
    if target_list.empty?
      echo 'fixme (2): no reachable target found'
      exit
    end
    target_id = target_list.sort { |a, b| shortest_distances[a] <=> shortest_distances[b] }.first

    unless target_id and (destination = Map[target_id])
      self.debug_unreachable_target(target_id, target_list)
      exit
    end

    if shortest_distances[destination.id] < 20
      confirm = false
    else
      confirm = true
    end

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[mrhoribu]

@ellipsis remove previous comments and review again

[ellipsis-dev]

Caution

Changes requested ❌

Reviewed everything up to 3927b43 in 3 minutes and 27 seconds. Click for details.

• Reviewed 550 lines of code in 2 files
• Skipped 0 files when reviewing.
• Skipped posting 4 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/go2.lic:1841

• Draft comment:
  When constructing the guild target tag, ensure Stats.profession is defined. If nil, calling downcase may raise an error.
• 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% This is new code being added in the PR, so the comment is about changes. The question is whether Stats.profession can actually be nil in practice. Looking at the context, this is a GemStone (GS) game script, and Stats.profession is likely a well-established API that always returns a value for a logged-in character. In similar code elsewhere in the file (line 612), I can see Stats.prof being used without nil checks. The code also has a condition checking XMLData.game =~ /^GS/ before this block, suggesting it only runs in appropriate contexts. If Stats.profession were commonly nil, we'd likely see defensive coding elsewhere in this mature codebase. The comment is speculative ("may raise an error") rather than definitive. Without strong evidence that Stats.profession can be nil in the context where this code runs, this seems like an overly cautious suggestion. However, I could be wrong - maybe Stats.profession can be nil in edge cases like during character creation, logout, or certain game states. The safe navigation operator is a low-cost defensive measure that wouldn't hurt. But the rules say not to make speculative comments, and this seems speculative without evidence that Stats.profession is actually nil in practice. While defensive programming is generally good, the rules explicitly state to only keep comments with STRONG EVIDENCE of correctness. There's no evidence provided that Stats.profession can be nil in the context where this code runs (after the GS game check, when a character is actively using the go2 script). The codebase appears to use Stats.prof elsewhere without nil checks, suggesting it's reliable. This is a speculative "what if" comment. This comment should be deleted. It's a speculative suggestion without strong evidence that Stats.profession can actually be nil in the context where this code executes. The mature codebase uses similar Stats methods elsewhere without defensive nil checks, suggesting this isn't a real issue.

2. scripts/go2.lic:1907

• Draft comment:
  Legacy distance‐based code is commented out using =begin/=end. If this code is no longer needed, remove it to keep the codebase clean.
• 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 PR author has explicitly documented why this code is being kept - as a fallback in case the new location-based approach doesn't work in all cases. They even dated it (5/2025) suggesting they plan to remove it later. The automated tool is suggesting to remove it to "keep the codebase clean," but this conflicts with the author's stated intention. The comment from the tool is generic advice about commented code, but doesn't account for the fact that the author has a specific, documented reason for keeping it temporarily. This appears to be a case where the automated tool doesn't understand the context and is making a suggestion that goes against the author's explicit decision. Could the author's comment be insufficient justification? Maybe keeping large blocks of commented code is still a bad practice even with a note. Perhaps the tool is right that this should be removed and tracked in version control instead. While it's generally true that version control can replace commented code, the author has made a deliberate, documented decision to keep this as a fallback during a transition period. They've explained their reasoning clearly. The automated comment doesn't add value beyond generic advice that the author has already considered and decided against. This is exactly the type of comment that should be removed - it's questioning a deliberate decision without providing new information. Delete this comment. The author has explicitly documented why they're keeping this commented code as a temporary fallback during a transition period. The automated tool's generic advice about code cleanliness doesn't override the author's documented, intentional decision.

3. scripts/go2.lic:1880

• Draft comment:
  Before using Char.che in string interpolation (e.g. in target_tags), validate that it is not nil to avoid potential runtime errors.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 30% vs. threshold = 50% The comment raises a valid concern. While Ruby won't crash on nil interpolation (it will just produce "meta:che::locker"), this could lead to incorrect behavior. The code on line 1892 shows awareness that Char.che could be nil by using || 'unknown'. However, the version check on line 1872 is supposed to ensure reliable CHE data, so maybe Char.che is guaranteed to be non-nil after that check? But the check only ensures the Lich version is correct, not that the data is actually populated. The comment seems reasonable, but I need to consider: 1) Is this a real issue? 2) Is it about changed code? Looking at the diff, lines 1871-2005 are all new code (the locker feature is new), so this comment IS about changes. The version check might guarantee that Char.che is populated with at least some value (possibly 'none' if not a member). If the Lich version check ensures Char.che is never nil, then this comment would be incorrect. Also, even if Char.che is nil, Ruby's string interpolation will just convert it to an empty string, which might be acceptable behavior for the tag matching logic (though unlikely to match anything). While the version check exists, there's no guarantee that Char.che is populated even in version 5.12.2+. The error message on line 1892 explicitly handles the nil case with || 'unknown', which strongly suggests the author knows Char.che can be nil. If it could be nil there, it could be nil on lines 1881-1884 too. However, the practical impact is unclear - if Char.che is nil, the tags would be malformed but the code would continue to run without crashing. The question is whether this is a bug worth fixing. This comment identifies a potential issue where Char.che might be nil, leading to malformed tag strings. However, this wouldn't cause a runtime error (crash), just incorrect behavior. The comment is technically correct but may be overstating the severity by saying "to avoid potential runtime errors" when it would more accurately be "to avoid malformed tags." Given the rules about only keeping comments with STRONG EVIDENCE of correctness and actionable issues, and considering this is more of a defensive programming suggestion than a clear bug, I'm inclined to delete it.

4. scripts/go2.lic:1897

• Draft comment:
  Replace placeholder error messages like 'fixme (1): no target found' with more descriptive, user‐friendly messages.
• 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% Looking at the code flow: lines 1891-1895 check if target_list is empty and exit if so. Then lines 1897-1900 check again if target_list is empty. This second check appears to be unreachable code since the first check would have already exited. The comment is suggesting to improve an error message in what appears to be dead/unreachable code. This makes the comment not useful - the real issue is that this code block is redundant, not that it needs a better error message. The comment doesn't recognize this fundamental issue. Perhaps the first check could fail to exit in some edge case I'm not seeing, or maybe the code structure could change in the future making this reachable. However, as written, this appears to be unreachable defensive code, and improving the error message doesn't address the real issue. Even if there's some theoretical edge case, the comment doesn't add value because: 1) The code appears unreachable given the first check exits, 2) If it were reachable, the developer should remove the redundant check rather than improve its error message, 3) The comment doesn't acknowledge or explain why this duplicate check exists. This comment should be deleted. It's suggesting an improvement to what appears to be unreachable/redundant code (since line 1891-1895 already checks for empty target_list and exits). The comment doesn't recognize that the real issue is the redundant check, not the error message quality.

Workflow ID: wflow_YQmUHd3QOjf9m03u

^{You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.}

[ellipsis-dev]

Duplicate check for an empty target_list detected in the locker branch. Consider removing the redundant empty-check (appears again at line 1897).

[ellipsis-dev]

The long if/elsif chain handling target resolution is complex. Consider extracting guild, locker, and other target branches into separate helper methods for improved readability and maintainability.