Skip to content

docs(cir): Phase 0 design lock — Cron Incident Review - #53219

Closed
laulopezreal wants to merge 1 commit into
NousResearch:mainfrom
laulopezreal:palmi/cir-phase0-design
Closed

laulopezreal wants to merge 1 commit into
NousResearch:mainfrom
laulopezreal:palmi/cir-phase0-design

Conversation

@laulopezreal

Copy link
Copy Markdown
Contributor

Summary

  • Adds docs/cron-incident-review-design.md — Phase 0 discovery/design artifact for Hermes Cron Incident Review (CIR), grounded in exact code paths from cron/jobs.py, cron/scheduler.py, plugins/platforms/discord/adapter.py, and agent/redact.py.
  • Covers all 11 required sections: product goal, code path map, failure taxonomy, data contracts (CronIncident v1 / ActionToken v1 / CronReviewRequest v1), state machine, security/threat model, dedup/alert policy, PR slice (dv9.2–dv9.9), test matrix, open questions, and MVP recommendation.
  • Docs-only — zero runtime code changes.

Key Design Decisions

  • CIR hooks in _process_job() after run_job() returns, before mark_job_run() persists state — minimal diff to scheduler.
  • Discord button UI is feasible by following the existing ExecApprovalView/send_exec_approval() pattern (adapter.py:4497, 5459). send_cron_review() + CronReviewView is a ~80-line addition.
  • Command fallback (/cron-review <token>) for all non-Discord platforms.
  • ActionToken: HMAC-signed, per-profile secret, nonce replay protection, 7-day TTL.
  • Zero new dependencies — uses stdlib hmac, hashlib, uuid, json, fcntl only.

Bead

hermes-agent-dv9.1

Test plan

  • git diff --check passed (no whitespace errors)
  • Document verified to cite exact file:function locations for all critical code paths
  • Reviewer: confirm PR slice ordering (dv9.2–dv9.9) aligns with implementation plan
  • Reviewer: confirm open questions (B1–B3, Q1–Q5) are addressed before dv9.2 starts

🤖 Generated with Claude Code

@laulopezreal
laulopezreal force-pushed the palmi/cir-phase0-design branch from 6d4e1e9 to 52c4886 Compare June 26, 2026 18:40
@alt-glitch alt-glitch added type/docs Documentation improvements comp/cron Cron scheduler and job management P3 Low — cosmetic, nice to have labels Jun 26, 2026
@laulopezreal

Copy link
Copy Markdown
Contributor Author

RSA evidence — Phase 0 CIR design lock

Head: 1ab0a177ea6dda025cb44da42edd1ea523212198

Reviewer + simplifier + final critic completed.

Resolved findings:

  • Corrected hook site from _process_job() to run_one_job() after run_job() and before mark_job_run().
  • Removed contradictory SUPPRESSED lifecycle state; duplicate failures now clearly update the existing OPEN incident in place.
  • Moved snooze-duration choice out of pre-dv9.2 blockers; it now gates dv9.7/dv9.8 only.
  • Removed dv9.6 review_session_id from the dv9.2 base CronIncident schema; CronReviewRequest is now a dv9.6 extension.
  • Converted incident storage and token-secret path into explicit design decisions instead of blockers.
  • Added mandatory CIR redaction language for HERMES_REDACT_SECRETS=false startup cases.

Verification:

  • git diff --check origin/main...HEAD passed.
  • PR files list is docs-only: docs/cron-incident-review-design.md.
  • Open unresolved current review threads: 0.
  • Final critic verdict: PASS.

Final critic summary: all six hard gates are satisfied; no new blocking issues were introduced.

@agy590

agy590 commented Jul 9, 2026

Copy link
Copy Markdown

Field data — 2026-07-09 incident review

For the CIR Phase 1 design, here's a concrete case to add to the failure taxonomy. daily-health-report (742eb8db275e, 0 21 * * *, agent-mode cron):

Observable

  • last_status=ok, last_run_at=2026-07-09T21:03:09
  • Telegram delivery completed successfully (truncated text "Now writing the markdown report and HTML in parallel.")
  • ~/.hermes/cron/output/<job_id>/2026-07-09_21-03-08.md = 6.7 KB but contains only the prompt dump (system-prompt scaffold), not the report body
  • ~/hermes-lab/shared/reports/health-report-2026-07-09.md = does not exist (last report 2026-07-08_21-02-52.md)
  • /home/cal7vin9/.hermes/cache/daily-health-report_2026-07-09.html = does not exist
  • User discovered at 22:25 by asking "where's the html?" — 80 minutes of silent-ok

