feat(cron): add optional agentId filter to cron.list#77191
feat(cron): add optional agentId filter to cron.list#77191lonexreb wants to merge 4 commits intoopenclaw:mainfrom
Conversation
openclaw-barnacle
Bot
added
app: web-ui
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 610eac9dd9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (agentIdFilter !== null && (job.agentId ?? "") !== agentIdFilter) { | ||
| return false; |
There was a problem hiding this comment.
Treating job.agentId as "" in the new filter drops valid default-agent jobs that intentionally omit agentId (main-session jobs are allowed to have no agentId), so cron.list with agentId set to the default agent can return incomplete results. This will surface as soon as callers auto-fill agentId from session context: legacy/default jobs without explicit agentId disappear from listings even though they belong to that agent, causing incorrect totals and missed jobs in heartbeats.
Useful? React with 👍 / 👎.
|
Codex review: needs real behavior proof before merge. Summary Reproducibility: yes. by source inspection: current main lacks Real behavior proof Next step before merge Security Review findings
Review detailsBest possible solution: Fix the filter to use the same default-agent fallback as other cron paths, then merge the gateway-contract slice with real behavior proof while tracking tool and CLI scoping separately. Do we have a high-confidence way to reproduce the issue? Yes, by source inspection: current main lacks Is this the best way to solve the issue? No for the current PR head: the additive protocol field and service predicate are the right narrow slice, but the default-agent fallback must match existing cron semantics before merge. Real behavior proof is also still required for this external PR. Full review comments:
Overall correctness: patch is incorrect What I checked:
Likely related people:
Remaining risk / open question:
Codex review notes: model gpt-5.5, reasoning high; reviewed against a17d4371d101. |
lonexreb
force-pushed
the
feat/77118-cron-list-agentid-filter
branch
from
610eac9 to
cf286f8
Compare
In multi-agent installs, every agent calling cron.list received every agent's jobs (often 50+ across 7 agents in the reporter's setup), creating prompt noise during heartbeat checks and accidental cross-agent modification risk. Add an optional agentId field to: - CronListPageOptions (cron service) - CronListParamsSchema (gateway protocol) - cron.list server-method handler The listPage op filters jobs by exact agentId match (treating empty/ whitespace as omitted) and composes with the existing query/enabled filters. Omitting agentId preserves the historical 'all jobs' result. Tool-layer auto-fill from session context and CLI --agent flag are follow-ups; this PR delivers the gateway-side foundation. Tests: 5 new cron listPage cases covering match, omit/empty, no-match, and composition with the query filter. All 7 listPage tests plus 12 cron-validators tests pass.
lonexreb
force-pushed
the
feat/77118-cron-list-agentid-filter
branch
from
cf286f8 to
98872f0
Compare
|
Addressed [P2] from codex review ( |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 98872f0304
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const agentIdFilter = | ||
| typeof opts?.agentId === "string" && opts.agentId.trim() ? opts.agentId.trim() : null; |
There was a problem hiding this comment.
The new agentId filter compares raw trimmed strings, but cron job ownership is normalized elsewhere (normalizeOptionalAgentId/normalizeAgentId) and can be case-normalized independently of caller input. As a result, requests like cron.list({ agentId: "MAIN" }) can miss jobs stored as "main", and implicit default-agent jobs can also be dropped when casing differs, even though the same IDs are treated as equivalent in create/update paths. Normalize the filter (and effective owner) with the same agent-id normalizer before comparing.
Useful? React with 👍 / 👎.
…param The new optional agentId parameter on CronListParams added in this PR needs the generated Swift protocol files to include the new field; CI fast-protocol drift check flagged the missing regeneration.
Lint check (oxlint) flagged two .sort() calls on the new test file as 'Use Array#toSorted() instead of Array#sort()' — toSorted() is the non-mutating variant that the repo lint rule prefers.
Codex follow-up on PR openclaw#77191 noted: cron job ownership is normalized via normalizeOptionalAgentId/normalizeAgentId on create/update paths, but the new agentId filter compared raw trimmed strings. As a result, requests like cron.list({ agentId: 'MAIN' }) missed jobs stored as 'main', and implicit default-agent jobs could also be dropped when the configured default-agent id casing differed from caller input — even though the same IDs are treated as equivalent in create/update paths. Route both the requested filter and the resolved default-agent id through normalizeOptionalAgentId, and normalize each job's agentId the same way when computing the effective owner. Add a regression test covering the 'MAIN' vs 'main' casing scenario and a whitespace+case variant.
|
Addressed [P2] from codex review ( |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 363137161a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // Mirror that here so `cron.list({ agentId: defaultAgentId })` returns | ||
| // default-agent jobs that omit the field, not an empty page. Normalize the | ||
| // default-agent id too so the comparison is symmetric. See #77118. | ||
| const defaultAgentId = normalizeOptionalAgentId(state.deps.defaultAgentId); |
There was a problem hiding this comment.
listPage now maps jobs without agentId to defaultAgentId, but this uses normalizeOptionalAgentId(state.deps.defaultAgentId), which returns undefined when the dependency is omitted. In that common fallback configuration, implicit default-agent jobs get effectiveOwner === "" and are excluded by cron.list({ agentId: "main" }), even though other cron paths treat missing default agent as main via normalizeAgentId. This makes filtered results inconsistent for embedded/non-gateway cron service users that rely on the default main agent behavior.
Useful? React with 👍 / 👎.
openclaw-barnacle
Bot
added
the
triage: needs-real-behavior-proof
|
Heads up — |
1 participant
Summary
Adds an additive optional
agentIdfilter tocron.listso multi-agent installs can scope cron job listings to a single agent's jobs. OmittingagentIdpreserves the historical "all jobs across all agents" result, so every existing client keeps working.This delivers the gateway-side foundation only. Tool-layer auto-fill from session context and a
--agentCLI flag are sensible follow-ups but stay out of this PR to keep the change reviewable as one tight contract evolution.Why
From the issue: in a 7-agent install with ~54 cron jobs, every agent's
cron listreturned all 54 jobs. Heartbeat prompts wasted tokens parsing irrelevant jobs, agents could see (and potentially modify viaupdate/add) other agents' jobs, and prompt confusion led to incorrect status reports. The reporter mitigated locally by adding inline HEARTBEAT.md instructions to ignore other agents' jobs — a workaround pointing at a missing server-side capability.Changes
src/cron/service/list-page-types.ts— AddagentId?: stringtoCronListPageOptionswith a docstring linking the issue.src/cron/service/ops.ts— InlistPage, normalize the input (treat empty/whitespace as omitted), computeagentIdFilteronce, and apply it inside the existingsource.filter(...)predicate so it composes withenabledFilterandquery.src/gateway/protocol/schema/cron.ts— AddagentId: Type.Optional(Type.String())toCronListParamsSchema. Schema isadditionalProperties: false, so this is the canonical place for the new field.src/gateway/server-methods/cron.ts— ThreadagentIdthrough thecron.listhandler intocontext.cron.listPage(...).src/cron/service.list-page-agent-filter.test.ts— 5 new regression tests:agentIdreturns every job (backward compatible)agentIdis treated as omittedqueryfilterCHANGELOG.md— Unreleased > Changes entry credited to me.Boundary considerations
Per
src/gateway/protocol/CLAUDE.md: schema changes are protocol changes, prefer additive evolution. This is purely additive — no existing field changes shape, no required field added, schema staysadditionalProperties: false. Existing clients that omitagentIdare unaffected.Test plan
pnpm test src/cron/service.list-page-agent-filter.test.ts src/cron/service.list-page-sort-guards.test.ts— 7/7 pass (5 new + 2 existing)pnpm test src/gateway/protocol/cron-validators.test.ts— 12/12 passpnpm exec oxfmt --checkon touched files — cleanFixes #77118.
Real behavior proof