Skip to content

chore(ci): Cap repo-relay notify job and clarify bot-skip comment - #7153

Merged
blamechris merged 1 commit into
mainfrom
fix/repo-relay-notify-timeout
Aug 7, 2026
Merged

chore(ci): Cap repo-relay notify job and clarify bot-skip comment#7153
blamechris merged 1 commit into
mainfrom
fix/repo-relay-notify-timeout

Conversation

@blamechris

Copy link
Copy Markdown
Owner

Summary

Propagates the two fixes from the canonical repo-relay workflow rollout (already merged in stock-keep, skill-templates, carebridge, and duskwright) to this repo's copy, which predates them:

  • timeout-minutes: 10 on the notify job — the concurrency group serializes runs, so a hung run would queue later notifications behind it for up to the 6-hour default.
  • Reworded bot-skip comment — the old "Skip bots (prevent cascades)" wording overstated the filter. Only this workflow's own actor (github-actions[bot]) is skipped, to prevent self-trigger cascades; other bots (dependabot, Copilot) relay on purpose because their activity belongs in the channel.

No behavioral change beyond the timeout cap — the if condition itself is untouched.

Testing

  • YAML parses; jobs.notify.timeout-minutes == 10 verified.

Propagate two fixes from the canonical repo-relay workflow rollout:

- Add timeout-minutes: 10 to the notify job. The concurrency group
  serializes runs, so a hung run would queue later notifications behind
  it for up to the 6-hour default.
- Reword the bot-skip comment to state its real purpose: only this
  workflow's own actor is skipped (preventing self-trigger cascades);
  other bots like dependabot and Copilot relay on purpose.
Copilot AI lite review requested due to automatic review settings August 7, 2026 03:42

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Updates this repo’s repo-relay GitHub Actions workflow to match the canonical rollout by capping the notify job runtime and clarifying the bot-skip rationale, improving reliability of serialized notifications without changing the job’s gating behavior.

Changes:

  • Add timeout-minutes: 10 to the jobs.notify job to prevent a hung run from blocking queued runs under the workflow concurrency group.
  • Reword the if-guard comment to accurately describe skipping only github-actions[bot] (self-trigger cascade prevention), while allowing other bots to relay.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@blamechris

Copy link
Copy Markdown
Owner Author

Code Review: PR #7153

Summary

A 9-line CI-only change to .github/workflows/repo-relay.yml: adds timeout-minutes: 10 to the notify job and rewrites the comment above the if: guard. Verified against the canonical copy on blamechris/skill-templates@main — after this PR the two files are byte-identical except the #6749 note, which is correctly kept in its repo-local form here (chroxy owns #6749; the canonical uses the qualified blamechris/chroxy#6749 for sibling repos). Nothing else moved: permissions, the if: expression itself, env, the step list and all three action SHA pins are untouched.

Mechanics check out. actionlint is clean, timeout-minutes is job-level and sits after runs-on — the same key order ci.yml uses. A job-level timeout starts counting when the job begins executing, not while it sits pending in the concurrency queue, so a busy queue cannot produce spurious timeouts. All 20 checks pass on 1f35e0a1.

The cap is well-sized. Recent notify job durations: 2m05s, 1m38s, 1m37s, 1m36s. Ten minutes is ~5x headroom and matches the 10-minute tier ci.yml already applies to comparably-sized jobs (e.g. ci.yml:116).

Two comment-accuracy points below. Both are wording-only, both apply equally to the canonical, and neither blocks this PR.

Strengths

  • Genuinely minimal. Diffed the PR head file against the canonical directly rather than trusting the diff summary; the only delta is the intended one plus the deliberate repo-local issue reference.
  • The repo-specific #6749 deviation was preserved, not clobbered by the canonical's qualified form. That is the exact thing a propagation PR usually gets wrong.
  • Timeout value is evidence-based, not a round number picked blind, and is consistent with the repo's existing timeout conventions.
  • Failure mode of the cap is benign: best_effort: 'true' already absorbs transient Discord/infra errors, so a timeout can only fire on a real hang.

