Repository navigation
Revert "feat(agy-core): Slice 2 — task-file-injection notifier for agy (#4303)" - #5166
Conversation
…y (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>
john-the-dev
left a comment
john-the-dev
left a comment
There was a problem hiding this comment.
Approve at f0126fd.
- Exact revert: the +/- lines of
git diff f0126fdf^ f0126fdfare identical to the inverse of 4a72ddf (#4303). 11 files, +20/-2068, nothing else touched. - Applies cleanly to current main:
git merge-tree --write-tree origin/main prsucceeds. - No dangling references: the 4 deleted files (
src/agent/agy/cli/task-notifier.shand the 3 agy test files) are referenced on main only by each other. - Not verified by me: I didn't run the suites. CI on this head is the proof that main goes green; it was still running at submit.
Coverage Gate✅ Diff coverage PASSES the 95% bar. Whole-tree (informational): 91%. Diff CoverageDiff: origin/main...HEAD, staged and unstaged changesNo lines with coverage information in this diff. |
keweichen
left a comment
keweichen
left a comment
There was a problem hiding this comment.
Reviewed exact head f0126fdf8e9adfacf13f88601b8193290fe985f8 against current base 54d6465fd8d54a4c1722dfca6dfb9f3d653ed0c4.
The source-level revert is mechanically clean: its patch is the exact inverse of 4a72ddf54, every affected blob matches the pre-#4303 parent, review-checks.sh passes, the focused suites pass, and all 26 applicable Actions checks are green. I found no extra code delta.
Blocking — the runtime rollback is not demonstrated and leaves an already-running notifier alive. #4303 starts a persistent ${SESSION}-watcher; this revert removes every reference to that session. At this head, start-cli.sh returns as soon as the core session exists, so rerunning it after an update cannot retire the old watcher. I reproduced that on an isolated tmux socket:
before:
review-agy
review-agy-watcher
launcher:
review-agy already running.
after:
review-agy
review-agy-watcher
With default queues that stale watcher is normally inert, and the feature was unreleased/manual, so the exposure is limited. With explicit queue overrides it can nevertheless keep running the removed injector after the checkout and docs say task injection is gone. This is exactly the teardown side required by REVIEW.md lesson 11.
Before approval, please document and exercise an explicit post-update cleanup for an existing ${SESSION}-watcher, then post the exact-head real post-restart agy round trip required for startup/live-path changes (real authenticated session, not only the harness). The witness should begin from a #4303-style core + watcher pair and show the stale watcher absent after rollback while the surviving manual agy path still completes a real interaction.
Follow-up sequencing: the current #5161 cannot simply land after this revert. git merge-tree --write-tree reports a start-cli.sh content conflict plus modify/delete conflicts for task-notifier.sh and its test; retaining #5161's side would still reference the --typable and watcher-ready-token contracts removed here. Please correct the rollout note: land #5166 first, rebuild #5161 on post-revert main by replaying #4303 plus its fixes, and rerun full CI and the live witness.
Separate merge gate: license/cla is absent at this head (total_count: 0, no status contexts), not failing. The repository's documented close/reopen or fresh-head recovery is still needed before merge.
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>
|
@cla-assistant check |
john-the-dev
left a comment
john-the-dev
left a comment
There was a problem hiding this comment.
Approve at 0a10892. My f0126fd approval did not cover the new commit; this review does. It addresses @keweichen's blocking point about the leftover ${SESSION}-watcher.
The revert is still exact. The full PR is f0126fd plus 0a10892, and 0a10892's parent is f0126fd. The +/- lines of git diff f0126fdf^ f0126fdf are identical to the inverse of 4a72ddf. 0a10892 touches only start-cli.sh (+14), the agy README (+4) and tests/agy-start-cli.test.py (+78).
The retirement is safe.
retire_stale_watcher(start-cli.sh:39-47) works only on$TMUX_SOCKET, the agy socket. It names one target,"=${SESSION}-watcher". The=prefix forces an exact session match, which is the right form forhas-session/kill-sessiontargets (a trailing:is only needed for window/pane targets).- It runs at :111, after the agy/tmux availability exits and before the
session_existsearly return, so it fires on both launch and attach.--checkand--helpexit earlier and never touch tmux. - It is idempotent: when the session is absent,
has-sessionfails and it returns 0 quietly.
Tests, run with a scrubbed env (env -i, no SUTANDO_*/TMUX) at 0a10892:
tests/agy-start-cli.test.py: 29 OK. All 3StaleWatcherRetirementTestsran against a real tmux server; none were skipped.tests/tmux-session-targets-are-exact.test.py: 2 OK.- Mutant 1, the call removed: 2 failures.
- Mutant 2,
=dropped so tmux prefix-matches: 1 failure. The-watcher2near-miss test catches it.
Independent live repro of keweichen's scenario (isolated socket and scratch tasks/results/workspace, stub agy, real fswatch):
- 4a72ddf's launcher started the real pair,
agyw+agyw-watcher, with the notifier's setsid'dwatch-tasks-stream.shandfswatchchildren alive. - I reran this head's launcher. It printed
Retired stale agyw-watcher ...thenagyw already running., and left onlyagywintmux ls. pgrep -f <scratch tasks dir>then found no processes. The notifier's HUP to EXIT trap tears down its setsid'd watcher group, so the whole process tree is gone, not just the tmux session.
Other checks:
scripts/review-checks.sh --diffon the PR diff: PASS.- CI on 0a10892: the first run (37535115318) was cancelled. Its "tsc + tests" gate failed only because the test jobs were cancelled (capacity/superseded), not because a test failed. Its replacement, 37535189631 on the same head, was still in progress when I submitted.
license/clais now present on this head.
Not verified by me:
- I did not do a real authenticated agy round trip. I used a stub agy; the author's PR body carries one at this head.
- I did not see the replacement CI run finish.
- #5161 sequencing is out of scope here.
Dismissed on the owner's word after verifying the fix at 0a10892: start-cli.sh retires a leftover '-watcher' tmux session before its early return (exact target, idempotent), with a regression on an isolated tmux socket that fails at the parent; the rollout note now says land this revert first and rebuild #5161 on post-revert main; CLA re-attached; the PR body carries a real agy round trip on this host (stale watcher retired, restarted agy answers). rui and Qingyun approved this head. Kewei, a re-review is still welcome.
…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>
…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>
Brings in the #4303 revert (#5166), the CI sharding change (#5000) and the rest of main since 54d6465. One conflict, in the CLI usage string of src/delivery/task_dispatch.py: this branch adds the `sweep-plan` line above `pending-candidates`/`next-pending`, while main's revert drops `[--typable]` from those two lines. Resolved by keeping the new line and taking main's text for the two existing ones. No other change. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Reverts #4303 (merged as 4a72ddf), which turned main red, and retires the notifier session #4303 left running.
Why. #4303 was green on its own head but was merged without re-running CI against two changes already on main: #4976 (the watcher clears
SUTANDO_INSTANCE_IDon the core's canonical inbox, so the agy notifier polled a sentinel nobody writes and never reported ready) and #4945 (exact tmux targets, which the new agy notifier did not use). Four tests have failed on main since:agy-task-notifier,agy-notifier-dependency-gate,claude-task-notifier-submit,tmux-session-targets-are-exact.What this changes.
f0126fdf8: a plaingit revertof 4a72ddf: 11 files, the agy notifier and its tests removed,start-cli.shback to its prior state.0a108922d: runtime rollback. feat(agy-core): Slice 2 — task-file-injection notifier for agy (Antigravity CLI) #4303 started a persistent<session>-watchertmux session, and the revertedstart-cli.shreturns early once the core session exists, so an update left the old notifier running.start-cli.shnow kills an exact-target=<session>-watcheron its own socket on every run (launch or attach), before that early return; idempotent, one log line. Documented insrc/agent/agy/README.md. Regressions intests/agy-start-cli.test.py(StaleWatcherRetirementTests) run a real tmux server on an isolated socket: leftover watcher over a running core, orphan watcher on a fresh launch, and a near-miss name (<session>-watcher2) that must survive.Rollout. Land #5166 first. #5161 does not land unchanged after it (
git merge-treereports astart-cli.shconflict plus modify/delete conflicts fortask-notifier.shand its test). It is rebuilt on post-revertmainby replaying #4303 plus its fixes, then gets full CI and a live witness before review.Evidence.
tests/agy-start-cli.test.py, scrubbed env (SUTANDO_*andTMUXunset):0a108922d:Ran 29 tests ... OKStaleWatcherRetirementTestsagainst the parent'sstart-cli.sh(f0126fdf8):FAILED (failures=2):test_rerun_over_a_running_core_retires_the_leftover_watcherandtest_fresh_launch_retires_an_orphan_watcher_and_starts_the_core. The near-miss test passes on both, as it should.scripts/review-checks.shon the diff: PASS.tmux-session-targets-are-exactandclaude-task-notifier-submitpass (both fail on main).Live witness on a real host, real authenticated
agy1.3.0 (auth on this host is a Gemini API key, no interactive login). Scratch tmux socket, scratch workspace and onboarding path; no live session, socket or workspace touched. It starts from #4303's own launcher at the base commit54d6465fd(real agy core plus the real notifier watcher, with itswatch-tasks-stream.shandfswatchchildren), reruns this head's launcher over it, then kills the core and restarts it from this head and does a real prompt round trip:The stale watcher session and its whole process tree are gone after the rerun, the running core is untouched, a second rerun is quiet, and the restarted manual agy path answers a real prompt.
🤖 Generated with Claude Code