Root-cause class — overlaps three known lanes

Cross-reference: this is the 4th state case for last_status

3 states (ok / handoff / error) per #48072 don't quite cover this — agent didn't write [HANDOFF], didn't raise, didn't time out. It just... paused, mid-write. A 4th data_missing state requires per-job required_artifacts config that the scheduler can verify post-run.

What we did locally (workaround, not upstream claim):

  • Hardened the cron prompt with explicit 4.1-4.5 write-then-verify steps
  • Added no_agent watchdog cron c454afe8b6e6 at 30 21 * * * that alerts MrFai on Telegram if either artifact is missing or under-sized
  • 7/10 21:30 first watchdog run

The CIR frame I think this fits under: artifact-existence-as-invariant, independent of marker schemes. Possibly listed under the existing failure-taxonomy entry for #36845 (LLM-fallback-masks-failure) as the no-marker sibling.

Cross-posting to #56790, #48072, #36845 with the same incident summary so the upstream view is consistent. Happy to be a test case if a data_missing-aware gateway build goes out for staging.

@teknium1 teknium1 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the detailed Phase 0 investigation. Before this can serve as a design lock, the current-main map and scope need updating.

Problems

  • docs/cron-incident-review-design.md:67 uses obsolete scheduler anchors. Current main runs run_job at cron/scheduler.py:3452, delivery at 3511-3516, and mark_job_run at 3532; a hook must be after delivery to classify delivery_error.
  • docs/cron-incident-review-design.md:428/:450 assigns root hermes cron work to hermes_cli/commands.py, but the current parser is hermes_cli/subcommands/cron.py:15-163 and dispatch is hermes_cli/main.py:4263.
  • The 2026-07-09 field-data comment reports a successful, delivered run with missing required artifacts. The taxonomy at :147-163 and tests at :456-479 lack that data_missing/artifact-invariant case.

Suggested changes

  • Re-trace the current pipeline and CLI surfaces, then update the slices and test matrix.
  • Add an explicit design decision and coverage for artifact verification before calling this a lock.

Automated hermes-sweeper review.

Comment thread docs/cron-incident-review-design.md Outdated
- Returns `(success, full_output_doc, final_response, error_message)`
- `full_output_doc` is what gets stored; `final_response` is what gets delivered

`_process_job(job)` (line 2907) is only a thin wrapper that calls `run_one_job(job)`. `run_one_job(job)` (line 2748) owns the execute → save → deliver → mark pipeline: it calls `run_job()` (line 2764), `save_job_output()`, `_deliver_result()`, and `mark_job_run()` (line 2803). CIR hooks in `run_one_job()` **after** `run_job()` returns and **before** `mark_job_run()` persists state, so it can see both job execution errors and delivery errors.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs re-tracing against current main. run_one_job now calls run_job at cron/scheduler.py:3452, then _deliver_result at 3511-3516, then mark_job_run at 3532; a hook immediately after run_job cannot observe delivery_error. Specify the post-delivery, pre-mark location.


## 3. Failure Taxonomy and Stage Mapping

| Stage | Trigger | `success` | CIR Class | Source |

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add the artifact-existence failure case from the 2026-07-09 field-data comment. A run can be marked ok and delivered while its required Markdown/HTML artifacts are absent, so this taxonomy and the test matrix need an explicit data_missing/required-artifact design decision.

Comment thread docs/cron-incident-review-design.md Outdated
| **dv9.4** | Scheduler integration — capture hook | `cron/scheduler.py`: hook in `run_one_job()` after `run_job()` returns and before `mark_job_run()` | Integration: fake-job run that fails → incident written; `[SILENT]` runs → no incident; `no_agent=True` failures → incident with correct stage |
| **dv9.5** | ActionToken generation + validation | `cron/action_token.py` (new) | Unit: token roundtrip; expired token rejected; wrong action rejected; replayed nonce rejected; forged HMAC rejected |
| **dv9.6** | Owner-agent review session | `cron/review_agent.py` (new); subprocess launch | Integration: `HERMES_CIR_REVIEW=1` blocks recursive CIR; subprocess env isolation; `CronReviewRequest` schema validation |
| **dv9.7** | CLI surface — `hermes cron incidents` | `hermes_cli/commands.py` + new subcommand | CLI: `incidents list`, `incidents show <id>`, `cron-review <token>` command; token redemption flow |

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The root hermes cron parser is currently hermes_cli/subcommands/cron.py:15-163, dispatched by hermes_cli/main.py:4263; hermes_cli/commands.py is only the interactive slash-command registry. Update this slice to cover the actual CLI surface and separately identify any gateway/TUI fallback routing.

