#Hi, could someone help review and merge PR [#44033]? Thanks!

1 messages · Page 1 of 1 (latest)

grizzled lightBOT
#

To help others find answers, you can mark your question as solved via Right click solution message -> Apps -> âś… Mark Solution

ocean charmBOT
#

Looked through the patch series (.patch) — the final shape of the fix makes sense and it’s nicely minimal:

  • resolveHeartbeatContext() now returns early only when the primary run context exists and is explicitly isHeartbeat === false (treat as interactive → don’t suppress), which is exactly the problematic case.
  • Otherwise it falls back to sourceRunId (preserves the old behavior for heartbeat-linked/synthetic runs), and the two new tests cover:
    • primary interactive (false) + source heartbeat (true) → should broadcast
    • primary missing/undefined + source heartbeat → should suppress

One thing I’d double-check (or add a quick assert/comment for): this relies on interactive runs always registering isHeartbeat: false (not just leaving it undefined). If there are any interactive contexts where isHeartbeat is unset, they’d still fall through to the source heartbeat and could reproduce the suppression. If that can’t happen in practice, then this is perfect; if it can, consider defaulting isHeartbeat to false at registration for normal user runs (or adjust the condition accordingly).

Otherwise: âś… logic looks correct, tests are targeted, and this feels safe to merge once CI is green.

hoary radish
#

Is the merge automatic once CI passes, or does it need to be done manually?

ocean charmBOT
#

It’s not automatic just because CI turns green.

On GitHub there are basically two modes:

  1. Manual merge (default): once required checks + required reviews (and any other branch protection rules) are satisfied, someone with write access still has to click “Merge”.

  2. Auto-merge (optional): if the repo has auto-merge enabled and you have permission, you’ll see an “Enable auto-merge” button on the PR. If that’s turned on, GitHub will merge it automatically once all requirements pass.

So: CI passing is necessary, but not sufficient unless auto-merge is explicitly enabled for that PR.

hoary radish
#

CI is green and the changes look good to me. Could someone with write access please merge this when they have a moment?

ocean charmBOT
#

Yep — at this point it’s just waiting on someone with write access.

A couple practical things you can do (without needing extra permissions):

  • Enable auto-merge on the PR if you see the button (then it’ll merge itself as soon as all requirements are satisfied).
  • If you don’t have that option, leave a final short comment on the PR like “CI green, ready to merge” (helps it show up in maintainer queues), and make sure the branch stays up-to-date with main if GitHub starts requiring it.

I can’t merge it from here, but the PR itself looks in good shape.