Make monitor-tick check-(7) startup-peak phase grep quote-tolerant

[tomerweller]

Closes #3844

Summary

Check-(7)'s startup-peak phase extraction in .claude/skills/monitor-tick/SKILL.md used the pattern phase=[a-z_-]+, but tracing's default field formatter quotes string values, so the sampler actually emits startup_peak_anon_rss_mb=<N> phase="<phase>". The class matched neither the leading " nor — because + requires a class member — the numeric prefix, so the whole alternation failed to match and STARTUP_PEAK_PHASE was silently the empty string on every tick. The (phase=…) suffix therefore never reached WATCH_ITEMS, and the daily summary could see the peak move but never which startup phase produced it.

The fix adds an optional quote "? on both sides of the class in both grep stages and strips captured quotes with tr -d '"'. Using "? rather than a mandatory " keeps the pattern robust to both the quoted Text form and any unquoted rendering (matching the sampler's own unit-test tolerance). The misleading format comment is corrected to the quoted form so the next reader isn't misled the same way.

Plan reference

Converged Plan comment

Test plan

• bash -n scripts/test-monitor-skill-snippets.sh — syntax OK
• bash scripts/test-monitor-skill-snippets.sh — all 489 TAP assertions pass (was 486; +3 for this fix)

Regression test (kind: bug-fix)

• Test: scripts/test-monitor-skill-snippets.sh → startup-peak phase block (extracts the check-(7) STARTUP_PEAK_PHASE= bash block from SKILL.md and runs it against the real emitted line startup_peak_anon_rss_mb=21047 phase="cache-scan").
• Pre-fix: committed as 85a77f7 — verified FAILED: not ok 488 … quote-tolerant extraction yields 'cache-scan' (STARTUP_PEAK_PHASE empty) and not ok 489 … pattern is quote-tolerant and strips quotes.
• Post-fix: verified PASSES after 3d72368 (ok 487/488/489, harness exit 0).

Deviations from plan

None.

🤖 Generated with Claude Code

[tomerweller]

🔍 Reviewer: Correctness

Verdict: APPROVE

Summary: The regex is made quote-tolerant exactly as the issue prescribes, and a new self-sourcing regression test provably fails at the pre-fix pattern and passes after — a complete cycle-1 change-list with no outstanding concerns.

Full review

Cycle-awareness: no prior ## 🔍 Reviewer: Correctness comment on this PR → this is a COMPLETE cycle-1 review.

Change under review (.claude/skills/monitor-tick/SKILL.md):

• Pattern phase=[a-z_-]+ → phase="?[a-z_-]+"? on both the match and the extraction grep, plus | tr -d '"' to strip captured quotes. This is precisely the fix the issue specifies and correctly handles tracing's quoted string-field output (phase="cache-scan"). Behavior on the unquoted form is preserved (the "? are optional), so no regression for any environment that emits the unquoted form.
• The # "...phase=<phase>..." comment is corrected to the quoted form phase="<phase>" with a (tracing quotes string fields) note — kills the same misreading for the next editor.

Test-verification gate (linked issue kind: bug): SATISFIED.