Issues Found

Critical

None.

Suggestions

# Location Issue
S1 repo-relay.yml bot-skip comment The new wording says the filter skips "this workflow's own actor" to "prevent self-trigger cascades." The condition is broader than that and the rationale does not apply to this workflow. github.actor != 'github-actions[bot]' matches every GITHUB_TOKEN-driven automation in the repo — stale.yml's labels and comments, release.yml's releases — not just repo-relay's own activity. And repo-relay makes no GitHub writes at all (it posts to Discord), so it has no way to self-trigger. This repo's own auto-tag-on-release.yml header documents the underlying rule: events triggered by the default GITHUB_TOKEN do not create a new workflow run. The guard is defensible belt-and-braces, but the comment should describe what it does: "Skip events attributed to github-actions[bot] — this repo's own GITHUB_TOKEN automation. Other bots (dependabot, Copilot) relay on purpose."
S2 timeout rationale comment "would queue later notifications behind it for up to the 6h default" understates the harm. The concurrency group has no cancel-in-progress, and GitHub keeps at most one pending run per group — each new run cancels the previously pending one. So a hung run does not delay the backlog, it drops all of it but the newest entry. That is visible in this repo's own history: on 2026-08-06 at 04:43, runs 31072070257, 31072070135 and 31072069742 were cancelled roughly one second after creation while 31072069600 held the group. This strengthens the case for the cap; the comment would be more accurate saying later notifications are dropped, not queued.

Both S1 and S2 are inherited from the canonical and are already merged in four sibling repos. Fixing them here would create drift. The right move is to correct the canonical in skill-templates and re-propagate; no change requested on this PR.

Nitpicks

# Location Note
N1 bot-skip comment "Other bots (dependabot, Copilot) relay on purpose" sits about twenty lines below the #6749 note explaining that Copilot reviews deliberately do not trigger this workflow. Read together the two can look contradictory. Naming the carve-out would help: their PRs, comments and issues relay; reviews come through the piggyback path per the note above. Related: #6958.
N2 interaction with the state cache A timed-out job fails, and actions/cache's post step is post-if: success() — so the ~/.repo-relay SQLite state from a killed run is never saved. The next run restores an older snapshot and may re-send. Identical to the trade-off on any hard failure (best_effort turns transient errors into exit 0 but cannot rescue a killed job), so this is acceptable, but it is one behavioral consequence not covered by the PR body's "no behavioral change beyond the timeout cap."

Architecture Notes

The interesting property here is that the timeout and the concurrency group are complementary rather than redundant. The group buys serialization (no races on the SQLite state cache) at the cost of head-of-line blocking, and without a cap that blocking window was the runner's 6-hour default. Ten minutes bounds it to something smaller than the interval between typical repo events, which is what keeps the group from converting one bad run into a silent notification outage.

The one place the design still leans on luck is state durability: relay state lives only in the run-keyed actions/cache entry, and cache saves are gated on job success. A timeout therefore trades a stuck queue for a small, bounded chance of duplicate notifications. That is the correct direction — duplicates are noise, a stalled group is data loss — and it is worth leaving as-is rather than reaching for if: always() on the cache save, which would persist state written by a run that died mid-write.

Deferred Items

None filed in this repo. S1 and S2 are wording fixes to the shared canonical workflow and belong in skill-templates, where the fix can be propagated to all five repos at once; opening them against chroxy would misfile a cross-repo change. Checked the 30 open from-review issues — none are resolved by this PR. #6958 is adjacent (review notifications after the #6749 trigger removal) but is untouched by this change and stays open.

Verdict

Approve. The diff does exactly what it claims, the YAML and Actions semantics are correct, the value is justified by measured runtimes, and the repo-local deviation survived the propagation. The two comment-accuracy points are inherited from the canonical and should be fixed there to keep the five copies identical.

@blamechris
blamechris merged commit dcc4106 into main Aug 7, 2026
21 checks passed
@blamechris
blamechris deleted the fix/repo-relay-notify-timeout branch August 7, 2026 03:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants