Add single-writer helper + schema validation for tick-history and archive metadata - #3924
Conversation
Add append-time schema + closed-vocabulary self-check tests for the new single-writer helper (scripts/lib/monitor-tick-artifacts.py) and rewrite the tick-history structural/behavioral tests (64/65) to expect the helper instead of the hand-rolled json.dumps heredoc. All 14 assertions fail on main because the helper does not exist yet and the SKILL still hand-rolls its artifacts. Refs #3791 Co-authored-by: Claude Code <claude-code@anthropic.com>
…facts Introduce scripts/lib/monitor-tick-artifacts.py as the SOLE constructor of every monitor per-tick artifact (tick-history.jsonl row, archive metadata.env, scrape_identity, counter_streak_snapshot) and replace the four hand-rolled heredocs in monitor-tick/SKILL.md with calls to it. Because the helper builds each row from typed args, every appended row is conformant by construction — the root fix for the schema fragmentation censused in #3757 (100+ row signatures, 571 warnings spellings for ~17 conditions). - emit-row: canonical 9-field row (corrected set: no `tick`, with `self_reflect`), UTC ISO-8601 `ts`, closed warnings/actions vocabulary, embedded-measurement promotion (`low-disk 90%` → warnings:["low-disk"], disk_free_pct:90), append-time self-check. Unknown tokens map to the registered `other` fallback; self-check failure is fail-loud-but-non-fatal (row routed to tick-history.rejected.jsonl, tick continues). - validate-row: required-subset + per-field-type contract (never set-equality, so diagnostic supersets pass), type-guard-before-regex on `ts` (null/int ts rejected without raising). Never emits a traceback. - warnings/actions vocabulary = the 37 alarm names in metric-alarms.toml UNION a new inert [warning_vocabulary].non_alarm table ∪ {other}. eval-alarms.py reads only [[alarm]]/schema_version, so the table is inert (catalog still validates). - Archive: cp -p the .prom pair so archived mtimes describe the archived bytes (prune stays mtime-based); ARCHIVE_VERSION is now a meaningful, helper-owned constant (2) instead of the inert 1. Deferred per plan: historical backfill (#3922), daily-summary read-side (#3923). Refs #3791 Co-authored-by: Claude Code <claude-code@anthropic.com>
🔍 Reviewer: CorrectnessVerdict: CHANGES_REQUESTED Summary: The single-writer helper is well-structured and conformant-by-construction, but CI is red (the Full reviewCycle 1 — complete class-labeled change list. A class not raised here should not be raised in a later cycle. [ci-failure / mirror-drift] — BLOCKING
[regression-risk] — BLOCKING This PR bumps the constant to
[test-coverage] — non-blocking note Nits (inline, non-blocking):
|
🔍 Reviewer: RiskVerdict: CHANGES_REQUESTED Summary: Non-parity change (skills/scripts only — no observable core surface). Two blocking risks: CI is red from an un-synced catalog mirror, and the ARCHIVE_VERSION bump breaks the archive replay tooling that reads it. Full reviewCycle 1 — complete class-labeled change list. [ci-failure] — BLOCKING [regression-risk] — BLOCKING [operational] — non-blocking Parity note: none of the touched paths ( |
Review: Bounce-Back Cycle 1Reason: Reviewer A + Reviewer B requested changes; CI failed (diff-attributable). Reviewer A (Correctness): CHANGES_REQUESTED — Two blocking findings for
Failed-check excerpt: Routing back to |
- Sync .agents/skills/shared/metric-alarms.toml byte-for-byte with the
.claude/ copy (add the [warning_vocabulary] block) — fixes the red
test_agents_catalog_matches_claude in the "Script self-tests" check.
- replay-alarms-on-history.sh: accept the known-good ARCHIVE_VERSION set
{1,2} instead of fail-closing on anything but 1, so archives written by
the #3791 single-writer (ARCHIVE_VERSION=2) are no longer rejected as
corrupt by the replay/regression tooling.
- Add regression test that validate_metadata accepts a v2 archive; bump
TAP_PLAN 477->478.
Refs #3791
Co-authored-by: Claude Code <claude-code@anthropic.com>
tomerweller
left a comment
There was a problem hiding this comment.
Addressed both blocking findings from Bounce-Back Cycle 1 in 5d22fe1:
- [ci-failure / mirror-drift] Synced
.agents/skills/shared/metric-alarms.tomlbyte-for-byte with the.claude/copy (added the[warning_vocabulary]block).test_agents_catalog_matches_claudenow passes locally (16/16 gauge-guard tests green). - [regression-risk]
replay-alarms-on-history.shnow accepts the known-good version set{1,2}instead of fail-closing on anything but 1, so post-#3791 archives (ARCHIVE_VERSION=2) are no longer rejected as corrupt.check-alarm-regression.shonly drives the replay script and reads no version itself, so no other reader needed changing. Added a regression test (validate_metadata accepts ARCHIVE_VERSION=2) — verified it fails on the pre-fix reader and passes after — and bumpedTAP_PLAN477→478. Full snippet suite: 478/478 green.
Note: the unrelated CI / Test red (henyey-ledger manager::tests::test_scan_and_merge_pending_buffer_bounded) is not diff-attributable to this skills/scripts-only PR; it will re-run on this push.
Ready for re-review.
🔍 Reviewer: CorrectnessVerdict: APPROVE Summary: Cycle 2 re-review. Both cycle-1 blocking findings ( Full reviewCycle 2 — sticking to the classes raised in cycle 1. No NEW CLASS DISCOVERED. [ci-failure / mirror-drift] — RESOLVED [regression-risk] — RESOLVED [test-coverage] — RESOLVED (was the non-blocking cycle-1 note)
Verdict: all cycle-1 concerns addressed; no new correctness concerns. |
🔍 Reviewer: RiskVerdict: APPROVE Summary: Non-parity change (skills/scripts/catalog only — no observable core surface per docs/PARITY.md). Both cycle-1 blocking risks ( Full reviewCycle 2 — sticking to the classes raised in cycle 1. No NEW CLASS DISCOVERED. [ci-failure] — RESOLVED [regression-risk] — RESOLVED [operational] — non-blocking (unchanged from cycle 1) Parity note: none of the touched paths ( |
✅ MergedCommit: ebbe574 Triple-green (cycle 2): Reviewer A (Correctness) APPROVE, Reviewer B (Risk) APPROVE, CI green (43/43). Both cycle-1 blocking findings (catalog mirror-drift, ARCHIVE_VERSION reader) confirmed resolved on the current head. Follow-up issues filed for unaddressed inline review comments: none (no inline threads). |
Closes #3791
Summary
The monitor's per-tick artifacts had fragmented into many incompatible schemas because each tick path (interactive vs headless) hand-rolled its own row/heredoc —
tick-history.jsonlhad 100+ key-signatures andwarningsaccumulated 571 spellings for ~17 real conditions (#3757 census). This PR introducesscripts/lib/monitor-tick-artifacts.pyas the single writer of every per-tick artifact and replaces the four hand-rolled heredocs inmonitor-tick/SKILL.mdwith calls to it, so every appended row is conformant by construction.Key pieces:
emit-rowbuilds the canonical 9-field row (corrected set: notick, withself_reflect), stamps a UTC ISO-8601ts, constrainswarnings/actionsto a closed vocabulary (unknown → registeredotherfallback), promotes embedded measurements to typed sibling keys (low-disk 90%→warnings:["low-disk"], disk_free_pct:90), and self-checks before append. Self-check failure is fail-loud-but-non-fatal: the row goes to atick-history.rejected.jsonlsidecar and the tick continues, so a helper bug can never wedge the monitor.validate-rowenforces a required-subset + per-field-type contract (never set-equality, so diagnostic supersets pass — the "canonical-9 equality rejects 100% of the corpus" trap) and type-guardstsbefore the regex (null/inttsrejected without raising).names inmetric-alarms.toml∪ a new top-level[warning_vocabulary].non_alarmtable ∪{other}.eval-alarms.pyreads only[[alarm]]/schema_version, so the new table is inert (catalog still validates:alarm_count=37, valid=true)..prompair copied withcp -pso archived mtimes describe the archived bytes (prune stays mtime-based);ARCHIVE_VERSIONis now a meaningful, helper-owned constant (2) instead of the inert1.Plan reference
Converged Plan comment
Test plan
scripts/test-monitor-skill-snippets.sh— 477/477 pass (was 465; +12 new monitor: single-writer helper + schema validation for tick-history.jsonl and archive metadata.env #3791 assertions, Tests 64/65 rewritten)cargo fmt --check(no Rust changed)python3 -m py_compile scripts/lib/monitor-tick-artifacts.pyeval-alarms.py --validate-onlyon the catalog with the new table:valid=truefiled-#Npreserved, watch retained)Note:
--strictmode reports a pre-existing structural-drift warning inmonitor-loop/SKILL.md(unrelated file, present onorigin/main); the default suite is green.Regression test (bug-fix)
scripts/test-monitor-skill-snippets.sh— 14 assertions (rewritten Tests 64/65 + 12 new; e.g.tick-row validate: null ts → clean reject,tick-row emit: embedded measurement promoted,archive metadata: ARCHIVE_VERSION=2).c04cdee— verified all 14 FAILED on main (helper absent →emit-row/validate-rowerror; SKILL still hand-rolls the heredoc).8c4d184.Deviations from plan
if ! ROW_ERR=$(... 2>&1 1>>"$HIST"); then <sidecar>(capturing stderr without a temp file) rather than a bareemit-row ... >> "$HIST", so the reject-sidecar path is wired inline while keeping the stdout-row/append contract. No behavior change on the happy path.actionsvocabulary is a small fixed verb set kept in the helper (withfiled-#<N>normalizing tofiledwhile preserving the full token) rather than a second TOML table — the plan specified only one new[warning_vocabulary]table.🤖 Generated with Claude Code