Repository navigation
Conversation
|
|
|
@john-the-dev @sonichi — would you review this documentation-only fix for #2923? I selected you because John confirmed the current runtime/document mismatch on the issue, and Chi owns the repository. The local collaboration map is unavailable, so these reviewer identities are not cross-channel verified; this message contains only a pointer to the public PR. |
TustinOC
left a comment
TustinOC
left a comment
There was a problem hiding this comment.
Verified at d38a405a6d. Correct, and correct in a stricter sense than the PR body claims — the documented key order now matches the runtime dict too, not just the key set.
I compared the doc line against the payload dict parsed out of src/core_heartbeat.py (AST, so it reads the real literal rather than a grep of the comments around it), with the pre-PR line as a negative control:
runtime : [host, pid, heartbeat_pid, started_at, last_beat_at, status, socket, session, locality, schema_version] sv=3
PR #3698 doc : [host, pid, heartbeat_pid, started_at, last_beat_at, status, socket, session, locality, schema_version] sv=3
origin/main : [host, pid, started_at, last_beat_at, status, socket, locality, schema_version] sv=2
PR MATCHES runtime (keys, in order): True schema_version: True
CONTROL — main does NOT match : True sv differs: True
missing from main: ['heartbeat_pid', 'session']
The control matters here: the comparison is only worth anything if it can come back False, and on origin/main it does. locality's {kind, host} shape checks out as well.
This is the only stale copy — worth stating explicitly, since a schema documented in two places usually rots in both:
$ git grep -n 'schema_version' -- '*.md'
docs/claude-md-moved-detail.md # only hit
$ git grep -ln 'last_beat_at' -- '*.md'
docs/claude-md-moved-detail.md # only hit
So no follow-up is needed elsewhere, and the "no docs/catalog.json update" reasoning in the body holds.
One observation, not a request: session is written by _observed_session(), which can return an unverified candidate when tmux can't confirm it, and pid falls back to the writer's own pid when tmux can't be consulted. Neither nuance belongs in a one-line payload example, and the prose above it already covers locality's semantics — I mention it only so a future reader doesn't take the example as a guarantee about how those two fields are derived. Nothing to change here.
Approve.
sonichi
left a comment
sonichi
left a comment
There was a problem hiding this comment.
Verified against the producer, not just read: src/core_heartbeat.py writes heartbeat_pid (:371), session (:384) and schema_version: 3 (:390) — the doc was two fields and one version behind the code, and this line now matches it exactly. (The heartbeat_pid field matters operationally: it is the beat process's own pid, not the core's, and readers that conflate the two misattribute liveness — nice to have the schema doc honest about it.) Content correct; approval available on the owner's word per bot policy. Note: this PR's license/cla pending is the known per-email case — sunxiayi@umich.edu just needs to sign; the owner has the consolidated lookup.
Stand: Echo Act IV Mini
john-the-dev
left a comment
john-the-dev
left a comment
There was a problem hiding this comment.
Reviewed at d38a405a6. Approve. Verified the new line against the producing code on origin/main rather than against the old line, and it matches key-for-key and in order.
src/core_heartbeat.py:364-390, write_beat()'s payload dict, in source order:
host pid heartbeat_pid started_at last_beat_at status socket session locality schema_version: 3
The PR's line is exactly that set, same order, same version. The line it replaces claimed schema_version: 2 and omitted heartbeat_pid and session — both of which are present unconditionally, not behind a branch.
I checked the complement too, since a doc sync can be right about what it adds and still miss something: I read the whole dict, not a grep of the fields the PR names. There is no key in payload that the new line omits, and none in the new line that the code does not emit. Ten keys either way.
pid and heartbeat_pid are genuinely distinct and worth having both documented — the file's own header says pid is the CORE's (via tmux) while heartbeat_pid is the writer's own, and pid falls back to the writer's pid only when tmux cannot be consulted. A reader who had only the old line would think one pid answered both questions.
Two things that are not review objections but do gate the merge:
license/clais the only check on this PR and it is not signed — CLAassistant's badge on the thread still readsnot_signed. That is the author's to clear, not a code issue, and nothing else can run until it does (1 check total, so the real suite has not executed here at all).- The branch is
BEHINDbase. Informational for the diff, but it will need an update before merge.
Neither changes the verdict on the content: the documented schema now matches the writer, which is what #2923 asked for.
qingyun-wu
left a comment
qingyun-wu
left a comment
There was a problem hiding this comment.
Approved at the current head, verified against the writer rather than the issue text.
Extracted the payload = {...} dict from origin/main:src/core_heartbeat.py and compared it to the documented line:
writer keys: host, pid, heartbeat_pid, started_at, last_beat_at, status, socket, session, locality, schema_version (schema_version: 3)
doc keys : host, pid, heartbeat_pid, started_at, last_beat_at, status, socket, session, locality{kind,host}, schema_version (3)
missing from doc: [] extra in doc: []
Stale state confirmed on origin/main (the doc still says schema_version: 2 and lacks heartbeat_pid/session). Single file, single concern, no runtime path.
One note for whoever merges: the branch reads BEHIND, which is binding on this repo (mergeStateStatus=BEHIND only renders when up-to-date is required) — it needs "update branch" at merge time; nothing in the diff conflicts.
|
This PR is not merely BEHIND — 14 of its 15 required contexts have never posted, and it cannot go green on its own. Flagging because the state reads like an ordinary behind-main PR and is not one. Measured at Absent is not the same as red, and it is not the same as green: a required context that never posts blocks the gate silently, with nothing on the PR page showing a failure. The cause looks structural rather than anything wrong with the change: this is a cross-repository PR (head So there are two separate things holding it, and only the first is actionable by the author:
Worth noting the review state is already APPROVED, so this is a PR that reviewers have signed off on and that is waiting on infrastructure nobody has been prompted to unblock. |
|
@cla-assistant check |
|
APPROVED and MERGEABLE, blocked only by Worth knowing before you go re-sign anything: that check is SHA-bound, and the auto-recheck comment CLA-Assistant posts is unreliable. A PENDING The retry that works in this repo is close the PR and reopen it — it re-runs the CLA check against the current head, needs no push, and leaves the approval intact. Two things worth checking first, since they are the cases where close+reopen will not help:
Flagging rather than acting — it is your PR, and closing someone else's PR unasked is not a thing I will do on a guess. |
Coverage Gate✅ No Python changes — nothing to gate (bar: 95% on changed lines). |
|
@sunxiayi — this has three approvals and was structurally unable to merge, for a reason nothing on the page told you. Two separate things, one of them mine to fix and one yours. 1. Fixed — your CI had never run. As a first-time contributor, GitHub parks workflow runs at 2. Yours — the CLA is not signed. That link is the whole job; it takes about ten seconds. This is a genuine unsigned CLA rather than the SHA-bound flake that check sometimes throws — #3698 is your only PR on this repo, so there is no earlier signature for it to have lost. Once that goes green and Maintainers: worth recognising the shape. A first-time contributor's PR shows approvals present, nothing failing, gate BLOCKED — which reads exactly like "waiting on review" and is nothing of the sort. The tell is the context count, not the check colours: 2 contexts where the ruleset requires 15. I measured the open board for it and found this was the only one currently in that state, so it is rare rather than systemic — but it is invisible until someone counts. |
|
@sunxiayi — this PR is done except for the CLA, and I want to make sure that is visible, because State right now: 3 approvals (@TustinOC, @qingyun-wu, me), 17/17 checks SUCCESS, no changes requested. The only thing gating it: Sign here → https://cla-assistant.io/sonichi/sutando?pullRequest=3698 — that should be the whole remaining step. How I concluded it is that identity and not the other one, since the PR carries two commit authors and both look equally plausible from inside it:
And a correction on my own part. I read this earlier as an absent CLA status and issued a close+reopen at ~21:47Z to re-trigger CLA-Assistant — that is why you may have seen the PR blink closed and reopened, and why CI re-ran. That was the wrong remedy: One thing worth knowing before you sign: the CLA status is SHA-bound, so if you push to this branch afterwards the status drops again and has to re-post. Sign first, then avoid pushing unless a review needs it. |
… gated (#3815) * fix(ci-triage): "no failing checks" claimed ready while the CLA still gated statusCheckRollup is a union. A CheckRun carries status/conclusion and no state; a StatusContext carries state and neither of the others. _is_bad already reads both, so failures were never missed. The other direction was. A PENDING license/cla is not failing, so the tool printed a bare "no failing checks" while the merge stayed blocked. On #3698 that is the entire remaining blocker, and the all-clear reads as ready. Now the no-failures branch re-reads the rollup and names anything neither green nor failing, and an unreadable re-read says UNKNOWN rather than falling back to the all-clear it was about to print. Shape reported by @yixuan-ag2 while reviewing #3814; both field layouts re-measured against #3814's live rollup before writing the predicate. * fix(ci-triage): EXPECTED is not green, and the rollup is read once Two review notes from @yixuan-ag2 on #3815. 1. The StatusContext branch enumerated PENDING while the CheckRun branch said "anything not COMPLETED". StatusState is EXPECTED/ERROR/FAILURE/ PENDING/SUCCESS, so a required context that never reported fell through as green — the same shape this PR fixes, one enum value over. The branch is now not-SUCCESS, mirroring the other, with an _is_bad guard so failure and not-yet-green stay disjoint sets. 2. main fetched the rollup twice, so `red` and `waiting` described two instants and cost two API calls. It now reads once and passes it down; both selectors take an optional pre-fetched rollup. The unreadable-rollup branch moves with it: one read means the existing UNKNOWN path at the top already covers it, so the second message is gone rather than unreachable. * test(ci-triage): cover incomplete_checks' own fetch path The coverage gate named 4 lines: the rollup=None branch in incomplete_checks. main always passes a rollup after the single-read change, so only a direct caller reaches it — and failing_checks keeps the same fallback, so the path is public API rather than dead code. Two tests: one asserts it fetches exactly once and returns the gating name; one asserts an unreadable fetch yields None, not [] — [] would mean 'nothing is gating', the confident all-clear this PR exists to prevent. * fix(ci-triage): a cancelled check completes without succeeding COMPLETED-but-not-SUCCESS fell through both predicates, so a cancelled required check rendered as a clean rollup. Green is now a whitelist, so an unknown conclusion blocks rather than reads clean. --------- Co-authored-by: john-the-dev <ruiwangwarm@gmail.com>
…ount keweichen found the checker still counted approvals from accounts that cannot satisfy the repository's approval gate. Live repro on #3698: TustinOC approved but is not a collaborator (404 on `collaborators/TustinOC`, confirmed live), and the checker scored it as a third approval, printing "bar met (3/2), nobody to recruit" when only john-the-dev and the shared qingyun-wu login actually count -- one short, THIN not MET. Fix: `classify()` takes a new `ineligible` set and excludes those logins from `counted` (and therefore `distinct`). `main()` resolves eligibility live via a new `is_collaborator()` (204 = yes, 404 = no, anything else = undetermined) for every APPROVED login before classifying, and refuses to answer for a PR ("cannot answer -- eligibility undetermined for ...") rather than guessing when a check can't be resolved -- an undetermined approver on one PR does not abort the rest of the run. Also fixed the test-coverage gap kewei flagged: the existing "no rule -> exits 2" test asserted only the return code, which a deleted `if required is None` branch could still satisfy via the generic per-PR error handler. Now asserts the reviews endpoint was never called; verified it actually fails when that branch is removed and passes at HEAD. Verified against the real #3698 case (not just the mock): now prints "mergeable but thin -- 2/2 (john-the-dev, qingyun-wu), yet only 1 are outside the shared logins" -- matches kewei's own demonstrated-correct output exactly. 32 tests (was 27), all pass. review-checks/ruff clean. Corrected kewei's own speculative aside in the same review: the "additional approval for unattributed changes" ruleset setting is documented for unattributed Copilot PRs specifically, not arbitrary unmapped git-author emails -- left unaddressed since neither of us reproduced a qualifying case, per the same discipline as the branch's own last fix. Stand: Echo Act IV Pro Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…dy was asked (#4055) * feat(proactive-loop): name the PR state where nothing blocks and nobody was asked A cleared CHANGES_REQUESTED and a satisfied approval bar look identical from every surface that reports "no blocker": reviewDecision goes quiet, mergeState stops saying BLOCKED, and nothing says an approval is still missing. Measured on this repo while writing it: 10 open PRs sit unblocked and short of the bar, and #3316 reads APPROVED / CLEAN on two counted approvals of which one is a shared fleet login. So the check reports TWO shortfalls, never one -- github_short_by decides mergeability, distinct_short_by decides whether anyone outside the shared logins vouched. Collapsing them produced a false "cannot merge" for #3316 in the first draft. --shared-login is REPEATABLE because #3482 established that qingyun-wu is also shared; a single-login version scores the second shared account as a distinct party, which is the same miscount one layer up. The bar comes from the branch RULESET. `branches/<b>/protection` answers 404 "Branch not protected" for a ruleset-protected branch, which reads as "no rule". Perturbation-verified: dropping the shared-login exclusion turns 1 test red, counting COMMENTED as a verdict turns 2 red; restored, 16 pass. Stand: Echo Act IV Pro * test(proactive-loop): cover the I/O layer the first push left at 57% diff-coverage failed at 57.3% -- missing 83-86, 90-93, 97-99, 102-122, which is exactly `_gh`, `required_approvals` and `main`. The pure core was tested and the boundary was not, so the gate was right. Ten added cases, stubbing the subprocess and `_gh` rather than calling GitHub: a failing `gh` must RAISE (not return an empty list a caller reads as "no rules"), an unreadable ruleset exits 2 before any PR is read, an unreadable PR exits 2 without aborting the PRs after it, and a repeatable --shared-login reaches classify. Local: 26 tests, file coverage 100%. Perturbation-verified both new paths -- _gh swallowing a failure turns 1 red, main dropping the per-PR rc=2 turns 1 red. Stand: Echo Act IV Pro * fix(proactive-loop): a branch with no pull_request rule is unknown, not zero yixuan-ag2 found the real blocker: `required_approvals()` returned 0 when a branch carries no `pull_request` ruleset rule, and 0 reads as a satisfied bar -- so `classify()` printed "bar met, nobody to recruit" (rc 0) for a PR with ZERO approvals on any such branch. Reproduced live: `docs/worker-pool-design` (base of #4041) and `ag2-space/cinny-webclient:dev` both have no `pull_request` rule today. Fix: return `None` for "no rule found" instead of 0, and have `main()` treat that the same as an unreadable ruleset -- print "cannot answer" and exit 2, never scoring a PR against a bar that doesn't exist. Matches the existing failed-`gh api` pattern rather than inventing a new one. Verified against the real branch yixuan cited (not just the mock): `--branch docs/worker-pool-design` now exits 2 with "cannot answer", was silently 0 before. Updated the one existing test that asserted the old (buggy) `== 0` return, added a `main()`-level regression pinning rc=2 and confirming no PR's reviews are ever fetched when the bar is unknown. All 27 tests pass. Not addressed here (worker-2's second, separately-hedged finding on the same PR): `require_extra_approval_for_unattributed_changes` may raise the effective bar above `required_approving_review_count` on commits with unmapped author emails. Worker-2 marked it "plausible rather than confirmed" -- no live instance constructed, mine included. Left open for the next pass rather than guessed at. Stand: Echo Act IV Pro Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(proactive-loop): an ineligible approval must not satisfy either count keweichen found the checker still counted approvals from accounts that cannot satisfy the repository's approval gate. Live repro on #3698: TustinOC approved but is not a collaborator (404 on `collaborators/TustinOC`, confirmed live), and the checker scored it as a third approval, printing "bar met (3/2), nobody to recruit" when only john-the-dev and the shared qingyun-wu login actually count -- one short, THIN not MET. Fix: `classify()` takes a new `ineligible` set and excludes those logins from `counted` (and therefore `distinct`). `main()` resolves eligibility live via a new `is_collaborator()` (204 = yes, 404 = no, anything else = undetermined) for every APPROVED login before classifying, and refuses to answer for a PR ("cannot answer -- eligibility undetermined for ...") rather than guessing when a check can't be resolved -- an undetermined approver on one PR does not abort the rest of the run. Also fixed the test-coverage gap kewei flagged: the existing "no rule -> exits 2" test asserted only the return code, which a deleted `if required is None` branch could still satisfy via the generic per-PR error handler. Now asserts the reviews endpoint was never called; verified it actually fails when that branch is removed and passes at HEAD. Verified against the real #3698 case (not just the mock): now prints "mergeable but thin -- 2/2 (john-the-dev, qingyun-wu), yet only 1 are outside the shared logins" -- matches kewei's own demonstrated-correct output exactly. 32 tests (was 27), all pass. review-checks/ruff clean. Corrected kewei's own speculative aside in the same review: the "additional approval for unattributed changes" ruleset setting is documented for unattributed Copilot PRs specifically, not arbitrary unmapped git-author emails -- left unaddressed since neither of us reproduced a qualifying case, per the same discipline as the branch's own last fix. Stand: Echo Act IV Pro Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * test(proactive-loop): exercise is_collaborator()'s own subprocess boundary The diff-coverage gate failed on 03e6d22 (93%, lines 98/100-104 uncovered): every existing test monkey-patched `g.is_collaborator` wholesale, so the function's own body -- the real `gh api` call and its three-way 204/404/other branching -- never ran under test, only its stand-ins did. Added a `Gh`-class trio mirroring the existing `_gh()` subprocess tests: 204 -> True, 404-in-stderr -> False, and any other failure (rate limit, network, auth) -> None -- pinning that the undetermined case is never guessed as False, which is the exact property #4055's fix depends on to avoid dropping a real collaborator's approval. 100% line coverage on this file locally (was 93%). 35 tests (was 32), all pass. review-checks/ruff clean. Stand: Echo Act IV Pro Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> --------- Co-authored-by: Chi Wang <sonichi@users.noreply.github.com> Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
liususan091219
left a comment
liususan091219
left a comment
There was a problem hiding this comment.
COMMENT: not ready to approve. #3930 already did this work, so I recommend closing this PR, not merging it. Head 610cbef5c9dd6e69aa44352e10a1fcdda99e8ec5.
The change was correct when it was opened (2026-09-02). @TustinOC, @qingyun-wu and @sonichi all checked it against src/core_heartbeat.py, and it brought the doc to schema 3. Since then main has moved past it:
- #2923, the issue this PR closes, is already CLOSED (COMPLETED) as of 2026-09-06T00:31:08Z, by
a40b6e3bb5, which is #3930: "feat(core-heartbeat): … payload doc at schema 4". main:docs/claude-md-moved-detail.md:47-50now documentsheartbeat_pidandsession(both fields this PR adds), plusbackend,tmux_binary,tmux_version,tmux_server_version,tmux_verified,tmux_candidates, and"schema_version": 4.main:src/core_heartbeat.py:474-503(payload = {...}) writes"schema_version": 4(line 502), with the tmux fields spread in at line 501.
So this PR's line (… "session": "...", "locality": {...}, "schema_version": 3}) is now stale relative to the code. That is the same defect #2923 was about, with the version numbers moved up by one. Resolving the conflict in this PR's favour would put the doc back to 3 and drop the tmux fields.
Gate state at head (checked 2026-10-07)
mergeable: CONFLICTING,mergeStateStatus: DIRTY; 611 commits behindmain. The conflict is the same doc block that #3930 rewrote.- CLA:
commits/<head>/statushas 1 context,license/cla=pending, with description "Contributor License Agreement is not signed yet" (last updated 2026-09-02T21:13:31Z, just after the head merge commit610cbef5). So CLA is present but not passing; it is not missing. - CI: the other 17 checks pass.
- Reviews (COMMENTED ignored, latest per author): @TustinOC APPROVED and @qingyun-wu APPROVED, both at head. @john-the-dev's is DISMISSED.
- No unresolved threads. Hardcoded-path scan over the added line: 0 hits.
Triage-table row 1 (a duplicate of work that has already landed) applies before the approve row. The suggested next step is a final notice to @sunxiayi, then close as superseded by #3930, with thanks: the diagnosis was right, and main simply fixed it in a larger change. If the author would rather keep it open, the only useful version would rebase onto main, and the doc is already current there, so the diff would be empty.
— Sutando (susans-MacBook-Pro)
Priority
Level: low
Reason: Documentation-only correction for a confirmed good-first-issue; no release or runtime path is blocked.
Closes
Closes #2923
Summary
Synchronize the documented heartbeat payload with
src/core_heartbeat.py: add the existingheartbeat_pidandsessionfields and updateschema_versionfrom 2 to 3.Review
docs/claude-md-moved-detail.mdfirst; it is the only changed file. Readers of the agent-facing reference now see the same payload contract the runtime writes.Feature/documentation consistency: this PR corrects the canonical documentation itself. No
docs/catalog.jsonor navigation update is needed because no feature, capability, or documentation path changes.Live path: N/A — documentation only; no bridge, network, delivery, or startup code changes.
Stacked PR: N/A — this is based directly on
upstream/main.Checklist
gh pr list --repo sonichi/sutando --state all --limit 100 --search "2923"returned[])sunxiayi@umich.edu; CLA status will be reported by CLA Assistantupstream/mainBefore / after evidence
Command: a small AST/Markdown comparison of the keys written by
write_heartbeat()and the documented JSON example.Before (
upstream/main):After (
d38a405a):Test plan
Known gaps / rollback risk: none. Reverting the single documentation line restores the prior text; runtime behavior is unchanged.