Improve roll formulas

[flashgnash]

• Add the ability to use special stats in roll formulas
• Add the start of allowing multiple users to edit the same character sheets
• Add the /edit_saved_rolls command which allows for writing a list of preset roll formulas for use with the /roll command

[llamapreview]

AI Code Review by LlamaPReview

🎯 TL;DR & Recommendation

Recommendation: Request Changes

This PR introduces a critical injection vulnerability in roll formula parsing that could allow users to override core stats, alongside a potential panic in JSON handling that risks command crashes.

📄 Documentation Diagram

This diagram documents the enhanced roll formula calculation workflow including special stats and saved rolls.

sequenceDiagram
    participant U as User
    participant B as Bot
    participant R as Roll Command
    participant S as Stat Block
    participant SR as Saved Rolls

    U->>B: /roll <formula>
    B->>R: parse formula
    R->>SR: replace saved rolls
    note over SR: PR adds saved rolls replacement
    R->>S: replace regular stats
    note over S: PR adds special stats replacement
    R->>R: calculate final roll
    R->>B: send result
    B->>U: display roll result

Loading

🌟 Strengths

• Adds valuable features like special stats and saved rolls for enhanced character sheet functionality.

Priority	File	Category	Impact Summary	Anchors
P1	src/rpg/mir/mod.rs	Bug	Injection vulnerability in roll formulas
P1	src/rpg/mir/mod.rs	Bug	Potential panics in JSON pretty-printing
P2	src/rpg/mir/mod.rs	Maintainability	Debug prints left in production code
P2	src/rpg/mir/stat_block.rs	Architecture	Duplicate stat system risking consistency	symbol:StatBlock
P2	src/rpg/mir/saved_rolls.rs	Maintainability	Dead code increasing maintenance burden
P2	src/rpg/mir/mod.rs	Bug	No validation in saved rolls modal

🔍 Notable Themes

• Input Validation Gaps: Multiple issues highlight lack of validation in user-provided data (e.g., saved rolls, JSON), increasing security and stability risks.
• Code Hygiene: Presence of debug statements and dead code indicates need for better cleanup practices to maintain code quality.

📈 Risk Diagram

This diagram illustrates the roll formula replacement sequence and its injection vulnerability risk.

sequenceDiagram
    participant R as Roll Command
    participant SR as Saved Rolls
    participant S as Stat Block

    R->>SR: replace saved rolls (first)
    note over SR: R1(P1): Injection risk - keys can override stats
    R->>S: replace regular stats (second)
    R->>S: replace special stats (third)
    note over S: Ambiguous order allows stat hijacking

Loading

⚠️ **Unanchored Suggestions (Manual Review Recommended)**

The following suggestions could not be precisely anchored to a specific line in the diff. This can happen if the code is outside the changed lines, has been significantly refactored, or if the suggestion is a general observation. Please review them carefully in the context of the full file.

---

📁 File: src/rpg/mir/saved_rolls.rs

Dead code left in the codebase creates maintenance burden and confusion. The commented-out SavedRollSheet implementation suggests an abandoned approach that conflicts with the current string-based saved rolls implementation.

Related Code:

// use std::fmt;
// 
// use crate::common::Error;
// 
// use crate::db::models::Character;

---

---

💡 Have feedback? We'd love to hear it in our GitHub Discussions.
✨ This review was generated by LlamaPReview Advanced, which is free for all open-source projects. Learn more.

[llamapreview]

P1 | Confidence: High

Double-unwrap chain creates potential panic points. If the original JSON is invalid or pretty-printing fails, the command will crash. This is particularly risky since it involves user-generated content from character sheets.

Code Suggestion:

let pretty_json = match serde_json::from_str::<serde_json::Value>(&json) {
    Ok(value) => serde_json::to_string_pretty(&value).unwrap_or(json),
    Err(_) => json,
};

[llamapreview]

P2 | Confidence: High

Debug print statements left in production code create noise in logs and expose potentially sensitive roll calculation details. These should be removed or gated behind proper logging levels.

Suggested change

	{
	println!("Testing");
	for (special_stat, value) in special_stats_object {
	str_replaced = str_replaced.replace(special_stat, &value.to_string());
	println!("special stat replaced: {special_stat}: {value}")
	}
	// Remove debug print statements

[flashgnash]

Not sure if you're able to reply

str_replaced is used later on, you're proposing removing the entire for loop and cutting out like 1/3 of the new feature

[llamapreview]

P2 | Confidence: Medium

The modal uses the raw database value without any validation or sanitization. If the saved_rolls contain malformed data (e.g., lines without colons), the editing interface will display corrupted content, and the roll command will silently ignore those lines.

Code Suggestion:

// Add basic validation display
if let Some(rolls) = &char.saved_rolls {
    let valid_lines: Vec<&str> = rolls.lines()
        .filter(|line| line.contains(':'))
        .collect();
    message_modal.message = Some(valid_lines.join("\n"));
}