# /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:

```python
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.

```python
# 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`:

```python
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:

```python
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()`:

```python
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:

```python
class LiteralScalarString(str):
    __slots__ = ()
```

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

**Fix:**

```python
from ruamel.yaml.scalarstring import LiteralScalarString
```

### 3.2 `format_reminder` manually builds YAML (fragile)

Current code concatenates strings to produce YAML:

```python
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:

```python
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

```python
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:

```python
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

```python
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:

```python
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:

```python
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:

```python
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`

```python
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:

```python
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:

```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:

```python
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:

```python
# 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:

```python
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:

```python
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:

```python
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:**

```python
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:

```python
@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:

```python
# 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):

```python
# 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:

```python
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`

```python
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

```python
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

```python
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

```python
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

```python
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.