nanobot-runtime/results/2026-06-02_remind-skill-analysis-and-improvements.md

/remind Skill — Codebase Analysis & Improvement Report

1. Executive Summary

The /remind skill consists of three scripts (remind_edit.py, remind_send.py, random_times.py) plus tests. The random_times.py module is well-structured and tested. The two main scripts (remind_edit.py, remind_send.py) suffer from:

• Manual YAML string construction instead of proper serialization
• No tests at all
• Missing core features (list, edit, deduplication, dry-run)
• Race conditions and data-loss risks
• One-time reminders firing repeatedly within the same minute

This report identifies 20+ concrete improvements with code examples.

---

2. Critical Issues

2.1 One-time at reminders fire repeatedly (BUG)

remind_send.py uses a 60-second window:

def should_fire(candidate: datetime, now: datetime) -> bool:
    return abs((now - candidate).total_seconds()) < 60

With a 1-minute cron, an at: "2026-06-02T09:20:00" reminder fires at 09:20:00 and 09:20:01..09:20:59 if the cron job happens to run multiple times or with slight delay. The log shows this:

2026-06-02T09:20:01 cedule proti kouření ve výtahu

Only one line, but if the cron ran twice in the same minute, it would duplicate.

Fix: Track fired one-time reminders in a state file, or narrow the window to <= 30 and ensure the cron runs at :00.

# Better: stateful deduplication for one-time reminders
FIRED_STATE_PATH = Path(__file__).parent.parent.parent / "db" / "remind_fired.sqlite"

# Or simpler: narrow window + minute-level dedup via log check

2.2 Non-atomic YAML writes = data loss risk

remind_edit.py writes directly to reminder.yaml:

with open(REMINDER_FILE, "w") as f:
    yaml.dump(data, f)

If the process crashes mid-write, the file is truncated/corrupted.

Fix: Atomic write via temp file + rename:

import os

def atomic_write(path: Path, data: dict, yaml: YAML) -> None:
    tmp = path.with_suffix(".tmp")
    with open(tmp, "w") as f:
        yaml.dump(data, f)
    os.replace(tmp, path)

2.3 Concurrent edit + send = race condition

remind_send.py reads reminder.yaml every minute. remind_edit.py writes to it. No file locking means the reader could get a partially-written file.

Fix: Use filelock (already available via uv) or atomic writes (above) + read retry.

2.4 remind_edit.py has no list command (advertised but missing)

SKILL.md documents list and remove commands, but remind_edit.py only implements add and remove. There is no list.

Fix: Add list to main():

elif command == "list":
    for i, r in enumerate(data.get("reminders", []), 1):
        print(f"{i}. {r.get('text', '(no text)')}")

---

3. Code Quality — Shorten & Improve

3.1 Remove custom LiteralScalarString (redundant)

remind_edit.py defines:

class LiteralScalarString(str):
    __slots__ = ()

ruamel.yaml already provides ruamel.yaml.scalarstring.LiteralScalarString. The custom class is unnecessary and confusing.

Fix:

from ruamel.yaml.scalarstring import LiteralScalarString

3.2 format_reminder manually builds YAML (fragile)

Current code concatenates strings to produce YAML:

def format_reminder(text, schedule):
    lines = [f"- text: {text}"]
    for key, value in schedule.items():
        if isinstance(value, list):
            lines.append(f"  {key}:")
            for item in value:
                lines.append(f"    - {item}")
        else:
            lines.append(f"  {key}: {value}")
    return "\n".join(lines)

This breaks on special characters (quotes, colons, newlines in text), doesn't handle indentation consistently, and duplicates YAML serialization logic.

Fix: Build a dict and let ruamel.yaml serialize it:

def build_reminder(text: str, schedule: dict) -> dict:
    reminder = {"text": LiteralScalarString(text)}
    for key, value in schedule.items():
        if key in ("at", "at_times", "cron_exprs") and isinstance(value, list):
            reminder[key] = [LiteralScalarString(v) for v in value]
        elif key in ("at", "window") and isinstance(value, str):
            reminder[key] = LiteralScalarString(value)
        else:
            reminder[key] = value
    return reminder

