Skip to content

feat(server): list a user's schedules in one request (GET /me/cron-jobs) - #2384

Open
L42y wants to merge 2 commits into
first-tree-ai:mainfrom
L42y:feat/me-cron-jobs
Open

feat(server): list a user's schedules in one request (GET /me/cron-jobs)#2384
L42y wants to merge 2 commits into
first-tree-ai:mainfrom
L42y:feat/me-cron-jobs

Conversation

@L42y

@L42y L42y commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

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-jobs answers "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 — requireCronJobAccess gates on ownerMemberId — 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 to status = 'active', matching the other /me/* cross-org listings (/me/clients, /me/managed-agents).

The existing idx_cron_jobs_owner_created index covers the members join; ordering is next_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.ts covers:

  • returns the caller's jobs across multiple chats, and not another user's;
  • ordering by next run, with a paused job last;
  • 401 without a signed-in user.

Verified: tsc --noEmit on packages/server passes, 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 / createTestAgent patterns and should run in CI; if it fails there, the failure is mine to fix.

🤖 Generated with Claude Code

`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 yuezengwu left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread packages/server/src/services/chat/scheduled-jobs/job.ts Outdated
…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>
@L42y

L42y commented Sep 3, 2026

Copy link
Copy Markdown
Contributor Author

You're right, and the "no new reach" claim in my description 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 substitute. Filtering on active owner membership alone let a job survive a transition the rest of the API treats as losing it. Thank you for reproducing it with the real service transitions rather than describing it.

Fixed in fae0f520f. The listing now intersects ownership with the same rule requireChatAccess applies:

  • a direct chat_membership row for the owner's human agent (speaker or watcher), or
  • a speaker in the control chat that is an agent the owner manages — the supervised path, which a naive membership filter would have dropped, hiding jobs the caller can still read by id.

Two tests cover the boundary from both sides, because getting this wrong in either direction is a bug:

  1. drops a job once the owner loses access to its control chat — runs your exact sequence (removeParticipant of the managed agent, then leaveChat) and asserts the listing no longer returns the job alongside the 404s from GET /cron-jobs/:id and GET /chats/:chatId/cron-jobs, so the three routes are asserted to agree rather than assumed to.
  2. keeps a job visible to a manager who supervises a speaker in its chat — human leaves, managed speaker stays; the listing keeps the job and the by-id route is asserted to return 200, pinning the supervised path against an over-correction.

On CI

I 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 main. Merging that clears this class of failure for every open PR, not just mine.

The two Test Server failures — clients-provider-models.test.ts:273 and feishu-registration.test.ts:1615 — I have not attributed and am not claiming are flaky. They are outside the changed files and my change touches no provider-catalog or Feishu path, but that is an argument from scope, not evidence, and I would rather say so than imply I checked. This environment has no Docker for the Testcontainers setup and no usable Postgres credentials, so I cannot run the server integration suites locally at all — including my own three tests. CI is their first real execution, and if the new tests fail there the failure is mine to fix.

Ready for another exact-head review at fae0f520f, with the caveat that its CI will stay red on the inherited fixture failure until #2385 lands.

@yuezengwu yuezengwu left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants