From 6f17c4cc6d232de88a6f77420846df25afccf293 Mon Sep 17 00:00:00 2001 From: "clawsweeper[bot]" <274271284+clawsweeper[bot]@users.noreply.github.com> Date: Mon, 22 Jun 2026 10:42:21 +0000 Subject: [PATCH] fix(doctor): stop promising --fix for working isolated shell-prompt cron jobs (#94655) (#94784) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Summary: - Merged fix(doctor): stop promising --fix for working isolated shell-prompt cron jobs (#94655) after ClawSweeper review. Automerge notes: - PR branch already contained follow-up commit before automerge: fix(doctor): stop promising --fix for working isolated shell-prompt c… Validation: - ClawSweeper review passed for head 0d71970a16e27cee368d9e310d92ec2badcd86dc. - Required merge gates passed before the squash merge. Prepared head SHA: 0d71970a16e27cee368d9e310d92ec2badcd86dc Review: https://github.com/openclaw/openclaw/pull/94784#issuecomment-4767423033 Co-authored-by: clawsweeper <274271284+clawsweeper[bot]@users.noreply.github.com> Co-authored-by: ZengWen-DT <290981215+ZengWen-DT@users.noreply.github.com> Co-authored-by: Altay Approved-by: altaywtf --- scripts/ci-run-timings.mjs | 8 +- src/commands/doctor/cron/index.test.ts | 138 ++++++++++++++++++ src/commands/doctor/cron/index.ts | 20 ++- src/commands/doctor/cron/payload-migration.ts | 28 +++- src/commands/doctor/cron/repair-plan.ts | 54 +++++-- .../doctor/cron/store-migration.test.ts | 2 + src/commands/doctor/cron/store-migration.ts | 30 +++- test/scripts/ci-run-timings.test.ts | 16 ++ 8 files changed, 262 insertions(+), 34 deletions(-) diff --git a/scripts/ci-run-timings.mjs b/scripts/ci-run-timings.mjs index 6b0f51879930..a945784809bc 100644 --- a/scripts/ci-run-timings.mjs +++ b/scripts/ci-run-timings.mjs @@ -26,7 +26,7 @@ function parseJsonCommand(command, args, options = {}) { } catch (error) { lastError = error; const message = error instanceof Error ? error.message : String(error); - const retryable = /HTTP 5\d\d|Server Error|ETIMEDOUT|ECONNRESET|EAI_AGAIN/u.test(message); + const retryable = isRetryableGhJsonErrorMessage(message); if (!retryable || attempt === GH_JSON_RETRY_DELAYS_MS.length) { throw error; } @@ -36,6 +36,12 @@ function parseJsonCommand(command, args, options = {}) { throw lastError; } +export function isRetryableGhJsonErrorMessage(message) { + return /HTTP 5\d\d|HTTP 429|Server Error|secondary rate limit|abuse detection|ETIMEDOUT|ECONNRESET|EAI_AGAIN/iu.test( + message, + ); +} + function normalizeRunJob(job) { return { completedAt: job.completedAt ?? job.completed_at ?? null, diff --git a/src/commands/doctor/cron/index.test.ts b/src/commands/doctor/cron/index.test.ts index 91a38867a851..55d69967b4f9 100644 --- a/src/commands/doctor/cron/index.test.ts +++ b/src/commands/doctor/cron/index.test.ts @@ -600,6 +600,144 @@ describe("maybeRepairLegacyCronStore", () => { expectNoteContaining("1 job still uses legacy", "Cron"); }); + it("advises on isolated shell-prompt jobs without a non-actionable --fix repair note (#94655)", async () => { + const storePath = await makeTempStorePath(); + const shellPromptJobs: Array> = [ + createCurrentCronJob({ + id: "shell-prompt-job-1", + name: "Shell prompt job 1", + schedule: { kind: "cron", expr: "*/30 * * * *", tz: "UTC" }, + sessionTarget: "isolated", + payload: { + kind: "agentTurn", + message: + "Run python3 scripts/check_mail.py and send a compact summary if anything changed.", + toolsAllow: ["*"], + }, + delivery: { mode: "announce" }, + }), + createCurrentCronJob({ + id: "shell-prompt-job-2", + name: "Shell prompt job 2", + schedule: { kind: "cron", expr: "15 * * * *", tz: "UTC" }, + sessionTarget: "isolated", + payload: { + kind: "agentTurn", + message: "Run node scripts/check_mail.js and summarize any new messages.", + toolsAllow: ["bash"], + }, + delivery: { mode: "announce" }, + }), + createCurrentCronJob({ + id: "shell-prompt-job-3", + name: "Shell prompt job 3", + schedule: { kind: "cron", expr: "45 * * * *", tz: "UTC" }, + sessionTarget: "isolated", + payload: { + kind: "agentTurn", + message: "Execute ./scripts/check_mail.sh and report changed mailbox counts.", + toolsAllow: ["shell"], + }, + delivery: { mode: "announce" }, + }), + ]; + const shellPromptJob = requirePersistedJob(shellPromptJobs, 0); + await writeCurrentCronStore(storePath, shellPromptJobs); + + const prompter = makePrompter(true); + await maybeRepairLegacyCronStore({ + cfg: createCronConfig(storePath), + options: {}, + prompter, + }); + + // The advisory is informational only: doctor --fix cannot rewrite a working + // isolated agentTurn job, so the misleading repair note must stay absent. + expectNoNoteContaining("Cron store issues detected", "Cron"); + expectNoteContaining( + "3 isolated cron jobs drive shell/process tools from the agent prompt and keep running as-is: `Shell prompt job 1`, `Shell prompt job 2`, `Shell prompt job 3`.", + "Cron", + ); + expectNoteContaining("informational only", "Cron"); + expectNoteContaining("Shell prompt job 1", "Cron"); + expectNoteContaining("Shell prompt job 2", "Cron"); + expectNoteContaining("Shell prompt job 3", "Cron"); + expectNoNoteContaining("openclaw doctor --fix", "Cron"); + expectNoNoteContaining("jobs.json", "Cron"); + expect(prompter.confirm).not.toHaveBeenCalled(); + + // No churn: the advisory does not rewrite the still-working jobs. + const persistedJobs = await readPersistedJobs(storePath); + expect(persistedJobs).toEqual(shellPromptJobs); + const job = requirePersistedJob(persistedJobs, 0); + expect(job).toEqual(shellPromptJob); + const reloaded = await loadCronJobsStoreWithConfigJobs(storePath); + expect(reloaded.configJobIndexes).toEqual([0, 1, 2]); + expect(reloaded.invalidConfigRows).toEqual([]); + const configJob = requirePersistedJob(reloaded.configJobs, 0); + expect(configJob).toEqual( + Object.fromEntries(Object.entries(shellPromptJob).filter(([key]) => key !== "updatedAtMs")), + ); + expect(reloaded.configJobRuntimeEntries[0]).toEqual({ + updatedAtMs: shellPromptJob.updatedAtMs, + state: {}, + scheduleIdentity: JSON.stringify({ + version: 1, + enabled: shellPromptJob.enabled, + schedule: shellPromptJob.schedule, + }), + }); + const payload = requireRecord(job.payload, "cron payload"); + expect(payload.kind).toBe("agentTurn"); + expect(payload.message).toContain("python3 scripts/check_mail.py"); + }); + + it("keeps restricted command prompts actionable without a --fix repair note", async () => { + const storePath = await makeTempStorePath(); + const commandPromptJob = createCurrentCronJob({ + id: "restricted-command-prompt", + name: "Restricted command prompt", + schedule: { kind: "cron", expr: "*/30 * * * *", tz: "UTC" }, + sessionTarget: "isolated", + payload: { + kind: "agentTurn", + message: [ + "Command to run:", + "- command: python3 scripts/check_mail.py", + "- workdir: /home/openclaw/.razor/clawd", + ].join("\n"), + toolsAllow: ["read", "message"], + }, + delivery: { mode: "announce" }, + }); + await writeCurrentCronStore(storePath, [commandPromptJob]); + + const prompter = makePrompter(true); + await maybeRepairLegacyCronStore({ + cfg: createCronConfig(storePath), + options: {}, + prompter, + }); + + expectNoNoteContaining("Cron store issues detected", "Cron"); + expectNoteContaining( + "1 isolated cron job describes a shell command in the agent prompt but lacks shell/process tool access: `Restricted command prompt`.", + "Cron", + ); + expectNoteContaining("not the supported shell-tool prompt shape", "Cron"); + expectNoteContaining("Recreate the job as a command cron job", "Cron"); + expectNoNoteContaining("informational only", "Cron"); + expectNoNoteContaining("keep running as-is", "Cron"); + expectNoNoteContaining("openclaw doctor --fix", "Cron"); + expect(prompter.confirm).not.toHaveBeenCalled(); + + const job = requirePersistedJob(await readPersistedJobs(storePath), 0); + const payload = requireRecord(job.payload, "cron payload"); + expect(payload.kind).toBe("agentTurn"); + expect(payload.message).toContain("python3 scripts/check_mail.py"); + expect(payload.toolsAllow).toEqual(["read", "message"]); + }); + it("repairs malformed persisted cron ids before list rendering sees them", async () => { const storePath = await makeTempStorePath(); await writeCronStore(storePath, [ diff --git a/src/commands/doctor/cron/index.ts b/src/commands/doctor/cron/index.ts index 6463ecda0d0c..40bf680d6494 100644 --- a/src/commands/doctor/cron/index.ts +++ b/src/commands/doctor/cron/index.ts @@ -30,6 +30,8 @@ import { } from "./legacy-store-migration.js"; import { formatLegacyIssuePreview, + formatUnresolvedCommandPromptAdvisory, + formatUnresolvedShellPromptAdvisory, mergeLegacyCronJobs, mergeRuntimeEntryIntoConfigJob, needsSqliteProjectionBackfill, @@ -336,9 +338,21 @@ export async function maybeRepairLegacyCronStore(params: { const normalized = normalizeStoredCronJobs(rawJobs); const notifyCount = rawJobs.filter((job) => job.notify === true).length; const dreamingStaleCount = countStaleDreamingJobs(rawJobs); - const previewLines = formatLegacyIssuePreview(normalized.issues, { - unresolvedAgentTurnShellToolPrompt: normalized.unresolvedAgentTurnShellToolPromptJobs, - }); + // Unresolved agentTurn command prompts are not auto-fixable; keep them out of the + // --fix preview so the repair note does not promise a fix that never lands (#94655). + const commandPromptAdvisory = formatUnresolvedCommandPromptAdvisory( + normalized.unresolvedAgentTurnCommandPromptJobs, + ); + if (commandPromptAdvisory) { + note(commandPromptAdvisory, "Cron"); + } + const shellPromptAdvisory = formatUnresolvedShellPromptAdvisory( + normalized.unresolvedAgentTurnShellToolPromptJobs, + ); + if (shellPromptAdvisory) { + note(shellPromptAdvisory, "Cron"); + } + const previewLines = formatLegacyIssuePreview(normalized.issues); if (legacyStoreDetected) { previewLines.unshift( legacyImportCount > 0 diff --git a/src/commands/doctor/cron/payload-migration.ts b/src/commands/doctor/cron/payload-migration.ts index a1ae5d798480..ee8a469fef86 100644 --- a/src/commands/doctor/cron/payload-migration.ts +++ b/src/commands/doctor/cron/payload-migration.ts @@ -12,6 +12,10 @@ type LegacyAgentTurnCommandPayload = { timeoutSeconds?: number; }; +export type UnresolvedAgentTurnShellToolPromptKind = + | "commandPromptWithoutShellAccess" + | "shellToolPrompt"; + const LEGACY_AGENT_TURN_COMMAND_MARKER_RE = /\bCommand to run\s*:/iu; const LEGACY_AGENT_TURN_COMMAND_FIELD_RE = /^\s*-\s*(command|workdir|timeout)\s*:\s*(.*?)\s*$/iu; const SHELL_TOOL_NAMES = new Set(["bash", "command", "exec", "process", "shell", "sh"]); @@ -217,17 +221,27 @@ export function migrateLegacyAgentTurnCommandPayload(payload: UnknownRecord): bo return true; } -export function hasUnresolvedAgentTurnShellToolPrompt(payload: UnknownRecord): boolean { +export function classifyUnresolvedAgentTurnShellToolPrompt( + payload: UnknownRecord, +): UnresolvedAgentTurnShellToolPromptKind | null { if (payload.kind !== "agentTurn") { - return false; + return null; } const message = readString(payload.message); if (typeof message !== "string") { - return false; + return null; } const parsed = parseLegacyAgentTurnCommandMessage(message); - return ( - Boolean(parsed) || - (hasShellToolAccess(payload.toolsAllow) && SHELL_COMMAND_MESSAGE_RE.test(message)) - ); + const shellToolAccess = hasShellToolAccess(payload.toolsAllow); + if (parsed && !shellToolAccess) { + return "commandPromptWithoutShellAccess"; + } + if (shellToolAccess && SHELL_COMMAND_MESSAGE_RE.test(message)) { + return "shellToolPrompt"; + } + return null; +} + +export function hasUnresolvedAgentTurnShellToolPrompt(payload: UnknownRecord): boolean { + return classifyUnresolvedAgentTurnShellToolPrompt(payload) !== null; } diff --git a/src/commands/doctor/cron/repair-plan.ts b/src/commands/doctor/cron/repair-plan.ts index 335e45835249..75063637782e 100644 --- a/src/commands/doctor/cron/repair-plan.ts +++ b/src/commands/doctor/cron/repair-plan.ts @@ -5,28 +5,55 @@ import { normalizeCronJobInput } from "../../../cron/normalize.js"; import type { CronJob } from "../../../cron/types.js"; type CronLegacyIssueCounts = Partial>; -type CronLegacyIssueDetails = { - unresolvedAgentTurnShellToolPrompt?: string[]; -}; function pluralize(count: number, noun: string) { return `${count} ${noun}${count === 1 ? "" : "s"}`; } -function formatJobNameList(names: string[] | undefined): string { - if (!names || names.length === 0) { - return ""; - } +function formatJobNameList(names: string[]): string { const preview = names.slice(0, 5).map((name) => `\`${name}\``); const remaining = names.length - preview.length; return remaining > 0 ? `: ${preview.join(", ")} (+${remaining} more)` : `: ${preview.join(", ")}`; } +/** + * Advisory for isolated agentTurn cron jobs that describe a command but cannot access shell tools. + * These need operator attention, but `doctor --fix` cannot safely infer whether to grant tool + * access or recreate them as command cron jobs. + */ +export function formatUnresolvedCommandPromptAdvisory(names: string[]): string | null { + if (names.length === 0) { + return null; + } + const describeVerb = names.length === 1 ? "describes" : "describe"; + const accessVerb = names.length === 1 ? "lacks" : "lack"; + return [ + `${pluralize(names.length, "isolated cron job")} ${describeVerb} a shell command in the agent prompt but ${accessVerb} shell/process tool access${formatJobNameList(names)}.`, + "- This is not the supported shell-tool prompt shape, so doctor cannot prove the job will execute the requested command.", + '- Recreate the job as a command cron job (`openclaw cron add ... --command ""`) or grant explicit shell/process tool access before relying on it.', + ].join("\n"); +} + +/** + * Advisory for isolated agentTurn cron jobs that drive shell/process tools from the prompt. + * These keep running and are not a legacy store row, so `doctor --fix` cannot rewrite them; + * routing this through the auto-repair preview made the finding persist after every --fix. + */ +export function formatUnresolvedShellPromptAdvisory(names: string[]): string | null { + if (names.length === 0) { + return null; + } + const verb = names.length === 1 ? "drives" : "drive"; + const keepVerb = names.length === 1 ? "keeps" : "keep"; + return [ + `${pluralize(names.length, "isolated cron job")} ${verb} shell/process tools from the agent prompt and ${keepVerb} running as-is${formatJobNameList(names)}.`, + "- This is a supported shape, not a legacy store row, so the doctor fix path cannot convert it and the finding is informational only.", + '- For a deterministic run, recreate the job as a command cron job (`openclaw cron add ... --command ""`).', + ].join("\n"); +} + /** Convert legacy cron issue counts into doctor preview lines. */ -export function formatLegacyIssuePreview( - issues: CronLegacyIssueCounts, - details: CronLegacyIssueDetails = {}, -): string[] { +export function formatLegacyIssuePreview(issues: CronLegacyIssueCounts): string[] { const lines: string[] = []; if (issues.jobId) { lines.push(`- ${pluralize(issues.jobId, "job")} still uses legacy \`jobId\``); @@ -58,11 +85,6 @@ export function formatLegacyIssuePreview( `- ${pluralize(issues.legacyAgentTurnCommandPayload, "job")} uses an agent prompt to run a shell command`, ); } - if (issues.unresolvedAgentTurnShellToolPrompt) { - lines.push( - `- ${pluralize(issues.unresolvedAgentTurnShellToolPrompt, "job")} asks an isolated agent for shell/process tools and needs manual command conversion${formatJobNameList(details.unresolvedAgentTurnShellToolPrompt)}`, - ); - } if (issues.legacyPayloadProvider) { lines.push( `- ${pluralize(issues.legacyPayloadProvider, "job")} still uses payload \`provider\` as a delivery alias`, diff --git a/src/commands/doctor/cron/store-migration.test.ts b/src/commands/doctor/cron/store-migration.test.ts index 2c784191eca2..6cade9a7f652 100644 --- a/src/commands/doctor/cron/store-migration.test.ts +++ b/src/commands/doctor/cron/store-migration.test.ts @@ -190,6 +190,8 @@ describe("normalizeStoredCronJobs", () => { expect(result.issues.legacyAgentTurnCommandPayload).toBeUndefined(); expect(result.issues.unresolvedAgentTurnShellToolPrompt).toBe(1); + expect(result.unresolvedAgentTurnCommandPromptJobs).toEqual(["Legacy job"]); + expect(result.unresolvedAgentTurnShellToolPromptJobs).toEqual([]); const payload = job.payload as Record; expect(payload.kind).toBe("agentTurn"); expect(payload.message).toContain(command); diff --git a/src/commands/doctor/cron/store-migration.ts b/src/commands/doctor/cron/store-migration.ts index f1878d031041..a136d75a5744 100644 --- a/src/commands/doctor/cron/store-migration.ts +++ b/src/commands/doctor/cron/store-migration.ts @@ -14,7 +14,7 @@ import { inferCronJobName } from "../../../cron/service/normalize.js"; import { normalizeCronStaggerMs, resolveDefaultCronStaggerMs } from "../../../cron/stagger.js"; import { normalizeLegacyDeliveryInput } from "./legacy-delivery.js"; import { - hasUnresolvedAgentTurnShellToolPrompt, + classifyUnresolvedAgentTurnShellToolPrompt, hasLegacyOpenAICodexCronModelRef, migrateLegacyAgentTurnCommandPayload, migrateLegacyCronPayload, @@ -41,6 +41,7 @@ type CronStoreIssues = Partial>; type NormalizeCronStoreJobsResult = { issues: CronStoreIssues; + unresolvedAgentTurnCommandPromptJobs: string[]; unresolvedAgentTurnShellToolPromptJobs: string[]; jobs: Array>; mutated: boolean; @@ -246,7 +247,12 @@ export function normalizeStoredCronJobs( jobs: Array>, ): NormalizeCronStoreJobsResult { const issues: CronStoreIssues = {}; + const unresolvedAgentTurnCommandPromptJobs: string[] = []; const unresolvedAgentTurnShellToolPromptJobs: string[] = []; + const unresolvedAgentTurnPromptJobsByKind = { + commandPromptWithoutShellAccess: unresolvedAgentTurnCommandPromptJobs, + shellToolPrompt: unresolvedAgentTurnShellToolPromptJobs, + }; let mutated = false; const keptJobs: Array> = []; const removedJobs: NormalizeCronStoreJobsResult["removedJobs"] = []; @@ -421,11 +427,14 @@ export function normalizeStoredCronJobs( if (migrateLegacyAgentTurnCommandPayload(payloadRecord)) { mutated = true; trackIssue("legacyAgentTurnCommandPayload"); - } else if (hasUnresolvedAgentTurnShellToolPrompt(payloadRecord)) { - trackIssue("unresolvedAgentTurnShellToolPrompt"); - const name = normalizeOptionalString(raw.name) ?? normalizeOptionalString(raw.id); - if (name) { - unresolvedAgentTurnShellToolPromptJobs.push(name); + } else { + const unresolvedPromptKind = classifyUnresolvedAgentTurnShellToolPrompt(payloadRecord); + if (unresolvedPromptKind) { + trackIssue("unresolvedAgentTurnShellToolPrompt"); + const name = normalizeOptionalString(raw.name) ?? normalizeOptionalString(raw.id); + if (name) { + unresolvedAgentTurnPromptJobsByKind[unresolvedPromptKind].push(name); + } } } } @@ -626,5 +635,12 @@ export function normalizeStoredCronJobs( jobs.splice(0, jobs.length, ...keptJobs); } - return { issues, unresolvedAgentTurnShellToolPromptJobs, jobs, mutated, removedJobs }; + return { + issues, + unresolvedAgentTurnCommandPromptJobs, + unresolvedAgentTurnShellToolPromptJobs, + jobs, + mutated, + removedJobs, + }; } diff --git a/test/scripts/ci-run-timings.test.ts b/test/scripts/ci-run-timings.test.ts index bcefa86f646e..2fca75d2e747 100644 --- a/test/scripts/ci-run-timings.test.ts +++ b/test/scripts/ci-run-timings.test.ts @@ -2,6 +2,7 @@ import { describe, expect, it } from "vitest"; import { collectRunJobsFromPages, + isRetryableGhJsonErrorMessage, parseRunTimingArgs, selectLatestMainPushCiRun, summarizePnpmStoreWarmupBarrier, @@ -143,6 +144,21 @@ describe("scripts/ci-run-timings.mjs", () => { ]); }); + it("retries transient GitHub API failures while preserving auth failures", () => { + for (const message of [ + "gh: API secondary rate limit exceeded (HTTP 403)", + "gh: HTTP 429: too many requests", + "Command failed: gh api repos/openclaw/openclaw/actions/runs/1/jobs\nHTTP 502", + "read ECONNRESET", + ]) { + expect(isRetryableGhJsonErrorMessage(message)).toBe(true); + } + + expect( + isRetryableGhJsonErrorMessage("gh: Resource not accessible by integration (HTTP 403)"), + ).toBe(false); + }); + it("summarizes the pnpm store warmup fanout barrier", () => { expect( summarizePnpmStoreWarmupBarrier({