feat(server): list a user's schedules in one request (GET /me/cron-jobs) - #2384
feat(server): list a user's schedules in one request (GET /me/cron-jobs)#2384L42y wants to merge 2 commits into
Conversation
`GET /chats/:chatId/cron-jobs` answers "what runs from this chat". A client asking the other question — "what do I have scheduled" — had no route for it, so it had to ask every chat it knew about and keep the few answers that were not empty. That is one request per chat to find a handful of jobs, and it gets worse as a workspace grows. `GET /me/cron-jobs` answers it directly: every job the caller owns, across the orgs they belong to, ordered by what runs next. Paused jobs carry no next occurrence, so Postgres sorts them after the scheduled ones, which is also how a reader wants them. Scope: ownership is already the mutation boundary for a job (`requireCronJobAccess` gates on `ownerMemberId`), so listing by owner grants no new reach — it returns exactly the jobs the caller can already read and modify one id at a time. The existing `idx_cron_jobs_owner_created` index covers the join. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
yuezengwu
left a comment
There was a problem hiding this comment.
Exact-head review: 772dd149c6292c6837001a5f640b7f5a03169be2.
Requesting changes for the current-chat-access regression described inline. The existing API makes chat visibility a prerequisite for both reads and mutations; job ownership is an additional mutation check, not a replacement for that prerequisite. The new listing changes that boundary.
Verification on a detached exact-head worktree: pnpm check passed (existing warnings), uncached build 5/5, uncached monorepo typecheck 9/9. The PR's 3 tests, 34 cron integration tests and 10 schedule tests all passed using the repository's isolated PostgreSQL 17 Testcontainers setup. A review-only revoked-chat-access test failed twice: after real removeParticipant and leaveChat transitions, GET /api/v1/cron-jobs/:id and /api/v1/chats/:chatId/cron-jobs both returned 404, but GET /api/v1/me/cron-jobs returned 200 and the same full job including its prompt. No author-branch changes were made.
CI is separately not green: run 33731581964 tested merge e421746f97e6496fa0cb580f4551f2cd755cad37, not just the PR head. The Antigravity handler.test.ts:84 fixture missing SessionContext.noteTurnStart is inherited from main parent 38752415320c97c0bd122aa4afb13c3ff60b1b06: the fixture blob is identical in main and the CI merge (431c36a360b71d7cd93df362f97a7200b7cc476c) and absent from the pure PR head. Test Server also reports two failures outside the changed files: clients-provider-models.test.ts:273 (504 vs 200, stored catalog after a lost wake) and feishu-registration.test.ts:1615 (registration already in progress after expiry). Those two failures have not been causally attributed here and must not be assumed fixed or flaky. The 3 new tests also passed in CI.
Next: preserve the existing current-chat visibility boundary in the user-wide listing, add a revocation regression test, and resolve the CI failures before requesting another exact-head review. If returning jobs after chat access is revoked is intentional, that needs explicit authorization-contract review rather than the current no-new-reach claim.
…d jobs Review is right and my "no new reach" claim was wrong. `requireCronJobAccess` calls `requireChatAccess` before BOTH reads and mutations, and only adds the ownership check for mutations — so chat visibility is the prerequisite and ownership is the extra gate, not a replacement for it. Filtering on an active owner membership alone let a job survive a transition the rest of the API treats as losing it: remove the owner's managed agent from the chat, have the owner leave, and the job is 404 by id and through its chat while the new listing still returned it in full, prompt included. The listing now intersects ownership with the same visibility rule `requireChatAccess` applies — a direct membership row (speaker or watcher), or a speaker in the chat the caller manages — so access revocation removes a job here exactly as it does everywhere else. Two tests cover the boundary from both sides: a job disappears after that revocation sequence (asserting alongside the 404s from the existing routes, so the three agree), and a job stays visible to a manager who has left the chat but still supervises a speaker in it — the supervised path `requireChatAccess` grants and a naive membership filter would drop. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
You're right, and the "no new reach" claim in my description was wrong. Fixed in
Two tests cover the boundary from both sides, because getting this wrong in either direction is a bug:
On CII traced the Antigravity failure and it is a main defect, not flakiness, with a specific cause:
Rather than carry it here, I opened #2385 with the one-line fixture fix against The two Test Server failures — Ready for another exact-head review at |
yuezengwu
left a comment
There was a problem hiding this comment.
Fresh exact-head review: fae0f520f96900c22506f7b3c17c232421822fb5. The previously reported current-chat-visibility blocker is resolved on this head. The ownership query now requires the same direct membership or supervised-speaker condition as the existing chat guard; the prior-head finding is no longer an active code blocker.
Independently verified in a clean detached worktree: pnpm check passed (36 warnings, 8 infos), uncached build 5/5, uncached monorepo typecheck 9/9, and 52/52 PostgreSQL-backed tests. That includes all 5 current route tests, 34 cron integration tests, 10 schedule tests, plus 3 isolated review fixtures for the true supervised fallback with no direct watcher row, direct watcher access without a remaining managed speaker, and inactive organization membership. Review fixtures were moved out afterward; the author branch and worktree source remain unchanged.
CI is still a separate merge gate. Run 33733689132 now passes Test Server and the aggregate Test job; the two server failures from the earlier run are not active failures of this run, and I am not claiming their underlying causes were fixed here. The remaining Lint & Type Check failure is the inherited Antigravity fixture missing SessionContext.noteTurnStart. I verified the CI checkout af5596c86038637cefd32a66ddcf89daca4b2669 has this exact PR head plus main 38752415320c97c0bd122aa4afb13c3ff60b1b06; its fixture blob is identical to main (431c36a360b71d7cd93df362f97a7200b7cc476c) and absent from this pure PR head. #2385 proposes the independent repair.
No remaining actionable finding in the current diff. This approval covers only the reviewed head, does not waive the failing CI gate, and does not authorize a merge with red checks. A successor head requires a fresh review.
What
Adds
GET /api/v1/me/cron-jobs— every schedule the caller owns, across the orgs they belong to, ordered by what runs next.Why
GET /chats/:chatId/cron-jobsanswers "what runs from this chat". There was no route for the other question — "what do I have scheduled" — so a client wanting that list had to ask every chat it knew about and keep the few answers that weren't empty. That is one request per chat to find a handful of jobs, and it degrades as a workspace grows: the client I hit this with capped its fan-out at 40 chats and still couldn't state a trustworthy count without paying for all 40.Scope and safety
Ownership is already the mutation boundary for a job —
requireCronJobAccessgates onownerMemberId— so listing by owner grants no new reach. The route returns exactly the jobs the caller can already read and modify one id at a time. Membership is filtered tostatus = 'active', matching the other/me/*cross-org listings (/me/clients,/me/managed-agents).The existing
idx_cron_jobs_owner_createdindex covers the members join; ordering isnext_run_at ASC, which puts paused jobs (next_run_at IS NULL) after the scheduled ones — the order a reader wants anyway, since a job with no next occurrence can't be sorted among ones that have one.Tests
packages/server/src/__tests__/me-cron-jobs-list.test.tscovers:Verified:
tsc --noEmitonpackages/serverpasses, Biome clean.Not verified by me: the integration tests need a live Postgres, and this environment has one running but no credentials I can use — so the new test has not been executed locally. It follows the existing
useTestApp/createTestAgentpatterns and should run in CI; if it fails there, the failure is mine to fix.🤖 Generated with Claude Code