Then append to data["reminders"] and dump the whole document.

3.3 parse_schedule is a long if-elif chain

def parse_schedule(args):
    if not args:
        return {"cron_exprs": ["0 9 * * *"]}
    elif args[0] == "at":
        ...
    elif args[0] == "times":
        ...
    elif args[0] == "cron":
        ...
    else:
        ...

Fix: Dispatch table:

SCHEDULE_PARSERS = {
    "at": lambda args: {"at": args[1]},
    "times": lambda args: {"at_times": args[1:]},
    "cron": lambda args: {"cron_exprs": args[1:]},
}

def parse_schedule(args: list[str]) -> dict:
    if not args:
        return {"cron_exprs": ["0 9 * * *"]}
    parser = SCHEDULE_PARSERS.get(args[0])
    if parser:
        return parser(args)
    # fallback: treat all args as cron expressions
    return {"cron_exprs": args}

3.4 remove_reminder dual-match logic is confusing

def remove_reminder(data, text):
    reminders = data.get("reminders", [])
    for i, reminder in enumerate(reminders):
        if reminder.get("text") == text:
            del reminders[i]
            return True
    for i, reminder in enumerate(reminders):
        if text.lower() in reminder.get("text", "").lower():
            del reminders[i]
            return True
    return False

This silently falls back to substring match, which could delete the wrong reminder.

Fix: Be explicit. Support exact match and --grep flag:

def remove_reminder(data: dict, text: str, grep: bool = False) -> bool:
    reminders = data.get("reminders", [])
    for i, reminder in enumerate(reminders):
        reminder_text = reminder.get("text", "")
        if (not grep and reminder_text == text) or (grep and text.lower() in reminder_text.lower()):
            del reminders[i]
            return True
    return False

3.5 main() in remind_edit.py is a big if-elif

Fix: Same dispatch pattern:

COMMANDS = {
    "add": cmd_add,
    "remove": cmd_remove,
    "list": cmd_list,
}

def main():
    args = sys.argv[1:]
    if not args:
        print("Usage: ...")
        sys.exit(1)
    cmd = COMMANDS.get(args[0])
    if not cmd:
        print(f"Unknown command: {args[0]}")
        sys.exit(1)
    cmd(args[1:])

3.6 remind_send.py should_fire window too wide

With 1-minute cron granularity, a 60-second window allows double-firing if there's any jitter. Use 30 seconds:

def should_fire(candidate: datetime, now: datetime, window_sec: int = 30) -> bool:
    delta = (now - candidate).total_seconds()
    return 0 <= delta < window_sec

This also ensures we only fire after the scheduled time, not before (which abs() allowed).

3.7 remind_send.py catches bare Exception

except Exception as e:
    print(f"Error sending reminder: {e}", file=sys.stderr)

Fix: Catch specific exceptions (telegram.error.TelegramError, NetworkError).

3.8 sys.path.insert hacks in both scripts

Both scripts do:

sys.path.insert(0, str(Path(__file__).parent))

This is a code smell. Since these are run via uv run, they should either:

• Be part of a proper Python package with __init__.py
• Or use PYTHONPATH in the cron job
• Or import via relative imports if refactored into a package

Fix: Add a pyproject.toml in skills/remind/ declaring the scripts directory as part of the package, or set PYTHONPATH in the cron:

* * * * * PYTHONPATH=/home/nanobot/.nanobot/workspace/skills/remind/scripts uv run /home/nanobot/.nanobot/workspace/skills/remind/scripts/remind_send.py

Then use normal imports: from random_times import compute_fire_times.

---

4. Missing Functionality

4.1 No list command in remind_edit.py

Users cannot view reminders without cat reminder.yaml.

4.2 No edit command

To change a reminder, users must remove and re-add. An edit command would be useful:

def edit_reminder(data: dict, old_text: str, new_text: str, new_schedule: dict | None = None) -> bool:
    for reminder in data.get("reminders", []):
        if reminder.get("text") == old_text:
            reminder["text"] = new_text
            if new_schedule:
                # Remove old schedule keys, add new ones
                for key in list(reminder.keys()):
                    if key != "text":
                        del reminder[key]
                reminder.update(new_schedule)
            return True
    return False

4.3 No deduplication / "fired" tracking for one-time reminders

at and at_times reminders should fire exactly once. Currently they rely on the 60s window and cron granularity.

Fix: SQLite state tracking:

# db/remind_state.sqlite
# table fired (text TEXT, fired_at TEXT PRIMARY KEY)

Or simpler: append a fired: list to each reminder in reminder.yaml (but this modifies user data). Better: separate state file.

4.4 No dry-run mode in remind_send.py

Users cannot preview what would fire without actually sending Telegram messages.

Fix: Add --dry-run flag:

if dry_run:
    print(f"[DRY-RUN] Would fire: {text} at {now}")
else:
    fire_reminder(text)

4.5 No way to see today's schedule

Users can't ask "what reminders do I have today?"

Fix: Add a today or schedule command to remind_edit.py that computes and prints all fire times for the current day.

4.6 No support for disabling reminders

Users must delete reminders to stop them. A disabled: true flag would be useful.

4.7 No validation before write

remind_edit.py doesn't validate that the produced YAML is loadable by remind_send.py. A malformed entry could break the cron job silently.

Fix: After building the reminder dict, run it through random_times.compute_fire_times (if it has random) or croniter (if it has cron_exprs) to validate:

def validate_reminder(reminder: dict) -> None:
    if "random" in reminder:
        compute_fire_times(date.today(), reminder["text"], reminder["random"])
    if "cron_exprs" in reminder:
        for expr in reminder["cron_exprs"]:
            croniter(expr)

4.8 No backup before edit

Fix: Keep last N backups:

import shutil
from datetime import datetime

def backup_reminders(path: Path) -> None:
    backup = path.with_suffix(f".yaml.{datetime.now():%Y%m%d%H%M%S}.bak")
    shutil.copy2(path, backup)

4.9 random_times.py lacks step syntax in days parser

Cron supports */2, 1-5/2. _parse_days doesn't handle this.

Fix:

def _parse_days(spec: object) -> set[int]:
    text = str(spec).strip()
    if text == "*":
        return set(range(7))
    result: set[int] = set()
    for part in text.split(","):
        part = part.strip()
        step = 1
        if "/" in part:
            part, step_str = part.split("/", 1)
            step = int(step_str)
        if "-" in part:
            low_str, high_str = part.split("-", 1)
            low, high = int(low_str), int(high_str)
            result.update(_normalize_dow(d) for d in range(low, high + 1, step))
        else:
            result.add(_normalize_dow(int(part)))
    return result

4.10 No __main__ guard in random_times.py

Not critical since it's a library, but good practice.

---

5. Testing Gaps

Component	Tests?	Coverage
random_times.py	Yes	Good (determinism, gaps, filters, errors)
remind_edit.py	No	Zero
remind_send.py	No	Zero

5.1 Tests needed for remind_edit.py

• parse_schedule with all input variants
• build_reminder / format_reminder roundtrip
• remove_reminder exact vs substring
• YAML dump/load roundtrip preserves formatting
• Atomic write doesn't corrupt file

5.2 Tests needed for remind_send.py

• should_fire boundary conditions
• fire_reminder with mocked Telegram bot
• main with mocked reminder.yaml and mocked bot
• One-time reminder deduplication
• Random reminder integration with random_times

5.3 Test infrastructure

conftest.py only adds sys.path. It should also provide fixtures:

@pytest.fixture
def sample_yaml(tmp_path):
    path = tmp_path / "reminder.yaml"
    path.write_text("reminders:\n- text: test\n  at: 2026-06-01T10:00:00\n")
    return path

@pytest.fixture
def mock_bot(monkeypatch):
    class FakeBot:
        def send_message(self, chat_id, text):
            self.last_call = (chat_id, text)
    bot = FakeBot()
    monkeypatch.setattr("remind_send.Bot", lambda token: bot)
    return bot

---

6. Architecture Improvements

6.1 Consolidate into a single CLI

The user has considered consolidating remind into a single script. Current split:

• remind_edit.py = user-facing CLI
• remind_send.py = cron daemon
• random_times.py = shared library

This split is actually reasonable. But remind_edit.py and remind_send.py share no code. Consider extracting common YAML I/O:

# remind_common.py
from pathlib import Path
from ruamel.yaml import YAML

REMINDER_FILE = Path(__file__).parent.parent.parent / "reminder.yaml"

def load_reminders() -> dict:
    yaml = YAML()
    yaml.preserve_quotes = True
    with open(REMINDER_FILE) as f:
        return yaml.load(f) or {"reminders": []}

def save_reminders(data: dict) -> None:
    yaml = YAML()
    yaml.default_flow_style = False
    yaml.indent(mapping=2, sequence=4, offset=2)
    atomic_write(REMINDER_FILE, data, yaml)

6.2 Use SQLite for state (not YAML)

The user is evaluating SQLite vs YAML for remind data storage. Current YAML approach:

• Pros: Human-readable, easy to edit by hand, version-control friendly
• Cons: No schema validation, race conditions, no querying, append-only log is separate

Recommendation: Keep YAML for the reminder definitions (human-editable), but use SQLite for runtime state (fired tracking, history query):

# db/remind_state.sqlite
CREATE TABLE fired (
    id INTEGER PRIMARY KEY,
    text TEXT NOT NULL,
    scheduled_at TEXT NOT NULL,
    fired_at TEXT NOT NULL DEFAULT CURRENT_TIMESTAMP
);
CREATE INDEX idx_scheduled ON fired(scheduled_at);

This gives:

• Exact-once firing for one-time reminders
• Queryable history ("when did X last fire?")
• No modification to reminder.yaml

6.3 Refactor remind_send.py into a class

Current procedural style makes testing hard. A class-based design:

class ReminderEngine:
    def __init__(self, yaml_path: Path, bot: Bot | None = None, dry_run: bool = False):
        self.yaml_path = yaml_path
        self.bot = bot
        self.dry_run = dry_run
        self.now = datetime.now(TIMEZONE)

    def load(self) -> list[dict]:
        ...

    def should_fire(self, candidate: datetime) -> bool:
        ...

    def fire(self, text: str) -> None:
        ...

    def run(self) -> list[str]:
        fired = []
        for reminder in self.load():
            for candidate in self.candidates(reminder):
                if self.should_fire(candidate) and not self.already_fired(reminder, candidate):
                    self.fire(reminder["text"])
                    fired.append(reminder["text"])
        return fired

---

7. Specific Code Examples

7.1 Atomic write for remind_edit.py

import os
from pathlib import Path
from tempfile import mkstemp

def atomic_write_yaml(path: Path, data: dict, yaml: YAML) -> None:
    fd, tmp = mkstemp(dir=path.parent, suffix=".tmp")
    try:
        with os.fdopen(fd, "w") as f:
            yaml.dump(data, f)
        os.replace(tmp, path)
    except Exception:
        os.unlink(tmp)
        raise

7.2 Proper LiteralScalarString usage

from ruamel.yaml.scalarstring import LiteralScalarString

def build_reminder(text: str, schedule: dict) -> dict:
    reminder = {"text": LiteralScalarString(text)}
    for key, value in schedule.items():
        if isinstance(value, list):
            reminder[key] = [LiteralScalarString(v) for v in value]
        elif isinstance(value, str):
            reminder[key] = LiteralScalarString(value)
        else:
            reminder[key] = value
    return reminder

7.3 Deduplication for one-time reminders

from pathlib import Path
import sqlite3

STATE_DB = Path(__file__).parent.parent.parent / "db" / "remind_state.sqlite"