@teknium1 teknium1 added the sweeper:blast-contained Sweeper blast radius: contained — one narrow path / opt-in / few users label Jul 15, 2026
…ata_missing case, mark NousResearch#94692 landed

Address maintainer review on PR NousResearch#53219:

1. Refresh scheduler/CLI/Discord anchors to current main:
   - run_job 1962 -> 5063, run_one_job 2748 -> 6663,
     _run_one_job_body 6726, _process_job 2907 -> 7463,
     _deliver_result 1060 -> 2856, tick 2830 -> 7263,
     SILENT_MARKER 244 -> 554 (cron/scheduler.py)
   - jobs.py anchors refreshed (mark_job_run 1173 -> 2552, etc.)
   - hermes_cli/commands.py -> hermes_cli/subcommands/cron.py
     (build_cron_parser at :15; dispatch via hermes_cli/main.py:13033)
   - Discord adapter anchors refreshed (ExecApprovalView 5797 -> 8787, etc.)

2. Add data_missing / artifact-invariant failure class to the taxonomy
   (post-run stage, success=True but required artifacts absent) plus
   DV-ARTIFACT design decision in 3.1 and a test-matrix case (dv9.10).

3. Mark Slice 1 (durable incidents, signature dedup, ack, hermes cron
   incidents CLI) as landed in PR NousResearch#94692 (open); restructure section 8
   into landed vs remaining roadmap; slices/files/tables now build on
   the existing cron/incidents.py and incidents CLI.

Docs-only change.
@laulopezreal
laulopezreal force-pushed the palmi/cir-phase0-design branch from 1ab0a17 to fe75643 Compare August 25, 2026 13:21
@laulopezreal

Copy link
Copy Markdown
Contributor Author

Refreshed the design doc to address the maintainer review: re-traced all scheduler/CLI/Discord anchors to current main, added the data_missing / artifact-invariant failure class (field-report case), fixed the CLI references to hermes_cli/subcommands/cron.py, and marked slice 1 (incident store, dedup, ack, hermes cron incidents) as landed in PR #94692. Head fe75643.

@laulopezreal

Copy link
Copy Markdown
Contributor Author

cc @teknium1, this addresses your July 15 review asks (stale anchors re-traced to current main, the data_missing / artifact-invariant failure case added, CLI refs moved to hermes_cli/subcommands/cron.py) and marks slice 1 as landed in PR #94692. Would appreciate a re-look whenever you have a moment.

@teknium1

Copy link
Copy Markdown
Collaborator

Thanks for the refresh — the re-traced anchors, the hermes_cli/subcommands/cron.py CLI mapping, and especially the data_missing/artifact-invariant failure class all address the review asks, and marking slice 1 as landed keeps it honest.

Resolving this PR now: the design has done its job. Slice 1 (incident store, signature dedup, ack, hermes cron incidents CLI) is on main via PR #95017 (@ 9de5460), with the lifecycle simplified to detected → alerted → closed — the reviewed state and the dv9.6 review-session machinery should be treated as future-slice territory, and the store was built so new states can be added without a table rebuild.

We're not merging the design document into the tree — long-range multi-phase design docs go stale against a fast-moving main faster than they pay rent, and this PR remains the durable reference for the roadmap. For the remaining slices, focused PRs against the landed cron/incidents.py are the path:

  • dv9.10 / data_missing artifact verification — the most valuable next slice given the 2026-07-09 field report; the open questions you captured (per-job required_artifacts in config vs inferred, path roots, size floor) can be settled in that PR's review.
  • dv9.7b token redemption / dv9.6 review launch / Discord surfaces — after that, each as its own slice.

Reference this design doc from each slice PR so the context follows the code. Appreciate the sustained, well-structured work on this — the Phase 0 → landed-slice arc is exactly how we like big features to arrive.

@teknium1 teknium1 closed this Aug 25, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/cron Cron scheduler and job management P3 Low — cosmetic, nice to have sweeper:blast-contained Sweeper blast radius: contained — one narrow path / opt-in / few users type/docs Documentation improvements

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants