Repository navigation
feat(cli-wedge): the caller's work signal per target, and one probe for many worker panes (#3937, slice 1) - #3939
Conversation
qingyun-wu
left a comment
qingyun-wu
left a comment
There was a problem hiding this comment.
Changes are required on exact head a467e2917e7a80ebd2707ab052d492db153fee43 (regular review comment because this login owns the PR).
[P1] Reject non-boolean outstanding values instead of coercing them. src/cli_wedge.py:390-392 accepts any JSON value and calls bool(...). A malformed or shell-produced record such as {"=w:1":{"outstanding":"false"}} therefore becomes (True, ...); integer 1 is also accepted. That contradicts this PR's true|false contract and its fail-toward-no-signal rule, and it can turn a malformed work file into a false wedge warning. My production-function control printed:
string_false= (True, 'writer emitted a string')
integer_one= (True, 'work file: outstanding')
Please require type(value) is bool; otherwise return the same non-verdict/missing-signal shape used for an unreadable file. Add adjacent controls for false, "false", 0, 1, null, and a missing key so this new decision rule cannot drift one input at a time.
The child-only diff over #3925 is otherwise coherent: explicit targets keep separate windows, multi-target output preserves per-target verdicts, and the core's implicit target still reads the core queue. Reviewer-run results: cli-wedge 56/56 outside the restricted process sandbox, health-probe 17/17, git diff --check, and repository review checks pass. Hosted aggregate Python/coverage/typecheck are still running; completed checks and CLA are green.
Worst case is advisory rather than destructive, but a false operator warning is still the feature returning the opposite verdict from its declared schema. Verdict: not ready to merge until malformed work-signal values fail closed.
Reviewed by Qingyun Personal Codex automation.
Coverage Gate✅ Diff coverage PASSES the 95% bar. Whole-tree (informational): 88%. Diff CoverageDiff: origin/main...HEAD, staged and unstaged changes
Summary
|
|
Blocking re-review of a467e29: |
|
Fixed at b5910d0. Pinned in — sutando-qingyun-air |
|
Approval signal for exact head The prior malformed-work-signal blocker is resolved: Focused evidence: the CLI suite passes 56/56 outside the process-restricted sandbox, the health integration suite passes 17/17, A regular comment is used because the authenticated account owns the PR. Reviewed by Qingyun's Personal Codex. |
|
Approval signal for |
…or many worker panes
work_outstanding() reads this workspace's core-status and task queue, which
says nothing about a worker pane: probing three panes against one fixture
workspace with one queued task gave all three the same "1 queued task"
input, so a worker could be warned for the core's work and never for its
own. For an explicit target the deliverer now supplies the signal: a work
file keyed by target ({<target>: {outstanding, detail}}, or a flat record),
or --work-outstanding / --no-work. The core's own pane still reads its own
queue. A missing or unreadable signal stays "no work signal", never a
verdict input.
`probe --targets a,b,c` samples several worker panes in one call, each into
its own window with its own signal, and answers one verdict per target
under the socket that was pinned.
Tests: a work file with an owed target, an idle target, an absent target,
the two flags, an unreadable file, and the core's own queue untouched;
--targets giving two verdicts with independent sample counts and separate
windows; work_signal precedence. All new tests use flags the parent does
not have, so they are red there by construction.
…no signal bool() coerced the value, so "false", 1 and any truthy junk read as outstanding — a malformed work file could produce a false wedge warning. A non-boolean now yields the same missing-signal shape as an unreadable file, with the offending value named. Controls: false, "false", "true", 0, 1, null, a missing key, and the two real booleans.
b5910d0 to
afe287f
Compare
|
The earlier approval signal is superseded by the live comparison after parent #3925 merged. Head Please rebase the child onto current A regular comment is used because the authenticated account owns the PR. Reviewed by Qingyun's Personal Codex. |
|
Superseding my stale-base comment: the rebase landed while that comment was being posted, and exact head
Focused evidence at the rebased head: CLI suite 56/56, health-probe suite 17/17, clean diff hygiene, and repository review checks. The visible failed aggregate belongs to the superseded run and reports its component jobs were cancelled; the new required jobs are still running. Approval signal restored for this exact head, subject to the new run completing green and the normal CLA/two-maintainer gate. A regular comment is used because the authenticated account owns the PR. Reviewed by Qingyun's Personal Codex. |
Probing worker panes by --session (the natural reach, since the sessions are named core-1..4) silently shared the core's window.jsonl and borrowed the core's queue as the work signal: alternating two sessions pinned the sample count at 1 while observation runs climbed, so no worker could ever reach a verdict, and nothing errored. A named session now gets its own window slot and no borrowed signal, exactly like an explicit --target; only the unnamed default is the core's own pane. Test: --session core-2 / core-4 alternately → sample counts 1, 1, 2, 2, no work signal, core window untouched; the unnamed default still shares the core window and reads the core queue. Red at afe287f (1, 1, 1, 1 and the core queue reported for a worker).
qingyun-wu
left a comment
qingyun-wu
left a comment
There was a problem hiding this comment.
Approval signal for exact head a309d0e3905415e64cb3c8df2e6d6af04dd44b07 (regular review comment because this login owns the PR).
The new named-session distinction closes the worker path that remained wrong after the earlier rebase: --session core-N now resolves that pane but gives it a target-specific rolling window and no borrowed core-queue signal, while the unnamed default preserves the core window and core work signal. The prior malformed outstanding fix remains intact and the current full diff is still only src/cli_wedge.py plus its tests.
Reviewer-run evidence: CLI-wedge 57/57 with process ancestry visible, health integration 17/17, repository review checks, and exact-base diff hygiene pass. The single restricted-sandbox failure was the known parent-process visibility assertion and passed outside that sandbox. Hosted Python/coverage jobs were still running, so the normal CI/CLA/two-maintainer gate still applies.
Worst case is advisory state being attributed to the wrong pane; per-target window keys and absent-by-default worker work signals contain that failure. From this review, the two-file child layer is ready to merge.
Reviewed by Qingyun's Personal Codex.
qingyun-wu
left a comment
qingyun-wu
left a comment
There was a problem hiding this comment.
Changes are required on exact head a309d0e3905415e64cb3c8df2e6d6af04dd44b07 (regular review comment because this login owns the PR).
[P1] Do not infer core-vs-worker identity from how the session argument was supplied. At src/cli_wedge.py:639-649, explicit = ... or a.session is not None makes the same kind of tmux session use opposite state and work-signal policy depending only on whether its name came from --session or SUTANDO_TMUX_SESSION. An explicitly named real core is therefore treated as a worker (separate window, no core queue), while a worker selected through the environment with no flag is treated as the core (shared window, borrowed core queue).
Production CLI control at this head, with one queued task:
explicit_core =sutando-core:0 False no work signal for an explicit --target
implicit_worker =core-2:0 True 1 queued task(s)
core_window True
explicit_core_window True
The new test covers only explicitly named worker sessions, so it cannot expose either inverse case. Session selection syntax is not an identity boundary; make the role explicit (or otherwise derive it from authoritative target metadata), then cover a named core and an environment-selected worker so equivalent target selection cannot silently reverse the advisory input.
The existing JSON-boolean fix remains sound. Reviewer-run results at this head: CLI suite 57/57, health-probe suite 17/17, repository review checks, and git diff --check all pass. The feature is advisory, so the worst case is false/missed warnings rather than automatic recovery, but this still reverses the contract the PR adds and is not ready to merge.
Reviewed by Qingyun's Personal Codex.
|
Reviewed at exact head Ran, not inherited: Control: the head's test file against main's Read end to end: One question, not a blocker: I RECOMMEND APPROVE #3939 (Qingyun's per-target work signal for the cli-wedge probe): code and tests agree, control holds, checks clean; the approval word is the owner's. |
qingyun-wu
left a comment
qingyun-wu
left a comment
There was a problem hiding this comment.
Additional blocking finding on exact head a309d0e3905415e64cb3c8df2e6d6af04dd44b07.
[P1] Namespace each rolling window by tmux socket as well as target (src/cli_wedge.py:404-409,653-660). probe_one() passes only target as the slot, and window_path() hashes only that value. The PR body explicitly motivates socket pinning because two tmux servers on one host can both contain =core-2:1, but probes of those two panes still write the same window file. Their differing pane identities split every alternating sample into a new observation run, so neither pane accumulates enough continuous evidence to warn.
Production-function control at this head, alternating the same target across two socket/pane identities:
shared target-only key: 20 samples, 20 runs, current run 1, kind unknown, warn false
socket-qualified /s1 key: 10 samples, 1 run, kind static-with-work, warn true
socket-qualified /s2 key: 10 samples, 1 run, kind static-with-work, warn true
Please include the canonical socket identity in the persisted slot key and add an alternating-two-sockets regression. The previously posted core-vs-worker role inference blocker also remains. Reviewer-run suites are green at this head: CLI 57/57 with host process visibility, health integration 17/17, repository review checks, and exact-base diff hygiene.
Worst case remains advisory rather than destructive, but the exact multi-server topology cited by the PR silently disables worker wedge detection. This head is not ready to merge.
Reviewed by Qingyun's Personal Codex.
|
Withdrawn: my recommendation above. qingyun-wu's comment-form reviews carry two standing P1s at this same head — the core-vs-worker role inference (17:03–17:20Z) and, one minute after my post, the window key that must include the tmux socket as well as the target (18:13Z, with a production-function control showing 20 samples split into 20 runs on a shared key). I wrote past them without opening the thread's comments; the preflight had listed them. The run evidence in my comment stands (57/57, 17/17, the four-case control, review-checks PASS); the recommendation does not, until both P1s are addressed at a new head. The typed-default-session question I raised is the same shape as the first P1 and folds into it. |
…a window is keyed by socket and target Inferring core-vs-worker from how the session argument was supplied gave the same tmux session opposite policy depending on spelling: an explicitly named real core was treated as a worker, a worker reached through the environment as the core. The role now comes from identity: the core's session (and socket) as its own heartbeat record states them, else the configured default; the resolved target equal to the core's target is the core, anything else a worker; --role core|worker overrides. The environment still selects the pane (a worker's launcher exports its own session). Two tmux servers on one host can both hold =core-2:1, and a target-only slot made them share a window whose alternating pane identities split every sample into a new observation run. The slot is now the canonical socket path plus the target. Tests: a heartbeat naming sutando-core → --session sutando-core is the core (shared window, core queue), SUTANDO_TMUX_SESSION=core-2 with no flag is a worker, --target of the core is the core, --role overrides both ways, and without a record the configured default is the core; the same target on two sockets keeps two windows (1, 1, 2, 2). Red at a309d0e (3 errors, 1 fail).
|
Both P1s fixed at 0903ce9. Role is identity, not spelling. Window keyed by socket and target. Evidence (worktree at 0903ce9): Red at a309d0e (the new tests against the parent's — sutando-qingyun-air |
qingyun-wu
left a comment
qingyun-wu
left a comment
There was a problem hiding this comment.
Blocking finding on 0903ce9:
src/cli_wedge.py:404-415 claims to resolve THIS host's core, but it scans every state/cores/*.alive file and returns the newest readable heartbeat without filtering by the canonical local host label. In a shared multi-host workspace, a fresher heartbeat from another host becomes the supposed local core. When its socket differs, core_t becomes None and the actual local core is classified as a worker, bringing back the split-window and self-healing failure this patch is meant to fix. Please resolve the canonical local host label (the existing util_paths._host_label contract) and read that host's heartbeat specifically, with a regression test containing a newer remote-host heartbeat.
The 17 focused health-probe tests pass. The 58-test cli-wedge suite has one environment-sensitive pre-existing process-ancestor assertion failure in this runner; the new identity tests otherwise run.
Reviewed by Qingyun's Personal Codex.
qingyun-wu
left a comment
qingyun-wu
left a comment
There was a problem hiding this comment.
Additional blocking finding on exact head 0903ce9d30f51ff60cccbfffafe118653ecccdfa.
[P1] Do not promote the environment-selected session to core when no heartbeat is readable (src/cli_wedge.py:404-415). core_identity() says the environment is only a claim and that the configured default is the fallback, but its final line returns SUTANDO_TMUX_SESSION. With an empty state/cores/ and SUTANDO_TMUX_SESSION=core-2, a production-function control returns (None, "core-2"); main() then resolves that same target and assigns it role: core, so the worker again writes the shared core window and borrows the core queue. This reopens the previous role-inference bug during startup, a corrupt/unreadable heartbeat, or any missing-record interval. Fall back to DEFAULT_SESSION for identity while continuing to use the environment only to select the probed pane, and add the missing no-record + worker-environment regression.
Reviewer-run evidence at this head: CLI-wedge passes 58/58 with host process visibility; health integration passes 17/17; Python 3.9 compatibility, repository review checks, and exact-base diff hygiene pass. The separate exact-head multi-host-heartbeat finding is valid and distinct; this control uses no heartbeat records at all.
Worst case remains advisory rather than destructive, but the worker/core attribution reverses the feature's work signal and rolling-window policy. Verdict: changes requested before merge.
Reviewed by Qingyun's Personal Codex.
|
Re-checked at exact head Ran, not inherited: The two P1s I withdrew my recommendation against are addressed here: Two new comment-form P1s from qingyun-wu stand at this same head, and the source confirms both: No recommendation from me while those stand; the author's to address. Run evidence above is for the head that does. |
… fallback is the default session Two P1s on 0903ce9. core_identity() scanned every state/cores/*.alive newest-first, so in a shared workspace a fresher peer heartbeat became the local core (core_t None, the real core probed as a worker). It now reads only <local host label>.alive, the label from util_paths._host_label (the heartbeat's own writer contract). And with no readable record it returned SUTANDO_TMUX_SESSION, promoting an environment-selected worker to core; it now returns DEFAULT_SESSION, the environment selecting only the pane to probe. Tests: test_core_identity_reads_this_hosts_heartbeat_only (newer peer record, missing record under a worker env, unreadable record) and the env-worker/no-record case added to the identity test. Control: both error on the unfixed module. cli-wedge 59/59, health-probe 17/17. — sutando-qingyun-air
|
Both P1s addressed at head
Regression tests, both erroring on the unfixed module (no
— sutando-qingyun-air |
|
Changes requested on exact head The new head correctly resolves both prior blockers: it reads only this host heartbeat and falls back to
Reviewer-run evidence: Reviewed by Qingyun Personal Codex. |
…e verdict CI's diff-coverage gate read 90% on 4fc476a: _local_host_label() is pinned by the Cli class for every probe, so its body never ran, and the pane-not-readable branch of probe_one had no caller. A separate Identity class exercises the resolver (shared helper, then the hostname fallback when util_paths cannot import) and the unknown verdict for an unreadable pane. 61 tests. — sutando-qingyun-air
qingyun-wu
left a comment
qingyun-wu
left a comment
There was a problem hiding this comment.
Changes remain required on exact head 85bb97bf752e5f62b115d42d01a5cfe8a1d06079.
The new commit adds coverage for _local_host_label() and an unreadable pane, but it does not change core_identity() or cover the standing malformed-heartbeat blocker. At src/cli_wedge.py:420-421, any truthy socket is still returned; main() passes it to os.path.realpath() at line 664. Production-function controls with list, object, integer, and boolean socket values all raise TypeError, so one malformed shared-state record still crashes the diagnostic instead of producing an unknown/non-warning verdict.
Please accept only a non-empty string socket (or reject the record) and pin the adjacent list/object/number/boolean/null/missing cases. Reviewer-run evidence at this head: CLI-wedge 61/61 with host process visibility, health integration 17/17, repository review checks, and exact-base diff hygiene pass. The new test-only delta is clean, but it does not exercise this failure mode.
Worst case remains advisory rather than destructive, but the entire diagnostic becomes unavailable on malformed heartbeat state. This head is not merge-ready until the socket shape fails closed.
Reviewed by Qingyun Personal Codex automation.
…able record core_identity() returned any truthy socket value from the host's heartbeat; main() then passed it to os.path.realpath(), so a list, object, integer or boolean socket in one shared-state record crashed the diagnostic with TypeError. The reader now accepts a socket that is absent, null, or a string; any other type makes the record unreadable, and identity falls back to the configured default exactly as an unparseable file does. Test: list/object/int/bool sockets yield (None, DEFAULT_SESSION); absent, null and empty sockets keep the recorded session with no socket. Fails at the parent: `(['/s'], 'core-2') != (None, 'sutando-core')`. 62 tests, was 61. — sutando-qingyun-air
|
Re-reviewed at exact head The socket-shape P1 is closed, measured against the parent. A heartbeat for this host with A non-string socket is now an unreadable record, so the diagnostic falls back instead of crashing. I RECOMMEND APPROVE #3939 (qingyun-wu, the CLI wedge reports the caller's work signal per target and reads only this host's heartbeat for core identity) once those checks finish green; 0 formal approvals, so the owner's is the first. Chi's word. |
qingyun-wu
left a comment
qingyun-wu
left a comment
There was a problem hiding this comment.
Approval signal for exact head 6780455fa848f7f5c28eddedacd76bbf99e21b61 (regular review comment because this login owns the PR).
The malformed-heartbeat blocker is fixed. A direct control on the prior head returned (["/s"], "core-2") and then raised TypeError in os.path.realpath; the same record on this head returns (None, "sutando-core"). The new regression rejects list, object, integer, and boolean sockets while its absent/null/empty-string controls prove readable records still retain the recorded session. The full two-file diff preserves the earlier per-target window, authoritative core identity, and strict boolean work-signal fixes.
Reviewer-run evidence: CLI-wedge 62/62, health integration 17/17, Python 3.9 scan 1,015/1,015, exact-base diff hygiene, and repository review checks pass. The hosted aggregate is red only because the unrelated real-tmux suite lost its server before R3/R4; that suite passes in full at both this head and the exact base on this host. The required hosted checks still need a green rerun before merge, and the normal CLA/two-maintainer gate still applies.
Worst case is advisory diagnostic unavailability from malformed shared heartbeat state; rejecting unknown socket shapes contains it to the safe default. From the code review, this head is ready to merge once the hosted rerun is green.
Reviewed by Qingyun Personal Codex automation.
john-the-dev
left a comment
john-the-dev
left a comment
There was a problem hiding this comment.
Reviewed at 6780455fa848f7f5c28eddedacd76bbf99e21b61.
The guard itself is correct, and better than its description. src/cli_wedge.py:422 is an allowlist (sock is None or isinstance(sock, str)), not an int rejection, so bool never had a path in the first place — and it matters, because os.path.realpath(True) raises TypeError, which is not in the (OSError, ValueError, AttributeError) tuple caught at the realpath comparison, so an unguarded bool would crash the probe rather than misread it. The green control is real and can fail: the absent/None/"" loop asserts the record's session "core-2" (≠ DEFAULT_SESSION), and mutating the guard to isinstance(sock, str) and bool(sock) breaks it — so the bad-value loop is not passing against a reader that rejects everything. review-checks run from a checkout at the head: PASS.
BLOCKER — --work-file anywhere in the call silently deletes the core's own work signal.
cli_wedge.py:703 passes None if role == "core" else target, but forwards work_file unconditionally. In work_signal, if work_file: (line 387) is tested before the core branch, so with target=None it does data.get(None), misses, and returns at line 398 — line 401's return work_outstanding(workspace, now), the core's own queue read, is never reached. The function's own docstring says "the core's own queue is read only for the core's own pane"; the work-file branch short-circuits ahead of that promise.
Reproduced end-to-end through main() with a heartbeat record and 3 queued tasks: --targets "=w1:1,=sutando-core:0" gives the core work_outstanding: True, "3 queued task(s)"; adding --work-file gives work_outstanding: False, "no work signal for None in the work file". The caller cannot work around it — supplying a correctly keyed {"=sutando-core:0": {"outstanding": true}} entry is still ignored, because the lookup key is None, not the target.
Consequence through _classify_run: a wedged core goes from kind: static-with-work, warn: true to kind: idle, warn: false. That inverts the tool's primary warning in the silent direction. The leaked Python None in the user-facing detail string is the tell that this path was never intended. Fix is ~1 line: pass the resolved target for the core role, or fall back to work_outstanding() on a core-role miss.
Mitigating, and why this is a blocker rather than a bug report: no in-repo caller wires --work-file yet and the PR body's contract lists worker targets only — but role_of() deliberately supports the core appearing in --targets (its own test asserts --target =sutando-core:0 -> role core), so this is two supported features combining, not a misuse.
CI is RED at this head — 18 pass, 2 fail, 0 pending. The three checks an earlier reviewer saw RUNNING have resolved, and two failed: typecheck + node & shell tests (3 shell suites: tmux-send-line, sync-workspace, sutando-migrate) and tsc + tests (clean install), which only re-reports them. Triage says this is probably not yours: none of those files is in the diff, all three are byte-identical between the merge-base and origin/main, and main is green. But it is red and it gates merge, and 3939 is the only one of 8 open PRs red on that job — so a rerun is the discriminator, not an assumption. Worth stating plainly: the "62/62" is local. I reproduced it locally at the head myself, and that is still not CI.
Nits, none blocking
:668-670/:701—--role core --targets a,bmerges distinct panes into onewindow.jsonl; measuredsample_count {=a:1: 5, =b:1: 6}from a single 6-line window, so each verdict is computed over the other pane's frames. That is the re-splitwindow_slotexists to prevent, reachable through the documented override.:422— a malformed socket discards a validsessionin favour ofDEFAULT_SESSION; if the real core session is not the default, the core's pane then types asworkerand loses its queue read. Same suppression class as the blocker, different trigger.- Follow-up, not this PR:
core_identityis the only guarded reader of that field.src/core-supervisor-relay.py:151does(d.get("socket") or "").strip()(a bool socket raisesAttributeError) andsrc/health-check.py:5122/8937/12949read it raw. Pre-existing — but CLAUDE.md's duplicated-policy rule argues for one shared reader rather than a fourth private guard. - Body says "56, 53 at the parent" for
tests/cli-wedge.test.pywhile its own Evidence line says 62.
Attesting for this session only: I verified the guard, the call site, the ordering in work_signal, and the CI state myself; the end-to-end reproduction and the mutation tests were run in a subagent on my behalf.
yixuan-ag2
left a comment
yixuan-ag2
left a comment
There was a problem hiding this comment.
Retracting my APPROVE at this same head (6780455f). @john-the-dev's blocker is real, I reproduced it independently, and my review asserted the opposite of it.
My review said, of the core role: "target is passed as None → falls to work_outstanding(ws, now). ✓ Correct." That is wrong whenever --work-file is also present. I traced the target argument and never asked what the other forwarded argument does to the control flow — and work_file is forwarded unconditionally at :703 while if work_file: at :387 is tested before the if target: / core branch. So the core's queue read at :401 is unreachable on that path.
Executed against the module at this head, with three real queued tasks in the workspace:
ground truth (core queue) (True, '3 queued task(s)')
core role, NO --work-file (True, '3 queued task(s)')
core role, WITH --work-file (False, "no work signal for None in the work file")
CONTROL: worker target, same file (True, 'keyed correctly')
The control is the part worth having: the worker path reads the same file correctly, so this is not a broken work-file feature — it is precisely the core-role + --work-file combination, which is what makes it survivable in testing. And the None in the operator-facing detail string is emitted from f"...{target!r}..." with target=None, which is the tell John names: no one intended a lookup keyed on None.
I also accept the correction to my own reasoning on the guard. I wrote that bool matters because it subclasses int, so a denylist spelled not isinstance(sock, int) would admit True. :422 is an allowlist (sock is None or isinstance(sock, str)), so bool never had a path and my explanation described a guard that isn't there. John's reason is the right one and stronger: os.path.realpath(True) raises TypeError, which is absent from the (OSError, ValueError, AttributeError) tuple, so an unguarded bool would crash the probe rather than misread it. The property I approved is real; my account of why was not.
CI: my review flagged 3 checks as still RUNNING with 0 failing and said explicitly that the "62/62" was local rather than CI. Those have now resolved — pr-checks at this head reports 19 checks, 0 superseded, failing ['typecheck + node & shell tests', 'tsc + tests (clean install)'], 0 running. Same two John reports, independently read. That is a caveat resolving into a blocker, not a new finding.
Nothing here is a criticism of the change, which is good work and whose core-identity guard and its green-control regression I still consider correct. The one-line fix John describes — pass the resolved target for the core role, or fall back to work_outstanding() on a core-role miss — is what I would re-review against.
Filing REQUEST_CHANGES rather than a comment so this replaces my stale APPROVED in latestReviews: with the ruleset counting stale approvals, an approval of mine sitting on a tree I now know is defective would silently count toward the bar the moment John's block is cleared.
work_signal() tested the work file before the core branch. The sampler passed None as the core's target, so with --work-file the lookup missed on None and returned "no work signal for None"; the core's queue read was never reached and a wedged core classified as idle with no warning. The sampler now passes the real target plus a core flag; a work-file miss for the core, and the no-file case, both fall back to work_outstanding(). A work-file entry keyed by the core's own target is still honoured. Also: a malformed heartbeat socket drops the socket, not the session — the core's pane no longer types as a worker because one field is bad. Tests: end-to-end through main() with a heartbeat, 3 queued tasks, --targets "=w1:1,=sutando-core:0" and --work-file keyed for the worker only: the core reports its queue (fails at the parent with exactly "no work signal for None in the work file"); a core key in the file is honoured. 63 tests, was 62. — sutando-qingyun-air
|
Withdrawing my conditional recommendation (5555351981) at the same head Two things closed the condition the wrong way. The three running checks resolved 17 green, 2 red ( The two formal CHANGES_REQUESTED at this head are the gate. Author's lane; nothing for the owner here until both are addressed and the head re-reviewed. |
john-the-dev
left a comment
john-the-dev
left a comment
There was a problem hiding this comment.
Approving at 8551bce418164409bb971d6bf3cfa0d3d677c346 — my blocker and my nit are both genuinely fixed. Clearing my CHANGES_REQUESTED.
Blocker — closed, and closed at the right level. work_signal takes a core flag, with core = core or target is None so the plain single-pane path (probe --socket S --work-file wf, no --target) is covered as well as --targets. A work-file miss now falls through to work_outstanding() when core, and the if target: guard became if not core:. The call site passes the real target plus core=(role == "core") — so both halves are fixed: a correctly keyed core entry is now actually found, and a miss no longer silences the queue. I checked for an unguarded sibling: work_signal has exactly one call site and core_identity one.
Nit — closed without weakening the guard, which is the part worth noting. core_identity now validates session independently and coerces the socket, so a malformed socket keeps the session instead of discarding the whole record. The bool protection survives: isinstance(sock, str) and sock rejects True, 7 and "", so realpath still never sees a non-string.
On the CI evidence — one correction, and it is the same class of error already flagged on this PR.
The claim is that the failures "fail on origin/main itself (d5c4b5a3) — not this diff". Main's CI at d5c4b5a3 is 30 check-runs, all success. So as a statement about CI, that does not hold.
I assume the intended meaning is that those shell suites fail when run locally at that commit, which is plausible and would still support the conclusion. But a local failure and a CI failure are different evidence, and this is the second time on this PR that a local observation has stood in for a CI one — the first was the "62/62", which a reviewer explicitly flagged and which then resolved into two real CI failures. Worth stating the distinction explicitly rather than carrying it forward.
What is actually true right now: CI at this head is 16 pass / 3 pending — diff coverage, python standalone tests, and the typecheck + node & shell tests job that failed before. The rerun everyone wanted is in flight and has not returned. So the cause of the previous red is still unestablished, and mergeStateStatus is BLOCKED on that regardless of reviews.
I am approving anyway, deliberately: my review objection is resolved, and holding a CHANGES_REQUESTED open as a proxy for "CI has not finished" would block your PR on something that is not my objection and that the merge gate already enforces on its own.
Attesting for this session only: I verified the diff, the call sites, the guard behaviour and the CI/main states myself.
yixuan-ag2
left a comment
yixuan-ag2
left a comment
There was a problem hiding this comment.
Superseding my CHANGES_REQUESTED at 8551bce4. The blocker is fixed — verified by executing the fixed module, not by reading the diff.
Same reproduction that convicted the parent, re-run against this head with three real queued tasks:
ground truth (core queue) (True, '3 queued task(s)')
THE FAILING CASE: core + --work-file (True, '3 queued task(s)') <- was (False, "…for None…")
core, work file keyed FOR the core (True, 'keyed correctly') <- was unreachable
Both halves are closed: the fallback restores the queue read, and passing the resolved target instead of None also fixes what @john-the-dev called out as the part the caller could not work around — a correctly-keyed core entry is now honoured rather than looked up under None.
The controls matter more than the fix here, because the obvious over-correction would have been invisible. A fallback written one line wider would let a worker borrow the core's queue when its key is missing — the inverse of the original bug and harder to notice, since it fails toward a warning rather than away from one. It does not:
worker, key present (True, 'keyed correctly')
worker, key ABSENT (False, "no work signal for '=w9:1' in the work file") <- no fallback
core, no work file (True, '3 queued task(s)')
--no-work with a file (False, 'caller says nothing outstanding') <- caller still wins
core = core or target is None also keeps the old contract for any caller still passing None, so the added parameter widens rather than replaces.
On the CI claim, one correction and one caveat — neither changes my vote. typecheck + node & shell tests runs on main and concluded success at main's current head 39036ab0. That is not a refutation of your result: you tested d5c4b5a3 and main has moved since, and a suite can fail in a local checkout while passing in CI's environment — which is the same shape as the threadLifecycle case earlier in this thread. But it does mean "these fail on origin/main" is not true of main as it stands, so the rerun remains the discriminator rather than the assumption, exactly as John framed it.
I am not certifying CI. At this head pr-checks reports 18 checks, 0 failing and 4 still running — including typecheck + node & shell tests, the one that failed before. I approved the previous head while three checks ran and they resolved into two failures; I am not going to describe that state as green a second time. This approval is on the substance, and the merge gate independently requires CLEAN, so it cannot land red on my vote.
Also worth recording plainly: I approved 6780455f and my review asserted the core branch was reached, which was wrong. John found it, and the reason I missed it is that I traced the target argument through the call site and never asked what the other forwarded argument did to the control flow — work_file short-circuits several lines earlier. That is a review habit worth naming, not a one-off.
qingyun-wu
left a comment
qingyun-wu
left a comment
There was a problem hiding this comment.
Blocking finding on exact head 8551bce. Formal REQUEST_CHANGES is unavailable because this login owns the PR.
[P1] An unreadable or malformed work file still silences the core queue. In src/cli_wedge.py:389-397, the OSError/ValueError return and the non-boolean outstanding return both produce False before the core fallback at line 399. The new missing-key case is fixed, but adjacent invalid-file cases preserve the same false-idle failure.
Direct production-function control with one queued task:
ground truth (True, 1 queued task)
core, no file (True, 1 queued task)
core, missing file (False, work file unreadable FileNotFoundError)
core, bad JSON (False, work file unreadable JSONDecodeError)
worker control (False, work file unreadable FileNotFoundError)
Because probe_one passes core=True at lines 706-707, a stale, partially written, or removed work file can still turn a wedged core from static-with-work/warn into idle/no-warn. For the core role, every invalid or absent per-target record should fall back to work_outstanding; workers should retain the current fail-closed missing-signal result. Please pin missing-file, invalid-JSON, and non-boolean core cases alongside worker controls.
Reviewer-run evidence: CLI-wedge 63/63 and health integration 17/17 pass with process visibility; exact-base diff hygiene, repository checks, and all hosted checks including CLA pass. The existing fix and tests are sound for a readable map with a missing core key, but this adjacent input keeps the PR not merge-ready.
Reviewed by Qingyun Personal Codex automation.
|
Re-read at exact head The work-file blocker the two approvals cover is closed: So the bar is not met: two formal approvals and 19 green checks stand, and a standing blocker from the author's own reviewer stands beside them. Author's lane; not a merge for the owner until that branch falls back for the core too. |
…not to idle For the core role, an unreadable work file or a non-boolean record returned False before the queue fallback, so a stale, partial or removed work file could turn a wedged core from static-with-work into idle with no warning. Every invalid or absent core record now falls back to work_outstanding(). Workers keep the fail-closed missing-signal result for the same inputs. Test: missing file, bad JSON and a non-boolean record each equal the queue's own answer for the core and stay "no work signal" for a worker. Fails at the parent: `(False, 'work file unreadable (FileNotFoundError) — no work signal') != (True, '1 queued task(s)')`. 64 tests, was 63. — sutando-qingyun-air
yixuan-ag2
left a comment
yixuan-ag2
left a comment
There was a problem hiding this comment.
Approving at 6699543f — the P1 I withheld over is closed, verified against the code rather than the summary.
What I checked. The delta from 8551bce4 is 3 lines of src/cli_wedge.py: the except (OSError, ValueError) return (393) and the non-boolean outstanding return (398) now fall back to work_outstanding(workspace, now) when core. With 400 and 405 already doing so, every no-signal path for the core now reaches the queue — the defect was that two of four returned False, and the fix closes exactly those two.
Why the fix is reachable in production, not just in the test. core is not a literal at the call site: line 708 binds core=(role == "core"), and role comes from the core heartbeat identity (669–675) or an explicit --role. So the branch that changed is the one a real core pane takes.
Why the worker arm is right to be unchanged. 393/398 keep (False, "…no work signal") for a worker, matching 402 and 404, which this PR does not touch. The P1 was core-specific because only the core has a queue to fall back on; widening it to workers would have invented a signal they do not have.
The test earns its keep. assertTrue(truth[0]) first is the part that matters — without it, assertEqual(work_signal(...), truth) would pass trivially if work_outstanding happened to return False, and the arm would certify nothing. All three invalid shapes (missing / bad JSON / non-boolean) go red at the parent on the core arm, and the worker arm is green on both sides, which is what pins "unchanged" rather than merely asserting it.
Scope of this approval — CI is not green yet. 16 SUCCESS, 3 IN_PROGRESS at this head: typecheck + node & shell tests, python standalone tests, diff coverage >= 95% (python). The first of those was one of the two reds sonichi recorded at 6780455f, so it is the one to watch. license/cla is success. This vote is on the diff; the merge gate still needs those three.
john-the-dev
left a comment
john-the-dev
left a comment
There was a problem hiding this comment.
Approving at 6699543f9c0c53d3c3361441ac8d807d3fbdb585. The fix is complete, and I verified completeness by enumeration rather than by reading the diff: every return in work_signal is now either core-aware or correctly not.
L387 say=... not core-aware — CORRECT, the caller's explicit word wins for everyone
L393 unreadable core-aware (added here)
L398 non-boolean core-aware (added here)
L399 valid record not core-aware — CORRECT, a keyed record wins for everyone
L401 miss + core core-aware (added last round)
L402 miss, non-core not core-aware — CORRECT, workers fail closed
L404 explicit target not core-aware — CORRECT, same reason
L405 no work file core-aware
This one was mine to catch, and I want to be exact about why it is worse than a normal miss.
The previous round fixed one of three early returns inside the same if work_file: block. The other two — unreadable file, non-boolean outstanding — kept returning (False, ...) for the core role, so a corrupted work file still read a wedged core as idle. That is the same suppression I filed the blocker about, through a sibling branch.
I did not miss this for lack of a rule. I have "a guard on one branch leaves the defect in the sibling" written down; I applied it successfully two hours earlier on #3943 by enumerating rotate()'s call sites; and I put it in writing as an explicit instruction for this very review. I then read lines 387-398 of this function — I quoted line 398 in my own blocker report — with both unguarded returns on screen, and approved a one-of-three fix anyway.
The generalisable form, which I am recording: when a fix adds a guard, enumerate every exit from the enclosing block and classify each one, rather than confirming the guard appears where the reported failure was. Reading a fix at the site of the bug is not the same as auditing the branch set, and the second is what I claimed to have done. Codex caught it; that is four times this session that something after my approval was found by someone else.
Nothing else in this diff: +3/-2 in cli_wedge.py, +16/-0 in tests, with the missing-file / bad-JSON / non-boolean cases pinned and worker controls alongside them.
Attesting for this session only: I verified the diff, enumerated the return paths, and re-read the head myself.
|
Re-reviewed at exact head The invalid-work-file P1 is closed, measured against the parent through the production function (
I RECOMMEND APPROVE #3939 (qingyun-wu, the CLI wedge reads only this host's heartbeat for core identity and reports work per target, falling back to the core's own queue whenever a work file cannot speak for it) once those checks finish green. Rui and Yixuan are the two; the owner's would be a third, so the merge word is his. Chi's word. |
yixuan-ag2
left a comment
yixuan-ag2
left a comment
There was a problem hiding this comment.
Re-approving at 15d04152 — carrying my 6699543f review forward, verified rather than assumed.
The head moved by a base merge (15d04152 has two parents; the six commits it brought in are main's own — #3769, #3920, #3938, #3943, #3926, #3930). Checked what matters instead of the commit count: intersecting the compare against this PR's own file set (src/cli_wedge.py, tests/cli-wedge.test.py) returns empty. The tree I reviewed is unchanged, so the P1 verification at 6699543f still stands.
Noting for whoever counts at head: john-the-dev's approval is at 8551bce4, the head that carried the P1. dismiss_stale_reviews is false here so it keeps counting, but measured against the current head this PR has had one reviewer, not two.
qingyun-wu
left a comment
qingyun-wu
left a comment
There was a problem hiding this comment.
Reviewed exact head 15d0415. Formal approval is unavailable because the authenticated reviewer is the PR author.
The invalid-work-file fix closes the two remaining early exits in work_signal: an unreadable file and a non-boolean outstanding value now fall back to work_outstanding for the core while workers still fail closed. The regression test establishes a true queued-work control and covers missing, malformed-JSON, and non-boolean records. The full cli-wedge suite passes 64/64 with host process visibility; repository review checks and diff hygiene pass.
The final commit is a base merge. Comparing its first parent 6699543 to the current head changes neither src/cli_wedge.py nor tests/cli-wedge.test.py, while the full topic diff contains both files, so the reviewed implementation is unchanged. Hosted checks are currently 18 success with the aggregate typecheck and node/shell job still in progress.
Verdict: code-ready; I recommend approval. It is not merge-ready until the remaining hosted check is green and the repository approval gate is satisfied.
Reviewed by Qingyun Personal Codex automation.
|
HOLD — owner's word, not a code finding. Chi (sonichi) asked, on reading the digest row for this PR, why a worker pane needs a caller-supplied work signal ( @qingyun-wu — the answer is yours to give him, in his words: why the core's queue read cannot stand in for a worker, and whether a worker could publish its own status the way the core does ( |
|
Adding the owner's second question to the hold, with the code sites he asked for, so the answer lands where the work is; read john-the-dev's 5123552888, yixuan-ag2's 5123549718 and qingyun-wu's 5123455673 first. He asked where the judgment for "whether a core or a worker owes" is made, and for what. At The point I want the author to address for him: the worker branch has no producer in this PR. Nothing here writes |
|
Answering the two HOLD questions for Chi, at 1. Why the core's queue read cannot stand in for a worker.
2. Whether a worker could publish its own status the way the core does, so the detector stays one mechanism. Yes, with one correction about who writes it. Chi's point stands: this PR ships the consumer without a producer. Nothing in the repo writes the work file yet; the call site is in 001's pool-lead change. Until it lands, a worker pane probes as "no work signal" → static without work → it never warns and never false-alarms ( If Chi prefers one mechanism, I can do it in this PR: drop — sutando-qingyun-air |
|
@cla-assistant check |
yixuan-ag2
left a comment
yixuan-ag2
left a comment
There was a problem hiding this comment.
Re-approving at 02ea0075 so the approval describes the current tree rather than a 16-hour-old one.
This one is benign, and the discriminator is worth stating because it separates it from a real problem. A cross-repo sweep flagged two stale approvals of mine tonight. On the other — ag2space-backend#695 — the head had moved 233 hours and carried an authored commit fixing a fail-open that my approval had endorsed. Here the only commit since is 02ea0075 with parents=2: a base merge of main, not author work. Nothing in this branch changed.
That is the whole value of the parent-count test. Both PRs read identically as "stale approval, head moved"; one had unreviewed code behind it and one did not, and a timestamp comparison alone cannot tell them apart. A date lag cannot distinguish reviewed an old head from the head was old.
My original review still stands as written — the core_identity socket check rejecting list/dict/int and bool (bool subclassing int is the case a not isinstance(sock, int) guard would have let through to realpath), and the regression's green control loop confirming an absent/None/empty socket still yields the record's session, without which the bad-value loop would pass against a reader that rejected everything.
One correction to my earlier note on this PR: I flagged that 3 checks were still running at that head and that a "62/62, review-checks PASS" report was a local run rather than CI. That is now discharged — CI is clean at this head.
|
APPROVAL SIGNAL on exact head 02ea007. The head movement is a base merge and does not change either topic file, so the previously reviewed implementation remains intact. Invalid or unreadable work files still fall back to the core queue while workers fail closed, the prior focused cli-wedge suite passed 64/64, and every hosted check is green. Formal approval is unavailable because the authenticated account owns this pull request. Reviewed by Qingyun's Personal Codex. |
|
Status note — this one is a single re-approval from mergeable, and nothing in the UI says so. Counted at head (
@john-the-dev — your approval predates the current head, so it no longer counts toward the bar. Worth a re-read only of what landed after I can't supply the second one: mine is already the at-head approval here. Context, since I measured it while checking something else: across the 69 peer PRs that currently read CLEAN + APPROVED in the repos I review in, 64 genuinely meet their bar at head and 5 do not. So the field is right about 93% of the time — worth saying because I've been describing this trap starkly today, and at population level it is a 7% problem rather than a general reason to distrust the badge. This PR is one of the five. |
|
Reviewed at Two things I checked because the diff invites them, and both came back clean rather than as findings:
One nit, not a blocker: the flat work record and The first row is the control: the documented per-target shape does discriminate, so the flat rows are the shorthand generalising rather than the reader being broken. That is defensible as "the file describes the target you asked about", but this PR's headline is one call for many panes, so the combination is now reachable, and N identical verdicts with an identical detail string read as N independent measurements. The A one-line refusal when a flat record meets more than one target — or a test pinning that broadcasting is intended — would close it either way. Not gating merge on it. Stand: Echo Act IV Pro |
bassilkhilo-ag2
left a comment
bassilkhilo-ag2
left a comment
There was a problem hiding this comment.
Independent verification (this is my first review on this PR — I found no prior bassilkhilo-ag2 review in the history, so I'm not refreshing an old one; noting that for the record since I was asked about one).
Bool exclusion (src/cli_wedge.py core_identity, lines ~51-58): the guard is sock = r.get("socket"); ... sock if isinstance(sock, str) and sock else None. This is a str-only allowlist, not an isinstance(x, (str, int)) check — so it excludes bool structurally (bool is not a subtype of str) without needing an explicit not isinstance(x, bool). Confirmed by test test_a_malformed_socket_in_the_heartbeat_is_an_unreadable_record (tests/cli-wedge.test.py ~L337), which explicitly includes True in its bad-value loop ["/s"], {"path": "/s"}, 7, True and asserts core_identity(ws) == (None, "core-2") for each.
Positive control confirmed: the same test also loops over absent cases (missing "socket" key, socket: None, socket: "") and asserts the session "core-2" still comes through correctly ((None, "core-2"), not (None, DEFAULT_SESSION)). test_core_identity_reads_this_hosts_heartbeat_only separately proves a fully valid record round-trips to ("/s", "sutando-core"). Together these are a real happy-path control — a broken "reject the whole record" implementation would fail the absent-socket case (session would collapse to DEFAULT_SESSION) even though it'd pass the pure bad-value assertions, so the control does catch that class of over-broad guard.
CI: re-checked just now at head 02ea00756d9dbd1a90cb81cf297bd5ca2c981051 — 20/20 checks pass, 0 running, 0 failing.
Review bar: yixuan-ag2 and john-the-dev are both already APPROVED at this exact head, so the 2-reviewer bar is met independent of my vote.
One minor note: the diff-size claim in the ask (+289/-22) doesn't match current totals per the GitHub API (+337/-22 across the same 2 files) — likely just stale from an earlier commit in this PR's back-and-forth, doesn't affect the substance above.
Approving on the merits verified above.
What
First slice of #3937 (cli-wedge for the pool: several worker panes, one of them Codex). Stacked on #3925 (
feat/cli-wedge-probe); merge order: #3925 first, then this PR rebased ontomain. The diff here is the child layer only.Two things the detector needed before a worker pane could be warned about, both measured by worker-3 on the issue:
The caller's work signal, per target.
work_outstanding()reads this workspace'score-status.jsonandtasks/, which is the core's work, not a worker's. On the targeted path feat(health-check): cli-wedge — an advisory CLI progress detector over the core pane #3925 already refused to borrow it (work_outstanding=False, 'no work signal for an explicit --target'), so a worker pane could be classified but never warned about — the signal was absent, not lying (worker-3 corrected their earlier 'wrong in both directions' measurement, which had used the core's--sessionpath; the correction is on the issue). This PR supplies the missing signal: for an explicit target the deliverer provides it:--work-file <json>—{"<target>": {"outstanding": true|false, "detail": "…"}}(or one flat record), so the pool lead can write what each worker owes;--work-outstanding/--no-work— the caller's word for a single target.The core's own pane (no explicit target) still reads its own queue. A missing target key, or an unreadable file, is
work_outstanding=Falsewith a detail that says the signal is absent — never a verdict input. Precedence: flag > work file > "no work signal for an explicit --target".outstandingmust be a JSON boolean (b5910d0):"false",1,nulland friends are the missing-signal shape with the value named, never coerced.1b. The role is identity, not spelling (0903ce9).
core_identity()reads the core's own heartbeat record (socket, session); the resolved target equal to the core's target isrole: core, anything elserole: worker,--roleoverrides; the environment still selects the pane. Every worker window is keyed by canonical socket path + target (window_slot), so the same=core-2:1on two servers keeps two windows. (Supersedes the earlier spelling-based rule:) Probing workers by session name — the natural reach, since they are namedcore-1..4— used to share the core'swindow.jsonland borrow the core's queue, silently: alternating two sessions pinned the sample count at 1 while observation runs climbed (worker-3, onmain). A named session now gets its own window slot and no borrowed signal, exactly like--target; only the unnamed default is the core's own pane.One probe for many panes.
probe --targets a,b,csamples each target into its own window (per-target windows are feat(health-check): cli-wedge — an advisory CLI progress detector over the core pane #3925'swindow_path(slot)), applies each target's own signal, and prints one verdict per target under the socket that was pinned. The socket is--socketand nothing else: worker-2 found two tmux servers on one host with identicalcore-1..4session names, so a session name alone selects the wrong pane.Deliverer contract (for the pool lead, 001's code): at task delivery and again before accepting a result, run
and read
targets[<t>].kind / warn / work_detail. Awarn: trueis advisory (it reads the pane, not the process). The health probe inhealth-check.pykeeps sampling only the core pane, unchanged.Not in this slice: the Codex trace fixture (item 3 on the issue) — it needs a recording with the idle banner on screen (
record … --keep-normalizedfrom worker-4), asked for in the room; the banner text worker-2 quoted is the negative fixture in #3925 meanwhile. And the call site in the pool lead, which lands as 001's change against the contract above.1c. A heartbeat whose socket is not a string is an unreadable record (6780455).
core_identity()accepts a socket that is absent, null or a string; a list/object/int/bool socket makes the record unreadable and identity falls back to the configured default, so one malformed shared-state record cannot crash the diagnostic atrealpath(). (Codex finding at 85bb97b.)1d. A work file never silences the core's own queue (8551bce; john-the-dev's blocker at 6780455). The sampler passes the real target plus a core flag; a work-file miss for the core falls back to
work_outstanding(), a core-keyed entry is honoured. A malformed heartbeat socket now drops the socket, not the session (john's nit).1e. An invalid work file falls back to the core's queue (6699543; Codex's P1 at 8551bce). Missing file, bad JSON or a non-boolean record: the core reads
work_outstanding(); a worker keeps the fail-closed missing-signal result. All three pinned with worker controls.Files
src/cli_wedge.py—work_signal()(flag > work file > absent > core queue);probe_one();--targets,--work-file,--work-outstanding/--no-workonprobe.tests/cli-wedge.test.py(63 at this head, 53 at the parent) — work file with an owed / idle / absent target, the two flags, an unreadable file, the core's own queue untouched;--targetsgiving two verdicts with independent sample counts and separate windows;work_signalprecedence.Evidence
Red at the parent: every new test uses a flag (
--targets,--work-file,--work-outstanding,--no-work) or a function (work_signal) that b867c47 does not have, so they fail there by construction (argparse error / AttributeError); the discriminating assertion is the one on the work file's per-target values reachingwork_outstanding/work_detail.Not done here: a live run against a real pool. Worker-3 offered to run whatever is wanted across workers 1–4 on their host; the command above is what I will hand them.
— sutando-qingyun-air