def ensure_state_db() -> None:
    STATE_DB.parent.mkdir(parents=True, exist_ok=True)
    conn = sqlite3.connect(STATE_DB)
    conn.execute("""
        CREATE TABLE IF NOT EXISTS fired (
            text TEXT NOT NULL,
            scheduled_at TEXT NOT NULL,
            fired_at TEXT NOT NULL DEFAULT CURRENT_TIMESTAMP,
            PRIMARY KEY (text, scheduled_at)
        )
    """)
    conn.commit()
    conn.close()

def already_fired(text: str, scheduled_at: datetime) -> bool:
    conn = sqlite3.connect(STATE_DB)
    row = conn.execute(
        "SELECT 1 FROM fired WHERE text = ? AND scheduled_at = ?",
        (text, scheduled_at.isoformat())
    ).fetchone()
    conn.close()
    return row is not None

def record_fired(text: str, scheduled_at: datetime) -> None:
    conn = sqlite3.connect(STATE_DB)
    conn.execute(
        "INSERT OR IGNORE INTO fired (text, scheduled_at) VALUES (?, ?)",
        (text, scheduled_at.isoformat())
    )
    conn.commit()
    conn.close()

7.4 Narrowed should_fire + dedup

def should_fire(candidate: datetime, now: datetime, window_sec: int = 30) -> bool:
    delta = (now - candidate).total_seconds()
    return 0 <= delta < window_sec

# In main loop for one-time reminders:
if "at" in reminder:
    candidate = parse_at(reminder["at"])
    if should_fire(candidate, now) and not already_fired(text, candidate):
        fire_reminder(text)
        record_fired(text, candidate)

7.5 remind_edit.py with dispatch table

from pathlib import Path
import sys
from ruamel.yaml import YAML
from ruamel.yaml.scalarstring import LiteralScalarString

from random_times import compute_fire_times
from croniter import croniter

REMINDER_FILE = Path(__file__).parent.parent.parent / "reminder.yaml"

# --- commands ---

def cmd_add(args: list[str]) -> None:
    text = " ".join(args)
    schedule = parse_schedule([])  # default cron
    add_reminder(text, schedule)

def cmd_remove(args: list[str]) -> None:
    text = " ".join(args)
    remove_reminder(text)

def cmd_list(_args: list[str]) -> None:
    data = load_reminders()
    for i, r in enumerate(data.get("reminders", []), 1):
        print(f"{i}. {r.get('text', '(no text)')}")

COMMANDS = {
    "add": cmd_add,
    "remove": cmd_remove,
    "list": cmd_list,
}

def main() -> None:
    args = sys.argv[1:]
    if not args or args[0] not in COMMANDS:
        print(f"Usage: {sys.argv[0]} [{'|'.join(COMMANDS)}] ...")
        sys.exit(1)
    COMMANDS[args[0]](args[1:])

---

8. Prioritized Action Plan

Priority	Task	Effort	Impact
P0	Fix one-time reminder double-firing (narrow window + dedup)	Small	High — prevents spam
P0	Add atomic writes to remind_edit.py	Small	High — prevents data loss
P1	Add list command to remind_edit.py	Small	Medium — advertised feature
P1	Replace custom LiteralScalarString with ruamel's	Tiny	Low — code cleanliness
P1	Replace manual YAML string building with dict+dump	Medium	High — robustness
P1	Add validation before write	Small	Medium — catches errors early
P2	Add tests for remind_edit.py and remind_send.py	Medium	High — enables refactoring
P2	Extract common YAML I/O to remind_common.py	Small	Medium — DRY
P2	Add --dry-run to remind_send.py	Small	Medium — safer testing
P3	Add SQLite state tracking for fired reminders	Medium	Medium — exact-once, queryable history
P3	Add edit command	Small	Low — convenience
P3	Add disabled flag	Small	Low — convenience
P3	Support cron step syntax in _parse_days	Small	Low — completeness

---

9. Summary

The random_times.py module is solid. The main pain points are in remind_edit.py (manual YAML construction, no atomic writes, missing commands) and remind_send.py (double-firing risk, no deduplication, no tests). The highest-impact fixes are: (1) atomic YAML writes, (2) one-time reminder deduplication, and (3) replacing manual YAML string building with proper serialization. Adding tests for the two untested scripts is essential before any major refactoring.