• scripts/test-monitor-skill-snippets.sh adds 3 assertions (TAP_PLAN 486 → 489, matching the +3 assertions exactly).
• The behavioral assertion sources the STARTUP_PEAK_PHASE block from SKILL.md itself (content-selected awk over the ```bash fences, not position), rewrites the hardcoded /home/tomer/data/$MONITOR_SESSION_ID base to a portable temp dir, and evals it against the real emitted quoted line, asserting `cache-scan`. At the pre-fix pattern this yields empty → the test provably fails on `origin/main` and passes only with the fix. This is a genuine regression test, not a tautology.
• A doc-consistency assertion additionally locks the pattern shape (phase="?[a-z_-]+"? + tr -d '"') so a future edit that drops quote-tolerance re-breaks the test.

Build/verify: the Monitor-Tick Skill Snippets CI check (ci.yml:173 → bash scripts/test-monitor-skill-snippets.sh) is green on the PR head, confirming the new assertions pass and the TAP plan count is consistent.

No error-handling, readability, or dead-code concerns. Not self-modifying (monitor-tick is outside the pipeline-skill self-mod set). No concerns to raise.

[tomerweller]

🔍 Reviewer: Risk

Verdict: APPROVE

Summary: Documentation/observability-only change to a monitor skill and its test harness; no runtime code, no observable/interop surface, no API/data-format or operational risk.

Full review

Cycle-awareness: no prior ## 🔍 Reviewer: Risk comment → COMPLETE cycle-1 review.

Blast radius: two files — .claude/skills/monitor-tick/SKILL.md (a shell snippet embedded in operator-facing skill docs) and scripts/test-monitor-skill-snippets.sh (its test). No crate/source code, no crates/{scp,herder,ledger,tx,overlay} parity surface, no wire/XDR/hash/RPC/CLI contract touched. Nothing here can affect validator behavior or determinism.

• Regression/perf: none — the grep runs once per tick against a log tail; adding "? and a tr -d '"' is negligible.
• Security: no new input surface; tr -d '"' narrows, not widens.
• Operational: strictly improves observability — the (phase=…) suffix on the startup_peak_mb watch item now populates instead of silently vanishing. No alarm/gating behavior changes.
• Robustness note (non-blocking, not filing): the char class [a-z_-] still excludes uppercase/digits in phase names; today's phase labels (cache-scan, etc.) are lowercase-kebab so this is fine, and it matches the pre-existing class — no new risk introduced.

No blocking concerns.

[tomerweller]

Review: Bounce-Back Cycle 1

Reason: CI failed (unrelated, will rebase)

Reviewer A: APPROVE — fix matches the issue's prescribed quote-tolerant pattern; new self-sourcing regression test provably fails pre-fix and passes after.
Reviewer B (Risk): APPROVE — docs/observability-only change (monitor skill + its test harness); no runtime code, no parity/interop surface, no operational risk.
CI: red (unrelated)

Both agent reviewers APPROVE. The blocking signal is CI, which is red on unrelated infrastructure flakiness — not the diff:

• test (local, rpc, test_stellar_rpc_up.go test_stellar_rpc_healthy.go test_friendbot.go) — exit 124 (timeout)
• test (local, core,rpc,horizon, ...) — exit 124 (timeout)

Both are Quickstart external-service (stellar-rpc / horizon / friendbot) container bring-up probes timing out at the tighter SHARD_PROBE_TIMEOUT=240 budget:

=== Failed (exit 124), not retryable ===
##[error]Process completed with exit code 1.

This PR touches only .claude/skills/monitor-tick/SKILL.md (a shell snippet in operator docs) and scripts/test-monitor-skill-snippets.sh (its test). Neither can affect docker container bring-up for RPC/horizon/friendbot. These same Quickstart tests are chronically cancelled/flaky on main (last green run 2026-08-22). The diff-relevant check — Monitor-Tick Skill Snippets (ci.yml:173) — is green.

Routing back to ready-for-doing for /do Mode B: rebase on origin/main and re-push to re-trigger CI. If the Quickstart integration tests are still red after a clean rebase, the next /review-pr will escalate to blocked for operator investigation of the flaky CI (not this diff).

[tomerweller]

Rebased on origin/main (no-op — branch was already on the latest main) and re-pushed to re-trigger CI. The prior bounce was caused only by unrelated flaky Quickstart external-service (rpc/horizon/friendbot) container bring-up timeouts; the diff-relevant Monitor-Tick Skill Snippets check was green and both agent reviewers APPROVED. Ready for re-review.

[tomerweller]

Review: Blocked — Unrelated CI Red Persists After Retry

Reviewer A (Correctness): APPROVE — quote-tolerant regex matches the issue prescription; self-sourcing regression test provably fails pre-fix and passes after.
Reviewer B (Risk): APPROVE — docs/observability-only change (monitor skill + its test harness); no runtime code, no parity/interop surface, no operational risk.
External reviewers: tomerweller: COMMENTED (non-blocking /do re-trigger note).
CI: red (unrelated).

Both agent reviewers APPROVE and the diff-relevant check (Monitor-Tick Skill Snippets) is green. The blocking signal is CI, red on unrelated infrastructure flakiness — the failing step in both jobs is Run probes through wrapper, the Quickstart external-service (stellar-rpc / horizon / friendbot / core) container health probes. The diff touches only .claude/skills/monitor-tick/SKILL.md and scripts/test-monitor-skill-snippets.sh, which have no causal path to Quickstart external-service bring-up.

Failing jobs:

• test (local, rpc, test_stellar_rpc_up.go test_stellar_rpc_healthy.go test_friendbot.go)
• test (local, core,rpc,horizon, test_core.go test_horizon_up.go test_horizon_core_up.go ...)

Why blocked, not bounced: This is the second consecutive unrelated-CI-red cycle. The prior /review-pr tick already bounced this PR for the same unrelated flakiness (## Review: Bounce-Back Cycle 1), and /do retried by pushing a clean-rebase empty commit (301508a) to re-trigger CI. CI came back red on the identical external-service probe flakiness. Per the unrelated-CI-red matrix rule ("If still red after rebase, the next /review-pr will mark blocked"), auto-retries cannot clear flaky external infra, so this needs operator attention rather than another /do loop.

Operator options:

• Re-run only the failed Quickstart probe jobs; if they pass, post ## Review: Reset and re-queue.
• If the Quickstart external-service flakiness is a known-broken-main condition, post ## Review: Reset with a one-line reason to re-attempt once infra is healthy.
• Merge manually with --admin if the unrelated red is accepted (docs-only diff, both reviewers APPROVE, diff-relevant check green).