fix(crm): harden validation, fix tag-leak in digest, correct cron docs - #95
Conversation
Follow-up to #88, found by a fan-out audit spanning models/pipeline math, store/CLI correctness, docs accuracy, and repo-convention compliance. Every finding below was reproduced against the real code before being fixed. Data integrity (all previously crashed every subsequent `dates`/`digest` call, or silently corrupted stored data): - ImportantDate now rejects impossible calendar days (Feb 30, Apr 31, ...) instead of persisting them and crashing later in pipeline._next_occurrence. - ImportantDate rejects 2-digit/implausible years instead of silently storing them and rendering nonsense ages ("turning 1928") in digests. - normalize_cadence caps at ~100 years and rejects non-integral floats and inf/nan with the documented ValueError, instead of letting OverflowError escape from every read command (list/due/board/digest) once a hand-edited or fat-fingered ("9999y") cadence is stored. compute_status also clamps the rare stored-overflow case defensively. - Contact.__post_init__ now coerces important_dates dicts to ImportantDate like every other field, instead of crashing on to_dict(). Store durability: - CrmStore no longer treats a corrupt or wrong-shaped store file as empty. Previously the next write would atomically replace a corrupt file, permanently destroying every contact; now it raises CrmStoreError (surfaced as a clean CLI error) and never touches the damaged file. CLI correctness: - `crm date`/`crm add --birthday` reject impossible dates before persisting (same validation, now enforced at the write path). - `crm log --at` rejects future timestamps (a plausible year typo could silently mark a contact "on track" for years and mute reminders). - `crm edit` now persists the same normalized field shape `add` does (deduped/trimmed lists), instead of writing raw argv verbatim. - `crm digest --tag` now filters upcoming dates too, not just the due list — previously a tagged digest leaked other groups' birthdays and could never go [SILENT] for a quiet tag. `crm dates` gained --tag. Docs: - The flagship cron recipe used shell `$(...)` command substitution, which evaluates once at `cron create` time and freezes that instant's digest text into the job forever. Replaced with the two patterns that actually re-run per fire: an agent-run prompt, and --script/--no-agent script mode (matching the repo's own cron automation guide). - Added the plugin to the bundled-plugins catalog (website/docs/user-guide/features/built-in-plugins.md). - Dropped the inert `platforms:` key from plugin.yaml (not a field PluginManifest reads — copied from the teams_pipeline template, where it's equally inert). Adds 20 tests covering every fix plus previously-untested handlers (edit, rm, touch, date, export, stats, tags, list filters/json, digest end-to-end) — 44 total, all against the real argparse tree via crm_command dispatch. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Bym5HhKPQ3CWb5r9bCvBq4
🔎 Lint report:
|
| Rule | Count |
|---|---|
invalid-argument-type |
1 |
First entries
tests/plugins/test_crm_plugin.py:189: [invalid-argument-type] invalid-argument-type: Argument is incorrect: Expected `list[ImportantDate]`, found `list[ImportantDate | dict[str, str | int]]`
✅ Fixed issues: none
Unchanged: 4605 pre-existing issues carried over.
Diagnostics are surfaced as warnings — this check never fails the build.
Pre-existing, unrelated to this PR's crm changes — found while triaging CI red across the fan-out audit follow-up PRs. _setup_standard_platform() (hermes_cli/gateway.py) calls its own imported prompt_yes_no, not hermes_cli.setup's. Since Matrix now shows as "already configured" in these tests' env fixture, _configure_platform() reaches the "Reconfigure Matrix?" prompt, which was only mocked on the setup_mod side — the real gateway_mod reference fell through to a live input() call and crashed under pytest's captured stdout with "reading from stdin while output is captured". Mock gateway_mod.prompt_yes_no too, matching the pattern PR #93 independently arrived at for the same root cause. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Bym5HhKPQ3CWb5r9bCvBq4
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ecfcbccf30
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| suppresses delivery (see the [cron automation guide](../website/docs/guides/automate-with-cron.md)): | ||
|
|
||
| ```bash | ||
| printf '#!/bin/sh\nexec hermes crm digest --silent-if-empty\n' > ~/.hermes/scripts/crm-digest.sh |
There was a problem hiding this comment.
When Hermes is running with a named profile or a HERMES_HOME override, this recipe writes the script to the default ~/.hermes/scripts, while --script crm-digest.sh is resolved against the active profile's get_hermes_home()/scripts (cron/scheduler.py:945-953). The resulting job cannot find the script when it fires. Use the active Hermes home in both this recipe and the duplicated CLI docstring rather than hard-coding ~/.hermes.
Useful? React with 👍 / 👎.
| ) | ||
| if self.year is not None: | ||
| self.year = int(self.year) | ||
| if not 1000 <= self.year <= 9999: |
There was a problem hiding this comment.
Stores created by the previous CLI can legitimately contain values this new check rejects, such as a birthday entered as 10-11-98. After upgrading, deserializing that contact now raises ValueError; _load_contacts() and _require_contact() do not translate it, and crm_command() only catches CrmCliError and CrmStoreError, so commands such as list, due, dates, and digest terminate with a traceback until the user manually edits the JSON. Migrate or tolerate legacy records on load while continuing to reject them on new writes.
Useful? React with 👍 / 👎.
2 participants
What does this PR do?
Follow-up to #88 (the Dex-style CRM plugin). After it merged, I ran a fan-out audit — 6 independent review lenses (model/pipeline math, store/CLI correctness, docs accuracy, repo-convention compliance, plus two dedicated to the gateway fix in #89) with adversarial verification on every finding. This PR fixes everything that survived verification on the CRM side.
Every fix below was reproduced against the real code (not just reasoned about) before being applied, and covered by a new test.
Related Issue
Fixes #
Type of Change
Changes Made
Data integrity (all previously crashed every subsequent
dates/digestcall, or silently corrupted stored data):ImportantDaterejects impossible calendar days (Feb 30, Apr 31, …) instead of persisting them and crashing later inpipeline._next_occurrence.ImportantDaterejects 2-digit/implausible years instead of silently storing them and rendering nonsense ages ("turning 1928") in digests.normalize_cadencecaps at ~100 years and rejects non-integral floats and inf/nan with the documentedValueError, instead of lettingOverflowErrorescape from every read command once a fat-fingered cadence (9999y) is stored.compute_statusalso clamps the rare stored-overflow case defensively.Contact.__post_init__now coercesimportant_datesdicts toImportantDatelike every other field, instead of crashing onto_dict().Store durability:
CrmStoreno longer treats a corrupt or wrong-shaped store file as empty. Previously the next write would atomically replace a corrupt file, permanently destroying every contact; now it raisesCrmStoreError(surfaced as a clean CLI error) and never touches the damaged file.CLI correctness:
crm date/crm add --birthdayreject impossible dates before persisting.crm log --atrejects future timestamps (a plausible year typo could silently mark a contact "on track" for years and mute reminders).crm editnow persists the same normalized field shapeadddoes, instead of writing raw argv verbatim.crm digest --tagnow filters upcoming dates too, not just the due list — previously a tagged digest leaked other groups' birthdays and could never go[SILENT]for a quiet tag.crm datesgained--tag.Docs:
$(...)command substitution, which evaluates once atcron createtime and freezes that instant's digest text into the job forever. Replaced with the two patterns that actually re-run per fire (agent-run prompt, and--script/--no-agentscript mode), matching the repo's own cron automation guide.platforms:key fromplugin.yaml(not a fieldPluginManifestreads).How to Test
pytest tests/plugins/test_crm_plugin.py -q→ 44 passed (was 24; 20 new tests covering every fix plus previously-untested handlers: edit, rm, touch, date, export, stats, tags, list filters/json, digest end-to-end).ruff check plugins/crm/ tests/plugins/test_crm_plugin.py→ clean.rc=1; valid flows (add→digest→due→stats) still work end-to-end.Checklist
Code
fix(crm): …)ruff checkpassesDocumentation & Housekeeping
cli-config.yaml.example— N/A (no config keys changed)🤖 Generated with Claude Code
https://claude.ai/code/session_01Bym5HhKPQ3CWb5r9bCvBq4
Generated by Claude Code