vylepseni /remind skill
This commit is contained in:
1
.gitignore
vendored
1
.gitignore
vendored
@@ -2,6 +2,7 @@ db/
|
|||||||
sessions/
|
sessions/
|
||||||
log/
|
log/
|
||||||
__pycache__/
|
__pycache__/
|
||||||
|
.pytest_cache/
|
||||||
tmp/
|
tmp/
|
||||||
backup/
|
backup/
|
||||||
tasks/
|
tasks/
|
||||||
|
|||||||
@@ -1,693 +0,0 @@
|
|||||||
# /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.
|
|
||||||
@@ -1,92 +1,68 @@
|
|||||||
|
---
|
||||||
|
name: remind
|
||||||
|
description: >
|
||||||
|
Create, list, edit, enable, disable, and remove recurring or one-time reminders.
|
||||||
|
Triggers on: "/remind", "remind me", "set a reminder".
|
||||||
|
---
|
||||||
|
|
||||||
# /remind
|
# /remind
|
||||||
|
|
||||||
Create, list, edit, enable, disable, and remove recurring or one-time reminders.
|
|
||||||
|
|
||||||
## How it works
|
|
||||||
|
|
||||||
- **Storage**: SQLite (`db/reminders.sqlite`) — atomic transactions, no YAML races.
|
|
||||||
- **Schema**: `reminders` (text, enabled, timezone, timestamps, soft-delete) + `schedule_at` / `schedule_cron` / `schedule_random` + `reminder_fires` (dedup + audit).
|
|
||||||
- **Sender**: `remind_send.py` runs every minute from the user crontab. Reads SQLite, finds due fires, sends to Telegram, logs delivery.
|
|
||||||
- **Deduplication**: Every delivery is recorded in `reminder_fires` with status `delivered`/`failed`. One-time `at` reminders fire exactly once; cron and random fire once per computed slot.
|
|
||||||
- **Audit**: All mutations and deliveries are logged to `log/reminder.log`.
|
|
||||||
|
|
||||||
## Commands
|
## Commands
|
||||||
|
|
||||||
### Create a reminder
|
### Create
|
||||||
|
|
||||||
```
|
```
|
||||||
/remind drink water every day at 9:00
|
/remind <text> every day at 9:00
|
||||||
/remind stand up every weekday at 9:30
|
/remind <text> every weekday at 9:30
|
||||||
/remind buy milk at 2026-06-15T18:00
|
/remind <text> at 2026-06-15T18:00
|
||||||
/remind stretch randomly 2 times between 08:00 and 20:00
|
/remind <text> randomly 2 times between 08:00 and 20:00
|
||||||
/remind občanka pana Přibyla once daily at random time between 8:00 and 21:00
|
/remind <text> once daily at random time between 8:00 and 21:00
|
||||||
```
|
```
|
||||||
|
|
||||||
The LLM parses natural language and calls `remind_edit.py add` with the appropriate flags:
|
Parse natural language, then call `remind_edit.py add` with flags:
|
||||||
|
|
||||||
- `--text "..."`
|
- `--text "..."`
|
||||||
- `--cron "0 9 * * *"` (repeatable)
|
- `--cron "0 9 * * *"` (repeatable)
|
||||||
- `--at "2026-06-15T18:00:00"` (repeatable)
|
- `--at "2026-06-15T18:00:00"` (repeatable)
|
||||||
- `--random-times-per-day N --random-window HH:MM-HH:MM [--random-days DOW] [--random-from YYYY-MM-DD] [--random-until YYYY-MM-DD]`
|
- `--random-times-per-day N --random-window HH:MM-HH:MM [--random-days DOW] [--random-from YYYY-MM-DD] [--random-until YYYY-MM-DD]`
|
||||||
|
|
||||||
### List reminders
|
### List
|
||||||
|
|
||||||
```
|
```
|
||||||
/remind list
|
/remind list
|
||||||
```
|
```
|
||||||
|
Call `remind_edit.py list` → JSON with all active reminders.
|
||||||
|
|
||||||
Calls `remind_edit.py list` → JSON with all active reminders and their schedules.
|
### Edit
|
||||||
|
|
||||||
### Edit a reminder
|
|
||||||
|
|
||||||
```
|
```
|
||||||
/remind edit keyword --text "new text"
|
/remind edit <keyword> --text "new text"
|
||||||
/remind edit keyword --replace-schedules --cron "0 10 * * *"
|
/remind edit <keyword> --replace-schedules --cron "0 10 * * *"
|
||||||
```
|
```
|
||||||
|
Call `remind_edit.py edit --keyword <keyword>`. Keyword matches case-insensitively against reminder text. Ambiguous matches are rejected.
|
||||||
Calls `remind_edit.py edit --keyword <keyword>`. Keyword is matched case-insensitively against reminder text. Ambiguous matches are rejected.
|
|
||||||
|
|
||||||
### Enable / Disable
|
### Enable / Disable
|
||||||
|
|
||||||
```
|
```
|
||||||
/remind disable keyword
|
/remind disable <keyword>
|
||||||
/remind enable keyword
|
/remind enable <keyword>
|
||||||
```
|
```
|
||||||
|
|
||||||
### Remove a reminder
|
### Remove
|
||||||
|
|
||||||
```
|
```
|
||||||
/remind remove keyword
|
/remind remove <keyword>
|
||||||
```
|
```
|
||||||
|
Soft delete. Hard delete only via direct DB access.
|
||||||
|
|
||||||
Soft delete (sets `deleted_at`). Hard delete happens only via direct DB access.
|
## Scripts
|
||||||
|
|
||||||
## Files
|
| Action | Command |
|
||||||
|
|--------|---------|
|
||||||
|
| list | `uv run skills/remind/scripts/remind_edit.py list` |
|
||||||
|
| add | `uv run skills/remind/scripts/remind_edit.py add --text "..." ...` |
|
||||||
|
| edit | `uv run skills/remind/scripts/remind_edit.py edit --keyword <kw> ...` |
|
||||||
|
| remove | `uv run skills/remind/scripts/remind_edit.py remove --keyword <kw>` |
|
||||||
|
| enable | `uv run skills/remind/scripts/remind_edit.py enable --keyword <kw>` |
|
||||||
|
| disable | `uv run skills/remind/scripts/remind_edit.py disable --keyword <kw>` |
|
||||||
|
|
||||||
| File | Purpose |
|
Sender runs every minute from crontab: `uv run skills/remind/scripts/remind_send.py`
|
||||||
|------|---------|
|
|
||||||
| `skills/remind/scripts/db.py` | Schema, connection factory (`get_db`), `init_db()`, audit `log_operation()` |
|
|
||||||
| `skills/remind/scripts/remind_edit.py` | CRUD CLI: `list`, `add`, `remove`, `edit`, `enable`, `disable` |
|
|
||||||
| `skills/remind/scripts/remind_send.py` | Sender: reads SQLite, finds due fires, sends Telegram, dedups |
|
|
||||||
| `skills/remind/scripts/random_times.py` | Deterministic random time generator (seeded by text + date) |
|
|
||||||
| `scripts/migrate_yaml_to_sqlite.py` | One-shot migration from old `reminder.yaml` to SQLite |
|
|
||||||
| `skills/remind/tests/` | pytest suite: `test_db.py`, `test_remind_edit.py`, `test_remind_send.py`, `test_random_times.py` |
|
|
||||||
|
|
||||||
## Crontab
|
|
||||||
|
|
||||||
```
|
|
||||||
* * * * * uv run /home/nanobot/.nanobot/workspace/skills/remind/scripts/remind_send.py >> /home/nanobot/.nanobot/workspace/log/reminder_cron.log 2>&1
|
|
||||||
```
|
|
||||||
|
|
||||||
## Environment
|
## Environment
|
||||||
|
|
||||||
- `REMIND_DB` — override SQLite path (used in tests).
|
- `REMIND_DB` — override SQLite path (used in tests).
|
||||||
- `python3` is required; `python` is not available in this runtime.
|
- `python3` required; `python` unavailable.
|
||||||
|
|
||||||
## Design decisions
|
|
||||||
|
|
||||||
- **SQLite WAL mode** — readers don't block writers.
|
|
||||||
- **Soft delete** — preserves history and foreign-key integrity.
|
|
||||||
- **Deterministic random** — same text + date always yields same times, so dedup works across restarts.
|
|
||||||
- **JSON output** — both edit and send scripts emit structured JSON for easy LLM parsing.
|
|
||||||
- **No YAML** — eliminated race conditions, manual string construction, and fragile parsing.
|
|
||||||
|
|||||||
@@ -1,39 +0,0 @@
|
|||||||
# Example reminder.yaml — shows every schedule type at a glance.
|
|
||||||
# This is documentation only; the live file is managed via scripts/remind_edit.py.
|
|
||||||
# Timezone is always Europe/Prague.
|
|
||||||
|
|
||||||
reminders:
|
|
||||||
# One-time reminder.
|
|
||||||
- text: "call the dentist"
|
|
||||||
at: "2026-06-10T10:00:00"
|
|
||||||
|
|
||||||
# Several one-time reminders in one entry.
|
|
||||||
- text: "order shoes"
|
|
||||||
at_times:
|
|
||||||
- "2026-06-01T10:00:00"
|
|
||||||
- "2026-06-01T11:00:00"
|
|
||||||
|
|
||||||
# Recurring via cron expressions.
|
|
||||||
- text: "pay the membership fee"
|
|
||||||
cron_exprs:
|
|
||||||
- "0 9 * * *"
|
|
||||||
- "0 14 * * *"
|
|
||||||
|
|
||||||
# Random but deterministic times: N fires per day inside a window, spaced at
|
|
||||||
# least MIN_GAP_MIN apart (constant in scripts/random_times.py). The times are
|
|
||||||
# derived from (date, text), so they are stable for a given day yet vary daily.
|
|
||||||
- text: "drink water / stretch"
|
|
||||||
random:
|
|
||||||
times_per_day: 5 # required: how many fires per day (int >= 1)
|
|
||||||
window: "09:00-21:00" # required: daily time window HH:MM-HH:MM (start < end)
|
|
||||||
days: "1-5" # optional: cron day-of-week filter (0/7=Sun, 1=Mon..6=Sat); default every day
|
|
||||||
from: "2026-06-01" # optional: start date, inclusive; omitted = active immediately
|
|
||||||
until: "2026-12-31" # optional: end date, inclusive; omitted = no end
|
|
||||||
|
|
||||||
# Fields can be combined freely in one entry.
|
|
||||||
- text: "water the plants"
|
|
||||||
cron_exprs:
|
|
||||||
- "0 19 * * *"
|
|
||||||
random:
|
|
||||||
times_per_day: 2
|
|
||||||
window: "08:00-12:00"
|
|
||||||
Reference in New Issue
Block a user