Skip to content

Gate check (11) urgent label on Verify Execution (Mainnet) failure - #3932

Open
tomerweller wants to merge 2 commits into
mainfrom
do/issue-3820
Open

Gate check (11) urgent label on Verify Execution (Mainnet) failure#3932
tomerweller wants to merge 2 commits into
mainfrom
do/issue-3820

Conversation

@tomerweller

Copy link
Copy Markdown
Collaborator

Closes #3820

Summary

monitor-tick check (11) steps 4/5 applied the urgent label to every
main-branch CI failure, resting on a "failing CI on origin/main blocks deploy"
rationale that §10 step 4 explicitly removed in #3351. Post-#3351 the deploy
signal targets the latest green Verify Execution (Mainnet), so a red run of
that workflow is the only CI conclusion that defers deploy (and independently
signals a hash/parity mismatch — a canonical urgent criterion). This gates
the urgent label on the failing workflow being Verify Execution (Mainnet)
in both the existing-issue and new-issue paths, strikes the stale rationale
sentence, and adds a one-line clarification to the canonical label policy.
Every other main-branch workflow (CI, Quickstart, Gitlinks,
History Publish, Push on main) now files/comments unlabeled, ending the
alarm-fatigue mislabeling described in the issue.

Adopts candidate shape (1) from the issue (gate on workflow role) per the
converged plan.

Plan reference

Converged Plan comment

Test plan

  • bash scripts/test-monitor-skill-snippets.sh passes (479/479, exit 0)
  • New structural TAP test [HIGH] [herder] max_slots can be weaponized to block legitimate envelopes for already-buffered slots #479 verifies check (11) gates urgent on
    Verify Execution (Mainnet), strikes the stale rationale, and drops the
    unconditional gh issue create --label urgent
  • check_filing_balance "^## CI check workflow" still satisfied (create
    and move-issue-status lines preserved)
  • cargo fmt/clippy/test — N/A: change touches only skill markdown and the
    shell test harness; no Rust surface

Note: --strict mode surfaces one pre-existing WARNING in
monitor-loop/SKILL.md (recover_session_from_stdout), a file this PR does not
touch; it is present on origin/main and unrelated to this change.

Regression test (kind: bug-fix)

Deviations from plan

None.

🤖 Generated with Claude Code

Tomer Weller and others added 2 commits August 25, 2026 00:05
Adds a structural TAP self-test asserting monitor-tick check (11) gates the
`urgent` label on a failing `Verify Execution (Mainnet)` workflow, strikes the
stale "blocks deploy and meets the urgent criteria" rationale, and drops the
unconditional `gh issue create --label urgent`. Fails on main today because the
stale sentence is present and check (11) applies urgent unconditionally.

Refs #3820

Co-authored-by: Claude Code <claude-code@anthropic.com>
Post-#3351, §10 step 4 removed the "CI failure blocks deploy" gate: the deploy
signal now targets the latest green Verify Execution (Mainnet), so a red run of
that workflow is the only CI conclusion that defers deploy (and independently
signals a hash/parity mismatch). Check (11) steps 4/5 still applied `urgent`
unconditionally to any main-branch CI failure, resting on the now-false "blocks
deploy" rationale — mislabeling benign failures (Quickstart, CI, Gitlinks, …)
as urgent and driving alarm fatigue.

Gate the urgent label on the failing workflow being Verify Execution (Mainnet)
in both the existing-issue (step 4) and new-issue (step 5) paths, strike the
stale rationale sentence, and add a one-line clarification to the canonical
label policy tying the CI criterion to that workflow.

Refs #3820

Co-authored-by: Claude Code <claude-code@anthropic.com>
@tomerweller tomerweller added the pdr-managed PR opened by the henyey project-tick pipeline /do skill label Aug 25, 2026
@tomerweller

Copy link
Copy Markdown
Collaborator Author

🔍 Reviewer: Correctness

Verdict: APPROVE

Summary: Doc/script-only change that correctly makes check (11) steps 4/5 gate the urgent label on the failing workflow being Verify Execution (Mainnet), strikes the now-false "blocks deploy" rationale, and mirrors the clarification in the canonical label policy. Backed by a scoped structural self-test that passed CI.

Full review

