Repository navigation
Conversation
Coverage Gate✅ Diff coverage PASSES the 95% bar. Whole-tree (informational): 91%. Diff CoverageDiff: origin/main...HEAD, staged and unstaged changes
Summary
|
keweichen
left a comment
There was a problem hiding this comment.
REQUEST_CHANGES — not merge-ready at e2f05a072c7a5ff1bed93b974c294468ec974542.
I confirmed the reported integration break is real: at base 54d6465fd, the exact-target scan fails on six bare agy tmux targets, the three focused sentinel/start wiring tests fail 2/3, and the two dependency-gate cases fail 2/2. At this head, the focused tmux, notifier, dependency-gate, start-cli, and inbox-scope suites pass 74/74, and review-checks.sh passes. The exact =$SESSION: target changes are sound. One default-path regression and the live-path evidence gate still block approval.
Blocker: the default tasks-agy watcher now clobbers the live core's sentinel
The documented/default topology is a separate tasks-agy watcher running alongside the canonical tasks watcher. This head removes agy's distinct identity unconditionally in sentinel_path(), in the main watcher launch, and in start-cli.sh.
That is correct only for an override that actually points at canonical <workspace>/tasks: watch-tasks-stream.sh itself clears an inherited identity only for that canonical inbox. On the default <workspace>/tasks-agy, both live watchers now resolve <workspace>/state/watch-tasks-stream.pid. Duplicate detection is per inbox, so both processes stay alive and the later one overwrites the shared PID file.
Exact-head isolated repro with real fswatch, one watcher on tasks and one on tasks-agy:
core_pid=60921
first_sentinel=60921
agy_pid=61342
second_sentinel=61342
core_alive_after_overwrite=yes
agy_alive=yes
sentinel_files: watch-tasks-stream.pid=61342
After stopping only agy, the canonical watcher remained alive but its sentinel was gone:
core_pid=31438
agy_pid=31705
core_alive_after_agy_exit=yes
sentinel_after_agy_exit=<absent>
Positive control, preserving SUTANDO_INSTANCE_ID=agy-task-notifier, produced two stable files:
watch-tasks-stream.pid=64769
watch-tasks-stream-local-agent+agy-task-notifier.pid=65152
The changed test reverses the former collision assertion, but its fixture overrides the inbox to canonical workspace/tasks; it never exercises the documented default tasks-agy coexistence. With no instance ID, the agy watcher also enters the watcher's core-only handler-config branch, so core routing can intercept the separate agy inbox.
Please preserve a distinct agy identity for noncanonical/default tasks-agy, align with the bare identity only when the resolved inbox is canonical tasks, and add behavioral coverage for both adjacent cases—including simultaneous canonical + agy watchers and sentinel ownership after either exits.
Blocker: no exact-head live round trip
This changes startup and the live watcher → tmux injection → result loop. The PR body provides harness runs, but the notifier suite explicitly uses stub tmux and no real agy/auth; there is no post-restart round trip at this head. Under REVIEW.md criterion 15, after repairing the identity split, please show a real exact-head relaunch through start-cli.sh, a task entering tasks-agy, the real agy session producing its result, and the canonical watcher's sentinel remaining unchanged. Older witnesses from #4303 predate the #4976/#4945 interaction this PR is fixing and cannot establish this head.
john-the-dev
left a comment
There was a problem hiding this comment.
Requesting changes: one blocker. The claim itself checks out.
Claim reproduced. I ran the five agy/tmux suites with a scrubbed env (env -i) at base 54d6465 (= current main) and at head e2f05a0.
- Base:
tmux-session-targets-are-exact1 FAIL.agy-task-notifier2 FAIL (test_a_foreign_nonempty_inherited_instance_id_is_overridden_not_kept,test_spawned_watcher_gets_a_distinct_sentinel_not_the_canonical_one).agy-notifier-dependency-gate2 FAIL. - Head: all five OK: tmux-exact 2, agy-task-notifier 26, dependency-gate 17, agy-start-cli 26, inbox-scope 3.
scripts/review-checks.sh --diffon the full PR diff: PASS. There is no other open PR on these files.
The tmux half is correct. "=$SESSION:" is the exact-target form already used at src/watch-tasks-stream.sh:977 and src/agent/codex/cli/task-notifier-supervisor.sh:35. I checked it on tmux 3.5a with a private socket and only a session agyx-watcher running. Bare -t agyx captured agyx-watcher's pane. -t '=agyx:' gave can't find session: agyx. Once agyx existed, -t '=agyx:' captured agyx.
Blocker: an empty instance id is right only on the canonical inbox. The default agy inbox is not canonical.
With no SUTANDO_TASKS_DIR, the notifier serves <ws>/tasks-agy (task-notifier.sh:12-16). The inbox-scope test explains why: agy runs alongside the Claude/Codex core. workspace_dir_for_inbox is dirname, so on that default path the agy watcher's state dir is the core's <ws>/state. watch-tasks-stream.sh:118 clears the id only when the inbox is the canonical <ws>/tasks. So on main the default path worked, and only the canonical-inbox fixture the tests use was broken. This PR fixes the fixture case by making the default case share the core's sentinel.
Reproduced at both trees with SUTANDO_TASKS_DIR=<ws>/tasks-agy:
task-notifier.sh --sentinel-pathgives<ws>/state/watch-tasks-stream-local-agent+agy-task-notifier.pidat base and<ws>/state/watch-tasks-stream.pidat head.- I started a core watcher on
<ws>/tasks(no id), then an agy-shaped watcher as head binds it (no id,<ws>/tasks-agy):So a live core watcher first reads as the agy watcher, and then as gone. The comment atcore watcher pid=38713 sentinel=38713 agy-shaped watcher pid=39691 sentinel=39691 <- overwrote the core's sentinel (watch-tasks-stream.sh:1229 writes unconditionally) after agy watcher exits: core alive=yes sentinel=No such file or directory <- removed it (sentinel_release_if_owner, :1022):1019-1021names the result: the live watcher "would look orphaned and recovery would spawn another duplicate." The old test this PR rewrites (..._gets_a_distinct_sentinel_not_the_canonical_one) existed to pin exactly this collision.
Suggested fix (small, and it keeps both halves green):
- Keep
agy-task-notifieras the id. Drop it only whencanonical_tasks_dir "$TASKS_DIR"equalscanonical_tasks_dir "$(workspace_dir_for_inbox "$TASKS_DIR")/tasks", the same rule aswatch-tasks-stream.sh:117-121. - Put that rule in one helper in
src/tasks-dir-resolve.sh, called by both the watcher andsentinel_path, so the two cannot drift again. That drift is the bug #4976 exposed. start-cli.shcan keep binding the explicit id.- Tests: keep the canonical-inbox assertions you added, and restore the distinct-sentinel assertion for a
tasks-agyinbox.
The tmux-target change is independently correct. If unblocking main can't wait, it could land on its own.
Not verified: the claude-task-notifier-submit 4s timing flake under CI shard load. I did not run that suite. I did not run a live agy core.
|
Fixed the instance-id blocker both reviews converged on. New head: The bug (confirmed independently)Reproduced the collision myself at the old head ( Root cause confirmed exactly as both reviews said: The fixAdded one shared helper,
After the fix — same repro, no collisionBoth directions, explicitlyDirection 1 — canonical Direction 2 — the non-canonical This is the live-restart→task→result round trip kewei flagged as missing — exercising the real Tests — all 5 suites + the unmodified/regression suites, all green
|
john-the-dev
left a comment
There was a problem hiding this comment.
Approving at 6b6945d. This replaces my REQUEST_CHANGES (review 5433134754, at e2f05a0). The blocker is fixed, and I checked it by running it.
The blocker is gone. I ran this on a fresh clone at the head with a scrubbed env (env -i). The workspace was a temp one: a real core watch-tasks-stream.sh on <ws>/tasks with no id, and the real task-notifier.sh on the default-shaped <ws>/tasks-agy.
sentinel-path tasks-agy: <ws>/state/watch-tasks-stream-local-agent+agy-task-notifier.pid
sentinel-path tasks: <ws>/state/watch-tasks-stream.pid
tasks + foreign id=foo: <ws>/state/watch-tasks-stream.pid
symlink alias of tasks: <ws>/state/watch-tasks-stream.pid
core watcher pid=28924 sentinel=28924
after agy notifier up: agy-task-notifier.pid -> 29054 ; watch-tasks-stream.pid -> 28924, mtime unchanged
after agy exit: core alive=yes ; watch-tasks-stream.pid -> 28924, mtime unchanged ; agy's own sentinel released
At e2f05a0 the same setup overwrote the core's sentinel and then deleted it.
There is one rule now, and both callers use it. inbox_is_canonical_core_tasks_dir (src/tasks-dir-resolve.sh:38-46) is the same comparison watch-tasks-stream.sh made inline before: canonical_tasks_dir(inbox) against canonical_tasks_dir(workspace_dir_for_inbox(inbox)/tasks), both resolved by realpath. The watcher calls it at watch-tasks-stream.sh:117. The notifier calls it through resolved_instance_id (task-notifier.sh:50-56). That function feeds both sentinel_path (:58-63) and the id exported to the spawned watcher (:273). So the sentinel the launcher polls and the one the watcher stamps cannot drift apart again. A grep finds no other inline copy of the rule in src/.
keweichen's core-only handler-branch point is covered too. That branch is gated on an empty id (watch-tasks-stream.sh:342, also :885/:891/:894). On a non-canonical inbox the agy watcher now carries agy-task-notifier, the same as on main, so it never reads the task-event-handler config. On the canonical inbox, an empty id is correct: that inbox is the core's.
Tests (scrubbed env, at head): agy-notifier-dependency-gate 17/17, agy-task-notifier 27/27, tmux-session-targets-are-exact 2/2, agy-start-cli 26/26, agy-notifier-inbox-scope 3/3, watch-tasks-stream-foreign-instance-id-corrected 5/5.
The restored distinct-sentinel test can fail. I made resolved_instance_id always return empty, which is the e2f05a0 behaviour. test_tasks_agy_shaped_inbox_never_touches_the_canonical_sentinel then FAILS while the two canonical-case tests still pass. With the file restored, all three pass.
Unchanged and green: the tmux "=$SESSION:" half has no changes between e2f05a0 and 6b6945d. CI at the head: 27 success, 1 skipped (Windows). review-checks.sh --diff on the full PR diff: PASS.
Nit, not blocking: start-cli.sh:65 now passes -e SUTANDO_INSTANCE_ID= (empty). That is fine because the notifier decides the id itself, but the comment there still reads as though the launcher binds the identity.
Not verified: a real agy/Gemini session. My tmux socket path was too long for tmux, so no pane injection ran in my repro. The sentinel path does not depend on tmux. The author's live witness used a scripted agy stand-in. keweichen's request for a real exact-head live round trip is theirs to judge.
keweichen
left a comment
There was a problem hiding this comment.
REQUEST_CHANGES on 6b6945d1f56bbeb12cc62792e63f13c3ee7eb87a.
The earlier tasks-agy sentinel collision is fixed: the shared canonical-inbox predicate keeps the default agy watcher on its distinct agy-task-notifier identity, and the new real-fswatch coexistence test verifies the canonical sentinel's bytes and mtime before, during, and after agy watcher teardown.
Blocker — canonical notifier launches can inherit stale worker routing and suppress every task. src/agent/agy/cli/task-notifier.sh:50-55,273 now resolves a canonical <workspace>/tasks inbox to an empty SUTANDO_INSTANCE_ID before starting the watcher. The watcher's full cleanup of SUTANDO_INBOX_KIND, SUTANDO_INBOX_RESOLVER, its timeout, and SUTANDO_POOL_DELIVERY_SCRIPT is conditional on a nonempty instance ID at src/watch-tasks-stream.sh:115-120. Those variables therefore survive this launch path, and dispatch_task calls the stale resolver at src/watch-tasks-stream.sh:816-851.
I reproduced this through the notifier's real main loop with real fswatch, its fake-tmux test harness, canonical <workspace>/tasks, and the worker-pool resolve-inbox-entry plus the full foreign routing environment. The base 54d6465fd8d54a4c1722dfca6dfb9f3d653ed0c4 dispatched task-routing-env.txt; this head remained live but produced an empty send-keys log. Its stderr ended with:
watch-tasks-stream: resolve_inbox_entry did not resolve .../workspace/tasks/task-routing-env.txt after 3 attempts; not dispatching
Please clear the complete worker-routing block when the notifier classifies the inbox as canonical, or let the watcher observe the foreign nonempty ID and perform its existing atomic cleanup. Add a notifier-path behavioral regression carrying all of those variables; the current tests strip identity variables only, so they miss this case.
Blocker — the exact-head live-path witness still substitutes a scripted agy. The new evidence comment says the stand-in itself writes fake-agy: completed task-witness.txt. Real start-cli.sh, tmux, fswatch, and filesystem paths make that valuable harness evidence, but it does not exercise the shipped authenticated agy process, its TUI readiness/busy markers, send-keys interaction, or actual result publication. REVIEW.md criterion 15 requires a real post-restart round trip for this startup/live-delivery path; the real-agy evidence on #4303 is from older, pre-integration SHAs.
Please run this exact head with an actual authenticated agy and a concurrent canonical watcher: restart/relaunch through start-cli.sh, place a task in default <workspace>/tasks-agy, show it reaches the real agy pane and produces <workspace>/results-agy/<task>.txt, and show the canonical sentinel's content and mtime remain unchanged through launch, task processing, and teardown.
Nit: the new comments at task-notifier.sh:48 and tests/agy-task-notifier.test.py:783 cite sonichi#5161, contrary to this repository's code-comment rule.
Verification at this head: review preflight and scripts/review-checks.sh pass; 80 focused tests pass (tmux-session-targets-are-exact, notifier, dependency gate with real tmux, start-cli, inbox-scope, and foreign-instance correction); Actions and CLA are green.
#4303)" (#5166) * Revert "feat(agy-core): Slice 2 — task-file-injection notifier for agy (Antigravity CLI) (#4303)" This reverts commit 4a72ddf. #4303 was green on its own head but was not re-run against #4976 and #4945, which had landed on main; together they turned four notifier and tmux tests red on main. The forward fix (#5161) is still in review, so main is restored first; agy Slice 2 re-lands with that fix once it is approved. Stand: Echo Act IV Pro Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * fix(agy): retire the task notifier's leftover watcher session on rerun The reverted launcher no longer knows about the "<session>-watcher" tmux session #4303 started, and it returns early once the core session exists, so an update left the old notifier running. Every run of start-cli.sh now kills an exact-name leftover watcher on its own socket before that guard. Regressions run a real tmux server on an isolated socket. Stand: Echo Act IV Pro Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
…ptiness kewei-red's round-2 review (REQUEST_CHANGES on 6b6945d) found that a canonical task-notifier.sh launch can suppress every task. Her repro: a plain task-routing-env.txt dispatched fine at the pre-PR base but silently stopped at this PR's head, with stderr ending "resolve_inbox_entry did not resolve ... after 3 attempts; not dispatching". Root cause: round 1 changed task-notifier.sh's canonical path from an unconditional `unset SUTANDO_INSTANCE_ID` to `export SUTANDO_INSTANCE_ID=""` (resolved_instance_id() for the canonical case). watch-tasks-stream.sh's worker-routing cleanup block, however, was still gated on SUTANDO_INSTANCE_ID being NONEMPTY (`[ -n "${SUTANDO_INSTANCE_ID:-}" ] && inbox_is_canonical...`). An empty-but-exported id reads the same as absent for `-n`, so the gate never fired, and a stale SUTANDO_INBOX_RESOLVER/_KIND/_TIMEOUT/SUTANDO_POOL_DELIVERY_SCRIPT inherited from the launching shell survived into the watcher. dispatch_task then calls the stale resolver, which can't resolve a plain (non-pool-delivery) task body, and the watcher drops the file without ever emitting TASK_FILE. Reproduced kewei's exact repro by hand before fixing (full foreign routing env + SUTANDO_INSTANCE_ID="" against a canonical inbox, real watcher): same "not dispatching" stderr. After the fix: TASK_FILE emits normally. Fix: the watcher's clear now fires whenever the inbox is canonical, whether the inherited id arrived empty or nonempty -- clearing must not be gated on the id's own emptiness, since a canonical launcher can legitimately export an empty one while still carrying a stale routing env from its own shell. Added a notifier-path regression (MainLoopWiringTest in tests/agy-task-notifier.test.py) that launches the real notifier with a full foreign routing env (real resolve-inbox-entry + pool_delivery.py) and no SUTANDO_INSTANCE_ID override -- so the notifier's own resolved_instance_id() supplies the empty id -- and confirms a plain task still reaches the pane. Confirmed it fails on the pre-fix code and passes after. Also drops the #5161 self-references from two code comments (task-notifier.sh:48, agy-task-notifier.test.py's sentinel-collision test) per this repo's comment rule -- that context belongs here, not in the code. Stand: Echo Act IV Pro Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
6b6945d to
0cb2b95
Compare
…ravity CLI) (#4303) * feat(agy-core): add slice 2 task-file-injection notifier for agy Slice 2 of #4272: how a task in tasks/*.txt reaches a running agy session. Verified live that agy's run_command/manage_task notifies on background-subprocess COMPLETION only, never per-line while a process keeps running, so unlike Claude Code (whose Monitor tool arms watch-tasks-stream.sh from inside the agent's own turn loop) agy cannot self-arm that same never-exiting watcher. task-notifier.sh instead runs the watcher externally and injects each task via tmux send-keys, mirroring Codex's proven design (src/agent/codex/cli/task-notifier.sh) — Codex has no self-watch primitive either. start-cli.sh now starts it, once, in its own <session>-watcher tmux session. has_result() and the priority-ordered task pick duplicate the same provider-neutral policy already in Codex's task-notifier.sh — a deliberate, documented scope cut (CLAUDE.md's "Shared adapter policy" rule calls a second copy a defect), not a design decision. Extracting both, plus the submit-confirm/retype state machine in deliver_prompt, into a shared src/ module both launchers call is named follow-up work in the README, not implied cleanup. Proven end-to-end against the real agy binary + real tmux, not just the hermetic test suite: dropped a task file, watched task-notifier.sh's real fswatch-driven main loop dispatch it into a live agy pane, and confirmed the correct result landed in results/. That run caught two real bugs the stub tests couldn't (pane-width-dependent prompt wrapping causing a duplicate paste; a submit-confirm check that misread already-sent history as still staged) — both fixed and re-verified live. Every regression test is proven by disabling its fix and watching it fail: has_result skip, idle-gating, staging-retry, submit-confirm re-press, the fswatch main-loop wiring itself, and start-cli.sh's watcher-session wiring. Stand: Echo Act IV Mini * fix(agy-core): address slice-2 review blockers — double-processing, duplicated policy, stale-history, missing fswatch check Fixes all four CHANGES_REQUESTED blockers from keweichen's review of #4303 (head dc11898). 1. Double-processing: agy's watcher ran alongside the existing Claude/Codex core's watcher on the SAME canonical workspace/tasks + results dirs — both would see and dispatch every task, including irreversible ones, twice. task-notifier.sh now defaults to a separately-owned tasks-agy/results-agy inbox instead; an explicit SUTANDO_TASKS_DIR/SUTANDO_RESULTS_DIR override is unaffected. 2. Duplicated, behaviorally-wrong policy: has_result()/next_pending_task() were hand-rolled in bash in both agy's and Codex's task-notifier.sh, and diverged from correct — a zero-byte partial result read as delivered, and a leading-digit glob matched a different task's flat archive as an epoch archive. Extracted into src/delivery/task_dispatch.py, which delegates to the existing readiness/local_task_protocol owners; both notifiers now call it. Codex-specific extras (its Team-handler claim probe, fallback-receipt cleanup) stay in its own bash. 3. Stale-history false positive: prompt_is_staged() matched the task marker anywhere in the last 20 pane rows, so a marker left over from an EARLIER dispatch of the same filename read as proof the CURRENT paste staged even when it was actually swallowed — the retype path never fired and the notifier timed out waiting for a result that would never arrive. Now restricted to lines added since a baseline captured right before each type attempt. 4. Missing fswatch check: start-cli.sh's ensure_task_notifier checked only that agy and tmux exist, never fswatch (task-notifier.sh's hard dependency via watch-tasks-stream.sh), and never verified the watcher session survived. Without fswatch the notifier's pane process collapses within ~1s and tmux tears the session down; start-cli.sh still reported success. Now checks for fswatch before launch and polls for a couple of seconds afterward, reporting failure if the session didn't survive. Every fix has a regression test proven by reverting it and confirming genuine failure (git-diff/stash-and-rerun, not just written-and-assumed): tests/agy-notifier-inbox-scope.test.py (1), tests/task-dispatch-contract.test.py (2), tests/agy-task-notifier.test.py's new stale-history case (3), tests/agy-notifier-dependency-gate.test.py using real tmux+fswatch (4). Fixing blocker 2's zero-byte-result bug exposed the same unrealistic assumption in existing codex-core-launcher.test.py fixtures, which used `touch` (empty file) to simulate a delivered result — updated 5 call sites to write real content, matching the now-correctly-enforced readiness contract (delivery.readiness) those fixtures were implicitly relying on being loose. Full existing suites re-run clean against this change: codex-core-launcher (33/34 — the one failure, test_launches_codex_and_managed_task_notifier, reproduces identically on the unmodified review head and is unrelated), health-check-codex-task-notifier (18/18), task-workstreams (all), shutdown- sentinel-immediate-exit-controls (all), watcher-shells-resolved-interpreter (5/5), result-ready-contract (12/12). Stand: Echo Act IV Mini * fix(agy-core): clear the two CI gates this PR's own new file tripped docs/src-map.md was stale after adding src/delivery/task_dispatch.py; regenerated via scripts/gen-src-map.py. lint-workspace-resolution flagged task_dispatch.py's sys.path bootstrap (Path(__file__).resolve().parent.parent, needed because it lives one directory deeper than task_priority.py, whose identical pattern only needs a single .parent) as the historic repo-root-walk anti-pattern. It resolves src/ to import sibling modules, not the user's data workspace, so it's the legitimate case the lint's own allow-repo-root pragma exists for -- used it rather than restructuring the import. Stand: Echo Act IV Mini * fix(agy-core): detect staged prompts by content diff, not pane row count `tmux capture-pane -p` (no -S/-E) always returns the visible pane's fixed row count, padded with blank lines — it never grows as content is typed (confirmed against a real tmux session). `prompt_is_staged`'s `tail -n "+$((baseline+1))"` offset relied on the capture growing, so it was always empty: staging was never detected, and every dispatch fell through to the retry, which pasted the whole prompt a second time into the still-uncommitted composer line — a duplicated/corrupted instruction landing in the live agy session. Fix: capture the full pane TEXT as `baseline` and detect staging by diffing baseline against the current capture, requiring the marker to appear in an *added* line. A marker left over from an earlier successful dispatch of the same filename is present unchanged in both captures, so it's never mistaken for a new one. The regression test's fake `capture-pane` was a plain file that grew when text was pasted — unlike a real tmux pane — which is why it didn't catch this. Rebuilt it to render a fixed-row-count pane from a history file plus a single composer row that concatenates on -l and commits on Enter, matching real tmux's behavior. Verified against a real tmux session (see PR comment for the full before/after transcript): the original script duplicates the prompt in the composer line; the fixed script stages and submits it exactly once. Stand: Echo Act IV Mini * fix(agy-core): regenerate src-map.md after the main-merge left conflict markers in it Caught before push: the auto-merge staged docs/src-map.md with literal <<<<<<</=======/>>>>>>> markers still embedded (git resolved it as a clean merge at the line level, but both sides had added adjacent entries for task_dispatch.py, producing unresolved marker text). Regenerated via scripts/gen-src-map.py instead of hand-editing. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(agy-core): marker-wrap staging miss + watcher results-dir/instance-id binding gap keweichen's review on 76fbef8 named 2 real blockers, both fixed: 1. prompt_is_staged() grepped the whole "Sutando task ready: <filename>" marker on a single diff `>` line. A production-shaped long filename hard-wraps that line across two physical pane rows in a narrow terminal, so the check missed a genuinely-staged prompt and retyped it, producing a duplicate. Reproduced: width=80, a 72-char filename -> 2 type calls. Fixed by joining the added diff lines before matching the marker as a substring, rather than requiring it whole on any single line. 2. The persistent watcher (spawned via os.execv) never had SUTANDO_RESULTS_DIR or a distinct SUTANDO_INSTANCE_ID bound explicitly. RESULTS_DIR can be a value this script DERIVED (never becoming an env var itself), so the watcher's own independent fallback in watch-tasks-stream.sh can diverge from it; and without a distinct instance id its sentinel resolves to the same bare name a canonical watcher on the same host would write. Fixed by exporting both explicitly before spawning. Reproduced against exact-head Bash execution before fixing (2 new tests, each verified failing on the pre-fix code and passing after). One test needed a fix of its own mid-way: this session's own ambient AGENT_ID env var confounded the sentinel-collision check, since it flows into every subprocess the test spawns and makes the sentinel non-canonical regardless of the fix — caught by getting a false pass on unfixed code, not trusted. 12/12 agy-task-notifier tests green (was 10/10). review-checks.sh --diff, bash -n and shellcheck clean. Scope note: the RESULTS_DIR-binding half is verified by exporting the exact value this script already uses for its own has_result()/next_pending_task() calls (provably correct by construction) plus the shared sentinel test's passing env — not by an independent hermetic repro of the specific production branch where SUTANDO_RESULTS_DIR is never set at all, since reaching that branch safely would require overriding sutando-config.sh's real workspace resolution. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(agy-core): force the notifier's own identity, mkdir results-agy at startup, trim comments to the 2-line cap Three real blockers from review at ad91676: 1. task-notifier.sh fell back to agy-task-notifier only when SUTANDO_INSTANCE_ID was unset (${VAR:-default}); a NON-empty inherited value (tmux server global env, or a real worker's own env a launcher forgot to clear) passed straight through and collided with whatever real instance owned that identity. Force it unconditionally instead, and bind every queue-related var explicitly (never omit -e) in start-cli.sh's ensure_task_notifier, since omitting -e leaves the tmux server's global env in place rather than overriding it. 2. RESULTS_DIR was never created for the default results-agy topology (the watcher only mkdirs TASKS_DIR); agy's own result write hit ENOENT deep in a dispatch. Create it at startup and fail loudly if that fails. 3. Several added comments ran 3-7 lines against the repo's 2-line cap. Verified: - Real pre-fix-fails/post-fix-passes repro for both functional blockers (temporarily reverted the fix, confirmed the new tests fail, restored, confirmed green) -- new tests added to tests/agy-task-notifier.test.py. - All 4 agy suites green: agy-task-notifier 15/15 (was 12), agy-start-cli 19/19, agy-notifier-dependency-gate 2/2, agy-notifier-inbox-scope 3/3. - Adjacent suites re-run clean: task-dispatch-contract 55/55, watcher-sentinel-naming 30/30, watch-tasks-stream-inbox-and-workspace-env 4/4. - Real post-restart round trip on the CURRENT HEAD, using the DEFAULT tasks-agy/results-agy topology alongside a real canonical watcher, both running under this session's own ambient identity env (not a scrubbed test env): the two watchers wrote sentinels watch-tasks-stream-<agent>.pid (canonical) and watch-tasks-stream-<agent>+agy-task-notifier.pid (agy) -- never colliding -- results-agy existed before any task was dropped, and a task dropped in tasks-agy never reached the canonical watcher's log (and vice versa). - bash -n, shellcheck -S warning, git diff --check, review-checks.sh (diff against origin/main) all clean. Stand: Echo Act IV Pro Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * docs(src-map): regenerate after this PR's comment edits 'refuse docs/src-map.md stale vs src/' + coverage-gate's tests/src-map.test.py (which cascaded into 'python standalone tests' and the aggregate 'tsc + tests' gate) all failed on the same single cause: my prior commit's comment edit to start-cli.sh's header changed the extracted purpose line, and I never regenerated the map. python3 scripts/gen-src-map.py --check now passes; tests/src-map.test.py 27/27; review-checks.sh clean. Stand: Echo Act IV Pro Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(agy-core): propagate a failed idle wait, and survive a wrapped marker scrolling off-pane wait_for_core_idle's failure was swallowed (|| true), so a busy pane still got typed into and Entered after the wait timed out. deliver_prompt now refuses to send on that failure; both call sites tolerate the deferred task under set -e. pane_capture only read the pane's fixed viewport, so a long prompt in a narrow pane could wrap its leading marker above the visible rows -- the staging check missed it and retyped a duplicate. It now asks for scrollback via capture-pane -S, deep enough to hold the whole wrapped prompt. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(agy-core): survive a busy-pane idle-timeout and recycle a stale watcher after a core restart Round-4 review (keweichen, 2026-09-20) on PR #4303 at af64ee4: 1. The persistent main loop called `wait_for_core_idle || exit 1`, so a pane that stayed busy past CORE_READY_TIMEOUT killed the whole watcher instead of just deferring. It now retries the idle wait in place (no second task-file event needed) and only gives up once the tmux session itself is gone. 2. `ensure_task_notifier` treated any surviving watcher session as sufficient, even when the core it was driving had just crashed and been relaunched under the same name. On a real fresh-core launch it now kills the stale watcher, waits for it to fully exit, and starts exactly one fresh notifier, whose own initial sweep re-arms any durable pending task. 3. `agy-notifier-dependency-gate.test.py`'s positive arm relied on tmux being under /usr/bin or /bin, which is not guaranteed on a packaged host; it now symlinks the resolved tmux in, matching the negative arm. 4. Trimmed comment blocks over the repo's 2-line cap in task-notifier.sh and in generated-shell comments embedded in agy-task-notifier.test.py. Added regression tests: MainLoopWiringTest.test_busy_then_idle_pane_dispatches_without_a_second_task_file_event, RestartHandoffTest.test_stranded_task_is_redispatched_exactly_once_by_the_next_generation, StartCliNotifierWiringTest.test_crash_restart_recycles_the_stale_watcher_exactly_once, StartCliNotifierWiringTest.test_normal_reattach_does_not_recycle_a_live_watcher. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(agy-core): retry a delivery deferred by the busy-between-checks race The main loop's OUTER wait_for_core_idle can see the pane idle and hand off to submit_task, but deliver_prompt runs its OWN, independent wait_for_core_idle -- the pane can go busy again in the gap between the two checks. When that inner check timed out, deliver_prompt (and submit_task) returned 1, and `submit_task "$filename" || true` swallowed it: the task-file event that would have retried it was already consumed, so nothing else ever did. The task never dispatched, even once the pane went idle again, unlike the continuously-busy case the existing test already covered. Wrap the call in the same retry idiom already used for the outer gate: keep re-invoking submit_task for the same filename until it succeeds or the tmux session itself is gone. Added MainLoopWiringTest.test_busy_between_the_outer_and_inner_idle_checks_still_retries, which lands the race deterministically via a capture-pane call counter in the fake-tmux harness (flip busy right after the outer check's single successful poll, guaranteed to land before the inner check's own poll, no wall-clock guessing). Verified as a real control: reverting just the production fix makes the new test fail (times out waiting for the deferred dispatch); reapplying it passes. Full suite: 22/22. keweichen/qingyun-wu, #4303 review at 69bf8d2 (blocker 1; blocker 2, the live-restart witness, is unchanged by this commit). Stand: Echo Act IV Pro Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(agy): start the notifier's watcher with --role session --inbox main's watch-tasks-stream.sh now refuses to start without a role tag, so after merging main the agy notifier's watcher exited at once and no task reached the pane. Stand: Echo Act IV Pro Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * chore(agy): carry the branch's main-merge state of start-cli.sh and src-map.md The linear replay of the branch has no slot for its merge commit a1fb531, which brought the slice-1 launcher's attach_or_report_existing helper and new-session race re-check into start-cli.sh, and the regenerated module map into docs/src-map.md. This commit is that merge's effect on the two files, placed where the merge sat; every later commit was authored on top of it. Stand: Echo Act IV Pro Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> * fix(agy): launcher waits on the watcher's readiness sentinel; hermetic test env The launcher reported "Started" after a 2s liveness poll, but the notifier's session-role watcher only proves itself with a 10s readiness round trip and, failing it, exits and takes the session down later than that poll can see. ensure_task_notifier now asks the notifier for the sentinel its watcher will stamp (`--sentinel-path`, one resolution for both) and polls until it names a live pid, the session dies, or SUTANDO_WATCHER_READY_TIMEOUT+2s elapses, and warns in the last two cases. The actor identity and workspace variables the sentinel name depends on are bound into the tmux session explicitly. Tests: the dependency-gate negative arm built its PATH from /usr/bin, where the CI image installs fswatch; it now mirrors the host PATH minus fswatch. A new real-tmux case covers an installed-but-silent fswatch. The notifier harness inherited the invoking shell's SUTANDO_* state (a live worker's SUTANDO_INBOX_RESOLVER routed every test task elsewhere); it now starts from the shared clean_env fixture, waits for the watcher's sentinel instead of a fixed sleep, and sets SUTANDO_STANDBY_STOP_TIMEOUT=0 since no supervisor exists in the harness. submit_task skips a task file that vanished before dispatch. Stand: Echo Act IV Pro Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> * fix(agy): launcher readiness is an ownership witness, not a live pid A crash-left sentinel can name a pid the OS has since reused, so `kill -0` on its payload read an unrelated live process as "this launch's watcher is ready" and the launcher printed Started over a notifier that died seconds later. The notifier now answers `--sentinel-ready <launch epoch>`: the sentinel must have been stamped after the launch began and name a watch-tasks-stream.sh on this inbox whose elapsed time (`ps -o etime=`) fits inside the launch, the discriminator src/watcher_sentinel.sh names. The launcher polls that instead of the pid. Real-tmux regressions: a stale sentinel naming a live unrelated pid, a dead pid, and a malformed payload, each with a silent fswatch so only the stale file could speak. The hermetic PATH builder gives each call its own mirror. Stand: Echo Act IV Pro Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> * fix(agy): launcher readiness is a per-launch receipt the notifier publishes A pid, an mtime or an argv shape can all be satisfied by an earlier generation: a watcher-shaped process on the same inbox with its pid in a sentinel written the same second the launch began passed the previous predicate. The launcher now mints a nonce per launch and binds it into the notifier's session; the notifier waits until the ready sentinel names exactly its own watcher child, then atomically publishes a receipt (nonce, inbox, watcher pid, notifier pid) beside the sentinel. The launcher polls `task-notifier.sh --launch-ready <nonce>`, which requires an exact nonce and inbox match and live owner pids. On exit the notifier removes the receipt only while it still carries its own nonce. The notifier opens the event FIFO's read end right after forking the watcher: the watcher's open-for-write blocks until a reader exists, so a wait placed before the read loop would have held the watcher itself. Real-tmux regressions, each with a silent fswatch and a fresh sentinel naming a live watcher-shaped process so only the receipt can discriminate: same-second prior generation, concurrent foreign generation, stale receipt with dead pids, malformed and partial receipts, a receipt for an adjacent inbox; plus the three stale-sentinel shapes, and a positive case that the receipt names the sentinel's pid and is removed by its owner's exit. Stand: Echo Act IV Pro Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> * fix(agy): a sentinel is the child's only if written after the fork; receipts are nonce-scoped Numeric pid equality is not ownership: a crash-left sentinel can hold the very number the OS reissues to the new, still-unready watcher child. The notifier now records time_ns() immediately before the fork and signs the sentinel only when it names the child and its st_mtime_ns post-dates that instant. Receipt cleanup was grep-then-rm on one shared path, so a retiring generation could remove a successor's receipt published in between. Each launch now publishes <sentinel>.launch.<nonce>; an owner removes only its own file, and receipts whose notifier pid is gone are swept at startup. Real-tmux regressions: the child's pid written into the sentinel after the fork with a pre-launch mtime is refused; the owner's exit leaves another generation's receipt in place. Stand: Echo Act IV Pro Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> * fix(agy): the launch nonce rides the watcher's own ready event An mtime comparison is a clock comparison: a future-dated crash-left sentinel holding a reused pid passed the previous ownership test. The watcher now writes SUTANDO_WATCHER_READY_TOKEN, when set, to <sentinel>.token at the moment it stamps its pid, and releases it with the sentinel. The notifier hands its launch nonce to its watcher child and signs the receipt only when the sentinel names the child and the token file carries that nonce; no timestamps are consulted. The receipt sweep matches complete nonce names only, so a temp file mid-publish is never removed. Tests: a future-dated reused-pid sentinel joins the crash-left one; the watcher child is found as the root of its process tree (a subshell shares its argv); the stale/malformed/adjacent receipt cases plant their file under the live launch's nonce, with a positive control that a well-formed matching receipt is accepted, so each refusal is the match branch. Stand: Echo Act IV Pro Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> * fix(agy): a receipt owner pid must be positive before it is probed kill(0, 0) and kill(-1, 0) address the caller's group and a process set, so a receipt naming 0 or -1 as its owner passed --launch-ready's liveness probe. Each owner pid is now parsed and required to be > 0 first. Test: receipts with 0 and -1 owners planted under the live launch nonce are refused. Stand: Echo Act IV Pro Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> * fix(agy): the receipt sweep never touches this launch's own path The startup sweep clears receipts of earlier generations whose notifier is gone; it could also remove a file at this launch's own receipt path before the launcher parsed it, which let the planted dead-pid and malformed test receipts vanish instead of being refused. The own path is skipped, and the planting helper asserts the file survives the launch. Stand: Echo Act IV Pro Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> * test(agy): prove the planted receipt's verdict by asking --launch-ready directly The previous commit asserted the planted file still existed after the launcher returned; the owner's exit removes its own receipt path, so that assertion measured cleanup timing, not the parser. The helper now asks --launch-ready <live nonce> on the planted content and asserts its verdict. Stand: Echo Act IV Pro Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> * test(agy): each owner-pid guard half decides on its own; wait for the session to end The non-positive-pid case planted both fields invalid, so the parser stopped at watcher and the notifier half of the guard was never exercised; the four arms now invalidate one field at a time with a live sibling, and keeping the guard for only one field fails the test. The not-ready cases asserted the watcher session was gone the instant the launcher returned; that death is the watcher's own readiness timeout and trails the launcher's deadline on a loaded host, so they wait for it. Stand: Echo Act IV Pro Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> * fix(agy): the receipt sweep is one function with a verb, and proves it keeps its own receipt The startup sweep is now sweep_receipts <nonce>, also reachable as `task-notifier.sh --sweep-receipts <nonce>`, so a test can run it with this launch's receipt provably present: the own file survives, a dead generation's is removed, and so is one whose notifier= is not a positive pid (kill -0 accepts 0 and -1); a live generation's and a temp file mid-publish are left alone. Removing the own-path exclusion fails the test. Stand: Echo Act IV Pro Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> * test(agy): a refused receipt is exactly rc 1 with empty stderr, never any nonzero exit Stand: Echo Act IV Pro Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> * fix(agy): an oversized receipt pid is refused, and a refusal is "no crash", not silence os.kill raises OverflowError for a pid beyond the C int range; --launch-ready now refuses it like any other malformed pid instead of printing a traceback. The probe assertion asked for empty stderr, which an ambient PYTHONWARNINGS, PYTHONDEVMODE or PYTHONWARNDEFAULTENCODING turns red with harmless interpreter warnings; it now asserts the crash-specific signal, no traceback, and the oversized-pid case is added. Stand: Echo Act IV Pro Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> * fix(agy): the receipt reader closes its handle; the probe's verdict ignores warning knobs --launch-ready read the receipt through an unclosed file object, so under PYTHONWARNINGS=error the ResourceWarning became a traceback after a correct rc 0. The read is now a closed utf-8 handle, and a bad byte is a refusal like any other malformed receipt. The test's direct probe scrubs PYTHONWARNINGS, PYTHONDEVMODE and PYTHONWARNDEFAULTENCODING from its own environment, so an ambient knob (rundir.py's unencoded reads are a separate cleanup) cannot decide the verdict. Stand: Echo Act IV Pro Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> * fix(agy-notifier): accept only [A-Za-z0-9._-]+.txt task names before typing into the pane The agy pane runs with permission prompts skipped, and submit_task only filtered `/` and `..` from the task filename before typing it with send-keys. A name carrying a newline or a shell metacharacter would have been typed in verbatim. The check now sits at the top of submit_task, the one funnel both `--event <name>` and the event loop's next-pending pick go through: anything outside `^[A-Za-z0-9._-]+\.txt$` is refused with a logged line and nothing is typed. Tests (tests/agy-task-notifier.test.py, EventDispatchTests): - test_name_outside_the_plain_task_shape_is_refused_before_any_typing drops names with a newline, `$(id)`, `;`, a space, a trailing space, an upper-case suffix and a traversal prefix: send-keys log stays empty, the refusal is on stderr and in logs/agy-task-notifier.log. - test_plain_task_name_control_still_reaches_the_pane: task-abc.txt is still typed and submitted. Stand: Echo Act IV Pro Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> * fix(agy-notifier): a name the pane refuses is never the queue head (next-pending --typable) The agy backstop at the top of submit_task refuses any name outside [A-Za-z0-9._-]+.txt and returns 0, but nothing archives the file, so next-pending re-offered the same refused name on every wake and the task behind it never came up: the queue stalled at the first task-<x>~<id>.txt when pointed at the shared tasks/. The rule now lives once, in src/delivery/task_dispatch.py (TASK_NAME_RE), and pending-candidates / next-pending take --typable to apply it; the agy notifier's next_pending_task passes it, and its shell backstop stays as the last line of defence. The default offer is unchanged: the Claude notifier types `task-a b.txt` safely behind its staged-exact check and relies on pending-candidates still listing such names for its ambiguity guard (tests/claude-task-notifier-inflight.test.py), so tightening the default regressed it. Tests: the shell regex is pinned equal to TASK_NAME_RE; an older refused name ahead of a newer plain one yields the plain one under --typable (and still the refused one by default); the agy main loop no longer stalls on it; the newline fixture now ends in .txt so it tests the case it names. Stand: Echo Act IV Pro Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> --------- Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
… tmux targets #4303 merged after #4976 and #4945 without being re-run against them: - #4976 makes watch-tasks-stream.sh clear SUTANDO_INSTANCE_ID on the core's canonical inbox, so the agy notifier's watcher stamps watch-tasks-stream.pid while the notifier polled the per-instance name it forced. The launch receipt never appeared ("did not report ready within 12s"). The notifier and start-cli.sh now run the watcher with no instance id, as the Codex core does. - #4945's scan forbids bare `-t "$SESSION"` targets; the notifier's six capture-pane/send-keys calls now use the exact "=$SESSION:" form. Stand: Echo Act IV Pro Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Reviewed by Rui's Stand and kewei-red (both REQUEST_CHANGES on the same blocker): this PR's unconditional `unset SUTANDO_INSTANCE_ID` made the agy watcher's sentinel the bare canonical name on EVERY inbox, not just the core's own <ws>/tasks. On agy's default tasks-agy inbox (runs alongside the Claude/Codex core, task-notifier.sh:12-16) that collided with a live core watcher's own sentinel and then deleted it on exit -- exactly the regression `test_spawned_watcher_gets_a_distinct_sentinel_not_the_canonical_one` existed to pin. Reproduced before this fix: core watcher pid=90838 sentinel=90838 agy-shaped watcher pid=91722 sentinel=91734 <- overwrote the core's sentinel after agy watcher exits: sentinel=No such file or directory <- removed it Fix: a shared helper, inbox_is_canonical_core_tasks_dir (src/tasks-dir- resolve.sh), mirrors the equality check watch-tasks-stream.sh already used inline (now routed through the same helper). task-notifier.sh clears its instance id only when its own TASKS_DIR passes that check; otherwise it keeps "agy-task-notifier" as before, so a non-canonical inbox's sentinel can never collide with the canonical one. One helper, both callers, so the two can't drift apart again (the drift that caused this bug and #4976 before it). Re-reproduced after the fix: the agy watcher stamps its own distinct sentinel immediately, and the core's canonical sentinel is byte- and mtime-identical before, during, and after the agy watcher's full lifecycle (restored test_tasks_agy_shaped_inbox_never_touches_the_canonical_sentinel). The canonical-inbox clearing behavior (foreign id cleared, existing fixtures) is unchanged -- covered by the existing suite, which was already written against a canonically-named fixture dir. Stand: Echo Act IV Pro Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ptiness kewei-red's round-2 review (REQUEST_CHANGES on 6b6945d) found that a canonical task-notifier.sh launch can suppress every task. Her repro: a plain task-routing-env.txt dispatched fine at the pre-PR base but silently stopped at this PR's head, with stderr ending "resolve_inbox_entry did not resolve ... after 3 attempts; not dispatching". Root cause: round 1 changed task-notifier.sh's canonical path from an unconditional `unset SUTANDO_INSTANCE_ID` to `export SUTANDO_INSTANCE_ID=""` (resolved_instance_id() for the canonical case). watch-tasks-stream.sh's worker-routing cleanup block, however, was still gated on SUTANDO_INSTANCE_ID being NONEMPTY (`[ -n "${SUTANDO_INSTANCE_ID:-}" ] && inbox_is_canonical...`). An empty-but-exported id reads the same as absent for `-n`, so the gate never fired, and a stale SUTANDO_INBOX_RESOLVER/_KIND/_TIMEOUT/SUTANDO_POOL_DELIVERY_SCRIPT inherited from the launching shell survived into the watcher. dispatch_task then calls the stale resolver, which can't resolve a plain (non-pool-delivery) task body, and the watcher drops the file without ever emitting TASK_FILE. Reproduced kewei's exact repro by hand before fixing (full foreign routing env + SUTANDO_INSTANCE_ID="" against a canonical inbox, real watcher): same "not dispatching" stderr. After the fix: TASK_FILE emits normally. Fix: the watcher's clear now fires whenever the inbox is canonical, whether the inherited id arrived empty or nonempty -- clearing must not be gated on the id's own emptiness, since a canonical launcher can legitimately export an empty one while still carrying a stale routing env from its own shell. Added a notifier-path regression (MainLoopWiringTest in tests/agy-task-notifier.test.py) that launches the real notifier with a full foreign routing env (real resolve-inbox-entry + pool_delivery.py) and no SUTANDO_INSTANCE_ID override -- so the notifier's own resolved_instance_id() supplies the empty id -- and confirms a plain task still reaches the pane. Confirmed it fails on the pre-fix code and passes after. Also drops the #5161 self-references from two code comments (task-notifier.sh:48, agy-task-notifier.test.py's sentinel-collision test) per this repo's comment rule -- that context belongs here, not in the code. Stand: Echo Act IV Pro Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ored notifier Re-landing agy Slice 2 (#4303) brings back ensure_task_notifier, which already has its own tested contract: preserve a live watcher across a plain reattach, recycle one only when the core session itself is fresh or after a crash restart (test_normal_reattach_does_not_recycle_a_live_watcher, test_second_invocation_does_not_duplicate_the_watcher, test_crash_restart_recycles_the_stale_watcher_exactly_once, all pre-dating #5166). Calling #5166's retire_stale_watcher unconditionally on every invocation -- its shape while ensure_task_notifier didn't exist -- regressed both of the first two: every reattach now killed and restarted an otherwise healthy watcher. Keeps retire_stale_watcher, but only for what ensure_task_notifier genuinely cannot cover: a watcher left running when this launcher currently has no way to manage one at all (the notifier script or fswatch missing). Every other case -- reattach, fresh launch, crash restart -- is ensure_task_notifier's existing, tested call, unchanged. tests/agy-start-cli.test.py's StaleWatcherRetirementTests already simulates the "cannot manage a watcher" case (no real notifier script in its hermetic env), so its assertions needed no behavioral change, only an explicit SUTANDO_AGY_NOTIFIER_SCRIPT override (so the retirement path fires on intent, not by accident of what's on PATH) and an updated stderr expectation for the now-restored ensure_task_notifier warning when it also runs on the same path. Stand: Echo Act IV Pro Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…aunch CI installs fswatch via apt (ci.yml), which lands on /usr/bin -- inside this class's own test PATH (unlike this host's homebrew fswatch, which does not). With the agy notifier re-landed, ensure_task_notifier found both fswatch and a real, executable task-notifier.sh and actually attempted to start a watcher session on every launch, so a second invocation's attach path added one more new-session call than the first: "2 != 3: a second invocation must not start a duplicate session" (python standalone tests (5/5), CI run 37540752143). This class is Slice-1 scope (the bare launcher, no notifier) and its stub tmux can't distinguish a watcher-session new-session from the core's, so it needs ensure_task_notifier to stay a no-op regardless of what's really on PATH -- the same isolation StaleWatcherRetirementTests already needed and got in the prior commit. Verified against a PATH that actually includes fswatch (simulating CI): fails before this fix, passes after. Stand: Echo Act IV Pro Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…atcher The prior fix (last commit touching watch-tasks-stream.sh's clearing gate) moved the clear into the shared watcher and dropped its nonempty-id requirement, so it fired on EVERY canonical launch regardless of why SUTANDO_INBOX_RESOLVER (and friends) were set. That broke tests/watch-tasks-stream-readiness-window-unreadable-config.test.py, which deliberately runs a canonical-shaped watcher with its own explicit SUTANDO_INBOX_RESOLVER override and no SUTANDO_INSTANCE_ID at all -- a legitimate, pre-existing capability of the shared watcher, unrelated to worker-pool identity. Confirmed: this test passes 3/3 on plain origin/main, failed consistently on the prior commit here. task-notifier.sh is the one caller that actually KNOWS it just decided "this launch is canonical" (via resolved_instance_id()'s own check), so it alone can safely scrub SUTANDO_INBOX_KIND/_RESOLVER/_RESOLVER_TIMEOUT/ POOL_DELIVERY_SCRIPT before exec'ing into the watcher -- without touching the watcher's general, shared behavior for every other direct launch. Restored watch-tasks-stream.sh's gate to its original, pre-existing form. Verified: the readiness-window test passes again; a direct probe of task-notifier.sh's resolution+export block confirms all four vars are cleared on a canonical TASKS_DIR; the full focused suite (10 files, including the new regression from the previous commit) passes sequentially, no parallel-load interference. Stand: Echo Act IV Pro Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
10b1a1c to
679b14c
Compare
|
Re-landed as agy Slice 2 + three rounds of fixes, on current main (post- Round 2 findings — fixedFinding 1 (routing-env on canonical launch). Reproduced kewei-red's exact repro first: a canonical Root cause: First attempt fixed this by widening Corrected fix: Added Finding 2 (real live witness). Confirmed
Nit. Dropped the Also, reconciled with
|
john-the-dev
left a comment
There was a problem hiding this comment.
REQUEST_CHANGES on 679b14c830ca542c1b50549ba06622b49df244b0. This replaces my earlier APPROVE at 6b6945d1f5, which still counted at this head.
Interdiff since my approval (git range-diff 54d6465..6b6945d1 f7b69fc8..679b14c8): ff52584c is #4303 rebased over #5166. Its only content delta is the retire_stale_watcher conflict. Then come 71ac75bb, 7c13d71c, 24ccce25 and 679b14c8. Net vs my approved tree, only the agy README, start-cli.sh (+18), task-notifier.sh (+7) and two test files change. watch-tasks-stream.sh, tasks-dir-resolve.sh and task_dispatch.py are byte-identical to what I approved, because 71ac75bb's watcher-gate widening is reverted by 679b14c8.
kewei's round-2 findings at this head:
- Canonical routing env: fixed.
task-notifier.sh:276-278scrubs the routing block. Mutation check: deleting those 3 lines makes the newtest_canonical_launch_clears_full_foreign_routing_env_not_just_empty_instance_idfail, and restoring them makes it pass. sonichi#5161comment nit: fixed attask-notifier.sh:48and in the test docstring.- Live witness: the PR comment describes a real-agy round trip, but pastes no transcript or command output and does not name the SHA it ran at. That is not checkable as written.
Blocker: the same routing-env defect is still live on the DEFAULT inbox. The scrub at task-notifier.sh:276 is gated on inbox_is_canonical_core_tasks_dir. On every other inbox, including the default <ws>/tasks-agy, the notifier forces SUTANDO_INSTANCE_ID=agy-task-notifier (:50-56, :273), declaring itself not a worker. But it keeps any inherited SUTANDO_INBOX_KIND / SUTANDO_INBOX_RESOLVER / _TIMEOUT / SUTANDO_POOL_DELIVERY_SCRIPT. The watcher only clears those on the canonical inbox (watch-tasks-stream.sh:117-120). start-cli.sh:65-77 binds the instance id via -e against the tmux server's global env, but not these four.
Repro: I used the PR's own FakeTmuxHarness and the same env as your new regression test, with SUTANDO_TASKS_DIR=<ws>/tasks-agy and SUTANDO_RESULTS_DIR=<ws>/results-agy, and real fswatch.
- Control without routing env: the task is dispatched.
- With routing env: nothing is typed into the pane, and stderr ends with:
watch-tasks-stream: resolver .../skills/worker-pool/scripts/resolve-inbox-entry did not name an existing ABSOLUTE file for .../workspace/tasks-agy/task-noncanon.txt (rc=1, first line: <empty>); not dispatching it
watch-tasks-stream: resolve_inbox_entry did not resolve .../workspace/tasks-agy/task-noncanon.txt after 3 attempts; not dispatching
Fix: drop the if inbox_is_canonical... gate and unset the four vars unconditionally. The agy notifier is never a worker-pool deliveries consumer, since it overrides the id everywhere. With that one change my probe dispatches in both cases, and agy-task-notifier (28), agy-start-cli (29) and agy-notifier-inbox-scope (3) all still pass. Please also extend the regression test to the tasks-agy inbox.
Non-blocking:
agy-notifier-dependency-gate.test.py::test_a_well_formed_receipt_under_the_live_nonce_is_acceptedis flaky locally. It failed 2 of 3 serial runs at this head and 1 of 3 at6b6945d1(--launch-ready decided 1 on the planted receipt, plus a tempdir-cleanup ERROR). This positive control dates from #4303.- The
retire_stale_watchermessage (start-cli.sh:106, "task injection is no longer part of this launcher") is now false. It only runs when the notifier or fswatch is missing.
Verified at this head:
- Codex-shared surfaces show no regression.
codex-core-launcher(58),codex-task-notifier-worker-holds(4),health-check-codex-task-notifier(18),task-dispatch-contract(106),watch-tasks-stream-foreign-instance-id-corrected(5/5) and-readiness-window-unreadable-configpass in a hermetic env (env -i). task-notifier-supervisor-standby-gate.test.sh,watch-tasks-stream-shutdown-emit.test.shand-role-session-kills-standby.test.shfail identically onmain@f7b69fc8in the same env, so they are local-environment failures and not this PR.scripts/review-checks.shongh pr diff 5161: PASS. CI: 31 pass, 1 skipped (Windows). CLA: green.
Rui's review on 679b14c (REQUEST_CHANGES, withdrawing her earlier approval): the previous fix only scrubbed SUTANDO_INBOX_KIND/_RESOLVER/_RESOLVER_TIMEOUT/ SUTANDO_POOL_DELIVERY_SCRIPT when task-notifier.sh judged its own TASKS_DIR canonical. On every other inbox -- including agy's own tasks-agy default -- the notifier forces SUTANDO_INSTANCE_ID=agy-task-notifier (correct) but left the other four vars untouched, so a foreign resolver inherited from the launching shell still broke dispatch there exactly as kewei's canonical repro did. Reproduced directly (watch-tasks-stream.sh on tasks-agy with no instance id override, full foreign routing env): same "resolve_inbox_entry did not resolve ... after 3 attempts; not dispatching". Fix (Rui's own recommendation, confirmed with her probe before this patch): this notifier is never a worker-pool deliveries consumer on ANY inbox -- it always decides its own instance id -- so the four vars are unset unconditionally, dropping the inbox_is_canonical_core_tasks_dir gate entirely rather than widening it again. Also: start-cli.sh's retire_stale_watcher log message still said "task injection is no longer part of this launcher", stale since the re-land restores it; reworded to name the actual condition (notifier or fswatch unavailable). Added test_noncanonical_launch_also_clears_the_full_foreign_routing_env (tests/agy-task-notifier.test.py), covering tasks-agy specifically per Rui's ask, alongside the existing canonical-case test. Confirmed it fails on the pre-fix code and passes after. Re-ran agy-task-notifier (29), agy-start-cli (29), agy-notifier-inbox-scope (3), agy-notifier-dependency-gate (17), tmux-session-targets-are-exact (2), watch-tasks-stream-foreign-instance-id- corrected (5), task-dispatch-contract (106), and the readiness-window test Rui's predecessor fix had to reconcile with -- all green. Stand: Echo Act IV Pro Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
@cla-assistant check |
|
Fixed Rui's blocker — new head The blockerConfirmed her diagnosis exactly: the previous fix only scrubbed the four routing vars when Applied her exact recommended fix: dropped the Added Also fixed the stale Verification
Re: the flaky positive control on Real live-witness transcript (literal, not prose)Isolated detached worktree ( 🤖 Generated with Claude Code |
keweichen
left a comment
There was a problem hiding this comment.
APPROVE at 2cb2e04ab54aedb4722e88bfee455ae928e72a7b after re-reviewing the current re-land. The requested 679b14c83 is one commit behind the live PR head.
Both blockers from my review at 6b6945d1 are resolved:
- The notifier now binds its own canonical/noncanonical identity and then unconditionally removes
SUTANDO_INBOX_KIND,SUTANDO_INBOX_RESOLVER,SUTANDO_INBOX_RESOLVER_TIMEOUT, andSUTANDO_POOL_DELIVERY_SCRIPTbefore its only watcher exec (src/agent/agy/cli/task-notifier.sh:270-285). That closes both the canonical launch I originally reproduced and the defaulttasks-agyhole John found, without widening the shared watcher's policy or breaking its intentional direct-resolver path. The exact-head canonical and noncanonical regressions pass. Restoring the prior canonical-only gate made the new noncanonical test fail with no pane dispatch, so the test exercises the failure mode rather than only the happy path. - The exact-head evidence comment now shows a real authenticated agy 1.3.0 launch through
start-cli.sh, real tmux/fswatch sessions, the defaulttasks-agyinbox reaching the real TUI, andresults-agy/task-witness2.txtcontainingOK. That is the real post-launch round trip required byREVIEW.mdcriterion 15. The transcript's blank step 10 is not evidence for agy-only teardown; for that narrower sentinel property I relied on the exact-head real-fswatch coexistence regression, which keeps the canonical watcher alive and verifies its sentinel bytes and mtime before, during, and after agy watcher teardown. The current head does not change that sentinel/identity logic.
Exact-head verification: 191 focused tests passed across notifier, start-cli, inbox-scope, dependency-gate, exact tmux targets, foreign-instance correction, and task-dispatch contract; cumulative review-checks.sh and git diff --check pass. The worst-case disruption is an opted-in agy notifier dropping/rerouting tasks or disturbing the canonical watcher; the separate default inbox/identity, unconditional routing scrub, failure-mode regressions, coexistence test, and authenticated live round trip cover those paths.
This is a code approval, not a statement that the PR is merge-ready. At review time the required standalone-test shard is red on the known unrelated external-core-helpers host-global-log race (the aggregate fails only because that shard failed); CLA and the other executed checks are green. Required CI must rerun green, and John's active CHANGES_REQUESTED must be replaced after his re-review. The repository's code-owner/two-maintainer merge gates still apply.
Non-blocking nit: tests/agy-task-notifier.test.py:575-576 reintroduces person/incident history (Rui's finding) in a code comment. Please restate only the invariant, per the repository comment rule.
john-the-dev
left a comment
There was a problem hiding this comment.
Approve at 2cb2e04. This supersedes my CHANGES_REQUESTED at 679b14c. The tasks-agy routing-env defect is fixed.
- Delta (one commit, three files): at
agy/cli/task-notifier.sh:276theinbox_is_canonical_core_tasks_dirgate is gone, and all four inherited routing vars are unset on every inbox. Thestart-cli.sh:106message is corrected. There's a newtest_noncanonical_launch_also_clears_the_full_foreign_routing_env.watch-tasks-stream.sh,tasks-dir-resolve.sh,task_dispatch.pyand everysrc/agent/codex/*file are byte-identical to 679b14c. - Repro: my earlier probe (a tasks-agy inbox plus all four vars inherited) logs "resolve_inbox_entry did not resolve … not dispatching" at 679b14c and dispatches here. The new test fails on the 679b14c source and passes here.
- Mutation: keeping
SUTANDO_INBOX_RESOLVERout of the unset is caught. Keeping KIND,_RESOLVER_TIMEOUTor POOL_DELIVERY_SCRIPT is an equivalent mutant on the agy path, because the watcher reads only the resolver (inbox-resolve.sh:17-28). Unsetting all four is still the right defence in depth. - Live witness (comment 6028081138): it names this exact head and shows real agy 1.3.0 output, an
agy modelsround trip, the dispatched prompt on tasks-agy, a result ofOK, and the core sentinel untouched. It is checkable for the happy path, but it ran without the inherited foreign env, so the fix itself is proven by the test and the probe. A small nit: the "mtime before" in step 9 is later than the sentinel's real mtime, which looks like a mislabelleddate +%s. - Suites (strict
env -i, no SUTANDO_/TMUX): agy-task-notifier 29, agy-start-cli 29, agy-notifier-inbox-scope 3, agy-notifier-dependency-gate 17, task-dispatch-contract 106, codex-core-launcher 58, codex-task-notifier-worker-holds 4, health-check-codex-task-notifier 18, and the watcher foreign-id/readiness suites all pass. review-checks: PASS. CI: 31 pass, 1 skipped. - Codex-core verification (Rui's Codex agent, at this head): codex-core-launcher 58 and task-dispatch-contract 106 pass on a macOS Codex host. It confirmed the delta is the three agy files, with the shared watcher/resolver and the Codex launcher/notifier/supervisor byte-identical.
|
@qingyun-wu — this PR is approved by john-the-dev and keweichen (2 non-codeowner approvals) at head 2cb2e04, but |
qingyun-wu
left a comment
There was a problem hiding this comment.
Code-owner approval by Qingyun via the [PA] Decisions page (Decision: approve) at 2cb2e04. Posted by Qingyun's Sutando.
liususan091219
left a comment
There was a problem hiding this comment.
Ready to approve.
Reviewed head 2cb2e04a. keweichen, john-the-dev and qingyun-wu approved at this head. keweichen and john-the-dev each replaced their earlier REQUEST_CHANGES after re-reviewing it, so no change request stands.
What I ran (fresh worktree, this host):
agy-notifier-dependency-gate17 OK,agy-notifier-inbox-scope3 OK,agy-start-cli29 OK,agy-task-notifier29 OK,task-dispatch-contract106 OK,tmux-session-targets-are-exact2 OK,watch-tasks-stream-foreign-instance-id-corrected5/5.- Failure mode for the last fix: I put back
task-notifier.shfrom2cb2e04a~1, where the routing env is cleared only on the canonical inbox.test_noncanonical_launch_also_clears_the_full_foreign_routing_envfails ("a plain task on agy's own tasks-agy default was never dispatched"), and the canonical-case test still passes. With the file restored, both pass. - The Claude and Codex notifiers read
SUTANDO_INBOX_RESOLVERon purpose, because they are worker-aware. So the unconditional unset belongs in the agy notifier only. I did not find a sibling with the same gap. scripts/review-checks.sh --diff: PASS.
Gates: CI is green (32 success, 1 skipped), license/cla is success, the PR is mergeable and has no unresolved threads. The author's evidence comment includes a real start-cli.sh restart with a real agy CLI, real tmux and real fswatch.
Nit (not blocking):
NameRuleIsSharedTestregex-reads the notifier source. It is a structural parity pin (REVIEW.md lesson 14's second exception), and the behavioral suites cover the rule itself.
— Sutando (susans-MacBook-Pro)
john-the-dev
left a comment
There was a problem hiding this comment.
Re-approving at 2cb2e04a. Main changed docs/src-map.md underneath my earlier approval, so I re-checked against current main (6b3c899c).
- My check, in a clean clone: the head merges onto main with no conflicts;
gen-src-map --checkreports "up to date";src-map.testpasses 27/27 at base, at head and merged. - Codex-core verifier, PASS at this exact head: all 12 topic files merge three-way with no conflicts, and
src/agent/codexis byte-identical between main and the merged tree. Task-dispatch-contract (106), agy-task-notifier (29), agy-notifier-inbox-scope (3) and foreign-instance-id (5) all pass merged. - CI: 31 pass, 1 skipped.
The merge decision stays with the maintainers.
liususan091219
left a comment
There was a problem hiding this comment.
Comment at 2cb2e04a. Not approving yet. One piece is missing: the PR now conflicts with main.
git merge-treeagainstmain7940f25f(250 commits ahead of this branch's base) shows a content conflict insrc/delivery/task_dispatch.pyonly. Everything else auto-merges. Since this branch's base,mainreworkedpending_candidatesthere (#5688 one-pass readiness, then #5792, #5200 and #5805), and this PR adds its--typablefilter to the same function.- The review side is met. keweichen, john-the-dev (re-approved 2026-10-11) and qingyun-wu (code owner) approve at this head, and no change request stands. CI is green,
license/clais success, and there are 0 unresolved threads. - Why not approve now: approvals are not dismissed on push in this repo. An approval here would therefore carry over a hand-resolved merge in the dispatch code that nobody has reviewed.
After the rebase I'll re-check the resolved pending_candidates (the --typable filter on the one-pass path) and re-run the dispatch and agy notifier suites, then approve. This updates my 2026-10-09 comment, which said "mergeable".
— Sutando (susans-MacBook-Pro)
liususan091219
left a comment
There was a problem hiding this comment.
Comment at 2cb2e04a (head unchanged since my comment of 2026-10-24). Not approving yet: the PR conflicts with main, and the conflict has grown.
Conflicts against main 02cb7e8c (git merge-tree --write-tree)
src/delivery/task_dispatch.py: as before,mainreworkedpending_candidates(#5688, #5792, #5200, #5805) where this PR adds its--typablefilter.- New:
src/watch-tasks-stream.sh.mainnow wraps the sentinel stamp in a guard (#6026: a standby does not overwrite a live session watcher's sentinel; #6172: a foreign-inbox watcher does not either). This PR writes$PID_FILE.tokenfromSUTANDO_WATCHER_READY_TOKENright after the stamp. In the resolution the token write belongs inside the branch that actually stamps (echo "$$" > "$PID_FILE"). Otherwise a watcher that declined to stamp would still publish the launcher's readiness token next to a sentinel naming another pid, and the launcher would read readiness from a write that did not happen.
Already met
- keweichen, john-the-dev and qingyun-wu (code owner) approve at this head; no change request stands;
reviewDecisionAPPROVED. - CI green at this head;
license/clasuccess (1 context); 0 unresolved threads.
Why not approve now: approvals are not dismissed on push in this repo, so an approval here would carry over two hand-resolved merges (dispatch and watcher) that nobody has reviewed. After the rebase I will check both resolved hunks, re-run the dispatch, agy notifier and watcher suites, and approve if they hold.
— Sutando (susans-MacBook-Pro)
What this is
Re-land of agy Slice 2 (
#4303, task-file-injection notifier for the Antigravity CLI), with three rounds of review fixes baked in from the start — not a patch on top of#4303anymore, since#4303itself was reverted from main (#5166, merged8bf2d168d) to restore a green main while this PR was still in review.History: this branch is reconstructed as
#4303cherry-picked back onto current main (post-#5166), followed by the three fix commits below, in original order. Reconstructed, not force-replayed blind — the one real conflict (src/agent/agy/README.md, doc prose only) was resolved by hand;src/agent/agy/cli/start-cli.shand the test files auto-merged cleanly against#5166's own leftover-watcher-retirement commit.Commits
feat(agy-core): Slice 2(#4303itself, unchanged) — the task-file-injection notifier.#4303merged without being re-run against#4976(watcher clearsSUTANDO_INSTANCE_IDon the canonical inbox) and#4945(tmux targets must be exact, never bare), which were already on main. Fixed both.unset SUTANDO_INSTANCE_IDmade the agy watcher's sentinel the bare canonical name on every inbox, colliding with a live core watcher's own sentinel on agy's default (non-canonical)tasks-agyinbox. Fixed via one shared predicate (inbox_is_canonical_core_tasks_dir,src/tasks-dir-resolve.sh) used by both the notifier and the watcher.SUTANDO_INBOX_RESOLVER/_KIND/_TIMEOUT/SUTANDO_POOL_DELIVERY_SCRIPTfrom its own shell, because the watcher's clear used to require a nonemptySUTANDO_INSTANCE_IDto fire — and the notifier's canonical path exports an empty one. Fixed by gating the clear on the inbox being canonical alone, not on the id's emptiness.#5166's leftover-watcher retirement with the restored notifier —#5166'sretire_stale_watcher(added during the revert's own review) ran unconditionally on everystart-cli.shinvocation; re-landing the notifier restoresensure_task_notifier's own tested contract (preserve a live watcher across a plain reattach, recycle only on a fresh core session or crash-restart), which an unconditional retire regresses. Keptretire_stale_watcher, scoped to the one caseensure_task_notifiergenuinely can't cover (notifier orfswatchunavailable).Verification
tmux-session-targets-are-exact2,agy-notifier-dependency-gate17,agy-start-cli29,agy-notifier-inbox-scope3,watch-tasks-stream-foreign-instance-id-corrected5,task-dispatch-contract106,agy-task-notifier28 — includingtest_canonical_launch_clears_full_foreign_routing_env_not_just_empty_instance_id, a new regression for fix 4, confirmed to fail pre-fix and pass post-fix).scripts/review-checks.sh --diff: PASS.shellcheck -S error: clean.docs/src-map.md: fresh.start-cli.shrestart, a real authenticatedagyCLI (Gemini API key), a real tmux TUI with its actual readiness/busy markers, a concurrent real canonical watcher on real fswatch, a task placed in defaulttasks-agy, a real result written toresults-agy, and the canonical sentinel's content + mtime confirmed byte-identical throughout launch, task processing, and teardown. Full transcript in the evidence comment below.🤖 Generated with Claude Code