feat(server): push what an overseeing human needs instead of raw attention events - #1648
Conversation
janicduplessis
left a comment
There was a problem hiding this comment.
Fresh review of #1648 against #1644. I ran the touched suites locally (oversight, push, oversight-agreement, server, device-activity): 126 tests pass. I checked the agent-device and Expo facts below against agent-device 0.21.12 on this Mac and the Expo push docs.
Bugs
1. [bug] agent-action recency never fires for agent-device's default sessions. The session name is used as a raw path segment.
packages/stim-cli/src/devices/activity.ts:261-264
agent-device stores a session under sessions/<sanitized name>/, and the sanitizer in dist/src/session-paths.js is name.replaceAll(/[^a-zA-Z0-9._-]/g, '_'). The default implicit session in a real claim is "session":"cwd:11fe14a563f7aed6:ios", and its directory on disk is ~/.agent-device/sessions/cwd_11fe14a563f7aed6_ios/events.ndjson. statSync(join(root, 'sessions', 'cwd:11fe…:ios', 'events.ndjson')) gets ENOENT, so the new basis is silently absent for every agent that doesn't pass --session. That's the common case. An agent tapping without app logs then still looks stuck after 15 minutes, which is the exact false positive this change is meant to fix. The unit test uses session: 'a1', which hides it.
Suggestion: apply the same sanitization (session.replace(/[^a-zA-Z0-9._-]/g, '_')). Also prefer the claim's own stateDir field (real claims carry "stateDir":"/Users/.../.agent-device") over AGENT_DEVICE_STATE_DIR ?? ~/.agent-device, because the stim process doesn't necessarily share the agent's environment. Change the test to a cwd:<hash>:ios session name. Per invariant 9, check it once against a real agent-device session.
2. [bug] One gh timeout turns off PR notifications until stim-server restarts.
packages/stim-cli/src/workspace/pull-request.ts:184 (sticky unavailable), used through worktreePullRequests() at :230, which the server builds once in startServer.
pullRequestLookups() keeps a sticky unavailable for the lifetime of the returned closure. That suits one stim gc run. In a long-lived daemon, one slow 20 s gh api graphql call (a network blip or a laptop waking from sleep) permanently turns off "PR ready for review" and "PR merged" pushes. The same happens after gh auth login or installing gh while the server runs. The README (packages/server/README.md:340) says "it stops asking" but not that this lasts until a restart.
Suggestion: in worktreePullRequests, create a fresh pullRequestLookups() per round (every 5 minutes), or give the sticky state an expiry. Stickiness then still stops repeated 20 s waits within one round. If you keep the current behavior, say in the README that it lasts until the server restarts.
Should-fix
3. [should-fix] Rule design: an agent that finishes without closing its agent-device session gets stuck, never finished.
packages/server/src/oversight.ts:449,453,472
The agent-device claim's ownerPid is the agent-device daemon (verified: a claim's owner pid is dist/src/internal/daemon.js), not the agent. The device stays driven for as long as the daemon keeps the session open, so releasedGreen (driven.length === 0) stays false after an agent finishes its work. Fifteen minutes later the human gets "No agent activity for 15 min; … still up" instead of "Agent stopped after a green … build". Please confirm how often agents actually close their session. If they rarely do, consider treating "driven, newest build green, quiet for FINISH_SETTLE_MS" as finished rather than stuck, or at least say in the README that finished needs the agent to release the device.
4. [should-fix] Chatty apps suppress both stuck and finished.
packages/server/src/oversight.ts:251-262
quietSince includes every device's lastActivityAt, which includes app log records. An RN app that logs on a timer (polling, analytics, a reconnecting socket) keeps quietSince within the last minute forever. Stuck then never fires, and the non-idle finished path waits forever. Consider leaving app-log recency out of quietSince for these two rules. Agent actions, builds, bundle requests, driver changes and new log errors show progress better than "the app printed something". If you keep it, document it as a known limitation.
5. [should-fix] Low disk re-notifies every minute when free space hovers around 5 GB.
packages/server/src/oversight.ts:389-394
state.disk is dropped as soon as lowest >= DISK_CRITICAL_BYTES, so the next dip starts a new episode and pushes again with sound. During builds, free space near the floor goes up and down minute to minute. Suggestion: add hysteresis, for example re-arm only after free space goes back above LOW_DISK_BYTES (20 GB) or stays above the floor for several minutes. Memory already has a 1-minute settle. Disk has none.
6. [should-fix] Quiet started pushes use up the hourly budget that stuck/looping need.
packages/server/src/push.ts:329 with perHour: 20.
Every warming workspace and every first agent drive costs a token, even though they're passive and collapse in place. With about 10 active workspaces, started alone can use the 20 per hour, and a later stuck or looping push is then dropped silently, after its episode was already marked notified. Suggestion: exempt quiet notifications from the budget, or give them a separate bucket. At minimum, don't mark an episode notified when take() refuses it.
7. [should-fix] The control-conflict hook in control.ts has no test.
packages/server/src/control.ts:397-403,436-445
push.test.ts covers PushNotifier.control(), but nothing checks that ControlHub calls options.conflict (a) with the previous owner's device id on a take-over and (b) once when an agent starts driving a controlled device, and not for the driver that was taken over from (session.driver), or for this server's own leases. Both conditions are easy to break. A ControlHub test with a fake status feed that asserts the conflict calls would catch that.
8. [should-fix] The README makes "stopped the workspace" sound unconditional.
packages/server/README.md:315-316 ("the agent stopped driving after a green build and nothing happened for 5 minutes, or it stopped the workspace")
The code needs a green newest build on the stop path too (releasedGreen in the condition at oversight.ts:453). The test "not after a red one" confirms that. Suggested wording: "…or it stopped the workspace after a green build". The PR body's table has the same ambiguity.
Nits
9. [nit] Critical memory at server start can still push, despite the quiet first look.
packages/server/src/push.ts:266-277 and oversight.ts:396-403
readPressure is async, so the baseline evaluation (first status item) usually runs with pressure === null. The next evaluation sees critical for the first time with a non-null state and notifies after a minute. This contradicts "What is already true when the server starts … does not push". Suggestion: wait for the first readMachine() before the first evaluation, or treat the first non-null pressure reading as part of the baseline.
10. [nit] A registration with only dropped legacy events keeps the server watching for nothing.
packages/server/src/registry.ts pushEvents() and server.ts registerPush
An old phone that registered only build-failed/log-errors/app-stopped/slow-build is stored with events: []. PushNotifier.refresh() still counts it as registered, so the stim status --watch child, the disk and pressure reads and the timers keep running with nothing to push. Suggestion: skip registrations with empty events in refresh().
11. [nit] A phone can get a take-over push about itself.
packages/server/src/control.ts:397-403
When current.owner.device.id === owner.device.id (the same phone takes over its own earlier session from a second connection), it's told " took over the device you were controlling". Skip the conflict call when the ids match.
12. [nit] The looping fallback key can join unrelated failures into one loop.
packages/server/src/oversight.ts:284-288
Without a file:line diagnostic the key is errorCode ?? 'failed'. Three different Gradle or xcodebuild failures that each end in the same generic code (or none) count as "the same failure 3x". That's acceptable under the issue's "or error code" wording, but consider adding the first diagnostic message to the key when it's present.
13. [nit] The summary text says "workspaces" when it may count machine or same-workspace notifications.
packages/server/src/push.ts:367: ${n} workspaces need a look can count a machine notification, or two categories of one workspace. Suggested wording: "N notifications on ".
14. [nit] The spec says "byte-for-byte", but the copies differ in their header comment.
docs/specs/2026-09-25-stim-server-design.md:370
The agreement test deliberately skips the header comment. Say "identical apart from its header".
15. [nit] The PR lookup runs synchronous git calls on the server's event loop.
selectPullRequest(..., ancestry(cwd)) in pullRequestLookups uses runFileQuiet (sync git merge-base --is-ancestor), up to once per candidate PR per worktree every 5 minutes, inside stim-server's event loop, where WebSocket frame streaming also runs. That's usually a few ms each, so it's not urgent, but it's worth knowing now that this lookup moved from a one-shot CLI into the daemon.
Checked with no issue found: episode dedupe and re-arm for stuck (stuckAt vs quietSince) and loops (a streak reset drops the loop entry); PR ready and merged dedupe with mergeNotified shared with the git mergedInto fallback (no double "merged"); the null baseline for PRs through pullRequests[path] === undefined; JSON round-trip of OversightState (the pr: undefined key disappears but reads back the same); quiet hours spanning midnight; the minuteOfDay h23 formatting; collapseId hashed under APNs' 64 bytes; collapseId and threadId are valid Expo push fields; the legacy disk → machine mapping in both registerPush and the stored-registration parse path; agentOnly still accepted; the conflict closure referencing push before its const (called only after init).
|
Review follow-up (c.f. the comment review above):
|
janicduplessis
left a comment
There was a problem hiding this comment.
Fresh re-review of 64b745e against the earlier findings, plus a pass over oversight.ts, push.ts, control.ts, registry.ts and devices/activity.ts in the full diff. The touched suites pass locally (oversight, push, oversight-agreement, server, device-activity: 129 tests).
No blocking correctness bugs remain.
Earlier findings, verified
- 1 (session dir):
activity.ts:267now applies agent-device's[^a-zA-Z0-9._-]->_sanitization. The test uses acwd:<hash>:iossession. Fixed. - 2 (sticky gh):
server.ts:339builds a freshworktreePullRequests()per call, sounavailablelasts one round. A failed round leaves its paths out ofpullRequests.overseeMergeskipsundefinedand keepsentry.pr, so a ready or merged transition during the outage is still reported on the next good round. Fixed. - 5 (disk): the episode now ends only at 6 GB free (
oversight.ts:396), and there is a test. Fixed. - 6 (budget): passive messages skip
take()(push.ts:331). Onlystartedis quiet, and an all-quiet summary is passive, so this is right. Fixed. - 9 (first memory reading):
memoryKnownmakes the first non-null reading quiet even when it iscritical. A read failure after that returnsnullwithout resetting it, and a normal reading ends the episode. Fixed. - 10:
refresh()skips registrations with emptyevents.control()looks them up inregistered, so they get no control pushes either. Fixed. - 11:
control.ts:399skips the conflict when the same phone takes over. Fixed. - 3, 4, 8, 13, 14: now documented or reworded as stated. 12 and 15 were left as is on purpose. That's fine.
Remaining (non-blocking)
- [low, test-coverage]
packages/server/src/push.ts:331and:178: the budget exemption and the empty-events skip have no test. The failure a test would catch: a later change routes quiet messages throughtake()again, and about 20startedpushes then silently drop astuckorloopingpush. A PushNotifier test withperHour: 1would cover it: send one quietstarted, then onestuck, and assert both are sent. - [nit]
packages/server/src/push.ts:331: whentake()refuses an alerting push, the episode is still marked notified inoversee, so that push is lost for the episode. That now needs more than 20 alerting pushes an hour, so it's acceptable. It's the part of the earlier item 6 that is still open. - [nit]
packages/server/src/oversight.ts:155: the 1 GB recovery band is narrow compared with how much a build writes and then frees in DerivedData or Gradle caches, so space moving between about 4.5 and 6.5 GB can still start new episodes. Consider a wider band, such asLOW_DISK_BYTES / 2, if this shows up in practice.
janicduplessis
left a comment
There was a problem hiding this comment.
Fresh review of 40b092c (activity.recent, oversight ignores device-log recency).
Checked: recent is additive and optional; DeviceActivity.swift in apps/desktop uses a plain Decodable struct, so the unknown key is ignored. gc idle, budget (workspaceActivity) and supervisor/idle-stop read only state, driver, basis and lastActivityAt, so they are unaffected. recordWorkspaceUse is not called by status, so the server's own polling does not refresh workspace-use. The server and mobile copies of oversight.ts match. The new oversight test fails without the change: a chatty lastActivityAt never goes quiet. The three affected suites pass locally.
Findings:
-
Low:
packages/stim-cli/src/devices/activity.ts:28-30.recentkeys are inserted in ascending order of raw millisecond time.statusActivityrounds only the values, so two polls in the same minute can give the same rounded values with the keys in a different order. For example, takeagent-actionat 09:00:30 anddevice-logat 09:00:20, then a new log line at 09:00:40. Therecentorder flips, the rendered text differs, andstatus --watch(text === lastincommands/status.ts) emits a payload that is identical except for key order. This happens once per interleaving of an agent action and a log line, not once per log line, but it breaks the "never repeats an identical one" contract. Fix: keep the newest time per basis without sorting, so keys follow the fixed order the evidence is collected in. For example:if (Number.isFinite(at) && !(Date.parse(recent[basis] ?? '') >= at)) recent[basis] = new Date(at).toISOString();. Or emit keys in a fixed order. AstatusActivitytest with two orderings in the same minute would cover it. -
Nit:
packages/stim-cli/src/devices/activity.ts:19,29.basis as ActivityRecencyBasiscasts astring. TypingActivityEvidence.recencyas{ basis: ActivityRecencyBasis; at: number }[]removes the cast and lets the compiler check the kinds thatcreateActivityReaderandworkspaceActivitypush. -
Nit:
packages/server/README.md:307. The stuck rule counts Stim runs (workspace-use: start, ios, android, reload, worktree warm), but the list of what resets it names only agent actions, builds, Metro bundle requests and new log errors. Add "Stim run" to matchWORK_EVIDENCEand its doc comment.
|
Follow-up on the review of 40b092c: 1. Fixed in 49fae8e: statusActivity lists recent in the fixed order of ACTIVITY_RECENCY_BASES, and classifyActivity keeps the newest time per basis without sorting; a status-state test covers two orderings in the same minute. 2. Fixed: the evidence basis is typed, so the cast is gone. 3. Fixed: the README lists Stim runs. |
A device an agent drives reports the agent's last recorded action in lastActivityAt, read from the modification time of its agent-device session's events file, so an agent that works without producing app logs does not look quiet.
stim-server needs the pull request lookup gc uses. The stim package exports it as stim/pull-requests, with worktreePullRequests resolving each worktree's HEAD, and the lookup now reads isDraft.
…ntion events Replaces the build-failed, log-errors, disk, app-stopped and slow-build pushes with started, stuck, looping, finished, machine and control notifications from a pure rule module the phone app mirrors. Fixes #1644.
Read agent-device's sanitized session directory names, ask GitHub again each round, keep one low-disk episode while space hovers at the floor, stay quiet about a first memory reading, keep quiet pushes out of the hourly budget, skip registrations with no events, and push take-overs only to another phone.
…ed ignore app log chatter An idle app keeps logging, about 200 UIKit info records a minute for the Stim app, so lastActivityAt never went quiet and stuck and finished never fired. activity.recent reports the newest agent action, device log, Metro bundle and Stim run separately, and the rules count only agent actions, reloads, Stim runs and new log errors.
…does not repeat a payload
49fae8e to
4c8dc7d
Compare
janicduplessis
left a comment
There was a problem hiding this comment.
Fresh review of 4c8dc7d and the rebase onto the web platform (#1633, #1643, #1650).
No blocking findings. One minor point to decide on:
Minor: page-log masks other devices' quiet time. lastActivityAt in oversight.ts takes the newest WORK_EVIDENCE time across all devices in the workspace. With page-log in WORK_EVIDENCE, the Chrome page's console output and network failures (every Runtime.consoleAPICalled, and each 4xx or failed request written by web/run.ts) reset quiet time for the whole workspace. That is the same kind of chatter this PR leaves out for device-log. Example: an agent stalls on the iOS simulator while the owned page polls an endpoint that returns 404, or logs on a timer. Then stuck never fires, and finished waits until the workspace goes idle. The README describes page-log as counting for the Chrome page but does not say it hides the other devices' quiet time. A narrow fix is to count page-log only while the web page is driven, so it tracks an attached CDP agent and not an idle page. If this trade-off is intended, one README sentence would cover it.
What I checked:
- Rebase resolution of
server.ts: the diff against main contains only this PR's push/oversight wiring. Main's web additions from #1643 are intact. - Deleted
attention.ts/attention-agreement.test.ts: main's two web additions were apage-webapp-stopped push and webdrivenfor agentOnly gating. Web driven carries over:devicesOfadds the Chrome page andagentDrivencovers it, and the new oversight test asserts thestartedpush and the device target. The failed-page push is dropped as intended, and the strip keeps it in #1653. oversight-agreement.test.tschecks that the server and phone rule files are identical. Both copies carry the web changes.ACTIVITY_RECENCY_BASESincludingpage-logis required and correct.readWebActivityemits that basis, andrecencyis now typed by the union.facts.tslists it too.- Control conflicts on web:
activityOfreadsenvironment.web.activityfor platform web, andDEVICE_NOUNnames it "web page". stim-server's own CDP connection (node .../stim-server.mjs) and stim-frames are excluded bywebDriverTool, so a phone viewing or controlling the page does not produce a self-conflict or a falsestartedpush. platformNamein oversight.ts still maps anything but ios to Android, but it is only called with build platforms, so web never reaches it.pnpm run typecheck,pnpm test packages/server(149 passed), device-activity, status-state and guide tests all pass.
…t drives the page
|
Follow-up on the web review: fixed, the Chrome page log counts as activity only while an agent drives the page; a test covers a stalled simulator next to a chatty page. |
Description
stim-server pushes (#1581) and the phone's local notifications (#1597) fire on raw attention events: a failed build, new log errors, low disk, a stopped app, a slow build. Most of those are normal agent iteration. A person overseeing several agents gets pinged for things that need nothing from them, and gets nothing when an agent stops making progress, repeats the same failure, or finishes.
This is the first PR of a stack. It adds the shared rule module, the server's push evaluator and the status inputs the rules need. The phone side (local notifications from the same module, settings, deep links) is the next PR, #1645. Protocol changes are additive, so the current phone app keeps working against this server (see Risk).
Solution
One pure rule module.
packages/server/src/oversight.tstakes a status payload, disk and memory facts, pull requests and the previous episode state. It returns notifications with a stable id per workspace and category, plus the next state.apps/mobile/src/lib/oversight.tsis the same file apart from its header comment.oversight-agreement.test.tsfails when the two differ. It replacessrc/attention.tsandsrc/notify.ts.started(quiet, iOSpassive, threaded per Mac)phasebecomeswarming, or an agent first drives a device (updates the warming one in place)stuckstuckMinutes(default 15)loopingfile:line, or with the same error code. A FATAL launch is a failed run withSTIM_LAUNCH_FAILED, so repeated launch crashes count.finishedgh, git'smergedIntoflipping to set counts as merged.machinecontrolExample body:
wide-insets/Same Swift error 3x at AppDelegate.swift:71.Each episode notifies once. A stuck episode re-arms only after new activity, and a loop only after a success or a different failure. Pushes carry
collapseId(a hash of ref, category and path, because APNs caps it at 64 bytes), so a later episode replaces the notification instead of stacking. A first look after a restart or a new registration records state without notifying, as before. Quiet hours hold a lasting problem until they end, and drop events that happen during them. The per-device hourly budget and the summary for more than three notifications carry over; quietstartedpushes don't count against the budget, so they can't starve a laterstuckorloopingpush.Inputs the rules needed:
lastActivityAtcovered app logs, Metro bundles and Stim runs, but not agent-device taps. While agent-device drives a device, the activity reader now adds the mtime of that session'sevents.ndjson(onestat; agent-device sanitizes the session name into the directory name) as a newagent-actionbasis.lastActivityAtnever went quiet and stuck or finished could never fire. Status now addsactivity.recent, the newest time of each kind of evidence (agent-action,device-log,metro-bundle,workspace-use), rounded to the minute likelastActivityAt, which is unchanged. The rules count agent actions, reloads, Stim runs, builds and new log errors, but not plain app log records. This deliberately narrows the issue's "new log records" to error records. Against an olderstimwithoutrecent, the rules fall back tolastActivityAt.stim/pull-requestsexport. It makes onegh api graphqlcall per repository every 5 minutes, only while a device wantsfinished, and only for branches with an upstream. Each round builds a fresh lookup, so a gh timeout or sign-out costs that round only, and git'smergedIntocovers it. The query now also readsisDraft.readMemoryPressure, read with disk every minute.Protocol (additive, still protocol 1).
push.registertakes the new event names, plus optionalstuckMinutes(1 to 240) andquietHours { start, end, timeZone }. The server evaluates quiet hours in the phone's IANA zone, because it can't hold back a push the phone never received. Legacy names are still accepted:diskmaps tomachine, and the others are dropped.agentOnlyis accepted and ignored.Risk
machinepushes. It keeps suppressing its own local notifications for the events it believes are pushed. So on old phones the noisy events go quiet before the new app ships. This matches the product decision, but it's a visible change until Phone app: notification categories, deep links and settings for the redesigned rules #1645 lands.lastActivityAt). They are covered by unit tests, not by a long real agent session. Two known blurs, documented in the README: an agent that finishes without closing its agent-device session still holds the device and getsstuck(whose text then names the green build), and an app that logs on a timer never looks stuck or finished.Test plan
packages/server/__tests__/oversight.test.tsruns the rule engine through status sequences at its pure boundary. It covers each category's transition, one notification per episode, re-arming, the quiet first look, quiet hours, switched-off categories, own-lease exclusion, and that a single failure, log errors, a stopped app and a slow build do not notify.push.test.tsdrivesPushNotifieragainst a local fake Expo endpoint. It checks the exact messages (collapseId,interruptionLevel,threadId, deep-linkdata), PR ready-for-review, control pushes going only to registered devices, quiet hours, summary, budget, token drops and masked logging.server.test.ts: a take-over throughcontrol.beginpushes to the displaced phone's token (fake Expo endpoint);push.registerstoresstuckMinutes/quietHours, maps legacy events, and refuses a bad threshold, a bad minute or an unknown time zone.device-activity.test.ts: a live claim's session events mtime (sanitizedcwd:<hash>:iossession name) becomeslastActivityAtandrecent['agent-action'];oversight.test.tshas a case where app log records keep arriving and stuck still fires.worktreePullRequests()from the builtdist/pull-requests.mjsagainst this repo's worktrees. It returned PR docs: add a batched lane for small UI-only polish changes #1478 asmerged,draft: falsethroughgh api graphqlwithisDraft, returnednullfor this branch, and omitted a deleted worktree whosegit rev-parse HEADfailed.Fixes #1644