Cycle 1 (no prior ## 🔍 Reviewer: Correctness comment on this PR) — complete class-labeled change-list below.

  • correctness: The change matches the linked issue's suggested resolution shape (1) exactly. SKILL.md check (11) step 4 now adds urgent ONLY IF the failing workflow is Verify Execution (Mainnet) (the sole workflow the post-monitor-tick: [needs-decision] deploy gate never fires on fast-moving main (CI cancelled per-head) #3351 deploy gate consumes per §10 step 4), and step 5's gh issue create no longer carries an unconditional --label urgent — it appends the label only via the same ONLY IF predicate. The stale "failing CI on origin/main blocks deploy and meets the urgent criteria" sentence is struck. This resolves the §10-vs-check-(11) contradiction the issue describes.
  • correctness: scripts/lib/monitor-label-policy.md — the urgent criteria row's CI clause is updated consistently to scope to a failing Verify Execution (Mainnet) run, keeping the "single source of truth" aligned with the skill.
  • test-coverage (kind: refactor/docs of a skill contract): A new structural self-test is added in scripts/test-monitor-skill-snippets.sh, scoped via extract_md_section to the ## CI check workflow section so unrelated §10 matches can't mask a regression. It asserts (a) the stale rationale string is gone, (b) Verify Execution (Mainnet) is present, (c) no unconditional gh issue create --label urgent, and (d) ONLY IF gating is present. TAP_PLAN is bumped 478→479 for the single added assertion — consistent. The "Monitor-Tick Skill Snippets" CI check is SUCCESS, so the new test compiles and passes.
  • readability: The ONLY IF predicate is stated once and referenced by step 5 ("same predicate as step 4"), avoiding drift between the two steps. Clear.

No blocking concerns. Note: the "Test" CI check is red, but the failure is manager::tests::test_scan_and_merge_pending_buffer_bounded in crates/ledger/src/manager.rs — a crate this PR does not touch. Unrelated to the diff (recent main CI is green). Not a correctness concern with this change.

@tomerweller

Copy link
Copy Markdown
Collaborator Author

🔍 Reviewer: Risk

Verdict: APPROVE

Summary: Low-risk change to a monitor-tick labeling contract. It narrows (never widens) when urgent is applied, so the failure mode is at most an under-escalation of one specific workflow — and that workflow's failure is independently caught by other urgent criteria (hash/parity mismatch). No code, data-format, or wire-surface impact.

Full review

Cycle 1 (no prior ## 🔍 Reviewer: Risk comment) — complete class-labeled change-list below.

  • regression-risk: The change makes the urgent label conditional where it was unconditional. The direction of change matters: previously every main-branch CI failure was labeled urgent (over-escalation / alarm fatigue, the stated bug); now only Verify Execution (Mainnet) failures are. The worst-case regression is under-labeling a genuinely deploy-blocking failure. Mitigating factor cited in both the diff and the issue: post-monitor-tick: [needs-decision] deploy gate never fires on fast-moving main (CI cancelled per-head) #3351 Verify Execution (Mainnet) is the only workflow the deploy gate consumes, and its failure is a hash/parity mismatch which independently meets the urgent criteria (the monitor-label-policy.md "Hash mismatch (any kind)" row). So no genuinely-urgent CI signal is dropped by this narrowing. Acceptable.
  • operational: The labeling contract is consumed by downstream triage. Both the skill and the canonical policy doc are updated together, so triage and the skill stay consistent — no split-brain between the "single source of truth" and the skill text.
  • doc-drift: The two edited docs now agree with §10 step 4. The specific stale sentence the issue flagged is removed. No remaining internal contradiction spotted in the CI-check section.
  • security / api-shape / parity: N/A — no code, no observable/interop surface (hashes, XDR, wire bytes, RPC/CLI), no crate touched.

No blocking concerns. The red "Test" CI check is an unrelated ledger-crate test failure (test_scan_and_merge_pending_buffer_bounded), not attributable to this doc-only diff.

@tomerweller

Copy link
Copy Markdown
Collaborator Author

Review: Bounce-Back Cycle 1

Reason: CI failed (unrelated, will rebase)

Reviewer A: APPROVE — change correctly gates urgent on Verify Execution (Mainnet), strikes the stale rationale, adds a scoped self-test (passing in CI).
Reviewer B: APPROVE — narrowing-only labeling change, no code/observable-surface impact, low risk.
CI: red

The only red check is Test (cargo test --workspace --all-targets): a single failure in
manager::tests::test_scan_and_merge_pending_buffer_bounded at crates/ledger/src/manager.rs:8027.
This PR touches only .claude/skills/monitor-tick/SKILL.md, scripts/lib/monitor-label-policy.md,
and scripts/test-monitor-skill-snippets.sh — it does not touch crates/ledger/. Recent main CI
runs are green, so this is an unrelated flaky/transient failure, not diff-attributable.

Both reviewers approve; the change cannot merge on red CI. Routing back to ready-for-doing for
/do Mode B to rebase on origin/main and re-run CI. If the same unrelated failure persists after a
clean rebase, the next /review-pr will mark this blocked for operator investigation of the flaky
ledger test.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pdr-managed PR opened by the henyey project-tick pipeline /do skill

Projects

None yet

Development

Successfully merging this pull request may close these issues.

monitor-tick: check (11) mandates urgent on every main CI failure via the "blocks deploy" rationale that section 10 removed (#3351)

1 participant